{"thread":{"id":"58952","subject":"[PATCH] diff: fix regression with --stat and unmerged file","startedAt":"2022-12-14T17:42:39Z","lastAt":"2022-12-14T22:01:15Z","messageCount":3,"participants":["Peter Grayson","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"469024","messageId":"20221214174150.404821-1-pete@jpgrayson.net","threadId":"58952","inReplyTo":null,"subject":"[PATCH] diff: fix regression with --stat and unmerged file","fromName":"Peter Grayson","fromEmail":"pete@jpgrayson.net","sentAt":"2022-12-14T17:41:51Z","receivedAt":"2022-12-14T17:42:39Z","isPatch":true,"sender":{"key":"pete@jpgrayson.net","avatar":"https://avatars.githubusercontent.com/u/1924182?v=4"},"body":"A regression was introduced in\n\n12fc4ad89e (diff.c: use utf8_strwidth() to count display width, 2022-09-14)\n\nthat causes missing newlines after \"Unmerged\" entries in `git diff --cached\n--stat` output.\n\nThis problem affects v2.39.0-rc0 through v2.39.0.\n\nThis patch adds the missing newline along with a new test to cover this\nbehavior.\n\nSigned-off-by: Peter Grayson <pete@jpgrayson.net>\n---\n diff.c                   |  2 +-\n t/t4046-diff-unmerged.sh | 10 ++++++++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 1054a4b732..85f035a9e8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2801,7 +2801,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\telse if (file->is_unmerged) {\n \t\t\tstrbuf_addf(&out, \" %s%s%*s | %*s\",\n \t\t\t\t    prefix, name, padding, \"\",\n-\t\t\t\t    number_width, \"Unmerged\");\n+\t\t\t\t    number_width, \"Unmerged\\n\");\n \t\t\temit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,\n \t\t\t\t\t out.buf, out.len, 0);\n \t\t\tstrbuf_reset(&out);\ndiff --git a/t/t4046-diff-unmerged.sh b/t/t4046-diff-unmerged.sh\nindex 0ae0cd3a52..ffaf69335f 100755\n--- a/t/t4046-diff-unmerged.sh\n+++ b/t/t4046-diff-unmerged.sh\n@@ -86,4 +86,14 @@ test_expect_success 'diff-files -3' '\n \ttest_cmp diff-files-3.expect diff-files-3.actual\n '\n \n+test_expect_success 'diff --stat' '\n+\tfor path in $paths\n+\tdo\n+\t\techo \" $path | Unmerged\" || return 1\n+\tdone >diff-stat.expect &&\n+\techo \" 0 files changed\" >>diff-stat.expect &&\n+\tgit diff --cached --stat >diff-stat.actual &&\n+\ttest_cmp diff-stat.expect diff-stat.actual\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"469045","messageId":"Y5pCHAWnNUUy6TW+@coredump.intra.peff.net","threadId":"58952","inReplyTo":"20221214174150.404821-1-pete@jpgrayson.net","subject":"Re: [PATCH] diff: fix regression with --stat and unmerged file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-14T21:37:32Z","receivedAt":"2022-12-14T21:37:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 14, 2022 at 12:41:51PM -0500, Peter Grayson wrote:\n\n> A regression was introduced in\n> \n> 12fc4ad89e (diff.c: use utf8_strwidth() to count display width, 2022-09-14)\n> \n> that causes missing newlines after \"Unmerged\" entries in `git diff --cached\n> --stat` output.\n\nOof, clearly we don't have good test coverage here. Thanks for catching\nit.\n\n> This patch adds the missing newline along with a new test to cover this\n> behavior.\n\nBoth look good to me, but two quick comments:\n\n> diff --git a/diff.c b/diff.c\n> index 1054a4b732..85f035a9e8 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2801,7 +2801,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n>  \t\telse if (file->is_unmerged) {\n>  \t\t\tstrbuf_addf(&out, \" %s%s%*s | %*s\",\n>  \t\t\t\t    prefix, name, padding, \"\",\n> -\t\t\t\t    number_width, \"Unmerged\");\n> +\t\t\t\t    number_width, \"Unmerged\\n\");\n>  \t\t\temit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,\n>  \t\t\t\t\t out.buf, out.len, 0);\n>  \t\t\tstrbuf_reset(&out);\n\nLooking at the offending patch, it also touches \"Bin\". But that one\nhandles its newline separately (since sometimes there is more data on\nthe line). So this is the only spot that needs to be fixed.\n\n> diff --git a/t/t4046-diff-unmerged.sh b/t/t4046-diff-unmerged.sh\n> index 0ae0cd3a52..ffaf69335f 100755\n> --- a/t/t4046-diff-unmerged.sh\n> +++ b/t/t4046-diff-unmerged.sh\n> @@ -86,4 +86,14 @@ test_expect_success 'diff-files -3' '\n>  \ttest_cmp diff-files-3.expect diff-files-3.actual\n>  '\n>  \n> +test_expect_success 'diff --stat' '\n> +\tfor path in $paths\n> +\tdo\n> +\t\techo \" $path | Unmerged\" || return 1\n> +\tdone >diff-stat.expect &&\n> +\techo \" 0 files changed\" >>diff-stat.expect &&\n> +\tgit diff --cached --stat >diff-stat.actual &&\n> +\ttest_cmp diff-stat.expect diff-stat.actual\n> +'\n\nThe rest of this script uses diff-files, but here we're using \"diff\n--cached\". It feels like it would be simple to use diff-files for\nconsistency, but strangely it errors out, complaining that the blob\ncan't be read.\n\nThis has to do with the setup for the test, which uses \"hash-object\"\nwithout \"-w\", meaning our index mentions objects we don't actually have.\nI'm not sure if this is the test trying to cleverly assert that we don't\nlook at the objects themselves, but regardless it seems weird to me that\n\"diff-files\" wants to look at the unmerged entries but \"diff --cached\"\ndoesn't (it does seem like \"diff --cached\" is right here; diff-files\nwould produce two lines).\n\nSo there may be another bug, but if so I don't think it's a recent one.\nAnd we are better off working around it for your regression fix, which\nwe'd hope to see merged quickly.\n\n-Peff\n"},{"id":"469046","messageId":"71f44490-6ee9-4208-92a4-5932b524d719@app.fastmail.com","threadId":"58952","inReplyTo":"Y5pCHAWnNUUy6TW+@coredump.intra.peff.net","subject":"Re: [PATCH] diff: fix regression with --stat and unmerged file","fromName":"Peter Grayson","fromEmail":"pete@jpgrayson.net","sentAt":"2022-12-14T22:01:06Z","receivedAt":"2022-12-14T22:01:15Z","isPatch":true,"sender":{"key":"pete@jpgrayson.net","avatar":"https://avatars.githubusercontent.com/u/1924182?v=4"},"body":"On Wed, Dec 14, 2022, at 4:37 PM, Jeff King wrote:\n> On Wed, Dec 14, 2022 at 12:41:51PM -0500, Peter Grayson wrote:\n>\n>> A regression was introduced in\n>> \n>> 12fc4ad89e (diff.c: use utf8_strwidth() to count display width, 2022-09-14)\n>> \n>> that causes missing newlines after \"Unmerged\" entries in `git diff --cached\n>> --stat` output.\n>\n> Oof, clearly we don't have good test coverage here. Thanks for catching\n> it.\n>\n>> This patch adds the missing newline along with a new test to cover this\n>> behavior.\n>\n> Both look good to me, but two quick comments:\n>\n>> diff --git a/diff.c b/diff.c\n>> index 1054a4b732..85f035a9e8 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -2801,7 +2801,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n>>  \t\telse if (file->is_unmerged) {\n>>  \t\t\tstrbuf_addf(&out, \" %s%s%*s | %*s\",\n>>  \t\t\t\t    prefix, name, padding, \"\",\n>> -\t\t\t\t    number_width, \"Unmerged\");\n>> +\t\t\t\t    number_width, \"Unmerged\\n\");\n>>  \t\t\temit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,\n>>  \t\t\t\t\t out.buf, out.len, 0);\n>>  \t\t\tstrbuf_reset(&out);\n>\n> Looking at the offending patch, it also touches \"Bin\". But that one\n> handles its newline separately (since sometimes there is more data on\n> the line). So this is the only spot that needs to be fixed.\n\nAgreed.\n\n>> diff --git a/t/t4046-diff-unmerged.sh b/t/t4046-diff-unmerged.sh\n>> index 0ae0cd3a52..ffaf69335f 100755\n>> --- a/t/t4046-diff-unmerged.sh\n>> +++ b/t/t4046-diff-unmerged.sh\n>> @@ -86,4 +86,14 @@ test_expect_success 'diff-files -3' '\n>>  \ttest_cmp diff-files-3.expect diff-files-3.actual\n>>  '\n>>  \n>> +test_expect_success 'diff --stat' '\n>> +\tfor path in $paths\n>> +\tdo\n>> +\t\techo \" $path | Unmerged\" || return 1\n>> +\tdone >diff-stat.expect &&\n>> +\techo \" 0 files changed\" >>diff-stat.expect &&\n>> +\tgit diff --cached --stat >diff-stat.actual &&\n>> +\ttest_cmp diff-stat.expect diff-stat.actual\n>> +'\n>\n> The rest of this script uses diff-files, but here we're using \"diff\n> --cached\". It feels like it would be simple to use diff-files for\n> consistency, but strangely it errors out, complaining that the blob\n> can't be read.\n>\n> This has to do with the setup for the test, which uses \"hash-object\"\n> without \"-w\", meaning our index mentions objects we don't actually have.\n> I'm not sure if this is the test trying to cleverly assert that we don't\n> look at the objects themselves, but regardless it seems weird to me that\n> \"diff-files\" wants to look at the unmerged entries but \"diff --cached\"\n> doesn't (it does seem like \"diff --cached\" is right here; diff-files\n> would produce two lines).\n\nI also noticed this when I originally wrote the test using diff-files,\nbut didn't dig deeper. Thanks for taking the next step with that. I\nfigure that the \"diff\" porcelain is a good vehicle to test this\nparticular behavior even if its a bit inconsistent with other cases\nin this test module.\n\n> So there may be another bug, but if so I don't think it's a recent one.\n> And we are better off working around it for your regression fix, which\n> we'd hope to see merged quickly.\n\nI would also like to see this repair for the clear-and-present\nregression merged sooner and independent of exploration of the\nother potential problem with diff-files.\n\nThanks for taking the time to review this change. I appreciate it!\n\nPete\n"}]}