Re: [PATCH 1/2] diff: do not display hunk context under -W

On Mon, Feb 15 2021, René Scharfe. wrote:

Does GNU diff have something like git's -W, both "diff -U 0 -F func a b"
and "diff -U 0 -p a b" don't extend the context window as we do.

I don't think the patch I'm submitting here would make sense for GNU
diff, since there it just shows the context without being guaranteed to
show the full set of lines leading up to it under -W, but with Git diff
we do that, so I think it makes sense to omit the context.

However, I'm not sure it would be a good idea to clear @@ lines of hunks
generated without -W that have function lines in their precontext, even
though it would be a logical thing to do.

Yes, I don't think that's a good idea either. I think it only makes
sense under -W where the user explicitly asks "show me the function this
change was in", and we're (before this patch) showing different context
on the basis of emergent behavior.

quoted

This new behavior does give us the edge case that if we e.g. view the
diff here with "-U150 -W" we'd previously extend the context to the
middle of the "is_func_rec()" function, and show that function in the
hunk context. Now we'll show nothing.

Well, the 150 lines of context are still shown (as they should be), but
the @@ line contains no function name anymore.

Yes, indeed. I'll reword that to "now we'll show no context in that
case" or something...

quoted

I think that change also makes sense. We're showing a change in the
"xdl_emit_diff()" function. That's our context for the change. It
doesn't make sense with -W to start fishing around for other
context.

It does make sense in the context of the diff(1) -p implementation, but
your change is consistent with the description of that option: "Show
which C function each change is in."

I hadn't spotted that, we just said:

    Show whole function as context lines for each change. The function
    names are determined in the same way as git diff works out patch
    hunk headers

Which I think can more obviously be read as the existing behavior being
desired, and this patch being a change to documented behavior.

(I think it is, I just think it makes sense to change the docs &
behavior int this case)

quoted

Arguably in that case we could save away the context we found in the
"XDL_EMIT_FUNCCONTEXT" in "xdl_emit_diff()" and show that if we end up
extending the diff past the function, either because of a high -U<n>
value, or because our change was right at the start.

I wouldn't really mind if we did that, perhaps it would be a useful
marker with high -U<n> values to remind the user of what they're
looking at, but I also don't see the usefulness in practice, so let's
punt that for now.

It could be confusing for someone who expects the old behaviour, leaving
it empty makes more sense to me.

FWIW it would be useful for:

    git log -U1000 -W

And then searching for "@@" in the pager to find changes to specific
functions.

quoted

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 Documentation/diff-options.txt | 4 ++++
 t/t4015-diff-whitespace.sh     | 2 +-
 t/t4018-diff-funcname.sh       | 7 +++++++
 xdiff/xemit.c                  | 4 +++-
 4 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index e5733ccb2d..8ca59effa7 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt

@@ -759,6 +759,10 @@ endif::git-format-patch[]
 	The function names are determined in the same way as
 	`git diff` works out patch hunk headers (see 'Defining a
 	custom hunk-header' in linkgit:gitattributes[5]).
++
+When showing the whole function for context the "@@" context line
+itself will always be empty, since the context that would otherwise be
+shown there will be the first line of the hunk being shown.

 ifndef::git-format-patch[]
 ifndef::git-log[]

diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 8c574221b2..0ffc845cdd 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh

@@ -2133,7 +2133,7 @@ test_expect_success 'combine --ignore-blank-lines with --function-context 2' '
 		--ignore-blank-lines --function-context a b >actual.raw &&
 	sed -n "/@@/,\$p" <actual.raw >actual &&
 	cat <<-\EOF >expect &&
-	@@ -5,11 +6,9 @@ c
+	@@ -5,11 +6,9 @@
 	 function
 	 1
 	 2

diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index 80f35c5e16..f3374abd98 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh

@@ -91,6 +91,13 @@ test_diff_funcname () {
 		fi
 	' &&

+	test_expect_success "$desc -W" '
+		git diff -U0 -W "$what" >W-U0-diff &&
+		echo >W-U0-expected &&
+		last_diff_context_line W-U0-diff >W-U0-actual &&
+		test_cmp W-U0-expected W-U0-actual
+	' &&
+
 	test_expect_success "$desc (accumulated)" '
 		git diff -U1 "$what".acc >diff &&
 		last_diff_context_line diff >actual.lines &&

diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 9d7d6c5087..02b5dbcc70 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c

@@ -274,7 +274,9 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 		 */

 		if (xecfg->flags & XDL_EMIT_FUNCNAMES) {
-			get_func_line(xe, xecfg, &func_line,
+			get_func_line(xe, xecfg,
+				      xecfg->flags & XDL_EMIT_FUNCCONTEXT
+				      ? NULL : &func_line,

Why still search?  It would be better to turn off XDL_EMIT_FUNCNAMES if
XDL_EMIT_FUNCCONTEXT is enabled -- a one-character change in diff.c.

I just didn't read the diff/xdiff code carefully enough. Will fix.

quoted

 				      s1 - 1, funclineprev);
 			funclineprev = s1 - 1;
 		}