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

From: Taylor Blau <me@ttaylorr.com>
Date: Dec 7, 2022, 01:50 UTC
Message-ID: <Y4/xSObs9QXvE+xR@nand.local>
In-Reply-To: <20221206170646.6lnpr6h7oprziy5b@vulcanus>

Hi David,

On Tue, Dec 06, 2022 at 06:06:46PM +0100, David Caro wrote:

Show 7 quoted lines

> 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

> I propose enabling it for commit, merge and am.

So I think that there are some legitimate uses outside of 'format-patch' that we may want to keep the existing behavior. So I don't think we should necessarily change the default to have other commands outside of 'format-patch' start passing APPEND_SIGNOFF_DEDUP.

But I could see a future where we add a new option that controls whether or not we pass that flag, perhaps:

    $ git commit --signoff[=[no-]dedup]

Thanks, Taylor