Hi,

after I upgraded my machine, I switched from git 1.7.12.2 to 2.6.4
and experienced an annoying regression when dealing with stable
kernel backports.

I'm using a "dorelease" script which relies on git-cherry-pick's
ability to properly detect duplicate s-o-b to ensure that all merged
commits are properly signed in a release. Today while preparing the
last 2.6.32 release, I did a git log before pushing and found some
commits having two s-o-b lines with myself. I found that these ones
were always those containing some backporting notes between the s-o-b
lines (which we all do in stable branches to indicate what was changed
in the backport process).

I didn't feel brave enough to individually deal with each offending
patch by hand so instead I bisected the git changes and found that the
behaviour changed with commit bab4d10 ("sequencer.c: teach append_signoff
how to detect duplicate s-o-b").

The reason is that function has_conforming_footer() immediately stops
after the first non-conforming line without checking if there are
conforming lines after. But if someone added signed-off-by anywhere
after a non-conforming block, it should always be considered as part
of the footer. Thus I adjusted the logic to check till the end of the
footer and report the presence of valid rfc2822 or cherry-picked lines
after the last non-conformant one and now it correctly handles all types
of commits I had to deal with (ie: only adds s-o-b when it doesn't match
the last one and doesn't add an empty line after a conformant one). For
example, this footer :

    Signed-off-by: Mike Galbraith <umgwanakikb...@gmail.com>
    [bwh: Backported to 3.2:
     - Adjust numbering in the comment
     - Adjust filename]
    Signed-off-by: Ben Hutchings <b...@decadent.org.uk>
    Cc: Byungchul Park <byungchul.p...@lge.com>
    Cc: Peter Zijlstra <pet...@infradead.org>
    Cc: Willy Tarreau <w...@1wt.eu>

Used to be turned into this :

    Signed-off-by: Mike Galbraith <umgwanakikb...@gmail.com>
    [bwh: Backported to 3.2:
     - Adjust numbering in the comment
     - Adjust filename]
    Signed-off-by: Ben Hutchings <b...@decadent.org.uk>
    Cc: Byungchul Park <byungchul.p...@lge.com>
    Cc: Peter Zijlstra <pet...@infradead.org>
    Cc: Willy Tarreau <w...@1wt.eu>

    Signed-off-by: Willy Tarreau <w...@1wt.eu>

And is now properly converted to :

    Signed-off-by: Mike Galbraith <umgwanakikb...@gmail.com>
    [bwh: Backported to 3.2:
     - Adjust numbering in the comment
     - Adjust filename]
    Signed-off-by: Ben Hutchings <b...@decadent.org.uk>
    Cc: Byungchul Park <byungchul.p...@lge.com>
    Cc: Peter Zijlstra <pet...@infradead.org>
    Cc: Willy Tarreau <w...@1wt.eu>
    Signed-off-by: Willy Tarreau <w...@1wt.eu>

Also, cherry-picking the last commit above again would produce this
before :

    Signed-off-by: Mike Galbraith <umgwanakikb...@gmail.com>
    [bwh: Backported to 3.2:
     - Adjust numbering in the comment
     - Adjust filename]
    Signed-off-by: Ben Hutchings <b...@decadent.org.uk>
    Cc: Byungchul Park <byungchul.p...@lge.com>
    Cc: Peter Zijlstra <pet...@infradead.org>
    Cc: Willy Tarreau <w...@1wt.eu>
    Signed-off-by: Willy Tarreau <w...@1wt.eu>

    Signed-off-by: Willy Tarreau <w...@1wt.eu>

And it now is properly left untouched since the last s-o-b line
is properly matched.

I'm appending the patch, please include it upstream.

Thanks!
Willy


>From be9624a0df4c649d452f898925953a81dc9163fc Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w...@1wt.eu>
Date: Sat, 12 Mar 2016 13:35:35 +0100
Subject: sequencer.c: fix detection of duplicate s-o-b

Commit bab4d10 ("sequencer.c: teach append_signoff how to detect
duplicate s-o-b") changed the method used to detect duplicate s-o-b,
but it introduced a regression for a case where some non-compliant
information are present in the footer. In maintenance branches, it's
very common to add some elements after the signed-off and to add your
s-o-b after. This is used a lot in the stable kernel series, for
example this commit backported from 3.2 to 2.6.32 :

    ALSA: usb-audio: avoid freeing umidi object twice

    commit 07d86ca93db7e5cdf4743564d98292042ec21af7 upstream.

    The 'umidi' object will be free'd on the error path by snd_usbmidi_free()
    when tearing down the rawmidi interface. So we shouldn't try to free it
    in snd_usbmidi_create() after having registered the rawmidi interface.

    Found by KASAN.

    Signed-off-by: Andrey Konovalov <andreyk...@gmail.com>
    Acked-by: Clemens Ladisch <clem...@ladisch.de>
    Signed-off-by: Takashi Iwai <ti...@suse.de>
    Signed-off-by: Ben Hutchings <b...@decadent.org.uk>
    [wt: file is sound/midi/usbmidi.c in 2.6.32]
    Signed-off-by: Willy Tarreau <w...@1wt.eu>

Prior to the commit above, a cherry-pick -s would not append an extra s-o-b.
After this commit, a new line and a second s-o-b are added, making the footer
look like this :

    Signed-off-by: Andrey Konovalov <andreyk...@gmail.com>
    Acked-by: Clemens Ladisch <clem...@ladisch.de>
    Signed-off-by: Takashi Iwai <ti...@suse.de>
    Signed-off-by: Ben Hutchings <b...@decadent.org.uk>
    [wt: file is sound/midi/usbmidi.c in 2.6.32]
    Signed-off-by: Willy Tarreau <w...@1wt.eu>

    Signed-off-by: Willy Tarreau <w...@1wt.eu>

This patch improves the parsing of the footer by considering the
presence of a valid rfc2822 line after possibly non-conformant lines.
Indeed, if someone added an s-o-b or CC after some stuff, this line
must properly be considered as part of the footer and not of the body.

Signed-off-by: Willy Tarreau <w...@1wt.eu>
---
 sequencer.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e66f2fe..ab2c18d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -64,6 +64,8 @@ static int has_conforming_footer(struct strbuf *sb, struct 
strbuf *sob,
        int len = sb->len - ignore_footer;
        const char *buf = sb->buf;
        int found_sob = 0;
+       int found_valid = 0;
+       int found_other = 0;
 
        /* footer must end with newline */
        if (!len || buf[len - 1] != '\n')
@@ -96,15 +98,18 @@ static int has_conforming_footer(struct strbuf *sb, struct 
strbuf *sob,
                if (found_rfc2822 && sob &&
                    !strncmp(buf + i, sob->buf, sob->len))
                        found_sob = k;
-
-               if (!(found_rfc2822 ||
-                     is_cherry_picked_from_line(buf + i, k - i - 1)))
-                       return 0;
+               else if (found_rfc2822 ||
+                        is_cherry_picked_from_line(buf + i, k - i - 1))
+                       found_valid = k;
+               else
+                       found_other = k;
        }
        if (found_sob == i)
                return 3;
-       if (found_sob)
+       if (found_sob > found_other)
                return 2;
+       if (found_other > found_valid)
+               return 0;
        return 1;
 }
 
-- 
2.6.4

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to