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

[Taylor Blau <me@ttaylorr.com>]

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

> 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