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

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.

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