{"thread":{"id":"41680","subject":"[PATCH] sequencer.c: fix detection of duplicate s-o-b","startedAt":"2016-03-12T13:08:44Z","lastAt":"2016-04-07T20:16:36Z","messageCount":5,"participants":["Willy Tarreau","Junio C Hamano","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"280680","messageId":"20160312130844.GA25639@1wt.eu","threadId":"41680","inReplyTo":null,"subject":"[PATCH] sequencer.c: fix detection of duplicate s-o-b","fromName":"Willy Tarreau","fromEmail":"w@1wt.eu","sentAt":"2016-03-12T13:08:44Z","receivedAt":"2016-03-12T13:08:44Z","isPatch":true,"sender":{"key":"w@1wt.eu","avatar":"https://avatars.githubusercontent.com/u/8141789?v=4"},"body":"Hi,\n\nafter I upgraded my machine, I switched from git 1.7.12.2 to 2.6.4\nand experienced an annoying regression when dealing with stable\nkernel backports.\n\nI'm using a \"dorelease\" script which relies on git-cherry-pick's\nability to properly detect duplicate s-o-b to ensure that all merged\ncommits are properly signed in a release. Today while preparing the\nlast 2.6.32 release, I did a git log before pushing and found some\ncommits having two s-o-b lines with myself. I found that these ones\nwere always those containing some backporting notes between the s-o-b\nlines (which we all do in stable branches to indicate what was changed\nin the backport process).\n\nI didn't feel brave enough to individually deal with each offending\npatch by hand so instead I bisected the git changes and found that the\nbehaviour changed with commit bab4d10 (\"sequencer.c: teach append_signoff\nhow to detect duplicate s-o-b\").\n\nThe reason is that function has_conforming_footer() immediately stops\nafter the first non-conforming line without checking if there are\nconforming lines after. But if someone added signed-off-by anywhere\nafter a non-conforming block, it should always be considered as part\nof the footer. Thus I adjusted the logic to check till the end of the\nfooter and report the presence of valid rfc2822 or cherry-picked lines\nafter the last non-conformant one and now it correctly handles all types\nof commits I had to deal with (ie: only adds s-o-b when it doesn't match\nthe last one and doesn't add an empty line after a conformant one). For\nexample, this footer :\n\n Signed-off-by: Mike Galbraith \n [bwh: Backported to 3.2:\n - Adjust numbering in the comment\n - Adjust filename]\n Signed-off-by: Ben Hutchings \n Cc: Byungchul Park \n Cc: Peter Zijlstra \n Cc: Willy Tarreau \n\nUsed to be turned into this :\n\n Signed-off-by: Mike Galbraith \n [bwh: Backported to 3.2:\n - Adjust numbering in the comment\n - Adjust filename]\n Signed-off-by: Ben Hutchings \n Cc: Byungchul Park \n Cc: Peter Zijlstra \n Cc: Willy Tarreau \n\n Signed-off-by: Willy Tarreau \n\nAnd is now properly converted to :\n\n Signed-off-by: Mike Galbraith \n [bwh: Backported to 3.2:\n - Adjust numbering in the comment\n - Adjust filename]\n Signed-off-by: Ben Hutchings \n Cc: Byungchul Park \n Cc: Peter Zijlstra \n Cc: Willy Tarreau \n Signed-off-by: Willy Tarreau \n\nAlso, cherry-picking the last commit above again would produce this\nbefore :\n\n Signed-off-by: Mike Galbraith \n [bwh: Backported to 3.2:\n - Adjust numbering in the comment\n - Adjust filename]\n Signed-off-by: Ben Hutchings \n Cc: Byungchul Park \n Cc: Peter Zijlstra \n Cc: Willy Tarreau \n Signed-off-by: Willy Tarreau \n\n Signed-off-by: Willy Tarreau \n\nAnd it now is properly left untouched since the last s-o-b line\nis properly matched.\n\nI'm appending the patch, please include it upstream.\n\nThanks!\nWilly\n\n\n>From be9624a0df4c649d452f898925953a81dc9163fc Mon Sep 17 00:00:00 2001\nFrom: Willy Tarreau \nDate: Sat, 12 Mar 2016 13:35:35 +0100\nSubject: sequencer.c: fix detection of duplicate s-o-b\n\nCommit bab4d10 (\"sequencer.c: teach append_signoff how to detect\nduplicate s-o-b\") changed the method used to detect duplicate s-o-b,\nbut it introduced a regression for a case where some non-compliant\ninformation are present in the footer. In maintenance branches, it's\nvery common to add some elements after the signed-off and to add your\ns-o-b after. This is used a lot in the stable kernel series, for\nexample this commit backported from 3.2 to 2.6.32 :\n\n ALSA: usb-audio: avoid freeing umidi object twice\n\n commit 07d86ca93db7e5cdf4743564d98292042ec21af7 upstream.\n\n The 'umidi' object will be free'd on the error path by snd_usbmidi_free()\n when tearing down the rawmidi interface. So we shouldn't try to free it\n in snd_usbmidi_create() after having registered the rawmidi interface.\n\n Found by KASAN.\n\n Signed-off-by: Andrey Konovalov \n Acked-by: Clemens Ladisch \n Signed-off-by: Takashi Iwai \n Signed-off-by: Ben Hutchings \n [wt: file is sound/midi/usbmidi.c in 2.6.32]\n Signed-off-by: Willy Tarreau \n\nPrior to the commit above, a cherry-pick -s would not append an extra s-o-b.\nAfter this commit, a new line and a second s-o-b are added, making the footer\nlook like this :\n\n Signed-off-by: Andrey Konovalov \n Acked-by: Clemens Ladisch \n Signed-off-by: Takashi Iwai \n Signed-off-by: Ben Hutchings \n [wt: file is sound/midi/usbmidi.c in 2.6.32]\n Signed-off-by: Willy Tarreau \n\n Signed-off-by: Willy Tarreau \n\nThis patch improves the parsing of the footer by considering the\npresence of a valid rfc2822 line after possibly non-conformant lines.\nIndeed, if someone added an s-o-b or CC after some stuff, this line\nmust properly be considered as part of the footer and not of the body.\n\nSigned-off-by: Willy Tarreau \n---\n sequencer.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex e66f2fe..ab2c18d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -64,6 +64,8 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n \tint len = sb->len - ignore_footer;\n \tconst char *buf = sb->buf;\n \tint found_sob = 0;\n+\tint found_valid = 0;\n+\tint found_other = 0;\n \n \t/* footer must end with newline */\n \tif (!len || buf[len - 1] != '\\n')\n@@ -96,15 +98,18 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n \t\tif (found_rfc2822 && sob &&\n \t\t !strncmp(buf + i, sob->buf, sob->len))\n \t\t\tfound_sob = k;\n-\n-\t\tif (!(found_rfc2822 ||\n-\t\t is_cherry_picked_from_line(buf + i, k - i - 1)))\n-\t\t\treturn 0;\n+\t\telse if (found_rfc2822 ||\n+\t\t\t is_cherry_picked_from_line(buf + i, k - i - 1))\n+\t\t\tfound_valid = k;\n+\t\telse\n+\t\t\tfound_other = k;\n \t}\n \tif (found_sob == i)\n \t\treturn 3;\n-\tif (found_sob)\n+\tif (found_sob > found_other)\n \t\treturn 2;\n+\tif (found_other > found_valid)\n+\t\treturn 0;\n \treturn 1;\n }\n \n-- \n2.6.4\n"},{"id":"282764","messageId":"xmqqr3eizsxu.fsf@gitster.mtv.corp.google.com","threadId":"41680","inReplyTo":"20160312130844.GA25639@1wt.eu","subject":"Re: [PATCH] sequencer.c: fix detection of duplicate s-o-b","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-06T14:57:01Z","receivedAt":"2016-04-06T14:57:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This seems to have been lost, perhaps because the top part that was\nquite long didn't look like a patch submission message or something.\n\nGit 1.7.12 is a quite ancient release and I wouldn't be surprised if\nwe made the behaviour change during the period leading to v2.6 on\npurpose, but nothing immediately comes to mind. Christian (as the\nadvocate for the trailer machinery) and Brandon (\"git shortlog\nsequencer.c\" suggests you), can you take a look?\n\nWilly Tarreau writes:\n\n> Hi,\n>\n> after I upgraded my machine, I switched from git 1.7.12.2 to 2.6.4\n> and experienced an annoying regression when dealing with stable\n> kernel backports.\n>\n> I'm using a \"dorelease\" script which relies on git-cherry-pick's\n> ability to properly detect duplicate s-o-b to ensure that all merged\n> commits are properly signed in a release. Today while preparing the\n> last 2.6.32 release, I did a git log before pushing and found some\n> commits having two s-o-b lines with myself. I found that these ones\n> were always those containing some backporting notes between the s-o-b\n> lines (which we all do in stable branches to indicate what was changed\n> in the backport process).\n>\n> I didn't feel brave enough to individually deal with each offending\n> patch by hand so instead I bisected the git changes and found that the\n> behaviour changed with commit bab4d10 (\"sequencer.c: teach append_signoff\n> how to detect duplicate s-o-b\").\n>\n> The reason is that function has_conforming_footer() immediately stops\n> after the first non-conforming line without checking if there are\n> conforming lines after. But if someone added signed-off-by anywhere\n> after a non-conforming block, it should always be considered as part\n> of the footer. Thus I adjusted the logic to check till the end of the\n> footer and report the presence of valid rfc2822 or cherry-picked lines\n> after the last non-conformant one and now it correctly handles all types\n> of commits I had to deal with (ie: only adds s-o-b when it doesn't match\n> the last one and doesn't add an empty line after a conformant one). For\n> example, this footer :\n>\n> Signed-off-by: Mike Galbraith \n> [bwh: Backported to 3.2:\n> - Adjust numbering in the comment\n> - Adjust filename]\n> Signed-off-by: Ben Hutchings \n> Cc: Byungchul Park \n> Cc: Peter Zijlstra \n> Cc: Willy Tarreau \n>\n> Used to be turned into this :\n>\n> Signed-off-by: Mike Galbraith \n> [bwh: Backported to 3.2:\n> - Adjust numbering in the comment\n> - Adjust filename]\n> Signed-off-by: Ben Hutchings \n> Cc: Byungchul Park \n> Cc: Peter Zijlstra \n> Cc: Willy Tarreau \n>\n> Signed-off-by: Willy Tarreau \n>\n> And is now properly converted to :\n>\n> Signed-off-by: Mike Galbraith \n> [bwh: Backported to 3.2:\n> - Adjust numbering in the comment\n> - Adjust filename]\n> Signed-off-by: Ben Hutchings \n> Cc: Byungchul Park \n> Cc: Peter Zijlstra \n> Cc: Willy Tarreau \n> Signed-off-by: Willy Tarreau \n>\n> Also, cherry-picking the last commit above again would produce this\n> before :\n>\n> Signed-off-by: Mike Galbraith \n> [bwh: Backported to 3.2:\n> - Adjust numbering in the comment\n> - Adjust filename]\n> Signed-off-by: Ben Hutchings \n> Cc: Byungchul Park \n> Cc: Peter Zijlstra \n> Cc: Willy Tarreau \n> Signed-off-by: Willy Tarreau \n>\n> Signed-off-by: Willy Tarreau \n>\n> And it now is properly left untouched since the last s-o-b line\n> is properly matched.\n>\n> I'm appending the patch, please include it upstream.\n>\n> Thanks!\n> Willy\n>\n>\n> From be9624a0df4c649d452f898925953a81dc9163fc Mon Sep 17 00:00:00 2001\n> From: Willy Tarreau \n> Date: Sat, 12 Mar 2016 13:35:35 +0100\n> Subject: sequencer.c: fix detection of duplicate s-o-b\n>\n> Commit bab4d10 (\"sequencer.c: teach append_signoff how to detect\n> duplicate s-o-b\") changed the method used to detect duplicate s-o-b,\n> but it introduced a regression for a case where some non-compliant\n> information are present in the footer. In maintenance branches, it's\n> very common to add some elements after the signed-off and to add your\n> s-o-b after. This is used a lot in the stable kernel series, for\n> example this commit backported from 3.2 to 2.6.32 :\n>\n> ALSA: usb-audio: avoid freeing umidi object twice\n>\n> commit 07d86ca93db7e5cdf4743564d98292042ec21af7 upstream.\n>\n> The 'umidi' object will be free'd on the error path by snd_usbmidi_free()\n> when tearing down the rawmidi interface. So we shouldn't try to free it\n> in snd_usbmidi_create() after having registered the rawmidi interface.\n>\n> Found by KASAN.\n>\n> Signed-off-by: Andrey Konovalov \n> Acked-by: Clemens Ladisch \n> Signed-off-by: Takashi Iwai \n> Signed-off-by: Ben Hutchings \n> [wt: file is sound/midi/usbmidi.c in 2.6.32]\n> Signed-off-by: Willy Tarreau \n>\n> Prior to the commit above, a cherry-pick -s would not append an extra s-o-b.\n> After this commit, a new line and a second s-o-b are added, making the footer\n> look like this :\n>\n> Signed-off-by: Andrey Konovalov \n> Acked-by: Clemens Ladisch \n> Signed-off-by: Takashi Iwai \n> Signed-off-by: Ben Hutchings \n> [wt: file is sound/midi/usbmidi.c in 2.6.32]\n> Signed-off-by: Willy Tarreau \n>\n> Signed-off-by: Willy Tarreau \n>\n> This patch improves the parsing of the footer by considering the\n> presence of a valid rfc2822 line after possibly non-conformant lines.\n> Indeed, if someone added an s-o-b or CC after some stuff, this line\n> must properly be considered as part of the footer and not of the body.\n>\n> Signed-off-by: Willy Tarreau \n> ---\n> sequencer.c | 15 ++++++++++-----\n> 1 file changed, 10 insertions(+), 5 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index e66f2fe..ab2c18d 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -64,6 +64,8 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n> \tint len = sb->len - ignore_footer;\n> \tconst char *buf = sb->buf;\n> \tint found_sob = 0;\n> +\tint found_valid = 0;\n> +\tint found_other = 0;\n> \n> \t/* footer must end with newline */\n> \tif (!len || buf[len - 1] != '\\n')\n> @@ -96,15 +98,18 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n> \t\tif (found_rfc2822 && sob &&\n> \t\t !strncmp(buf + i, sob->buf, sob->len))\n> \t\t\tfound_sob = k;\n> -\n> -\t\tif (!(found_rfc2822 ||\n> -\t\t is_cherry_picked_from_line(buf + i, k - i - 1)))\n> -\t\t\treturn 0;\n> +\t\telse if (found_rfc2822 ||\n> +\t\t\t is_cherry_picked_from_line(buf + i, k - i - 1))\n> +\t\t\tfound_valid = k;\n> +\t\telse\n> +\t\t\tfound_other = k;\n> \t}\n> \tif (found_sob == i)\n> \t\treturn 3;\n> -\tif (found_sob)\n> +\tif (found_sob > found_other)\n> \t\treturn 2;\n> +\tif (found_other > found_valid)\n> +\t\treturn 0;\n> \treturn 1;\n> }\n"},{"id":"282771","messageId":"20160406163726.GG28596@1wt.eu","threadId":"41680","inReplyTo":"xmqqr3eizsxu.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] sequencer.c: fix detection of duplicate s-o-b","fromName":"Willy Tarreau","fromEmail":"w@1wt.eu","sentAt":"2016-04-06T16:37:26Z","receivedAt":"2016-04-06T16:37:26Z","isPatch":true,"sender":{"key":"w@1wt.eu","avatar":"https://avatars.githubusercontent.com/u/8141789?v=4"},"body":"On Wed, Apr 06, 2016 at 07:57:01AM -0700, Junio C Hamano wrote:\n> This seems to have been lost, perhaps because the top part that was\n> quite long didn't look like a patch submission message or something.\n\nDon't worry, we all know it's the submitter's responsibility to retransmit,\nI apply the same principle :-)\n\n> Git 1.7.12 is a quite ancient release and I wouldn't be surprised if\n> we made the behaviour change during the period leading to v2.6 on\n> purpose, but nothing immediately comes to mind. Christian (as the\n> advocate for the trailer machinery) and Brandon (\"git shortlog\n> sequencer.c\" suggests you), can you take a look?\n\nFWIW it wad changed in 1.8.3 by commit bab4d10 (\"sequencer.c: teach\nappend_signoff how to detect duplicate s-o-b\").\n\nThe change made a lot of sense but it didn't assume that this practice\nwas common. And indeed I think this practice only happens in maintenance\nbranches where people have to make a lot of adaptations to existing\npatches that they're cherry-picking. We do that a lot in stable kernels\nto keep track of what we may need to revisit if we break something.\n\nThanks!\nWilly\n"},{"id":"282903","messageId":"CAP8UFD2OQfcogZj-hELxBB+-rWSW1YwPBCOXK1+oM2kKGaMwaA@mail.gmail.com","threadId":"41680","inReplyTo":"20160406163726.GG28596@1wt.eu","subject":"Re: [PATCH] sequencer.c: fix detection of duplicate s-o-b","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-04-07T20:06:59Z","receivedAt":"2016-04-07T20:06:59Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Apr 6, 2016 at 12:37 PM, Willy Tarreau wrote:\n> On Wed, Apr 06, 2016 at 07:57:01AM -0700, Junio C Hamano wrote:\n>> This seems to have been lost, perhaps because the top part that was\n>> quite long didn't look like a patch submission message or something.\n>\n> Don't worry, we all know it's the submitter's responsibility to retransmit,\n> I apply the same principle :-)\n>\n>> Git 1.7.12 is a quite ancient release and I wouldn't be surprised if\n>> we made the behaviour change during the period leading to v2.6 on\n>> purpose, but nothing immediately comes to mind. Christian (as the\n>> advocate for the trailer machinery) and Brandon (\"git shortlog\n>> sequencer.c\" suggests you), can you take a look?\n\nOk, I will try to have a look at that next week.\n\n> FWIW it wad changed in 1.8.3 by commit bab4d10 (\"sequencer.c: teach\n> append_signoff how to detect duplicate s-o-b\").\n\nSo the change is quite old and was made before I started working on\nthe trailer machinery.\n\n> The change made a lot of sense but it didn't assume that this practice\n> was common. And indeed I think this practice only happens in maintenance\n> branches where people have to make a lot of adaptations to existing\n> patches that they're cherry-picking. We do that a lot in stable kernels\n> to keep track of what we may need to revisit if we break something.\n\nYeah, we know for some time, but after the above patch breakage\nhappened and after I worked on interpret-trailers, that some lines\ninside [] are added by kernel people in the trailer part and that the\ntrailer machinery doesn't work properly with such lines.\n\nAnyway if you want your patch to be applied, it will probably need tests.\n\nThanks,\nChristian.\n"},{"id":"282904","messageId":"20160407201636.GA29322@1wt.eu","threadId":"41680","inReplyTo":"CAP8UFD2OQfcogZj-hELxBB+-rWSW1YwPBCOXK1+oM2kKGaMwaA@mail.gmail.com","subject":"Re: [PATCH] sequencer.c: fix detection of duplicate s-o-b","fromName":"Willy Tarreau","fromEmail":"w@1wt.eu","sentAt":"2016-04-07T20:16:36Z","receivedAt":"2016-04-07T20:16:36Z","isPatch":true,"sender":{"key":"w@1wt.eu","avatar":"https://avatars.githubusercontent.com/u/8141789?v=4"},"body":"Hi Christian,\n\nOn Thu, Apr 07, 2016 at 04:06:59PM -0400, Christian Couder wrote:\n> On Wed, Apr 6, 2016 at 12:37 PM, Willy Tarreau wrote:\n> > On Wed, Apr 06, 2016 at 07:57:01AM -0700, Junio C Hamano wrote:\n> >> This seems to have been lost, perhaps because the top part that was\n> >> quite long didn't look like a patch submission message or something.\n> >\n> > Don't worry, we all know it's the submitter's responsibility to retransmit,\n> > I apply the same principle :-)\n> >\n> >> Git 1.7.12 is a quite ancient release and I wouldn't be surprised if\n> >> we made the behaviour change during the period leading to v2.6 on\n> >> purpose, but nothing immediately comes to mind. Christian (as the\n> >> advocate for the trailer machinery) and Brandon (\"git shortlog\n> >> sequencer.c\" suggests you), can you take a look?\n> \n> Ok, I will try to have a look at that next week.\n> \n> > FWIW it wad changed in 1.8.3 by commit bab4d10 (\"sequencer.c: teach\n> > append_signoff how to detect duplicate s-o-b\").\n> \n> So the change is quite old and was made before I started working on\n> the trailer machinery.\n> \n> > The change made a lot of sense but it didn't assume that this practice\n> > was common. And indeed I think this practice only happens in maintenance\n> > branches where people have to make a lot of adaptations to existing\n> > patches that they're cherry-picking. We do that a lot in stable kernels\n> > to keep track of what we may need to revisit if we break something.\n> \n> Yeah, we know for some time, but after the above patch breakage\n> happened and after I worked on interpret-trailers, that some lines\n> inside [] are added by kernel people in the trailer part and that the\n> trailer machinery doesn't work properly with such lines.\n> \n> Anyway if you want your patch to be applied, it will probably need tests.\n\nThanks. I've discussed with Junio yesterday who notified me that my patch\nbroke some existing tests that I hadn't updated (I didn't know they existed)\nnamely one supposed to make the distinction between a normal footer and a\nspecial case where the s-o-b is in fact part of the last paragraph. So I\nsaid I'll rework the patch to only consider the parts between brackets\nabove a footer. Thus no need to waste your time testing the current patch\nnext week.\n\nThanks,\nWilly\n"}]}