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

["René Scharfe." <l.s.r@web.de>]

Am 15.02.21 um 16:50 schrieb Ævar Arnfjörð Bjarmason:
> Fix what I believe to be a long-standing bug in how "-W" interacts
> with displaying the hunk context on the @@ line: It should not be
> displayed at all under -W.
>
> The long-standing semantics of how -W works and interacts with -U<n>
> are rather easy to reason about:
>
>  * -W extends the context line up to the start of the function. With
>     userdiff this means the language-aware regex rules in userdiff.c,
>     or user-supplied rules.
>
>  * -U<n>, which defaults to -U3 shows at least <n> lines of context,
>     if that's greater than what we'd extend the context to under -W
>     then -U<n> wins.
>
>  * When showing the hunk context we look up from the first line we
>    show of the diff, and find whatever looks like useful context above
>    that line.
>
> Thus in e.g. the xdiff/xemit.c change being made in this commit we'll
> correctly show "xdl_emit_diff()" in the hunk context under default
> diff settings.
>
> But if we viewed it with the -W option we'd show "is_empty_rec()",
> because we'd first find the "xdl_emit_diff()" context line, extend the
> diff to that, and then would go look for context to show again.
>
> I don't think this behavior makes any sense, our context in this case
> is what we're guaranteed to show as part of the diff itself.
>
> The user already asked us to find that context line and show it, we
> don't need to then start showing the context above that line, which
> they didn't ask for.

Hmm, that's subtle.

Your reasoning applies to patches generated without -W as well.  If the
precontext contains a function line then the @@ line should not contain
a function comment.  However, e.g. with this:

-- snip --
cat >a <<EOF
func a

func b
1
2
3
EOF
sed 's/3/three/' <a >b
diff -up a b
-- snap --

... I get this:

--- a	2021-02-15 18:30:21.000000000 +0100
+++ b	2021-02-15 18:30:21.000000000 +0100
@@ -3,4 +3,4 @@ func a
 func b
 1
 2
-3
+three

So diff(1) shows the previous function line.  git diff does the same.

The behaviour of diff(1) and git diff does make sense to me: It's easy
to implement and the only downside is that it produces extra output in
some cases.

I can understand that users would rather have a tidy diff without
distractions, though.  So I like the output change you propose.

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.

> 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.

> 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."

> 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.

>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  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.

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