{"thread":{"id":"60939","subject":"[BUG] git commit --trailer --verbose puts trailer below scissors line","startedAt":"2024-02-16T21:04:20Z","lastAt":"2024-03-08T02:21:33Z","messageCount":9,"participants":["Philippe Blain","Junio C Hamano","Linus Arver","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"488811","messageId":"8b4738ad-62cd-789e-712e-bd45a151b4ac@gmail.com","threadId":"60939","inReplyTo":null,"subject":"[BUG] git commit --trailer --verbose puts trailer below scissors line","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2024-02-16T21:04:18Z","receivedAt":"2024-02-16T21:04:20Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hello,\n\nI've just found a bug in the current master: running\n\n\tgit commit --trailer=\"key: value\" --verbose\n\nputs the trailer below the diff, and thus below the scissors\nline (and so it is discarded).\n\nI checked that it works OK in 2.42.1 but not in 2.43.2 so it is not\na new regression in the 2.44 cycle, but I thought I'd write now in case\nsomeone spots the culprit commit faster than me.\n\nI'll start a bisection now.\n\nCheers,\n\nPhilippe.\n"},{"id":"488813","messageId":"xmqqmss0gl9e.fsf@gitster.g","threadId":"60939","inReplyTo":"8b4738ad-62cd-789e-712e-bd45a151b4ac@gmail.com","subject":"Re: [BUG] git commit --trailer --verbose puts trailer below scissors line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-16T21:19:25Z","receivedAt":"2024-02-16T21:19:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philippe Blain <levraiphilippeblain@gmail.com> writes:\n\n> I checked that it works OK in 2.42.1 but not in 2.43.2 so it is not\n> a new regression in the 2.44 cycle, but I thought I'd write now in case\n> someone spots the culprit commit faster than me.\n\n2.43.2 has changes that are meant to be bugfix only without any new\nfeatures, taken from 2.44 cycle, so checking 2.43.0 may be worth\ndoing.\n"},{"id":"488820","messageId":"owlyeddc82d3.fsf@fine.c.googlers.com","threadId":"60939","inReplyTo":"8b4738ad-62cd-789e-712e-bd45a151b4ac@gmail.com","subject":"Re: [BUG] git commit --trailer --verbose puts trailer below scissors line","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-02-16T22:34:48Z","receivedAt":"2024-02-16T22:34:50Z","isPatch":false,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"Philippe Blain <levraiphilippeblain@gmail.com> writes:\n\n> Hello,\n>\n> I've just found a bug in the current master: running\n>\n> \tgit commit --trailer=\"key: value\" --verbose\n>\n> puts the trailer below the diff, and thus below the scissors\n> line (and so it is discarded).\n>\n> I checked that it works OK in 2.42.1 but not in 2.43.2 so it is not\n> a new regression in the 2.44 cycle, but I thought I'd write now in case\n> someone spots the culprit commit faster than me.\n>\n> I'll start a bisection now.\n>\n> Cheers,\n>\n> Philippe.\n\nThank you for the bug report. Looking forward to reviewing it (I am also\ninterested because I am in the middle of a large cleanup of\ntrailer.{c,h} [1]).\n\n[1] https://lore.kernel.org/git/owlyplwx87s9.fsf@fine.c.googlers.com/\n"},{"id":"488843","messageId":"20240217060434.GE539459@coredump.intra.peff.net","threadId":"60939","inReplyTo":"8b4738ad-62cd-789e-712e-bd45a151b4ac@gmail.com","subject":"Re: [BUG] git commit --trailer --verbose puts trailer below scissors line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-17T06:04:34Z","receivedAt":"2024-02-17T06:04:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 16, 2024 at 04:04:18PM -0500, Philippe Blain wrote:\n\n> Hello,\n> \n> I've just found a bug in the current master: running\n> \n> \tgit commit --trailer=\"key: value\" --verbose\n> \n> puts the trailer below the diff, and thus below the scissors\n> line (and so it is discarded).\n> \n> I checked that it works OK in 2.42.1 but not in 2.43.2 so it is not\n> a new regression in the 2.44 cycle, but I thought I'd write now in case\n> someone spots the culprit commit faster than me.\n> \n> I'll start a bisection now.\n\nLooks like it bisects to 97e9d0b78a (trailer: find the end of the log\nmessage, 2023-10-20). Here's a test that demonstrates it (signed-off-by:\nme in case anyone wants to incorporate it into a fix):\n\ndiff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh\nindex b5bf7de7cd..d8e216613f 100755\n--- a/t/t7502-commit-porcelain.sh\n+++ b/t/t7502-commit-porcelain.sh\n@@ -485,6 +485,24 @@ test_expect_success 'commit --trailer not confused by --- separator' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'commit --trailer with --verbose' '\n+\tcat >msg <<-\\EOF &&\n+\tsubject\n+\n+\tbody\n+\tEOF\n+\tGIT_EDITOR=: git commit --edit -F msg --allow-empty \\\n+\t\t--trailer=\"my-trailer: value\" --verbose &&\n+\t{\n+\t\tcat msg &&\n+\t\techo &&\n+\t\techo \"my-trailer: value\"\n+\t} >expected &&\n+\tgit cat-file commit HEAD >commit.msg &&\n+\tsed -e \"1,/^\\$/d\" commit.msg >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'multiple -m' '\n \n \t>negative &&\n\n-Peff\n"},{"id":"488919","messageId":"ca6a73d3-58b4-e65c-4a8f-e6ddb3e86902@gmail.com","threadId":"60939","inReplyTo":"20240217060434.GE539459@coredump.intra.peff.net","subject":"Re: [BUG] git commit --trailer --verbose puts trailer below scissors line","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2024-02-19T17:41:13Z","receivedAt":"2024-02-19T17:41:15Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Peff,\n\nLe 2024-02-17 à 01:04, Jeff King a écrit :\n> On Fri, Feb 16, 2024 at 04:04:18PM -0500, Philippe Blain wrote:\n> \n>> Hello,\n>>\n>> I've just found a bug in the current master: running\n>>\n>> \tgit commit --trailer=\"key: value\" --verbose\n>>\n>> puts the trailer below the diff, and thus below the scissors\n>> line (and so it is discarded).\n>>\n>> I checked that it works OK in 2.42.1 but not in 2.43.2 so it is not\n>> a new regression in the 2.44 cycle, but I thought I'd write now in case\n>> someone spots the culprit commit faster than me.\n>>\n>> I'll start a bisection now.\n> \n> Looks like it bisects to 97e9d0b78a (trailer: find the end of the log\n> message, 2023-10-20). Here's a test that demonstrates it (signed-off-by:\n> me in case anyone wants to incorporate it into a fix):\n\n\nYes, this is also what I found - not without fighting a bit with 'git bisect' though, \nbut I'll start a new thread for that. \n\nSo, it is indeed a regression in the 2.44 cycle.\n\n> \n> diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh\n> index b5bf7de7cd..d8e216613f 100755\n> --- a/t/t7502-commit-porcelain.sh\n> +++ b/t/t7502-commit-porcelain.sh\n> @@ -485,6 +485,24 @@ test_expect_success 'commit --trailer not confused by --- separator' '\n>  \ttest_cmp expected actual\n>  '\n>  \n> +test_expect_success 'commit --trailer with --verbose' '\n> +\tcat >msg <<-\\EOF &&\n> +\tsubject\n> +\n> +\tbody\n> +\tEOF\n> +\tGIT_EDITOR=: git commit --edit -F msg --allow-empty \\\n> +\t\t--trailer=\"my-trailer: value\" --verbose &&\n> +\t{\n> +\t\tcat msg &&\n> +\t\techo &&\n> +\t\techo \"my-trailer: value\"\n> +\t} >expected &&\n> +\tgit cat-file commit HEAD >commit.msg &&\n> +\tsed -e \"1,/^\\$/d\" commit.msg >actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n>  test_expect_success 'multiple -m' '\n>  \n>  \t>negative &&\n\nThanks for the test case!\n\nPhilippe.\n"},{"id":"488920","messageId":"xmqqfrxo9ty2.fsf@gitster.g","threadId":"60939","inReplyTo":"ca6a73d3-58b4-e65c-4a8f-e6ddb3e86902@gmail.com","subject":"Re: [BUG] git commit --trailer --verbose puts trailer below scissors line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-19T18:42:45Z","receivedAt":"2024-02-19T18:42:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philippe Blain <levraiphilippeblain@gmail.com> writes:\n\n> Hi Peff,\n>> ...\n>> diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh\n>> index b5bf7de7cd..d8e216613f 100755\n>> --- a/t/t7502-commit-porcelain.sh\n>> +++ b/t/t7502-commit-porcelain.sh\n>> @@ -485,6 +485,24 @@ test_expect_success 'commit --trailer not confused by --- separator' '\n>>  \ttest_cmp expected actual\n>>  '\n>>  \n>> +test_expect_success 'commit --trailer with --verbose' '\n>> +\tcat >msg <<-\\EOF &&\n>> +\tsubject\n>> +\n>> +\tbody\n>> +\tEOF\n>> +\tGIT_EDITOR=: git commit --edit -F msg --allow-empty \\\n>> +\t\t--trailer=\"my-trailer: value\" --verbose &&\n>> +\t{\n>> +\t\tcat msg &&\n>> +\t\techo &&\n>> +\t\techo \"my-trailer: value\"\n>> +\t} >expected &&\n>> +\tgit cat-file commit HEAD >commit.msg &&\n>> +\tsed -e \"1,/^\\$/d\" commit.msg >actual &&\n>> +\ttest_cmp expected actual\n>> +'\n>> +\n>>  test_expect_success 'multiple -m' '\n>>  \n>>  \t>negative &&\n>\n> Thanks for the test case!\n>\n> Philippe.\n\nThanks, both, for finding a rather unfortunate regression.  Perhaps\nit is worth delaying 2.44 final by a week or so to include a fix (or\na revert if it comes to it).\n\n\n"},{"id":"488945","messageId":"20240220010936.GA1793660@coredump.intra.peff.net","threadId":"60939","inReplyTo":"xmqqfrxo9ty2.fsf@gitster.g","subject":"[PATCH] trailer: fix comment/cut-line regression with opts->no_divider","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-20T01:09:36Z","receivedAt":"2024-02-20T01:09:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 19, 2024 at 10:42:45AM -0800, Junio C Hamano wrote:\n\n> Thanks, both, for finding a rather unfortunate regression.  Perhaps\n> it is worth delaying 2.44 final by a week or so to include a fix (or\n> a revert if it comes to it).\n\nHmm, I had thought this was pre-2.44, but it was actually in the 2.43.x\nmaintenance series (so it is not a regression going from 2.43.2 to\n2.44.0, but it is from 2.43.0 to 2.44.0).\n\nAnyway, the fix is pretty simple, so I think it may be OK to apply it\nfor 2.44.0-rc2. Here it is (prepared on top of la/trailer-cleanups, so\nit could also be merged down for a v2.43.3 if you want to).\n\n-- >8 --\nSubject: trailer: fix comment/cut-line regression with opts->no_divider\n\nCommit 97e9d0b78a (trailer: find the end of the log message, 2023-10-20)\ncombined two code paths for finding the end of the log message. For the\n\"no_divider\" case, we used to use find_trailer_end(), and that has now\nbeen rolled into find_end_of_log_message(). But there's a regression;\nthat function returns early when no_divider is set, returning the whole\nstring.\n\nThat's not how find_trailer_end() behaved. Although it did skip the\n\"---\" processing (which is what \"no_divider\" is meant to do), we should\nstill respect ignored_log_message_bytes(), which covers things like\ncomments, \"commit -v\" cut lines, and so on.\n\nThe bug is actually in the interpret-trailers command, but the obvious\nway to experience it is by running \"commit -v\" with a \"--trailer\"\noption. The new trailer will be added at the end of the verbose diff,\nrather than before it (and consequently will be ignored entirely, since\neverything after the diff's intro scissors line is thrown away).\n\nI've added two tests here: one for interpret-trailers directly, which\nshows the bug via the parsing routines, and one for \"commit -v\".\n\nThe fix itself is pretty simple: instead of returning early, no_divider\njust skips the \"---\" handling but still calls ignored_log_message_bytes().\n\nReported-by: Philippe Blain <levraiphilippeblain@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nViewing the diff with \"-w\" makes the change a little more obvious.\n\n t/t7502-commit-porcelain.sh   | 18 ++++++++++++++++++\n t/t7513-interpret-trailers.sh | 19 +++++++++++++++++++\n trailer.c                     | 15 +++++++--------\n 3 files changed, 44 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh\nindex b5bf7de7cd..d8e216613f 100755\n--- a/t/t7502-commit-porcelain.sh\n+++ b/t/t7502-commit-porcelain.sh\n@@ -485,6 +485,24 @@ test_expect_success 'commit --trailer not confused by --- separator' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'commit --trailer with --verbose' '\n+\tcat >msg <<-\\EOF &&\n+\tsubject\n+\n+\tbody\n+\tEOF\n+\tGIT_EDITOR=: git commit --edit -F msg --allow-empty \\\n+\t\t--trailer=\"my-trailer: value\" --verbose &&\n+\t{\n+\t\tcat msg &&\n+\t\techo &&\n+\t\techo \"my-trailer: value\"\n+\t} >expected &&\n+\tgit cat-file commit HEAD >commit.msg &&\n+\tsed -e \"1,/^\\$/d\" commit.msg >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'multiple -m' '\n \n \t>negative &&\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex 97f10905d2..3343ad0eb6 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -1560,4 +1560,23 @@ test_expect_success 'suppress --- handling' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'suppressing --- does not disable cut-line handling' '\n+\techo \"real-trailer: before the cut\" >expected &&\n+\n+\tgit interpret-trailers --parse --no-divider >actual <<-\\EOF &&\n+\tsubject\n+\n+\tThis input has a cut-line in it; we should stop parsing when we see it\n+\tand consider only trailers before that line.\n+\n+\treal-trailer: before the cut\n+\n+\t# ------------------------ >8 ------------------------\n+\t# Nothing below this line counts as part of the commit message.\n+\tnot-a-trailer: too late\n+\tEOF\n+\n+\ttest_cmp expected actual\n+'\n+\n test_done\ndiff --git a/trailer.c b/trailer.c\nindex 816f8b28a9..009ee80dee 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -837,16 +837,15 @@ static size_t find_end_of_log_message(const char *input, int no_divider)\n \t/* Assume the naive end of the input is already what we want. */\n \tend = strlen(input);\n \n-\tif (no_divider)\n-\t\treturn end;\n-\n \t/* Optionally skip over any patch part (\"---\" line and below). */\n-\tfor (s = input; *s; s = next_line(s)) {\n-\t\tconst char *v;\n+\tif (!no_divider) {\n+\t\tfor (s = input; *s; s = next_line(s)) {\n+\t\t\tconst char *v;\n \n-\t\tif (skip_prefix(s, \"---\", &v) && isspace(*v)) {\n-\t\t\tend = s - input;\n-\t\t\tbreak;\n+\t\t\tif (skip_prefix(s, \"---\", &v) && isspace(*v)) {\n+\t\t\t\tend = s - input;\n+\t\t\t\tbreak;\n+\t\t\t}\n \t\t}\n \t}\n \n-- \n2.44.0.rc1.413.gdc9df0ba3d\n\n"},{"id":"488957","messageId":"xmqqttm3x1cc.fsf@gitster.g","threadId":"60939","inReplyTo":"20240220010936.GA1793660@coredump.intra.peff.net","subject":"Re: [PATCH] trailer: fix comment/cut-line regression with opts->no_divider","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-20T03:26:43Z","receivedAt":"2024-02-20T03:26:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Feb 19, 2024 at 10:42:45AM -0800, Junio C Hamano wrote:\n>\n>> Thanks, both, for finding a rather unfortunate regression.  Perhaps\n>> it is worth delaying 2.44 final by a week or so to include a fix (or\n>> a revert if it comes to it).\n>\n> Hmm, I had thought this was pre-2.44, but it was actually in the 2.43.x\n> maintenance series (so it is not a regression going from 2.43.2 to\n> 2.44.0, but it is from 2.43.0 to 2.44.0).\n\nI've been trying to be a bit aggressive this cycle to merge various\nclean-up topics, together with real bugfixes, to 'maint'.  Those who\noften skip the -rcX but diligently follow numbered releases found a\nfew potential regressions in 2.44-rc as a result, which I could say\nis a great success ;-).  In addition, I was planning to have only\none -rc release without going -rc2 before the final, but we may need\none if only for this fix.\n\nThe fix in the patch looks quite straight-forward.  Thanks.\n"},{"id":"490231","messageId":"owlyfrx14g63.fsf@fine.c.googlers.com","threadId":"60939","inReplyTo":"20240220010936.GA1793660@coredump.intra.peff.net","subject":"Re: [PATCH] trailer: fix comment/cut-line regression with opts->no_divider","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-03-08T02:21:24Z","receivedAt":"2024-03-08T02:21:33Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Feb 19, 2024 at 10:42:45AM -0800, Junio C Hamano wrote:\n> [...]\n>\n> The fix itself is pretty simple: instead of returning early, no_divider\n> just skips the \"---\" handling but still calls ignored_log_message_bytes().\n\nI realize I am late to the discussion, but this fix (and patch)\nlooks right to me. FWIW I independently discovered the same problem and\nfigured out a fix locally in my larger refactor of this area (with the\nsame fix, to always call ignored_log_message_bytes() regardless of\nno_divider). Thank you Peff!\n\nSorry for introducing the regression. My enthusiasm in changing things\nup without unit tests is regrettable.\n"}]}