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
