diff mbox series

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

Message ID 20210215155020.2804-2-avarab@gmail.com (mailing list archive)
State New
Headers show
Series userdiff: refactor + test + doc + misc improvements | expand

Commit Message

Ævar Arnfjörð Bjarmason Feb. 15, 2021, 3:50 p.m. UTC
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

Comments

René Scharfe Feb. 15, 2021, 6:47 p.m. UTC | #1
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;
>  		}
>
Ævar Arnfjörð Bjarmason Feb. 15, 2021, 7:24 p.m. UTC | #2
On Mon, Feb 15 2021, René Scharfe. wrote:

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

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.

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

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

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

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

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

>>  				      s1 - 1, funclineprev);
>>  			funclineprev = s1 - 1;
>>  		}
>>
René Scharfe Feb. 15, 2021, 9:17 p.m. UTC | #3
Am 15.02.21 um 20:24 schrieb Ævar Arnfjörð Bjarmason:
>
> On Mon, Feb 15 2021, René Scharfe. wrote:
>
>> 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.
>
> 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.

Exactly my point: GNU diff doesn't have something like -W, but still can
show an unchanged function at the @@ line, because it searches upwards
starting just above the first shown line, and the name of the actually
changed function might be in one of the shown lines.

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

Given the goal to show the name of the changed function it *would* make
sense to start searching at the first changed line instead.

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

Right, -W never needs diff(1)'s -p, while the case is less clear for
diffs generated without -W.

René
Junio C Hamano Feb. 16, 2021, 1:30 a.m. UTC | #4
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> 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.
Junio C Hamano Feb. 16, 2021, 1:37 a.m. UTC | #5
Junio C Hamano <gitster@pobox.com> writes:

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

Ehh, I did not mean "the current behaviour with -W that shows
is_empty_rec makes sense".  The observation you made (which lead to
the conclusion in your next paragraph that it is not a good idea
to show is_empty_rec on the hunk header line) made sense to me.

But I do not think any change from the current behaviour should be
made if -U<n> wins -W (i.e. the first precontext line shown in the
hunk header is not due to -W).  We should hunt for the name of the
function whose latter half we are seeing at the beginning of the
hunk in that case.

Thanks.
Johannes Sixt Feb. 16, 2021, 7:20 a.m. UTC | #6
Am 16.02.21 um 02:30 schrieb Junio C Hamano:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>> 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
Junio C Hamano Feb. 16, 2021, 5:51 p.m. UTC | #7
Johannes Sixt <j6t@kdbg.org> writes:

> Am 16.02.21 um 02:30 schrieb Junio C Hamano:
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>> 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.

Ditto.

The information on @@ ... @@ line may look misleading especially to
those who are not used to reading patches in the "-W wins" case.  It
is otherwise not hurting anybody and it loses information to remove
it, but it is not as grave as in the "-U<n> wins" case, so I do not
mind losing it, if it helps them.
diff mbox series

Patch

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.

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.

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.

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.

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,
 				      s1 - 1, funclineprev);
 			funclineprev = s1 - 1;
 		}