[PATCH v5 9/9] line-log: consult diff process for range tracking

["Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com>]

From: Michael Montalbo <mmontalbo@gmail.com>

git log -L tracks line ranges by diffing each commit against its
parent in collect_diff().  This pass used the builtin diff while the
displayed diff (builtin_diff()) consults a configured
diff.<driver>.process, so the two could disagree: a reformat-only
commit selected by builtin tracking was then rendered with an empty
diff because the tool reported the files equivalent.

Consult the process in collect_diff() too, mirroring the blame
integration.  When the tool reports the files equivalent, collect no
ranges; the tracked range then maps across unchanged and the commit
drops out of the log, matching what is displayed.  Like the summary
formats, the tracking pass diffs raw content, so the tool is consulted
on the raw blobs here.

Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
---
 Documentation/gitattributes.adoc | 20 ++++++++++---------
 line-log.c                       | 33 ++++++++++++++++++++++++++++----
 t/t4080-diff-process.sh          | 33 ++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 13 deletions(-)

diff --git a/Documentation/gitattributes.adoc b/Documentation/gitattributes.adoc
index 7cdede6b21..8021dc8e39 100644
--- a/Documentation/gitattributes.adoc
+++ b/Documentation/gitattributes.adoc
@@ -1037,12 +1037,16 @@ Features that ask "which lines changed" use the tool's hunks in place
 of the builtin algorithm:
 
 - `git diff` patch output, together with everything layered on it:
-  word diff, function context (`-W`), `--color-moved`, the `@@` hunk
-  headers, and the `-L` line-range display.  These operate on the
-  lines the patch step already emitted, so they reflect the tool's
-  hunks without any further negotiation.
+  word diff, function context (`-W`), `--color-moved`, and the `@@`
+  hunk headers.  These operate on the lines the patch step already
+  emitted, so they reflect the tool's hunks without any further
+  negotiation.
 - `git blame`: a commit whose change the tool reports as equivalent is
   skipped, and its lines are attributed to an earlier commit.
+- `git log -L`: both the line-range display and the underlying range
+  tracking consult the tool, so a commit it reports as equivalent is
+  dropped from the log (its tracked range maps across unchanged)
+  rather than selected and then shown with an empty diff.
 - `--stat`, `--numstat`, and `--shortstat`: the inserted and deleted
   counts come from the tool's hunks, so a file the tool calls
   equivalent contributes no stat line, matching the empty patch that
@@ -1079,11 +1083,9 @@ design:
 - `--raw`, `--name-only`, and `--name-status` compare object ids at
   the tree level and never run a line-level diff at all.
 
-Two cases ask "which lines changed" but still use the builtin
-algorithm, and may consult the process in a later change: `git log
--L`'s commit selection and parent range propagation (as distinct from
-its display, which is covered above), and combined diffs (`--cc` and
-merge diffs), whose protocol would have to be extended from a single
+Combined diffs (`--cc` and merge diffs) ask "which lines changed" but
+still use the builtin algorithm, and may consult the process in a
+later change; their protocol would have to be extended from a single
 old/new pair to one comparison per merge parent.
 
 `--no-ext-diff` and `--diff-algorithm` bypass the process entirely,
diff --git a/line-log.c b/line-log.c
index 5fc75ae275..97b3e0a31d 100644
--- a/line-log.c
+++ b/line-log.c
@@ -7,11 +7,11 @@
 #include "tag.h"
 #include "tree.h"
 #include "diff.h"
+#include "diff-process.h"
 #include "commit.h"
 #include "decorate.h"
 #include "repository.h"
 #include "revision.h"
-#include "xdiff-interface.h"
 #include "strbuf.h"
 #include "line-log.h"
 #include "setup.h"
@@ -330,12 +330,15 @@ static int collect_diff_cb(long start_a, long count_a,
 	return 0;
 }
 
-static int collect_diff(mmfile_t *parent, mmfile_t *target, struct diff_ranges *out)
+static int collect_diff(struct diff_options *diffopt, const char *path,
+			mmfile_t *parent, mmfile_t *target,
+			struct diff_ranges *out)
 {
 	struct collect_diff_cbdata cbdata = {NULL};
 	xpparam_t xpp;
 	xdemitconf_t xecfg;
 	xdemitcb_t ecb;
+	int ret = 0;
 
 	memset(&xpp, 0, sizeof(xpp));
 	memset(&xecfg, 0, sizeof(xecfg));
@@ -345,7 +348,23 @@ static int collect_diff(mmfile_t *parent, mmfile_t *target, struct diff_ranges *
 	xecfg.hunk_func = collect_diff_cb;
 	memset(&ecb, 0, sizeof(ecb));
 	ecb.priv = &cbdata;
-	return xdi_diff(parent, target, &xpp, &xecfg, &ecb);
+
+	/*
+	 * Consult the diff process so range tracking agrees with the
+	 * diff that will be shown.  When the tool reports the files as
+	 * equivalent we collect no ranges, so the tracked range maps
+	 * across unchanged and the commit drops out of the log, rather
+	 * than being selected here but rendered with an empty diff by
+	 * the process-aware builtin_diff().  Blob oids are not threaded to
+	 * this path yet, so pass NULL and send no old-oid/new-oid (a later
+	 * change can supply the pair, where they would let the tool cache
+	 * across the range-tracking and display passes over the same
+	 * commit).
+	 */
+	if (xdi_diff_process(diffopt, path, parent, target,
+			     NULL, NULL, &xpp, &xecfg, &ecb) == DIFF_PROCESS_ERROR)
+		ret = -1;
+	return ret;
 }
 
 /*
@@ -927,7 +946,13 @@ static int process_diff_filepair(struct rev_info *rev,
 	}
 
 	diff_ranges_init(&diff);
-	if (collect_diff(&file_parent, &file_target, &diff))
+	/*
+	 * Select the driver by the old (parent) path, as builtin_diff() does
+	 * with name_a, so a renamed file resolves to the same driver for
+	 * range tracking as for the diff that is shown.
+	 */
+	if (collect_diff(&rev->diffopt, pair->one->path,
+			 &file_parent, &file_target, &diff))
 		die("unable to generate diff for %s", pair->one->path);
 
 	/* NEEDSWORK should apply some heuristics to prevent mismatches */
diff --git a/t/t4080-diff-process.sh b/t/t4080-diff-process.sh
index e1c7256747..c20ab15ecd 100755
--- a/t/t4080-diff-process.sh
+++ b/t/t4080-diff-process.sh
@@ -914,4 +914,37 @@ test_expect_success 'blame -w bypasses diff process' '
 	test_path_is_missing backend.log
 '
 
+#
+# Line-log (git log -L) range tracking.
+#
+
+test_expect_success 'diff process drops equivalent commit from log -L' '
+	test_when_finished "rm -f backend.log" &&
+	cat >linelog.c <<-\EOF &&
+	int tracked(void) { return 1; }
+	EOF
+	git add linelog.c &&
+	git commit -m "add linelog.c" &&
+
+	cat >linelog.c <<-\EOF &&
+	int tracked(void) { return 2; }
+	EOF
+	git commit -am "change tracked line" &&
+
+	# Builtin line tracking selects the change commit.
+	git log --no-ext-diff -L1,1:linelog.c --format="%s" >builtin &&
+	test_grep "change tracked line" builtin &&
+
+	# With the tool reporting the change as equivalent, tracking
+	# drops the commit (the range maps across unchanged) instead of
+	# selecting it and rendering an empty diff.
+	git -c diff.cdiff.process="$BACKEND --mode=no-hunks --log=backend.log" \
+		log -L1,1:linelog.c --format="%s" >actual &&
+	test_grep ! "change tracked line" actual &&
+	# The creating commit still appears, so the change commit was
+	# selectively dropped rather than the whole log going empty.
+	test_grep "add linelog.c" actual &&
+	test_grep "command=hunks pathname=linelog.c" backend.log
+'
+
 test_done
-- 
gitgitgadget