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

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