{"thread":{"id":"45771","subject":"[GIT 2.12.2 REGRESSION] git cherry-pick -x","startedAt":"2017-04-21T22:02:09Z","lastAt":"2017-04-26T20:50:15Z","messageCount":18,"participants":["Brian Norris","Stefan Beller","Jeff King","Jonathan Tan","Johannes Schindelin","Jonathan Nieder","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"317523","messageId":"20170421220155.GA142345@google.com","threadId":"45771","inReplyTo":null,"subject":"[GIT 2.12.2 REGRESSION] git cherry-pick -x","fromName":"Brian Norris","fromEmail":"computersforpeace@gmail.com","sentAt":"2017-04-21T22:01:55Z","receivedAt":"2017-04-21T22:02:09Z","isPatch":false,"sender":{"key":"computersforpeace@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2268082?v=4"},"body":"Hi all,\n\nI haven't tried bisecting precisely, but somewhere along the way git\ncherry-pick -x regressed between 2.9.3 and 2.12.2.\n\nLooking at upstream linux.git, I can do:\n\ngit fetch git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next\ngit cherry-pick -x 7b309aef0463340d3ad5449d1f605d14e10a4225\n\nAnd see the following ending to the new commit:\n\nAcked-by: Arnd Bergmann (cherry picked from commit 7b309aef0463340d3ad5449d1f605d14e10a4225)\n\nOn 2.9.3, this worked fine:\n\nAcked-by: Arnd Bergmann \n(cherry picked from commit 7b309aef0463340d3ad5449d1f605d14e10a4225)\n\nThis issue doesn't happen on all commits; maybe it's bad parsing looking\nat 'Signed-off' and the like?\n\nRegards,\nBrian\n"},{"id":"317526","messageId":"CAGZ79kYBgvzkO66pL3buffAChKtPu2Xt5Q4FUoiijYHj591aCg@mail.gmail.com","threadId":"45771","inReplyTo":"20170421220155.GA142345@google.com","subject":"Re: [GIT 2.12.2 REGRESSION] git cherry-pick -x","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-21T22:10:03Z","receivedAt":"2017-04-21T22:10:09Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"+Jonathan, who worked on trailers\n\nOn Fri, Apr 21, 2017 at 3:01 PM, Brian Norris\n wrote:\n> Hi all,\n>\n> I haven't tried bisecting precisely, but somewhere along the way git\n> cherry-pick -x regressed between 2.9.3 and 2.12.2.\n>\n> Looking at upstream linux.git, I can do:\n>\n> git fetch git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next\n> git cherry-pick -x 7b309aef0463340d3ad5449d1f605d14e10a4225\n>\n> And see the following ending to the new commit:\n>\n> Acked-by: Arnd Bergmann (cherry picked from commit 7b309aef0463340d3ad5449d1f605d14e10a4225)\n>\n> On 2.9.3, this worked fine:\n>\n> Acked-by: Arnd Bergmann \n> (cherry picked from commit 7b309aef0463340d3ad5449d1f605d14e10a4225)\n>\n> This issue doesn't happen on all commits; maybe it's bad parsing looking\n> at 'Signed-off' and the like?\n>\n> Regards,\n> Brian\n"},{"id":"317527","messageId":"20170421221350.xlhhrsk7m2bxh4tz@sigill.intra.peff.net","threadId":"45771","inReplyTo":"CAGZ79kYBgvzkO66pL3buffAChKtPu2Xt5Q4FUoiijYHj591aCg@mail.gmail.com","subject":"Re: [GIT 2.12.2 REGRESSION] git cherry-pick -x","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-21T22:13:50Z","receivedAt":"2017-04-21T22:13:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 21, 2017 at 03:10:03PM -0700, Stefan Beller wrote:\n\n> +Jonathan, who worked on trailers\n> [...]\n> > I haven't tried bisecting precisely, but somewhere along the way git\n> > cherry-pick -x regressed between 2.9.3 and 2.12.2.\n\nYeah, it bisects to 967dfd4d5 (sequencer: use trailer's trailer layout,\n2016-11-02).\n\n-Peff\n"},{"id":"317830","messageId":"20170425190651.8910-1-jonathantanmy@google.com","threadId":"45771","inReplyTo":"20170421220155.GA142345@google.com","subject":"[PATCH] sequencer: require trailing NL in footers","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-04-25T19:06:51Z","receivedAt":"2017-04-25T19:07:10Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In commit 967dfd4 (\"sequencer: use trailer's trailer layout\",\n2016-11-29), sequencer was taught to use the same mechanism as\ninterpret-trailers to determine the nature of the trailer of a commit\nmessage (referred to as the \"footer\" in sequencer.c). However, the\nrequirement that a footer end in a newline character was inadvertently\nremoved. Restore that requirement.\n\nWhile writing this commit, I noticed that if the \"ignore_footer\"\nparameter in \"has_conforming_footer\" is greater than the distance\nbetween the trailer start and sb->len, \"has_conforming_footer\" will\nreturn an unexpected result. This does not occur in practice, because\n\"ignore_footer\" is either zero or the return value of an invocation to\n\"ignore_non_trailer\", which only skips empty lines and comment lines.\nThis commit contains a comment explaining this in the function's\ndocumentation.\n\nReported-by: Brian Norris \nSigned-off-by: Jonathan Tan \n---\n\nThanks for the bug report. Here's a fix - I've verified this with the\nway to reproduce provided in the original e-mail, and it seems to work\nnow.\n\nThe commit message of the referenced commit\n(7b309aef0463340d3ad5449d1f605d14e10a4225) does not end in a newline,\nwhich is probably why different behavior was observed with this commit\n(as compared to others).\n\n sequencer.c | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 77afecaeb..4cbe64b7f 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -151,6 +151,12 @@ static const char *get_todo_path(const struct replay_opts *opts)\n * Returns 1 for conforming footer\n * Returns 2 when sob exists within conforming footer\n * Returns 3 when sob exists within conforming footer as last entry\n+ *\n+ * A footer that does not end in a newline is considered non-conforming.\n+ *\n+ * ignore_footer, if not zero, should be the return value of an invocation to\n+ * ignore_non_trailer. See the documentation of that function for more\n+ * information.\n */\n static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n \tint ignore_footer)\n@@ -159,6 +165,11 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n \tint i;\n \tint found_sob = 0, found_sob_last = 0;\n \n+\tif (sb->len <= ignore_footer)\n+\t\treturn 0;\n+\tif (sb->buf[sb->len - ignore_footer - 1] != '\\n')\n+\t\treturn 0;\n+\n \ttrailer_info_get(&info, sb->buf);\n \n \tif (info.trailer_start == info.trailer_end)\n-- \n2.13.0.rc0.306.g87b477812d-goog\n\n"},{"id":"317837","messageId":"CAGZ79kapU5AL_iPJXCavCKAQ0Fw=pqWZS4F6Vri-Q1M1WMVs_w@mail.gmail.com","threadId":"45771","inReplyTo":"20170425190651.8910-1-jonathantanmy@google.com","subject":"Re: [PATCH] sequencer: require trailing NL in footers","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-25T21:04:17Z","receivedAt":"2017-04-25T21:04:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Apr 25, 2017 at 12:06 PM, Jonathan Tan wrote:\n> In commit 967dfd4 (\"sequencer: use trailer's trailer layout\",\n> 2016-11-29), sequencer was taught to use the same mechanism as\n> interpret-trailers to determine the nature of the trailer of a commit\n> message (referred to as the \"footer\" in sequencer.c). However, the\n> requirement that a footer end in a newline character was inadvertently\n> removed. Restore that requirement.\n>\n> While writing this commit, I noticed that if the \"ignore_footer\"\n> parameter in \"has_conforming_footer\" is greater than the distance\n> between the trailer start and sb->len, \"has_conforming_footer\" will\n> return an unexpected result. This does not occur in practice, because\n> \"ignore_footer\" is either zero or the return value of an invocation to\n> \"ignore_non_trailer\", which only skips empty lines and comment lines.\n> This commit contains a comment explaining this in the function's\n> documentation.\n>\n> Reported-by: Brian Norris \n> Signed-off-by: Jonathan Tan \n> ---\n>\n> Thanks for the bug report. Here's a fix - I've verified this with the\n> way to reproduce provided in the original e-mail, and it seems to work\n> now.\n>\n> The commit message of the referenced commit\n> (7b309aef0463340d3ad5449d1f605d14e10a4225) does not end in a newline,\n> which is probably why different behavior was observed with this commit\n> (as compared to others).\n\nThanks for the fix. :)\nDo we want to test for this use case in the future?\n\n> @@ -151,6 +151,12 @@ static const char *get_todo_path(const struct replay_opts *opts)\n> * Returns 1 for conforming footer\n> * Returns 2 when sob exists within conforming footer\n> * Returns 3 when sob exists within conforming footer as last entry\n> + *\n> + * A footer that does not end in a newline is considered non-conforming.\n> + *\n> + * ignore_footer, if not zero, should be the return value of an invocation to\n> + * ignore_non_trailer. See the documentation of that function for more\n> + * information.\n> */\n\nMakes sense. Maybe s/ignore_non_trailer/ignore_non_trailer()/ which makes\nit easier to recognize it as a function? I'd also drop the last\nsentence as it is\nimplied in the previous sentence (sort of).\n\nThanks,\nStefan\n"},{"id":"317863","messageId":"f49a858b-26da-7e8c-e0f2-8d66158f016a@google.com","threadId":"45771","inReplyTo":"CAGZ79kapU5AL_iPJXCavCKAQ0Fw=pqWZS4F6Vri-Q1M1WMVs_w@mail.gmail.com","subject":"Re: [PATCH] sequencer: require trailing NL in footers","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-04-25T21:51:17Z","receivedAt":"2017-04-25T21:51:29Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 04/25/2017 02:04 PM, Stefan Beller wrote:\n> Thanks for the fix. :)\n> Do we want to test for this use case in the future?\n\nThanks!\n\nI'm not sure of the value of including a test for this specific use \ncase, because Git normally does not create commit messages with no \ntrailing newlines. (To test this, I suspect I would need to use \nhash-object with a specifically crafted commit object.)\n\n>\n>> @@ -151,6 +151,12 @@ static const char *get_todo_path(const struct replay_opts *opts)\n>> * Returns 1 for conforming footer\n>> * Returns 2 when sob exists within conforming footer\n>> * Returns 3 when sob exists within conforming footer as last entry\n>> + *\n>> + * A footer that does not end in a newline is considered non-conforming.\n>> + *\n>> + * ignore_footer, if not zero, should be the return value of an invocation to\n>> + * ignore_non_trailer. See the documentation of that function for more\n>> + * information.\n>> */\n>\n> Makes sense. Maybe s/ignore_non_trailer/ignore_non_trailer()/ which makes\n> it easier to recognize it as a function? I'd also drop the last\n> sentence as it is\n> implied in the previous sentence (sort of).\n\nOK, I'll add the parentheses if I need to reroll. As for the last \nsentence, the documentation for ignore_non_trailer is a bit unusual in \nthat it says specifically \"to be fed as the second parameter to \nappend_signoff()\", so I want to call extra attention to that.\n"},{"id":"317865","messageId":"alpine.DEB.2.20.1704252355440.3480@virtualbox","threadId":"45771","inReplyTo":"20170425190651.8910-1-jonathantanmy@google.com","subject":"Re: [PATCH] sequencer: require trailing NL in footers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-04-25T21:56:36Z","receivedAt":"2017-04-25T21:56:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jonathan,\n\nOn Tue, 25 Apr 2017, Jonathan Tan wrote:\n\n> Thanks for the bug report. Here's a fix - I've verified this with the\n> way to reproduce provided in the original e-mail, and it seems to work\n> now.\n\nIf there is a straight-forward way to convert the manual test into an\nautomated one, it would be good to make it part of the patch so that we\ncan stave off future regressions of the fix.\n\nCiao,\nDscho\n"},{"id":"317866","messageId":"CAGZ79kbhxjO-tFW4_kObTZiauetjsgY1NBSKvo1NV5nKNEy2xA@mail.gmail.com","threadId":"45771","inReplyTo":"f49a858b-26da-7e8c-e0f2-8d66158f016a@google.com","subject":"Re: [PATCH] sequencer: require trailing NL in footers","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-25T21:56:53Z","receivedAt":"2017-04-25T21:56:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Apr 25, 2017 at 2:51 PM, Jonathan Tan wrote:\n> On 04/25/2017 02:04 PM, Stefan Beller wrote:\n>>\n>> Thanks for the fix. :)\n>> Do we want to test for this use case in the future?\n>\n>\n> Thanks!\n>\n> I'm not sure of the value of including a test for this specific use case,\n> because Git normally does not create commit messages with no trailing\n> newlines. (To test this, I suspect I would need to use hash-object with a\n> specifically crafted commit object.)\n\nOkay, makes sense to omit a test.\nIn that case: Is it needed to hint at how this bug occurred in the wild?\n(A different Git implementation, which may be fixed now?)\n"},{"id":"317867","messageId":"alpine.DEB.2.20.1704260003570.3480@virtualbox","threadId":"45771","inReplyTo":"CAGZ79kbhxjO-tFW4_kObTZiauetjsgY1NBSKvo1NV5nKNEy2xA@mail.gmail.com","subject":"Re: [PATCH] sequencer: require trailing NL in footers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-04-25T22:05:04Z","receivedAt":"2017-04-25T22:05:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn Tue, 25 Apr 2017, Stefan Beller wrote:\n\n> On Tue, Apr 25, 2017 at 2:51 PM, Jonathan Tan wrote:\n> > On 04/25/2017 02:04 PM, Stefan Beller wrote:\n> >>\n> >> Thanks for the fix. :)\n> >> Do we want to test for this use case in the future?\n> >\n> >\n> > Thanks!\n> >\n> > I'm not sure of the value of including a test for this specific use case,\n> > because Git normally does not create commit messages with no trailing\n> > newlines. (To test this, I suspect I would need to use hash-object with a\n> > specifically crafted commit object.)\n> \n> Okay, makes sense to omit a test.\n> In that case: Is it needed to hint at how this bug occurred in the wild?\n> (A different Git implementation, which may be fixed now?)\n\nJust because Git usually does not create commit messages without trailing\nnewlines does not mean that it is a rare thing. We got bug reports about\nthis, so I think it is frequent enough that we could save time by adding\nthat test and avoid future bug reports/bug hunts.\n\nCiao,\nJohannes\n"},{"id":"317869","messageId":"20170425223056.GA104886@google.com","threadId":"45771","inReplyTo":"CAGZ79kbhxjO-tFW4_kObTZiauetjsgY1NBSKvo1NV5nKNEy2xA@mail.gmail.com","subject":"Re: [PATCH] sequencer: require trailing NL in footers","fromName":"Brian Norris","fromEmail":"computersforpeace@gmail.com","sentAt":"2017-04-25T22:30:56Z","receivedAt":"2017-04-25T22:31:05Z","isPatch":true,"sender":{"key":"computersforpeace@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2268082?v=4"},"body":"On Tue, Apr 25, 2017 at 02:56:53PM -0700, Stefan Beller wrote:\n> In that case: Is it needed to hint at how this bug occurred in the wild?\n> (A different Git implementation, which may be fixed now?)\n\nI might contact the original author, but it's easy enough to imagine\nautomated 'filter-branch' scripts that could produce this. e.g., this\ntrivial example:\n\ngit filter-branch --msg-filter \"perl -pe 'chomp if eof'\" HEAD^..\n"},{"id":"317874","messageId":"20170425233407.GO28740@aiede.svl.corp.google.com","threadId":"45771","inReplyTo":"20170425190651.8910-1-jonathantanmy@google.com","subject":"Re: [PATCH] sequencer: require trailing NL in footers","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-04-25T23:34:07Z","receivedAt":"2017-04-25T23:34:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJonathan Tan wrote:\n\n> In commit 967dfd4 (\"sequencer: use trailer's trailer layout\",\n> 2016-11-29), sequencer was taught to use the same mechanism as\n> interpret-trailers to determine the nature of the trailer of a commit\n> message (referred to as the \"footer\" in sequencer.c). However, the\n> requirement that a footer end in a newline character was inadvertently\n> removed. Restore that requirement.\n>\n> While writing this commit, I noticed that if the \"ignore_footer\"\n> parameter in \"has_conforming_footer\" is greater than the distance\n> between the trailer start and sb->len, \"has_conforming_footer\" will\n> return an unexpected result. This does not occur in practice, because\n> \"ignore_footer\" is either zero or the return value of an invocation to\n> \"ignore_non_trailer\", which only skips empty lines and comment lines.\n> This commit contains a comment explaining this in the function's\n> documentation.\n>\n> Reported-by: Brian Norris \n> Signed-off-by: Jonathan Tan \n> ---\n[...]\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -151,6 +151,12 @@ static const char *get_todo_path(const struct replay_opts *opts)\n> * Returns 1 for conforming footer\n> * Returns 2 when sob exists within conforming footer\n> * Returns 3 when sob exists within conforming footer as last entry\n> + *\n> + * A footer that does not end in a newline is considered non-conforming.\n> + *\n> + * ignore_footer, if not zero, should be the return value of an invocation to\n> + * ignore_non_trailer. See the documentation of that function for more\n> + * information.\n> */\n> static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n> \tint ignore_footer)\n> @@ -159,6 +165,11 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n> \tint i;\n> \tint found_sob = 0, found_sob_last = 0;\n> \n> +\tif (sb->len <= ignore_footer)\n> +\t\treturn 0;\n> +\tif (sb->buf[sb->len - ignore_footer - 1] != '\\n')\n> +\t\treturn 0;\n> +\n\nThis is super subtle, but it does the right thing. The caller will\nnotice it's not a conforming footer, add a newline to separate the new\nfooter from it, and repair the footer as a side effect.\n\nReviewed-by: Jonathan Nieder \n\nFollowup question: what should happen if there is a non-footer-shaped\nthing with no trailing newline at the end of the commit message? Should\nwe add two newlines in that case when producing a new footer?\n\nThanks,\nJonathan\n"},{"id":"317877","messageId":"20170425233914.GP28740@aiede.svl.corp.google.com","threadId":"45771","inReplyTo":"alpine.DEB.2.20.1704260003570.3480@virtualbox","subject":"Re: [PATCH] sequencer: require trailing NL in footers","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-04-25T23:39:14Z","receivedAt":"2017-04-25T23:39:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Johannes Schindelin wrote:\n>> On Tue, Apr 25, 2017 at 2:51 PM, Jonathan Tan wrote:\n>>> On 04/25/2017 02:04 PM, Stefan Beller wrote:\n\n>>>> Do we want to test for this use case in the future?\n[...]\n>>> I'm not sure of the value of including a test for this specific use case,\n>>> because Git normally does not create commit messages with no trailing\n>>> newlines. (To test this, I suspect I would need to use hash-object with a\n>>> specifically crafted commit object.)\n[...]\n> Just because Git usually does not create commit messages without trailing\n> newlines does not mean that it is a rare thing. We got bug reports about\n> this, so I think it is frequent enough that we could save time by adding\n> that test and avoid future bug reports/bug hunts.\n\nTo put it another way: it wouldn't have been worth writing this patch\nif it wasn't worth keeping the behavior. So we do need a test.\n\nThanks,\nJonathan\n"},{"id":"317882","messageId":"20170425235741.32546-1-jonathantanmy@google.com","threadId":"45771","inReplyTo":"20170425190651.8910-1-jonathantanmy@google.com","subject":"[PATCH v2] sequencer: require trailing NL in footers","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-04-25T23:57:41Z","receivedAt":"2017-04-25T23:58:23Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In commit 967dfd4 (\"sequencer: use trailer's trailer layout\",\n2016-11-29), sequencer was taught to use the same mechanism as\ninterpret-trailers to determine the nature of the trailer of a commit\nmessage (referred to as the \"footer\" in sequencer.c). However, the\nrequirement that a footer end in a newline character was inadvertently\nremoved. Restore that requirement.\n\nWhile writing this commit, I noticed that if the \"ignore_footer\"\nparameter in \"has_conforming_footer\" is greater than the distance\nbetween the trailer start and sb->len, \"has_conforming_footer\" will\nreturn an unexpected result. This does not occur in practice, because\n\"ignore_footer\" is either zero or the return value of an invocation to\n\"ignore_non_trailer\", which only skips empty lines and comment lines.\nThis commit contains a comment explaining this in the function's\ndocumentation.\n\nReported-by: Brian Norris \nSigned-off-by: Jonathan Tan \n---\n\njrnieder pointed out the existence of commit-tree to me, which I have\nused to write the test below.\n\nChanges from v1:\n - added test\n - used one of sbeller's documentation suggestions\n\n sequencer.c | 11 +++++++++++\n t/t3511-cherry-pick-x.sh | 14 ++++++++++++++\n 2 files changed, 25 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 77afecaeb..500a76260 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -151,6 +151,12 @@ static const char *get_todo_path(const struct replay_opts *opts)\n * Returns 1 for conforming footer\n * Returns 2 when sob exists within conforming footer\n * Returns 3 when sob exists within conforming footer as last entry\n+ *\n+ * A footer that does not end in a newline is considered non-conforming.\n+ *\n+ * ignore_footer, if not zero, should be the return value of an invocation to\n+ * ignore_non_trailer(). See the documentation of that function for more\n+ * information.\n */\n static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n \tint ignore_footer)\n@@ -159,6 +165,11 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n \tint i;\n \tint found_sob = 0, found_sob_last = 0;\n \n+\tif (sb->len <= ignore_footer)\n+\t\treturn 0;\n+\tif (sb->buf[sb->len - ignore_footer - 1] != '\\n')\n+\t\treturn 0;\n+\n \ttrailer_info_get(&info, sb->buf);\n \n \tif (info.trailer_start == info.trailer_end)\ndiff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\nindex bf0a5c988..6f518020b 100755\n--- a/t/t3511-cherry-pick-x.sh\n+++ b/t/t3511-cherry-pick-x.sh\n@@ -208,6 +208,20 @@ test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists fo\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'cherry-pick -x handles commits with no NL at end of message' '\n+\tpristine_detach initial &&\n+\tsha1=$(printf \"title\\n\\nSigned-off-by: a\" | git commit-tree -p initial mesg-with-footer^{tree}) &&\n+\tgit cherry-pick -x $sha1 &&\n+\tcat <<-EOF >expect &&\n+\t\ttitle\n+\n+\t\tSigned-off-by: a\n+\t\t(cherry picked from commit $sha1)\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'cherry-pick -x treats \"(cherry picked from...\" line as part of footer' '\n \tpristine_detach initial &&\n \tsha1=$(git rev-parse mesg-with-cherry-footer^0) &&\n-- \n2.13.0.rc0.306.g87b477812d-goog\n\n"},{"id":"317883","messageId":"20170426000741.GS28740@aiede.svl.corp.google.com","threadId":"45771","inReplyTo":"20170425235741.32546-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] sequencer: require trailing NL in footers","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-04-26T00:07:41Z","receivedAt":"2017-04-26T00:08:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n\n> Reported-by: Brian Norris \n> Signed-off-by: Jonathan Tan \n> ---\n[...]\n> sequencer.c | 11 +++++++++++\n> t/t3511-cherry-pick-x.sh | 14 ++++++++++++++\n> 2 files changed, 25 insertions(+)\n\nReviewed-by: Jonathan Nieder \n\nThis is still pretty subtle (using the added newline that is added after\na non-footer to turn the invalid footer into a valid one), but\n\n * it is clear from the code that it will work correctly\n * the new test ensures we'll continue to support this case\n * it is understandable after a little head scratching\n * I'm hoping the subtlety will go away once the code learns to deal\n with ordinary non-footer text that has a missing newline\n\n[...]\n> --- a/t/t3511-cherry-pick-x.sh\n> +++ b/t/t3511-cherry-pick-x.sh\n> @@ -208,6 +208,20 @@ test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists fo\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'cherry-pick -x handles commits with no NL at end of message' '\n> +\tpristine_detach initial &&\n> +\tsha1=$(printf \"title\\n\\nSigned-off-by: a\" | git commit-tree -p initial mesg-with-footer^{tree}) &&\n\nnit: Should this use a more typical sign-off line with an email\naddress, to avoid a false-positive success in case git becomes more\nstrict about its signoffs in the future?\n\nSomething like\n\n\tprintf \"title\\n\\nSigned-off-by: S. I. Gner \" >msg &&\n\tsha1=$(git commit-tree -p initial mesg-with-footer^{tree} writes:\n\n> Jonathan Tan wrote:\n>\n>> Reported-by: Brian Norris \n>> Signed-off-by: Jonathan Tan \n>> ---\n> [...]\n>> sequencer.c | 11 +++++++++++\n>> t/t3511-cherry-pick-x.sh | 14 ++++++++++++++\n>> 2 files changed, 25 insertions(+)\n>\n> Reviewed-by: Jonathan Nieder \n>\n> This is still pretty subtle (using the added newline that is added after\n> a non-footer to turn the invalid footer into a valid one), but\n>\n> * it is clear from the code that it will work correctly\n> * the new test ensures we'll continue to support this case\n> * it is understandable after a little head scratching\n> * I'm hoping the subtlety will go away once the code learns to deal\n> with ordinary non-footer text that has a missing newline\n\nHmph, perhaps we need another test that documents a known failure as\nwell in the meantime?\n\n> [...]\n>> --- a/t/t3511-cherry-pick-x.sh\n>> +++ b/t/t3511-cherry-pick-x.sh\n>> @@ -208,6 +208,20 @@ test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists fo\n>> \ttest_cmp expect actual\n>> '\n>> \n>> +test_expect_success 'cherry-pick -x handles commits with no NL at end of message' '\n>> +\tpristine_detach initial &&\n>> +\tsha1=$(printf \"title\\n\\nSigned-off-by: a\" | git commit-tree -p initial mesg-with-footer^{tree}) &&\n>\n> nit: Should this use a more typical sign-off line with an email\n> address, to avoid a false-positive success in case git becomes more\n> strict about its signoffs in the future?\n>\n> Something like\n>\n> \tprintf \"title\\n\\nSigned-off-by: S. I. Gner \" >msg &&\n> \tsha1=$(git commit-tree -p initial mesg-with-footer^{tree} \t...\n\nThat is a good point and has an added benefit that the test script\nbecomes easier to follow.\n\n t/t3511-cherry-pick-x.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\nindex 6f518020b2..c2b143802d 100755\n--- a/t/t3511-cherry-pick-x.sh\n+++ b/t/t3511-cherry-pick-x.sh\n@@ -210,12 +210,14 @@ test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists fo\n \n test_expect_success 'cherry-pick -x handles commits with no NL at end of message' '\n \tpristine_detach initial &&\n-\tsha1=$(printf \"title\\n\\nSigned-off-by: a\" | git commit-tree -p initial mesg-with-footer^{tree}) &&\n+\tsigner=\"S. I. Gner \" &&\n+\tprintf \"title\\n\\nSigned-off-by: %s\" \"$signer\" >msg &&\n+\tsha1=$(git commit-tree -p initial mesg-with-footer^{tree} expect &&\n \t\ttitle\n \n-\t\tSigned-off-by: a\n+\t\tSigned-off-by: $signer\n \t\t(cherry picked from commit $sha1)\n \tEOF\n \tgit log -1 --pretty=format:%B >actual &&\n"},{"id":"317910","messageId":"alpine.DEB.2.20.1704261101250.3480@virtualbox","threadId":"45771","inReplyTo":"xmqqinlrudij.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] sequencer: require trailing NL in footers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-04-26T09:09:06Z","receivedAt":"2017-04-26T09:29:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 25 Apr 2017, Junio C Hamano wrote:\n\n> Jonathan Nieder writes:\n> \n> > Jonathan Tan wrote:\n> >\n> > [...]\n> >> --- a/t/t3511-cherry-pick-x.sh\n> >> +++ b/t/t3511-cherry-pick-x.sh\n> >> @@ -208,6 +208,20 @@ test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists fo\n> >> \ttest_cmp expect actual\n> >> '\n> >> \n> >> +test_expect_success 'cherry-pick -x handles commits with no NL at end of message' '\n> >> +\tpristine_detach initial &&\n> >> +\tsha1=$(printf \"title\\n\\nSigned-off-by: a\" | git commit-tree -p initial mesg-with-footer^{tree}) &&\n> >\n> > nit: Should this use a more typical sign-off line with an email\n> > address, to avoid a false-positive success in case git becomes more\n> > strict about its signoffs in the future?\n> >\n> > Something like\n> >\n> > \tprintf \"title\\n\\nSigned-off-by: S. I. Gner \" >msg &&\n> > \tsha1=$(git commit-tree -p initial mesg-with-footer^{tree} > \t...\n> \n> That is a good point and has an added benefit that the test script\n> becomes easier to follow.\n\nIf you already try to make it easier to follow, you might just as well go\nthe whole nine yards:\n\n> diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\n> index 6f518020b2..c2b143802d 100755\n> --- a/t/t3511-cherry-pick-x.sh\n> +++ b/t/t3511-cherry-pick-x.sh\n> @@ -210,12 +210,14 @@ test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists fo\n> \n> test_expect_success 'cherry-pick -x handles commits with no NL at end of message' '\n> \tpristine_detach initial &&\n> -\tsha1=$(printf \"title\\n\\nSigned-off-by: a\" | git commit-tree -p initial mesg-with-footer^{tree}) &&\n> +\tsigner=\"S. I. Gner \" &&\n> +\tprintf \"title\\n\\nSigned-off-by: %s\" \"$signer\" >msg &&\n> +\tsha1=$(git commit-tree -p initial mesg-with-footer^{tree} \tgit cherry-pick -x $sha1 &&\n> \tcat <<-EOF >expect &&\n> \t\ttitle\n> \n> -\t\tSigned-off-by: a\n> +\t\tSigned-off-by: $signer\n> \t\t(cherry picked from commit $sha1)\n> \tEOF\n> \tgit log -1 --pretty=format:%B >actual &&\n\nIt is even easier to read `--format=%B` than `--pretty=format:%B`, and as\n`%B` does *not* indent, the indentation in the lines writing the `expect`\nfile is bogus anyway. And with the `msg` file having most of the stuff\nalready, we Do Not Need To Repeat Ourselves:\n\n\tprintf '\\n(cherry picked from commit %s)\\n' $sha1 >>msg &&\n\tgit log -1 --format=%B >actual &&\n\ttest_cmp msg actual\n\nCiao,\nDscho\n"},{"id":"317996","messageId":"20170426203116.GA37189@google.com","threadId":"45771","inReplyTo":"20170425223056.GA104886@google.com","subject":"Re: [PATCH] sequencer: require trailing NL in footers","fromName":"Brian Norris","fromEmail":"computersforpeace@gmail.com","sentAt":"2017-04-26T20:31:16Z","receivedAt":"2017-04-26T20:31:40Z","isPatch":true,"sender":{"key":"computersforpeace@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2268082?v=4"},"body":"On Tue, Apr 25, 2017 at 03:30:56PM -0700, Brian Norris wrote:\n> On Tue, Apr 25, 2017 at 02:56:53PM -0700, Stefan Beller wrote:\n> > In that case: Is it needed to hint at how this bug occurred in the wild?\n> > (A different Git implementation, which may be fixed now?)\n> \n> I might contact the original author\n\nOriginal author was using 'stgit', FYI.\n"},{"id":"317997","messageId":"20170426205003.30825-1-jonathantanmy@google.com","threadId":"45771","inReplyTo":"20170425190651.8910-1-jonathantanmy@google.com","subject":"[PATCH v3] sequencer: add newline before adding footers","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-04-26T20:50:03Z","receivedAt":"2017-04-26T20:50:15Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When encountering a commit message that does not end in a newline,\nsequencer does not complete the line before determining if a blank line\nshould be added. This causes the \"(cherry picked...\" and sign-off lines\nto sometimes appear on the same line as the last line of the commit\nmessage.\n\nThis behavior was introduced by commit 967dfd4 (\"sequencer: use\ntrailer's trailer layout\", 2016-11-29). However, a revert of that commit\nwould not resolve this issue completely: prior to that commit, a\nconforming footer was deemed to be non-conforming by\nhas_conforming_footer() if there was no terminating newline, resulting\nin both conforming and non-conforming footers being treated the same\nwhen they should not be.\n\nResolve this issue, both for conforming and non-conforming footers, and\nin both do_pick_commit() and append_signoff(), by always adding a\nnewline to the commit message if it does not end in one before checking\nthe footer for conformity.\n\nReported-by: Brian Norris \nSigned-off-by: Jonathan Tan \n---\n\nThe failure in the case of non-footers not being terminated by a newline\nturns out to be quite easy to fix, so I've added that fix here. I think\nthis makes the overall fix more obvious too.\n\nI've used Jonathan Nieder's and Dscho's suggestions for the tests\n(except for --format vs --pretty, since --format seems to add an extra\nnewline to the output).\n\n sequencer.c | 11 ++++-------\n t/t3511-cherry-pick-x.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 48 insertions(+), 7 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 77afecaeb..ffac95545 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1045,6 +1045,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\t\tstrbuf_addstr(&msgbuf, p);\n \n \t\tif (opts->record_origin) {\n+\t\t\tstrbuf_complete_line(&msgbuf);\n \t\t\tif (!has_conforming_footer(&msgbuf, NULL, 0))\n \t\t\t\tstrbuf_addch(&msgbuf, '\\n');\n \t\t\tstrbuf_addstr(&msgbuf, cherry_picked_prefix);\n@@ -2335,6 +2336,9 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)\n \t\t\t\tgetenv(\"GIT_COMMITTER_EMAIL\")));\n \tstrbuf_addch(&sob, '\\n');\n \n+\tif (!ignore_footer)\n+\t\tstrbuf_complete_line(msgbuf);\n+\n \t/*\n \t * If the whole message buffer is equal to the sob, pretend that we\n \t * found a conforming footer with a matching sob\n@@ -2355,13 +2359,6 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)\n \t\t\t * the title and body to be filled in by the user.\n \t\t\t */\n \t\t\tappend_newlines = \"\\n\\n\";\n-\t\t} else if (msgbuf->buf[len - 1] != '\\n') {\n-\t\t\t/*\n-\t\t\t * Incomplete line. Complete the line and add a\n-\t\t\t * blank one so that there is an empty line between\n-\t\t\t * the message body and the sob.\n-\t\t\t */\n-\t\t\tappend_newlines = \"\\n\\n\";\n \t\t} else if (len == 1) {\n \t\t\t/*\n \t\t\t * Buffer contains a single newline. Add another\ndiff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\nindex bf0a5c988..9888bf34b 100755\n--- a/t/t3511-cherry-pick-x.sh\n+++ b/t/t3511-cherry-pick-x.sh\n@@ -208,6 +208,50 @@ test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists fo\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'cherry-pick -x handles commits with no NL at end of message' '\n+\tpristine_detach initial &&\n+\tprintf \"title\\n\\nSigned-off-by: A \" >msg &&\n+\tsha1=$(git commit-tree -p initial mesg-with-footer^{tree} actual &&\n+\n+\tprintf \"\\n(cherry picked from commit %s)\\n\" $sha1 >>msg &&\n+\ttest_cmp msg actual\n+'\n+\n+test_expect_success 'cherry-pick -x handles commits with no footer and no NL at end of message' '\n+\tpristine_detach initial &&\n+\tprintf \"title\\n\\nnot a footer\" >msg &&\n+\tsha1=$(git commit-tree -p initial mesg-with-footer^{tree} actual &&\n+\n+\tprintf \"\\n\\n(cherry picked from commit %s)\\n\" $sha1 >>msg &&\n+\ttest_cmp msg actual\n+'\n+\n+test_expect_success 'cherry-pick -s handles commits with no NL at end of message' '\n+\tpristine_detach initial &&\n+\tprintf \"title\\n\\nSigned-off-by: A \" >msg &&\n+\tsha1=$(git commit-tree -p initial mesg-with-footer^{tree} actual &&\n+\n+\tprintf \"\\nSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\\n\" >>msg &&\n+\ttest_cmp msg actual\n+'\n+\n+test_expect_success 'cherry-pick -s handles commits with no footer and no NL at end of message' '\n+\tpristine_detach initial &&\n+\tprintf \"title\\n\\nnot a footer\" >msg &&\n+\tsha1=$(git commit-tree -p initial mesg-with-footer^{tree} actual &&\n+\n+\tprintf \"\\n\\nSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\\n\" >>msg &&\n+\ttest_cmp msg actual\n+'\n+\n test_expect_success 'cherry-pick -x treats \"(cherry picked from...\" line as part of footer' '\n \tpristine_detach initial &&\n \tsha1=$(git rev-parse mesg-with-cherry-footer^0) &&\n-- \n2.13.0.rc0.306.g87b477812d-goog\n\n"}]}