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@xxxxxxxxx>
> ---
> 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;
> }
>
