[PATCH] diff: fix regression with --stat and unmerged file

[PATCH] diff: fix regression with --stat and unmerged file

[Peter Grayson]

A regression was introduced in

12fc4ad89e (diff.c: use utf8_strwidth() to count display width, 2022-09-14)

that causes missing newlines after "Unmerged" entries in `git diff --cached
--stat` output.

This problem affects v2.39.0-rc0 through v2.39.0.

This patch adds the missing newline along with a new test to cover this
behavior.

Signed-off-by: Peter Grayson <pete@jpgrayson.net>
---
 diff.c                   |  2 +-
 t/t4046-diff-unmerged.sh | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 1054a4b732..85f035a9e8 100644
--- a/diff.c
+++ b/diff.c
@@ -2801,7 +2801,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 		else if (file->is_unmerged) {
 			strbuf_addf(&out, " %s%s%*s | %*s",
 				    prefix, name, padding, "",
-				    number_width, "Unmerged");
+				    number_width, "Unmerged\n");
 			emit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,
 					 out.buf, out.len, 0);
 			strbuf_reset(&out);
diff --git a/t/t4046-diff-unmerged.sh b/t/t4046-diff-unmerged.sh
index 0ae0cd3a52..ffaf69335f 100755
--- a/t/t4046-diff-unmerged.sh
+++ b/t/t4046-diff-unmerged.sh
@@ -86,4 +86,14 @@ test_expect_success 'diff-files -3' '
 	test_cmp diff-files-3.expect diff-files-3.actual
 '
 
+test_expect_success 'diff --stat' '
+	for path in $paths
+	do
+		echo " $path | Unmerged" || return 1
+	done >diff-stat.expect &&
+	echo " 0 files changed" >>diff-stat.expect &&
+	git diff --cached --stat >diff-stat.actual &&
+	test_cmp diff-stat.expect diff-stat.actual
+'
+
 test_done
-- 
2.39.0

Re: [PATCH] diff: fix regression with --stat and unmerged file

[Jeff King]

On Wed, Dec 14, 2022 at 12:41:51PM -0500, Peter Grayson wrote:

> A regression was introduced in
> 
> 12fc4ad89e (diff.c: use utf8_strwidth() to count display width, 2022-09-14)
> 
> that causes missing newlines after "Unmerged" entries in `git diff --cached
> --stat` output.

Oof, clearly we don't have good test coverage here. Thanks for catching
it.

> This patch adds the missing newline along with a new test to cover this
> behavior.

Both look good to me, but two quick comments:

> diff --git a/diff.c b/diff.c
> index 1054a4b732..85f035a9e8 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -2801,7 +2801,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>  		else if (file->is_unmerged) {
>  			strbuf_addf(&out, " %s%s%*s | %*s",
>  				    prefix, name, padding, "",
> -				    number_width, "Unmerged");
> +				    number_width, "Unmerged\n");
>  			emit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,
>  					 out.buf, out.len, 0);
>  			strbuf_reset(&out);

Looking at the offending patch, it also touches "Bin". But that one
handles its newline separately (since sometimes there is more data on
the line). So this is the only spot that needs to be fixed.

> diff --git a/t/t4046-diff-unmerged.sh b/t/t4046-diff-unmerged.sh
> index 0ae0cd3a52..ffaf69335f 100755
> --- a/t/t4046-diff-unmerged.sh
> +++ b/t/t4046-diff-unmerged.sh
> @@ -86,4 +86,14 @@ test_expect_success 'diff-files -3' '
>  	test_cmp diff-files-3.expect diff-files-3.actual
>  '
>  
> +test_expect_success 'diff --stat' '
> +	for path in $paths
> +	do
> +		echo " $path | Unmerged" || return 1
> +	done >diff-stat.expect &&
> +	echo " 0 files changed" >>diff-stat.expect &&
> +	git diff --cached --stat >diff-stat.actual &&
> +	test_cmp diff-stat.expect diff-stat.actual
> +'

The rest of this script uses diff-files, but here we're using "diff
--cached". It feels like it would be simple to use diff-files for
consistency, but strangely it errors out, complaining that the blob
can't be read.

This has to do with the setup for the test, which uses "hash-object"
without "-w", meaning our index mentions objects we don't actually have.
I'm not sure if this is the test trying to cleverly assert that we don't
look at the objects themselves, but regardless it seems weird to me that
"diff-files" wants to look at the unmerged entries but "diff --cached"
doesn't (it does seem like "diff --cached" is right here; diff-files
would produce two lines).

So there may be another bug, but if so I don't think it's a recent one.
And we are better off working around it for your regression fix, which
we'd hope to see merged quickly.

-Peff

Re: [PATCH] diff: fix regression with --stat and unmerged file

[Peter Grayson]

On Wed, Dec 14, 2022, at 4:37 PM, Jeff King wrote:
> On Wed, Dec 14, 2022 at 12:41:51PM -0500, Peter Grayson wrote:
>
>> A regression was introduced in
>> 
>> 12fc4ad89e (diff.c: use utf8_strwidth() to count display width, 2022-09-14)
>> 
>> that causes missing newlines after "Unmerged" entries in `git diff --cached
>> --stat` output.
>
> Oof, clearly we don't have good test coverage here. Thanks for catching
> it.
>
>> This patch adds the missing newline along with a new test to cover this
>> behavior.
>
> Both look good to me, but two quick comments:
>
>> diff --git a/diff.c b/diff.c
>> index 1054a4b732..85f035a9e8 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -2801,7 +2801,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>>  		else if (file->is_unmerged) {
>>  			strbuf_addf(&out, " %s%s%*s | %*s",
>>  				    prefix, name, padding, "",
>> -				    number_width, "Unmerged");
>> +				    number_width, "Unmerged\n");
>>  			emit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,
>>  					 out.buf, out.len, 0);
>>  			strbuf_reset(&out);
>
> Looking at the offending patch, it also touches "Bin". But that one
> handles its newline separately (since sometimes there is more data on
> the line). So this is the only spot that needs to be fixed.

Agreed.

>> diff --git a/t/t4046-diff-unmerged.sh b/t/t4046-diff-unmerged.sh
>> index 0ae0cd3a52..ffaf69335f 100755
>> --- a/t/t4046-diff-unmerged.sh
>> +++ b/t/t4046-diff-unmerged.sh
>> @@ -86,4 +86,14 @@ test_expect_success 'diff-files -3' '
>>  	test_cmp diff-files-3.expect diff-files-3.actual
>>  '
>>  
>> +test_expect_success 'diff --stat' '
>> +	for path in $paths
>> +	do
>> +		echo " $path | Unmerged" || return 1
>> +	done >diff-stat.expect &&
>> +	echo " 0 files changed" >>diff-stat.expect &&
>> +	git diff --cached --stat >diff-stat.actual &&
>> +	test_cmp diff-stat.expect diff-stat.actual
>> +'
>
> The rest of this script uses diff-files, but here we're using "diff
> --cached". It feels like it would be simple to use diff-files for
> consistency, but strangely it errors out, complaining that the blob
> can't be read.
>
> This has to do with the setup for the test, which uses "hash-object"
> without "-w", meaning our index mentions objects we don't actually have.
> I'm not sure if this is the test trying to cleverly assert that we don't
> look at the objects themselves, but regardless it seems weird to me that
> "diff-files" wants to look at the unmerged entries but "diff --cached"
> doesn't (it does seem like "diff --cached" is right here; diff-files
> would produce two lines).

I also noticed this when I originally wrote the test using diff-files,
but didn't dig deeper. Thanks for taking the next step with that. I
figure that the "diff" porcelain is a good vehicle to test this
particular behavior even if its a bit inconsistent with other cases
in this test module.

> So there may be another bug, but if so I don't think it's a recent one.
> And we are better off working around it for your regression fix, which
> we'd hope to see merged quickly.

I would also like to see this repair for the clear-and-present
regression merged sooner and independent of exploration of the
other potential problem with diff-files.

Thanks for taking the time to review this change. I appreciate it!

Pete