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

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

[David Caro]

[-- Attachment #1: Type: text/plain, Size: 856 bytes --]


Hi!

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.

I propose enabling it for commit, merge and am.

I can easily send a patch for it if the change is welcome.


Thanks!

[1] https://github.com/git/git/commit/bab4d1097c8be7d688a53b992232063dbf300fd4

-- 
David Caro
SRE - Cloud Services
Wikimedia Foundation <https://wikimediafoundation.org/>
PGP Signature: 7180 83A2 AC8B 314F B4CE  1171 4071 C7E1 D262 69C3

"Imagine a world in which every single human being can freely share in the
sum of all knowledge. That's our commitment."

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Taylor Blau]

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

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

>> 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]

That sounds sensible.

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

[David Caro]

[-- Attachment #1: Type: text/plain, Size: 984 bytes --]

On 12/07 13:11, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
> 
> >> 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]
> 
> That sounds sensible.

Agree, I will implement this then for commit for now, thanks!

-- 
David Caro
SRE - Cloud Services
Wikimedia Foundation <https://wikimediafoundation.org/>
PGP Signature: 7180 83A2 AC8B 314F B4CE  1171 4071 C7E1 D262 69C3

"Imagine a world in which every single human being can freely share in the
sum of all knowledge. That's our commitment."

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Taylor Blau]

On Wed, Dec 07, 2022 at 09:40:27AM +0100, David Caro wrote:
> On 12/07 13:11, Junio C Hamano wrote:
> > Taylor Blau <me@ttaylorr.com> writes:
> >
> > >> 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]
> >
> > That sounds sensible.
>
> Agree, I will implement this then for commit for now, thanks!

Thanks, I look forward to seeing your work. It would be nice to
standardize on this `--signoff[=[no-]dedup]` thing throughout all of the
different commands that support it.

According to:

    $ git grep -- '--signoff' -- Documentation/git-*.txt

that list is: `git am`, `git cherry-pick`, `git format-patch`, `git
rebase`, and `git revert`. But there are others, too, via an include of
`signoff-option.txt`, so that is `git commit`, `git merge`, and `git
pull`.

Phew ;-).

Thanks,
Taylor

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

>> > >     $ git commit --signoff[=[no-]dedup]
> ...
> Thanks, I look forward to seeing your work. It would be nice to
> standardize on this `--signoff[=[no-]dedup]` thing throughout all of the
> different commands that support it.

Also, if I am not mistaken, each of trailers can be configured to
have its own semantics (e.g. .where and .ifExists).

 * Should we have similar override to these trailer tokens, not just
   sign-off?

 * Should we offer not just [no-]dedup (which is equivalent to
   switching from addIfDifferentNeighbor to addIfDifferent) but
   other customization?  This affects what --signoff should be
   allowed to take for consistency across trailers.

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

[Jeff King]

On Thu, Dec 08, 2022 at 08:04:46AM +0900, Junio C Hamano wrote:

> Taylor Blau <me@ttaylorr.com> writes:
> 
> >> > >     $ git commit --signoff[=[no-]dedup]
> > ...
> > Thanks, I look forward to seeing your work. It would be nice to
> > standardize on this `--signoff[=[no-]dedup]` thing throughout all of the
> > different commands that support it.
> 
> Also, if I am not mistaken, each of trailers can be configured to
> have its own semantics (e.g. .where and .ifExists).
> 
>  * Should we have similar override to these trailer tokens, not just
>    sign-off?

This made me curious about the opposite: is there config you can set to
get this behavior for --signoff? I think the answer is "no". You can do:

  git -c trailer.ifExists=addIfDifferent \
      commit --amend --trailer="Signed-off-by: Jeff King <peff@peff.net>"

to get the desired behavior, but using "--signoff" does not respect
trailer settings.

I feel like config is probably a better match for the use cases here,
because the decision about de-duping is not something you'd usually set
for one particular operation, but is more likely to be a project policy
about what the trailer means (and that includes Signed-off-by). So you'd
want to set it per-repo, not per-operation.

>  * Should we offer not just [no-]dedup (which is equivalent to
>    switching from addIfDifferentNeighbor to addIfDifferent) but
>    other customization?  This affects what --signoff should be
>    allowed to take for consistency across trailers.

Yeah, writing the above made me wonder if --signoff should behave
exactly like:

  --trailer="Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"

For the most part that would not change any behavior unless you set
trailer.signed-off-by.*, and setting those presumably signals intent
that you expect them to take effect. The one exception is that the
generic trailer.ifExists, etc, would start affecting --signoff, which
_might_ be a surprise. If we wanted to retain the behavior there, we
could say "--signoff is special, and doesn't respect generic trailer
config".

Alternatively, it would be nice if there was an easy way to put your
ident into a trailer (what I wrote above doesn't really work unless you
have those variables in your environment, and of course it's a lot of
typing). I think you can hack it up like:

  git config trailer.sign.key Signed-off-by
  git config trailer.sign.cmd \
    'git var GIT_COMMITTER_IDENT | sed "s/>.*/>/";:'
  git commit --trailer=sign

which is only a little more typing than --signoff, but it's not very
ergonomic.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This made me curious about the opposite: is there config you can set to
> get this behavior for --signoff? I think the answer is "no". You can do:
>
>   git -c trailer.ifExists=addIfDifferent \
>       commit --amend --trailer="Signed-off-by: Jeff King <peff@peff.net>"
>
> to get the desired behavior, but using "--signoff" does not respect
> trailer settings.

A customization to allow addIfDifferent may be an interesting idea.

> I feel like config is probably a better match for the use cases here,
> because the decision about de-duping is not something you'd usually set
> for one particular operation, but is more likely to be a project policy
> about what the trailer means (and that includes Signed-off-by). So you'd
> want to set it per-repo, not per-operation.

Sure.

> ... The one exception is that the
> generic trailer.ifExists, etc, would start affecting --signoff, which
> _might_ be a surprise. If we wanted to retain the behavior there, we
> could say "--signoff is special, and doesn't respect generic trailer
> config".

Yeah, that may be safe, however it is very unsatisfying.

> Alternatively, it would be nice if there was an easy way to put your
> ident into a trailer (what I wrote above doesn't really work unless you
> have those variables in your environment, and of course it's a lot of
> typing). I think you can hack it up like:
>
>   git config trailer.sign.key Signed-off-by
>   git config trailer.sign.cmd \
>     'git var GIT_COMMITTER_IDENT | sed "s/>.*/>/";:'
>   git commit --trailer=sign
>
> which is only a little more typing than --signoff, but it's not very
> ergonomic.

It does not look _too_ bad, though.

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

[Jeff King]

On Fri, Dec 09, 2022 at 10:23:52AM +0900, Junio C Hamano wrote:

> > ... The one exception is that the
> > generic trailer.ifExists, etc, would start affecting --signoff, which
> > _might_ be a surprise. If we wanted to retain the behavior there, we
> > could say "--signoff is special, and doesn't respect generic trailer
> > config".
> 
> Yeah, that may be safe, however it is very unsatisfying.

Agreed. ;)

> > Alternatively, it would be nice if there was an easy way to put your
> > ident into a trailer (what I wrote above doesn't really work unless you
> > have those variables in your environment, and of course it's a lot of
> > typing). I think you can hack it up like:
> >
> >   git config trailer.sign.key Signed-off-by
> >   git config trailer.sign.cmd \
> >     'git var GIT_COMMITTER_IDENT | sed "s/>.*/>/";:'
> >   git commit --trailer=sign
> >
> > which is only a little more typing than --signoff, but it's not very
> > ergonomic.
> 
> It does not look _too_ bad, though.

What I don't like about it is:

  - the external cmd is complicated and slow. It would be nice if you
    could just set trailer.sign.ident=true or something, and it would
    use your ident by default if no value is given (and maybe even do
    completion similar to "commit --author" if a value is given).

  - you have to know to be clever enough to define and use
    --trailer=sign. If --signoff didn't exist, that's not too big a
    stretch. But since it does, everyone will naturally reach for it
    first.

The two could be solved independently, of course. And even if we did
want to solve both by making --signoff behave more like other trailers,
having trailer.*.ident might be nice in general.

I admit I just thought of this concept, though, so I don't have plans to
work up patches, and I make no claims there isn't a gotcha waiting
somewhere. ;)

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> >   git config trailer.sign.key Signed-off-by
>> >   git config trailer.sign.cmd \
>> >     'git var GIT_COMMITTER_IDENT | sed "s/>.*/>/";:'
>> >   git commit --trailer=sign
>> >
>> > which is only a little more typing than --signoff, but it's not very
>> > ergonomic.
>> 
>> It does not look _too_ bad, though.
>
> What I don't like about it is:
>
>   - the external cmd is complicated and slow. It would be nice if you
>     could just set trailer.sign.ident=true or something, and it would
>     use your ident by default if no value is given (and maybe even do
>     completion similar to "commit --author" if a value is given).

Ah, "trailer.sign.value" to use the same value does not exist, and
the closest kludge we can use is the .cmd thing?  Then it is a shame
but it should be easy to correct?

>   - you have to know to be clever enough to define and use
>     --trailer=sign. If --signoff didn't exist, that's not too big a
>     stretch. But since it does, everyone will naturally reach for it
>     first.

We could reimplement --signoff to actually take attention to the
"trailer.sign.*" thing, if we wanted to, and that makes it very easy
to explain, I guess.

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

[Jeff King]

On Fri, Dec 09, 2022 at 01:03:44PM +0900, Junio C Hamano wrote:

> > What I don't like about it is:
> >
> >   - the external cmd is complicated and slow. It would be nice if you
> >     could just set trailer.sign.ident=true or something, and it would
> >     use your ident by default if no value is given (and maybe even do
> >     completion similar to "commit --author" if a value is given).
> 
> Ah, "trailer.sign.value" to use the same value does not exist, and
> the closest kludge we can use is the .cmd thing?  Then it is a shame
> but it should be easy to correct?

Yeah, I almost suggested trailer.sign.value at first, but on further
thought, it would be kind of weird. When the user asks for a trailer to
be included, it usually already has a value along with it. This could be
thought of as a default value, which might work like:

  git config trailer.signed-off-by.default "Jeff King <peff@peff.net>"

That's pretty flexible, but it seems like what you really want is not
some string, but to say "my usual committer ident". And so we force you
to repeat what's in user.email, etc, or we have to invent some new token
that means "my usual ident". So if we instead have a flag for "this is
an ident", then it can just do the expected thing.

> >   - you have to know to be clever enough to define and use
> >     --trailer=sign. If --signoff didn't exist, that's not too big a
> >     stretch. But since it does, everyone will naturally reach for it
> >     first.
> 
> We could reimplement --signoff to actually take attention to the
> "trailer.sign.*" thing, if we wanted to, and that makes it very easy
> to explain, I guess.

Yeah. My only reservation would be whether anybody would be surprised by
the change in behavior. I have the feeling that hardly anybody uses
trailer.* config, but I admit that's not based in any real data.

-Peff

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

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 06 2022, Taylor Blau wrote:

> 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?

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> 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?

The difference, I think, is that it is not worth recording if it is
just you regurgitated the same idea a few times yourself without
input from others.

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

[Konstantin Ryabitsev]

On Wed, Dec 07, 2022 at 05:31:18AM +0100, Ævar Arnfjörð Bjarmason wrote:
> 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 sure the trailer order matters much for the copyright info, because
after all this information is in the output of "git blame" and "git log". That
said, "trailer order" is a touchy subject with kernel developers, because
it means different things to pretty much every kernel maintainer. With b4, I'm
sticking to the "chain of custody" (COC) approach, which treats "S-o-b"
trailers as markers for where the chain of custody boundaries are.

For example, consider the following patch:

    | [PATCH] foo: implement libbar
    |
    | This patch implements libbar.
    |
	| Suggested-by: Reporter 1 <...>
    | Link: https://msgid.link/some-msgid
	| Signed-off-by: Developer 1 <...>     -- initial COC boundary
	| Reviewed-by: Reviewer 1 <...>
	| Tested-by: Tester 1 <...>
	| Signed-off-by: Submaintainer 1 <...> -- intermediate COC boundary
	| Acked-by: Submaintainer 2 <...>
	| Signed-off-by: Maintainer 1 <...>    -- final COC boundary

In terms of COC, this patch makes the following claims:

Developer 1:
 - I am responsible for this code
 - It was suggested by Reporter 1
 - You can read about it at this URL

Submaintainer 1:
 - I am signing off on this code
 - I am the one who added the trailers from Reviewer 1 and Tester 1

Maintainer 1:
 - I am signing off on this code
 - I am the one who added the trailer from Submaintainer 2

In the chain-of-custody scheme multiple identical trailers don't make sense,
as far as I can tell. If the patch doesn't pass review and someone returns it
back to the original developer, their "S-o-b: Developer 1" trailer simply
moves down to be the last entry in v2 of the patch, and the developer should
remove any code review trailers that were issued for the previous version of
the patch.

This is really the only order that makes sense to me. :)

-K