[PATCH v3 7/7] diffcore-pickaxe: limit -G to the -L tracked range

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

From: Michael Montalbo <mmontalbo@gmail.com>

Teach -G to only search the line ranges specified by -L.
Teaching -S is left as future work, so it still matches the entire
file even if -L is specified.

Rather than being part of diff.c's builtin implementations, the
diffcore-pickaxe functionality interacts with xdiff-interface as a
separate component. Add a sibling to xdi_diff_outf(), called
diff_emit_line_ranges(), that limits emitted lines to the given line
ranges.

Use diff_emit_line_ranges() when searching text if line ranges have
been specified. If textconv is enabled, use normal diffing instead of
diff_emit_line_ranges() since line range tracking relies on the line
coordinates of the original, pre-textconv file.

Update documentation and add tests accordingly.

Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
---
 Documentation/line-range-options.adoc |  4 +-
 diff.c                                | 11 ++++
 diffcore-pickaxe.c                    | 30 ++++++++--
 t/t4211-line-log.sh                   | 81 ++++++++++++++++++++++-----
 xdiff-interface.h                     | 10 ++++
 5 files changed, 115 insertions(+), 21 deletions(-)

diff --git a/Documentation/line-range-options.adoc b/Documentation/line-range-options.adoc
index 4a7ab97d75..52e1262fd7 100644
--- a/Documentation/line-range-options.adoc
+++ b/Documentation/line-range-options.adoc
@@ -19,6 +19,8 @@
 +
 Patch formatting options such as `--word-diff`, `--color-moved`,
 `--no-prefix`, and whitespace options (`-w`, `-b`) are supported,
-as are pickaxe options (`-S`, `-G`) and `--diff-filter`.
+as are pickaxe options (`-S`, `-G`) and `--diff-filter`. `-G` is
+limited to the tracked range. In contrast, `-S` is evaluated over the whole
+file and may select a commit with a change outside the tracked range.
 +
 include::line-range-format.adoc[]
diff --git a/diff.c b/diff.c
index 49b6732c81..1a3571d229 100644
--- a/diff.c
+++ b/diff.c
@@ -2701,6 +2701,17 @@ static int line_range_filter_diff(struct line_range_filter *filter,
 	return ret;
 }
 
+int diff_emit_line_ranges(mmfile_t *one, mmfile_t *two,
+			  const struct range_set *ranges,
+			  xdiff_emit_line_fn line_fn, void *cb_data,
+			  xpparam_t *xpp, xdemitconf_t *xecfg)
+{
+	struct line_range_filter filter;
+
+	line_range_filter_init(&filter, ranges, line_fn, cb_data);
+	return line_range_filter_diff(&filter, one, two, xpp, xecfg);
+}
+
 static void pprint_rename(struct strbuf *name, const char *a, const char *b)
 {
 	const char *old_name = a;
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index b0915be86f..2425fe8101 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -16,7 +16,8 @@
 
 typedef int (*pickaxe_fn)(mmfile_t *one, mmfile_t *two,
 			  struct diff_options *o,
-			  regex_t *regexp, kwset_t kws);
+			  regex_t *regexp, kwset_t kws,
+			  const struct range_set *ranges);
 
 struct diffgrep_cb {
 	regex_t *regexp;
@@ -42,7 +43,8 @@ static int diffgrep_consume(void *priv, char *line, unsigned long len)
 
 static int diff_grep(mmfile_t *one, mmfile_t *two,
 		     struct diff_options *o,
-		     regex_t *regexp, kwset_t kws UNUSED)
+		     regex_t *regexp, kwset_t kws UNUSED,
+		     const struct range_set *ranges)
 {
 	struct diffgrep_cb ecbdata;
 	xpparam_t xpp;
@@ -65,8 +67,12 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,
 	 * An xdiff error might be our "data->hit" from above. See the
 	 * comment for xdiff_emit_line_fn in xdiff-interface.h
 	 */
-	ret = xdi_diff_outf(one, two, NULL, diffgrep_consume,
-			    &ecbdata, &xpp, &xecfg);
+	if (ranges)
+		ret = diff_emit_line_ranges(one, two, ranges, diffgrep_consume,
+					    &ecbdata, &xpp, &xecfg);
+	else
+		ret = xdi_diff_outf(one, two, NULL, diffgrep_consume,
+				    &ecbdata, &xpp, &xecfg);
 	if (ecbdata.hit)
 		return 1;
 	if (ret)
@@ -119,8 +125,13 @@ static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws,
 
 static int has_changes(mmfile_t *one, mmfile_t *two,
 		       struct diff_options *o UNUSED,
-		       regex_t *regexp, kwset_t kws)
+		       regex_t *regexp, kwset_t kws,
+		       const struct range_set *ranges UNUSED)
 {
+	/*
+	 * -S counts needle occurrences in each whole blob. Limiting this to
+	 * an -L range is left as a follow-up; for now -S ignores the range.
+	 */
 	unsigned int c1 = one ? contains(one, regexp, kws, 0) : 0;
 	unsigned int c2 = two ? contains(two, regexp, kws, c1 + 1) : 0;
 	return c1 != c2;
@@ -132,6 +143,7 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
 	struct userdiff_driver *textconv_one = NULL;
 	struct userdiff_driver *textconv_two = NULL;
 	mmfile_t mf1, mf2;
+	const struct range_set *ranges;
 	int ret;
 
 	/* ignore unmerged */
@@ -169,7 +181,13 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
 	mf1.size = fill_textconv(o->repo, textconv_one, p->one, &mf1.ptr);
 	mf2.size = fill_textconv(o->repo, textconv_two, p->two, &mf2.ptr);
 
-	ret = fn(&mf1, &mf2, o, regexp, kws);
+	/*
+	 * -L limits the search to the tracked range, but the range is in
+	 * pre-textconv line coordinates that do not map onto textconv
+	 * output, so search the whole file when textconv is enabled.
+	 */
+	ranges = (textconv_one || textconv_two) ? NULL : p->line_ranges;
+	ret = fn(&mf1, &mf2, o, regexp, kws, ranges);
 
 	if (textconv_one)
 		free(mf1.ptr);
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index 2a542aa643..2354400d1c 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -703,24 +703,18 @@ test_expect_success '-L suppresses deletions outside tracked range' '
 	test $(grep -c "^diff --git" actual) = 1
 '
 
-test_expect_success '-L with -S filters to string-count changes' '
+test_expect_success '-L with -S selects only the matching commit' '
 	git checkout parent-oids &&
-	git log -L:func2:file.c -S "F2 + 2" --format= >actual &&
-	# -S searches the whole file, not just the tracked range;
-	# combined with the -L range walk, this selects commits that
-	# both touch func2 and change the count of "F2 + 2" in the file.
-	test $(grep -c "^diff --git" actual) = 1 &&
-	test_grep "F2 + 2" actual
+	git log -L:func2:file.c -S "F2 + 2" --format=%s --no-patch >actual &&
+	echo "Modify func2() in file.c" >expect &&
+	test_cmp expect actual
 '
 
-test_expect_success '-L with -G filters to diff-text matches' '
+test_expect_success '-L with -G selects only the matching commit' '
 	git checkout parent-oids &&
-	git log -L:func2:file.c -G "F2 [+] 2" --format= >actual &&
-	# -G greps the whole-file diff text, not just the tracked range;
-	# combined with -L, this selects commits that both touch func2
-	# and have "F2 + 2" in their diff.
-	test $(grep -c "^diff --git" actual) = 1 &&
-	test_grep "F2 + 2" actual
+	git log -L:func2:file.c -G "F2 [+] 2" --format=%s --no-patch >actual &&
+	echo "Modify func2() in file.c" >expect &&
+	test_cmp expect actual
 '
 
 test_expect_success 'setup for trailing deletion test' '
@@ -1007,4 +1001,63 @@ test_expect_success '--check does not report blank-at-eof outside the range' '
 	test_cmp expect actual
 '
 
+test_expect_success '-L -G is limited to the tracked range' '
+	git checkout --orphan grep-range &&
+	git reset --hard &&
+	cat >gp.c <<-\EOF &&
+	int func1()
+	{
+	    return ALPHA;
+	}
+
+	int func2()
+	{
+	    return BETA;
+	}
+	EOF
+	git add gp.c &&
+	test_tick &&
+	git commit -m "add gp.c" &&
+	sed -e "s/ALPHA/ALPHA2/" -e "s/BETA/BETA2/" gp.c >tmp &&
+	mv tmp gp.c &&
+	git commit -a -m "touch both functions" &&
+	git log -L:func2:gp.c -G BETA --format=%s --no-patch >actual &&
+	cat >expect <<-\EOF &&
+	touch both functions
+	add gp.c
+	EOF
+	test_cmp expect actual &&
+	git log -L:func2:gp.c -G ALPHA --format=%s --no-patch >actual &&
+	test_must_be_empty actual
+'
+
+test_expect_success '-L -G searches the whole file under textconv' '
+	git checkout --orphan grep-textconv &&
+	git reset --hard &&
+	cat >tc.c <<-\EOF &&
+	int func1()
+	{
+	    return F1;
+	}
+
+	int func2()
+	{
+	    return F2;
+	}
+	EOF
+	git add tc.c &&
+	test_tick &&
+	git commit -m "add tc.c" &&
+	sed -e "s/F1/F1 + 1/" -e "s/return F2/return FINDME/" tc.c >tmp &&
+	mv tmp tc.c &&
+	git commit -a -m "change both funcs" &&
+	echo "tc.c diff=tc" >.gitattributes &&
+	git log -L:func1:tc.c -G FINDME --format=%s --no-patch >actual &&
+	test_must_be_empty actual &&
+	git config diff.tc.textconv cat &&
+	git log -L:func1:tc.c -G FINDME --format=%s --no-patch >actual &&
+	echo "change both funcs" >expect &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/xdiff-interface.h b/xdiff-interface.h
index 2428456629..4151bc2097 100644
--- a/xdiff-interface.h
+++ b/xdiff-interface.h
@@ -46,6 +46,16 @@ int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,
 		  xdiff_emit_line_fn line_fn,
 		  void *consume_callback_data,
 		  xpparam_t const *xpp, xdemitconf_t const *xecfg);
+
+struct range_set;
+/*
+ * Like xdi_diff_outf(), but forwards only the lines within the given
+ * postimage line ranges to line_fn.
+ */
+int diff_emit_line_ranges(mmfile_t *mf1, mmfile_t *mf2,
+			  const struct range_set *ranges,
+			  xdiff_emit_line_fn line_fn, void *cb_data,
+			  xpparam_t *xpp, xdemitconf_t *xecfg);
 int read_mmfile(mmfile_t *ptr, const char *filename);
 void read_mmblob(mmfile_t *ptr, struct object_database *odb,
 		 const struct object_id *oid);
-- 
gitgitgadget