git log -L, all new and shiny

I too thought it would never happen -- but then again this is still not ready, I'm just trying to give it some exposure.

My mail archive says we were at v6 last time, but this is a rewrite rather than a reroll. In efforts to make the code understandable, I tried to compose it of little functions that do little operations on little data structures, to at least some success.

If you just want to dive in, patch it into your git or pull from

git://github.com/trast/git.git line-log-WIP

and then try something like

  git log -L '/^int main/,/^}/':git.c
  git log --graph -L '/^static void sha1_object/,/^}/':builtin/index-pack.c 681b07de11

The TODO list:

* I dropped the tests for now, so we need new ones.

* A good split of the main patch would be nice :-)

* Much of the code leaks memory like a sieve because I was lazy. It's workable on the git repository, but eats a few hundred MB for simple tasks even there.

* Missing features:
- detect/filter merges at the hunk level
(and ideally show a combined diff for the nontrivial ones)
- -M and -C support
- ...?

There's also a longer-term wishlist hinted at in the commit message of the main patch: the diff machinery currently makes no provisions for chaining its various bells and whistles. As the most obvious example, it is not possible to run a word diff over the result of 'log -L'. Changes in this direction would probably also make the implementation of 'log -L' fit nicely in there somewhere.

Bo Yang (3):
Refactor parse_loc
Export three functions from diff.c
Export rewrite_parents() for 'log -L'

Thomas Rast (2):
blame: introduce $ as "end of file" in -L syntax Implement line-history search (git log -L)

 Documentation/blame-options.txt     |   19 +-
 Documentation/git-log.txt           |   22 +
 Documentation/line-range-format.txt |   24 +
 Makefile                            |    2 +
 builtin/blame.c                     |   99 +--
 builtin/log.c                       |   53 ++
 diff.c                              |    6 +-
 diff.h                              |   17 +
 line.c                              | 1245 +++++++++++++++++++++++++++++++++++
 line.h                              |   79 +++
 log-tree.c                          |    3 +
 revision.c                          |   22 +-
 revision.h                          |   16 +-
 t/t8003-blame-corner-cases.sh       |    6 +
 14 files changed, 1491 insertions(+), 122 deletions(-)
 create mode 100644 Documentation/line-range-format.txt
 create mode 100644 line.c
 create mode 100644 line.h

1.7.11.rc1.243.gbf4713c

From: Bo Yang <[email protected]>

We want to use the same style of -L n,m argument for 'git log -L' as for git-blame. Refactor the argument parsing of the range arguments from builtin/blame.c to the (new) file that will hold the 'git log -L' logic.

To accommodate different data structures in blame and log -L, the file contents are abstracted away; parse_range_arg takes a callback that it uses to get the contents of a line of the (notional) file.

The new test is for a case that made me pause during debugging: the 'blame -L with invalid end' test was the only one that noticed an outright failure to parse the end *at all*. So make a more explicit test for that.

---
 Documentation/blame-options.txt     |  19 +------
 Documentation/line-range-format.txt |  18 +++++++
 Makefile                            |   2 +
 builtin/blame.c                     |  99 +++--------------------------------
 line.c                              | 100 ++++++++++++++++++++++++++++++++++++
 line.h                              |  23 +++++++++
 t/t8003-blame-corner-cases.sh       |   6 +++
 7 files changed, 158 insertions(+), 109 deletions(-)
 create mode 100644 Documentation/line-range-format.txt
 create mode 100644 line.c
 create mode 100644 line.h

index d4a51da..1d9305e 100644
@@ -13,24 +13,7 @@
 	Annotate only the given line range.  <start> and <end> can take
 	one of these forms:
 
-	- number
-+
-If <start> or <end> is a number, it specifies an
-absolute line number (lines count from 1).
-+
-
-- /regex/
-+
-This form will use the first line matching the given
-POSIX regex.  If <end> is a regex, it will search
-starting at the line given by <start>.
-+
-
-- +offset or -offset
-+
-This is only valid for <end> and will specify a number
-of lines before or after the line given by <start>.
-+
+include::line-range-format.txt[]
 
 -l::
 	Show long rev (Default: off).

new file mode 100644
index 0000000..265bc23
@@ -0,0 +1,18 @@
+- number
++
+If <start> or <end> is a number, it specifies an
+absolute line number (lines count from 1).
++
+
+- /regex/
++
+This form will use the first line matching the given
+POSIX regex.  If <end> is a regex, it will search
+starting at the line given by <start>.
++
+
+- +offset or -offset
++
+This is only valid for <end> and will specify a number
+of lines before or after the line given by <start>.
++

index 4592f1f..9166b86 100644
@@ -626,6 +626,7 @@ LIB_H += hash.h
 LIB_H += help.h
 LIB_H += kwset.h
 LIB_H += levenshtein.h
+LIB_H += line.h
 LIB_H += list-objects.h
 LIB_H += ll-merge.h
 LIB_H += log-tree.h

@@ -731,6 +732,7 @@ LIB_OBJS += hex.o
 LIB_OBJS += ident.o
 LIB_OBJS += kwset.o
 LIB_OBJS += levenshtein.o
+LIB_OBJS += line.o
 LIB_OBJS += list-objects.o
 LIB_OBJS += ll-merge.o
 LIB_OBJS += lockfile.o

new file mode 100644
index 0000000..5878c94
@@ -0,0 +1,23 @@
+#ifndef LINE_H
+#define LINE_H
+
+/*
+ * Parse one item in an -L begin,end option w.r.t. the notional file
+ * object 'cb_data' consisting of 'lines' lines.
+ *
+ * The 'nth_line_cb' callback is used to determine the start of the
+ * line 'lno' inside the 'cb_data'.  The caller is expected to already
+ * have a suitable map at hand to make this a constant-time lookup.
+ *
+ * Returns 0 in case of success and -1 if there was an error.  The
+ * caller should print a usage message in the latter case.
+ */
+
+typedef const char *(*nth_line_fn_t)(void *data, long lno);
+
+extern int parse_range_arg(const char *arg,
+			   nth_line_fn_t nth_line_cb,
+			   void *cb_data, long lines,
+			   long *begin, long *end);
+
+#endif /* LINE_H */

index 230143c..e7cac1d 100755
@@ -175,6 +175,12 @@ test_expect_success 'blame -L with invalid end' '
 	grep "has only 2 lines" errors
 '
 
+test_expect_success 'blame parses <end> part of -L' '
+	git blame -L1,1 tres >out &&
+	cat out &&
+	test $(wc -l < out) -eq 1
+'
+
 test_expect_success 'indent of line numbers, nine lines' '
 	git blame nine_lines >actual &&
 	test $(grep -c "  " actual) = 0

1.7.11.rc1.243.gbf4713c

To save the user a lookup of the last line number, introduce $ as a shorthand for the last line. This is mostly useful to spell "until the end of the file" as '-L<begin>,$'.

---
 Documentation/line-range-format.txt | 6 ++++++
 line.c                              | 8 ++++++++
 2 files changed, 14 insertions(+)

index 265bc23..9ce0688 100644
@@ -16,3 +16,9 @@ starting at the line given by <start>.
 This is only valid for <end> and will specify a number
 of lines before or after the line given by <start>.
 +
+
+- `$`
++
+A literal dollar sign can be used as a shorthand for the last line in
+the file.
++

index afd2e3b..a7f33ed 100644
@@ -15,6 +15,14 @@ const char *parse_loc(const char *spec, nth_line_fn_t nth_line,
 	regmatch_t match[1];
 
 	/*
+	 * $ is a synonym for "the end of the file".
+	 */
+	if (spec[0] == '$') {
+		*ret = lines;
+		return spec + 1;
+	}
+
+	/*
 	 * Allow "-L <something>,+20" to mean starting at <something>
 	 * for 20 lines, or "-L <something>,-5" for 5 lines ending at
 	 * <something>.

1.7.11.rc1.243.gbf4713c

Hmph. This is mostly useful not to error out when user (perhaps mistakenly) expects that the end of file is spelled as "$"; both "git blame -L<begin>" and "git blame L<begin>," have always meant "til the end", IIRC.

Because I do not offhand think of other & better uses of "$" as a special case that conflicts with "the end of file", I do not think it is a bad patch that needs to be rejected, though.

Heh. That's actually a very good point; I think neither Bo nor me have thought about allowing git log -L <begin>:<file>.

Thomas Rast
trast@{inf,student}.ethz.ch

From: Bo Yang <[email protected]>

Use fill_metainfo to fill the line level diff meta data, emit_line to print out a line and quote_two to quote paths.

---
 diff.c |  6 +++---
 diff.h | 17 +++++++++++++++++
 2 files changed, 20 insertions(+), 3 deletions(-)

index 77edd50..f9673f3 100644
@@ -220,7 +220,7 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
-static char *quote_two(const char *one, const char *two)
+char *quote_two(const char *one, const char *two)
 {
 	int need_one = quote_c_style(one, NULL, NULL, 1);
 	int need_two = quote_c_style(two, NULL, NULL, 1);

@@ -410,7 +410,7 @@ static void emit_line_0(struct diff_options *o, const char *set, const char *res
 		fputc('\n', file);
 }
 
-static void emit_line(struct diff_options *o, const char *set, const char *reset,
+void emit_line(struct diff_options *o, const char *set, const char *reset,
 		      const char *line, int len)
 {
 	emit_line_0(o, set, reset, line[0], line+1, len-1);

@@ -2925,7 +2925,7 @@ static int similarity_index(struct diff_filepair *p)
 	return p->score * 100 / MAX_SCORE;
 }
 
-static void fill_metainfo(struct strbuf *msg,
+void fill_metainfo(struct strbuf *msg,
 			  const char *name,
 			  const char *other,
 			  struct diff_filespec *one,

index e027650..4a7d085 100644
@@ -14,6 +14,7 @@
 struct userdiff_driver;
 struct sha1_array;
 struct commit;
+struct diff_filepair;
 
 typedef void (*change_fn_t)(struct diff_options *options,
 		 unsigned old_mode, unsigned new_mode,

@@ -329,6 +330,22 @@ extern size_t fill_textconv(struct userdiff_driver *driver,
 
 extern int parse_rename_score(const char **cp_p);
 
+/* some output functions line.c need */
+extern void fill_metainfo(struct strbuf *msg,
+			  const char *name,
+			  const char *other,
+			  struct diff_filespec *one,
+			  struct diff_filespec *two,
+			  struct diff_options *o,
+			  struct diff_filepair *p,
+			  int *must_show_header,
+			  int use_color);
+
+extern void emit_line(struct diff_options *o, const char *set, const char *reset,
+		      const char *line, int len);
+
+extern char *quote_two(const char *one, const char *two);
+
 extern int print_stat_summary(FILE *fp, int files,
 			      int insertions, int deletions);
 

1.7.11.rc1.243.gbf4713c

Why? Neither 4/5 or 5/5 seem to use any of these.

From: Bo Yang <[email protected]>

The function rewrite_one is used to rewrite a single parent of the current commit, and is used by rewrite_parents to rewrite all the parents.

Decouple the dependence between them by making rewrite_one a callback function that is passed to rewrite_parents. Then export rewrite_parents for reuse by the line history browser.

We will use this function in line.c.

---
 revision.c | 13 ++++---------
 revision.h | 10 ++++++++++
 2 files changed, 14 insertions(+), 9 deletions(-)

index 935e7a7..183ca58 100644
@@ -2109,12 +2109,6 @@ int prepare_revision_walk(struct rev_info *revs)
 	return 0;
 }
 
-enum rewrite_result {
-	rewrite_one_ok,
-	rewrite_one_noparents,
-	rewrite_one_error
-};
-
 static enum rewrite_result rewrite_one(struct rev_info *revs, struct commit **pp)
 {
 	struct commit_list *cache = NULL;

@@ -2136,12 +2130,13 @@ static enum rewrite_result rewrite_one(struct rev_info *revs, struct commit **pp
 	}
 }
 
-static int rewrite_parents(struct rev_info *revs, struct commit *commit)
+int rewrite_parents(struct rev_info *revs, struct commit *commit,
+	rewrite_parent_fn_t rewrite_parent)
 {
 	struct commit_list **pp = &commit->parents;
 	while (*pp) {
 		struct commit_list *parent = *pp;
-		switch (rewrite_one(revs, &parent->item)) {
+		switch (rewrite_parent(revs, &parent->item)) {
 		case rewrite_one_ok:
 			break;
 		case rewrite_one_noparents:

@@ -2213,7 +2208,7 @@ enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)
 	if (action == commit_show &&
 	    !revs->show_all &&
 	    revs->prune && revs->dense && want_ancestry(revs)) {
-		if (rewrite_parents(revs, commit) < 0)
+		if (rewrite_parents(revs, commit, rewrite_one) < 0)
 			return commit_error;
 	}
 	return action;

index 863f4f6..6d09550 100644
@@ -231,4 +231,14 @@ enum commit_action {
 extern enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit);
 extern enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit);
 
+enum rewrite_result {
+	rewrite_one_ok,
+	rewrite_one_noparents,
+	rewrite_one_error
+};
+
+typedef enum rewrite_result (*rewrite_parent_fn_t)(struct rev_info *revs, struct commit **pp);
+
+extern int rewrite_parents(struct rev_info *revs, struct commit *commit,
+	rewrite_parent_fn_t rewrite_parent);
 #endif

1.7.11.rc1.243.gbf4713c

This is a rewrite of much of Bo's work, mainly in an effort to split it into smaller, easier to understand routines.

The algorithm is built around the struct range_set, which encodes a series of line ranges as intervals [a,b). This is used in two contexts:

* A set of lines we are tracking (which will change as we dig through history).
* To encode diffs, as pairs of ranges.

The main routine is range_set_map_across_diff(). It processes the diff between a commit C and some parent P. It determines which diff hunks are relevant to the ranges tracked in C, and computes the new ranges for P.

The algorithm is then simply to process history in topological order from newest to oldest, computing ranges and (partial) diffs. At branch points, we need to merge the ranges we are watching. We will find that many commits do not affect the chosen ranges, and mark them TREESAME (in addition to those already filtered by pathspec limiting). Another pass of history simplification then gets rid of such commits.

This is wired as an extra filtering pass in the log machinery. This currently only reduces code duplication, but should allow for other simplifications and options to be used.

Finally, we hook a diff printer into the output chain. Ideally we would wire directly into the diff logic, to optionally use features like word diff. However, that will require some major reworking of the diff chain, so we completely replace the output with our own diff for now.

---
 Documentation/git-log.txt |   22 +
 builtin/log.c             |   53 +++
 line.c                    | 1139 ++++++++++++++++++++++++++++++++++++++++++++-
 line.h                    |   56 +++
 log-tree.c                |    3 +
 revision.c                |    9 +
 revision.h                |    6 +-
 7 files changed, 1286 insertions(+), 2 deletions(-)

index 1f90620..9104a6d 100644
@@ -68,6 +68,23 @@ produced by --stat etc.
 	Note that only message is considered, if also a diff is shown
 	its size is not included.
 
+-L <start>,<end>:<file>::
+	Trace the evolution of the line range given by "<start>,<end>"
+	within the <file>.  You may not give any pathspec limiters.
+	This is currently limited to a walk starting from a single
+	revision, i.e., you may only give zero or one positive
+	revision arguments.
+
+<start> and <end> can take one of these forms:
+
+include::line-range-format.txt[]
+You can specify this option more than once.
+
+
+--full-line-diff::
+	Always print the interesting range even if the current commit
+	does not change any line of the range.
+
 [\--] <path>...::
 	Show only commits that are enough to explain how the files
 	that match the specified paths came to be.  See "History

@@ -137,6 +154,11 @@ Examples
 	This makes sense only when following a strict policy of merging all
 	topic branches when staying on a single integration branch.
 
+git log -L '/int main/',/^}/:main.c::
+
+	Shows how the function `main()` in the file 'main.c' evolved
+	over time.
+
 
 Discussion
 ----------

index c894930..3fb025d 100644
@@ -809,6 +809,9 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
 	log.parent = NULL;
 	opt->loginfo = &log;
 
+	if (opt->line_level_traverse)
+		return line_log_print(opt, commit);
+
 	shown = log_tree_diff(opt, commit, &log);
 	if (!shown && opt->loginfo && opt->always_show_header) {
 		log.parent = NULL;

index 183ca58..a92e359 100644
@@ -13,6 +13,7 @@
 #include "decorate.h"
 #include "log-tree.h"
 #include "string-list.h"
+#include "line.h"
 
 volatile show_early_output_fn_t show_early_output;
 

@@ -1851,6 +1852,12 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	if (revs->combine_merges)
 		revs->ignore_merges = 0;
 	revs->diffopt.abbrev = revs->abbrev;
+
+	if (revs->line_level_traverse) {
+		revs->limited = 1;
+		revs->topo_order = 1;
+	}
+
 	if (diff_setup_done(&revs->diffopt) < 0)
 		die("diff_setup_done failed");
 

@@ -2102,6 +2109,8 @@ int prepare_revision_walk(struct rev_info *revs)
 			return -1;
 	if (revs->topo_order)
 		sort_in_topological_order(&revs->commits, revs->lifo);
+	if (revs->line_level_traverse)
+		line_log_filter(revs);
 	if (revs->simplify_merges)
 		simplify_merges(revs);
 	if (revs->children.name)

index 6d09550..01902cf 100644
@@ -92,7 +92,8 @@ struct rev_info {
 			cherry_mark:1,
 			bisect:1,
 			ancestry_path:1,
-			first_parent_only:1;
+			first_parent_only:1,
+			line_level_traverse:1;
 
 	/* Diff flags */
 	unsigned int	diff:1,

@@ -168,6 +169,9 @@ struct rev_info {
 	int count_left;
 	int count_right;
 	int count_same;
+
+	/* line level range that we are chasing */
+	struct decoration line_log_data;
 };
 
 #define REV_TREE_SAME		0

1.7.11.rc1.243.gbf4713c

You mentioned "splitting" in the cover letter, but it does seem to need a bit more work.

Yes, I think I also mentioned that it's not ready for inclusion ;-)

Most of your points are spot on, however:

That should end up being used a few times...

...

If the code ever passed anything but an empty struct diff_ranges as the 'out' argument, yes. But it doesn't. In general I'm usually doing the 'out' dance to save one heap allocation. Perhaps it would be cleaner to allocate all of them on the heap instead, and return as pointers, dunno.

That was actually Bo's idea.

Thomas Rast
trast@{inf,student}.ethz.ch

Hi,

this is absolutely great.

When I run your example invocations, nothing is displayed for a very long time. I understand that this is because of the extra 'simplification pass', which means that whole results need to be ready before anything is displayed. Adding something like '-10' doesn't seem to have any effect, so I guess it is ignored. I hope that making '-<n>' work would not be to complicated, but more important would be getting incremental results. Will it be possible?

Zbyszek

I am not convinced that it is "diff machinery makes no provivsions" that is the problem. Isn't it coming from the way the series limits the output line range and reimplements its own output routine?

All the "bells and whistles" like diffstat, word coloring, etc. go through the xdi_diff_outf() interface, so isn't it the matter of limiting lines that this interface calls back the "bells and whistles" callback functions with?

When you enter the diff machinery, you have the path and the line range you are interested in of one side (lets say you are comparing side A and B, and have line range for side A).

If you

- add a mechanism to pass the "interesting" line range and path down to the callchain from xdi_diff_outf() to xdiff_outf();

- make one of these functions filter out (i.e. not call the callback xdiff_emit_consume_fn) hunks that do not overlap with the line range you are interested in (I would presume that they would be a few new fields in xdemitconf_t structure); and

- while recording the corresponding line ranges in the other side of the hunks that are output,

that would give you

- output that is limited to the "interesting" input range of side A;

- the corresponding "interesting" range in the other side of the comparison, so that you can update the "interesting" range to feed to the next diff that compares side B with something else; and

- for whatever processing the various "bells and whistles" callers already implement, as all their callbacks see are the lines in your "interesting" range.

No? Am I missing something?

Well, in a very circular logic sense, yes: I reimplement the output routine because that's the only way I could think of doing it right now :-)

However, notice that word-diff also reimplements its own output routine, though it probably has a better standing since it is a different format.

Hrm.

This would be the first backwards coupling between the revision-walk and the diff generation parts, at least that I know of. Normally the revision walker just calls out to the (line-wise, not tree-based) diff engine when it wants to show a commit. Now suddenly the diff engine is used (a lot, too) in simplifying the history.

Ideally we would want to reuse diffs that have already been generated, as this is a very expensive process. The current log -L implementation manages to do this at the cost of reimplementing the diff output routines instead.

You solve it instead by mandating that the diff engine itself updates the "interesting" ranges, but that needs a lot of inside knowledge: like in blame, we sometimes explore alternatives (e.g. for merges; or with -M, though log -L in this version does not implement that feature).

So we would end up with redoing diffs, or a very tight coupling, that IMHO just makes the mess worse.

Or am I missing something?

I instead have the vision that eventually diffs should be represented internally as something like my pairs of struct range_set. Then we could run more passes on them as needed, and have a "common currency" between all diff-related work. Only the last one should then actually output the diff.

That still doesn't properly account for the case where the data format is no longer in terms of hunks (such as for word-diff, or the stat formats), though.

Thomas Rast
trast@{inf,student}.ethz.ch

Also notice that word-diff uses the same xdi_diff_outf() machinery to grab line-oriented diff as its input (cf. fn_out_consume), and then does its thing on it. If you limit what fn_out_consume sees, you can have word-diff do exactly what you want, no?

I am not convinced if you need to have any unusual back-coupling to begin with, by the way.

If you say "git log -p [--options] -- pathspec", the revision machinery does filter commits that do not touch any paths that patch pathspec with the TREESAME logic, but that does not necessarily mean you will see _all_ the commits that are not TREESAME. If you for example use ignore-space-change options, even the preimage and the postimage differ at the object name level (hence not TREESAME), the diff machinery already knows how to tell the revision machinery not to show the log message and stuff, causing the commit to be skipped from the output, no?

I do not know why you think you would need to do the filtering "range comparison and union" computation more than necessary. If the user asks "log -p", you need to do it once per parent-child pair that is not TREESAME at the place the current code calls run_diff(). I suspect that "log -p --stat" could be improved to eliminate the separate call to run_diffstat() by restructuring the code so that the statistics is gathered inside run_diff(), but that is independent of this series. If this series hooked into the level I hinted in my earlier message, such an optimization will reduce calls to your "range comparision and union" computation for free.

Let's clarify this part a bit.

Imagine you have three commits on a single strand of pearls.

	A---B---C

Between A and B, you only corrected indentation errors in file F as a clean-up step, and then between B and C, you did a real change. "git diff A B -- F" gives changes, "git diff -w A B -- F" does not. Both "git diff B C -- F" and "git diff -w B C -- F" give you some output.

Now, "git log --no-root -p -w C -- F" will give you C but not B.

Let's see how that happens.

The revision machinery looks at C and finds its parent B. It runs object level tree comparison and finds that their trees are different at path F. It makes a mental note that it may need to show the log message of C, and asks the diff machinery to run diff-tree between B and C. The diff machinery finds that it needs to show something even in the presense of -w option by actual comparison, and just before showing the very first line of patch output, it shows the log message of C (due to the earlier "mental note").

Then the revision machinery looks at B. It does the same between B and A, but this time around, the diff machinery finds that, even though A and B were _not_ TREESAME at the revision traversal level, there is nothing to be shown after filtering with the -w option. Hence no patch is shown and log message for B is not shown, either.

The interesting part of the above is that we did not bother using the -w comparison to affect TREESAME check at the revision traversal level.

I consider your "-L bottom,top" essentially the same "two blobs may be different at the object name level, but after filtering the diff, there may be no output" filter just like the "-w" option is. Instead of filtering hunks that have changes only in whitespaces, it filters hunks that do not overlap the given line range. So if you replace "-w" in the above example with "-L bottom,top", exactly the same thing should happen.

The current code structure does not consider the content level filtering done by the "-w" when computing TREESAME. This DOES affect how the merges are simplified. If you and I start from a commit X with full of indentation mistakes, you fix the earlier half of these mistakes and make commit T while I fix the later half of these mistakes and make commit J, and we merge our results into merge M:

      J---M
     /   /
    X---T

none of "diff -w J M", "diff -w T M", "diff -w X J", "diff -w X T" will output anything. "git diff -p -w M" will however traverse both branches, even though in this example nothing will be shown in the output, because no two trees in these four commits are TREESAME.

In the longer term, this _may_ be something we want to fix, but teaching the revision machinery about only "-L bottom,top" is not the way to do it. Instead, we should devise a new mechanism to call into the diff_patch machinery from the place where the revision machinery computes TREESAME, and let it ask the filtered content level differences if any of these options (these flags flip the DIFF_FROM_CONTENTS diff option) are in use. As a part of the implementation of that new mechanism, we may want to cache the patch such an early comparison produces, and reuse it when we actually produce output, as an optimization. But I think that is not limited to the "-L bottom,top" option and an orthogonal issue, as it will work equally well for existing "-w" filter (there may be others).

The above is where my recommendations are coming from. The first step for "-L bottom,top" should be to hook it as a new kind of DIFF_FROM_CONTENTS option that compares two non-identical blobs but possibly yield empty result into the diff machinery at the same level as "-w". Once that is solidly done, we can think about updating the merge simplification logic to take DIFF_FROM_CONTENTS changes into account when it computes TREESAME.

[...]

Thanks for the great explanations.

Having spent some time letting this sink in (and being busy doing other things), I think it's actually a good idea. It forces us to go back and change it around so that the diff machinery gets a say _before_ we simplify history. I think this bit will be important for log -L history to make sense, and it's a bug waiting to happen for the -w case.

Thomas Rast
trast@{inf,student}.ethz.ch

Note that this is not limited to "diff_patch() already filters -w".

If you are running with --diff-filter=A to grab only the additions, for example, you may want the merge simplification to know about this filtering as well.

So it is likely that you would want to hook diffcore_std(), not just diff_flush(), to the TREESAME machinery. Obviously you would want to do this only for the merge commits; there is no point doing this for single strand of pearls where the output phase already knows how to squelch output correctly.