Re: [PATCH] diff: fix regression with --stat and unmerged file
- From
Jeff King <peff@peff.net>
- Date
- Dec 14, 2022, 21:37 UTC
- Message-ID
- <Y5pCHAWnNUUy6TW+@coredump.intra.peff.net>
- In-Reply-To
- <20221214174150.404821-1-pete@jpgrayson.net>
On Wed, Dec 14, 2022 at 12:41:51PM -0500, Peter Grayson wrote:
Show 6 quoted lines
> 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:
Show 13 quoted lines
> 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.
Show 17 quoted lines
> 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