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
