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

From: "Peter Grayson" <pete@jpgrayson.net>
To: "Jeff King" <peff@peff.net>
Cc: git <git@vger.kernel.org>
Subject: Re: [PATCH] diff: fix regression with --stat and unmerged file
Date: Wed, 14 Dec 2022 17:01:06 -0500	[thread overview]
Message-ID: <71f44490-6ee9-4208-92a4-5932b524d719@app.fastmail.com> (raw)
In-Reply-To: <Y5pCHAWnNUUy6TW+@coredump.intra.peff.net>

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