Re: Skipping adding Signed-off-by even if it's not the last on git commit

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 7, 2022, 04:31 UTC
Message-ID: <221207.86y1rjako8.gmgdl@evledraar.gmail.com>
In-Reply-To: <Y4/xSObs9QXvE+xR@nand.local>

On Tue, Dec 06 2022, Taylor Blau wrote:

Show 55 quoted lines

> Hi David,
>
> On Tue, Dec 06, 2022 at 06:06:46PM +0100, David Caro wrote:
>> I have noticed that when requesting git commit to add the
>> Signed-off-by header, it will add it even if it already exists as long
>> as it's not the last in the footer.
>>
>> It seems that the functionality to do so has been there for a while
>> (see [1]) and it's being used effectively on format-patch, but it was
>> never enabled for the rest of commands.
>
> Right; bab4d1097c (sequencer.c: teach append_signoff how to detect
> duplicate s-o-b, 2013-02-12) introduced the APPEND_SIGNOFF_DEDUP flag,
> which instructs append_signoff() to avoid adding a Signed-off-by
> trailer if one already exists anywhere in the trailers section of the
> target commit.
>
> As bab4d1097c hints, this is desirable in some situations. However,
> adding duplicated S-o-b trailers is useful in other situations. For
> example, if you and I exchange a patch back and forth on this list, it
> might go something like:
>
>   1. I write up an initial patch, add my Singed-off-by, and send it to
>      the list.
>
>   2. You notice that some aspects can be improved, so you apply and
>      modify the commit locally, adding your own Signed-off-by.
>
>   3. I pick up your changes as part of a new version of the patch
>      series, and resubmit it to the list, adding my own Signed-off-by
>      again.
>
> There are a couple of small things worth noting there. First, in (2),
> the changes that you introduced were large enough to be worth crediting
> (e.g., you didn't suggest a typofix or to modify the amount of spacing
> on a line or similar), but not large enough to change the author of the
> patch.
>
> Likewise, in (3), the second instance of my Signed-off-by indicates that
> I am OK with the changes that you wrote, and I certify the new version
> of the patch as my own work.
>
> One such instance is in 0ca6ead81e (alias.c: reject too-long cmdline
> strings in split_cmdline(), 2022-09-28). Another is in b3c5f5cb04
> (submodule: move core cmd_update() logic to C, 2022-03-15).
>
> If you're curious to dig up more in your own project, here's a little
> shell snippet I wrote up to find some examples:
>
>     git rev-list --no-merges HEAD |
>     while read rev
>     do
>       git -P show -s -1 --format='%(trailers:key=Signed-off-by,unfold=true)' "$rev" |
>       grep -v '^$' | sort | uniq -c | grep -vq "^      1 " && echo "$rev"
>     done

This is pretty much what I would have said, i.e. that it is intentional, as the SOB has do do with the DCO chronology of events.

The SOB is a proxy for passing the copyrightable work around, and to certify that you have permission to license the work per the DCO etc.

But it's also interesting in that context that we choose to omit this for re-rolls. I.e.:

 1. I write a patch, that has 10 lines of original work, add a SOB
 2. You pick it up, add another 10 lines, add your SOB
 3. I pick it up again, add another 10 lines, add my SOB

So at the end we have a SOB sequence of: Ævar, Taylor, Ævar, and 30 copyrightable lines of code.

But if I submit three versions of my own patch with the same growth pattern over those three iterations shouldn't I have 3 of my own SOB lines: Ævar, Ævar, Ævar?

I'm not suggesting that's a sane idea, but at the same time I can't really square the internal logic of why we'd do one, but not the other.

If we assume that an earlier SOB from a v1 is carried forward to my own v2 and v3 re-rolls, shouldn't the same convention apply for my v1 SOB if you the v2 re-roll was yours?