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

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

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.

OK.

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

Sorry, but I cannot quite guess what you mean here.  Do you mean:

    We first go back <n> lines due to -U<n>.  If we have not found a
    function header line that comes before the first changed line
    within that <n> lines, we keep going back to satisfy -W (i.e. -W
    "wins" if "-U<n>" is small).  On the other hand, after going
    back <n> lines, if we have already seen a function header before
    the first changed line within these <n> lines, there is no need
    to go back further to satisfy -W (i.e. -U<n> "wins" and makes -W
    irrelevant).

If that is the case, I think I understand it.

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

Yes.

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.

Makes sense.  As long as we apply this when "-W wins", i.e we extend
the precontext so that the hunk begins on the line that match the
function header pattern, there isn't much point in showing only the
name of the function that comes before this hunk.

On the other hand, if "-U<n> won", i.e. the first precontext line
that is shown in the hunk is _before_ the line -W chose to extend
the hunk to (in this case, where xdl_emit_diff begins) because -U<n>
gave a large enough number, I do want to see the name of the fuction
I am looking at the tail of in the precontext on the hunk header
line.  E.g. "git diff -W -U120 xdiff/xemit.c" on this patch should
show is_empty_rec() as the hunk header.

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.

To me, that sounds like a grave regression.  Why lose the
information?

This may be coming from the difference between us, i.e. I read a lot
more patches written by other people than my own changes written for
my next commit, so every bit of hint helps, and the name of the
function I am seeing its latter half in the precontext is sometimes
a useful thing to see.

Am 16.02.21 um 02:30 schrieb Junio C Hamano:

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

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.

To me, that sounds like a grave regression.  Why lose the
information?

This may be coming from the difference between us, i.e. I read a lot
more patches written by other people than my own changes written for
my next commit, so every bit of hint helps, and the name of the
function I am seeing its latter half in the precontext is sometimes
a useful thing to see.

I totally agree with your assessment. I wouldn't even have removed the 
hunk header in the case of "-W wins", either, but that is a case that I 
can live with when others think it makes sense.

-- Hannes