threads / patch / 44781

patcham: add am.signoff add config variable

Subject: [PATCH] am: add am.signoff add config variable

## tl;dr

11 messages between Dec 28, 2016 and Dec 29, 2016. Diffs are folded; open one to read it.

replies: 10people: 5as markdown or json

Eduardo Habkost· Dec 28, 2016, 17:40 UTC · lore

git-am has options to enable --message-id and --3way by default, but no option to enable --signoff by default. Add a "am.signoff" config option.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 builtin/am.c | 2 ++
 1 file changed, 2 insertions(+)
Show changes to builtin/am.c +2 −0
diff --git a/builtin/am.c b/builtin/am.c
index 31fb605..d2e0233 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -154,6 +154,8 @@ static void am_state_init(struct am_state *state, const char *dir)
 
 	git_config_get_bool("am.messageid", &state->message_id);
 
+	git_config_get_bool("am.signoff", &state->signoff);
+
 	state->scissors = SCISSORS_UNSET;
 
 	argv_array_init(&state->git_apply_opts);

-- 
2.7.4
Stefan Beller· Dec 28, 2016, 17:45 UTC · re: Eduardo Habkost · lore

Re: [PATCH] am: add am.signoff add config variable

On Wed, Dec 28, 2016 at 9:40 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> git-am has options to enable --message-id and --3way by default,
> but no option to enable --signoff by default. Add a "am.signoff"
> config option.
I think this is a good idea (from a design standpoint and what the user needs).

Just like e97a5e765d (git-am: add am.threeWay config variable), we'd prefer if you'd also update Documentation/config.txt as well as a new test. :)

Thanks, Stefan

Eduardo Habkost· Dec 28, 2016, 17:50 UTC · re: Stefan Beller · lore

Re: [PATCH] am: add am.signoff add config variable

On Wed, Dec 28, 2016 at 09:45:24AM -0800, Stefan Beller wrote:
Show 10 quoted lines
> On Wed, Dec 28, 2016 at 9:40 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > git-am has options to enable --message-id and --3way by default,
> > but no option to enable --signoff by default. Add a "am.signoff"
> > config option.
> 
> I think this is a good idea (from a design standpoint and what the user needs).
> 
> Just like e97a5e765d (git-am: add am.threeWay config variable), we'd
> prefer if you'd
> also update Documentation/config.txt as well as a new test. :)

Sorry, I was using commit e97a5e765d as reference when adding the new option, but I was looking at "git log -p builtin/am.c" and didn't see the rest of commit. :)

I will send a new version with the appropriate documentation and test code.

-- 
Eduardo
Eric Wong· Dec 29, 2016, 08:47 UTC · re: Eduardo Habkost · lore

Re: [PATCH] am: add am.signoff add config variable

Eduardo Habkost <ehabkost@redhat.com> wrote:
> git-am has options to enable --message-id and --3way by default,
> but no option to enable --signoff by default. Add a "am.signoff"
> config option.

I'm not sure this is a good idea. IANAL, but a sign-off has some sort of legal meaning for this project (DCO) and that would be better decided on a patch-by-patch basis rather than a blanket statement.

I don't add my SoB to patches (either my own or received) until I'm comfortable with it; and I'd rather err on the side of forgetting and being prodded to resubmit rather than putting an SoB on the wrong patch.

(I'm barely online today, no rush needed in responding)
Eduardo Habkost· Dec 29, 2016, 15:45 UTC · re: Eric Wong · lore

Re: [PATCH] am: add am.signoff add config variable

On Thu, Dec 29, 2016 at 08:47:01AM +0000, Eric Wong wrote:
Show 14 quoted lines
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> > git-am has options to enable --message-id and --3way by default,
> > but no option to enable --signoff by default. Add a "am.signoff"
> > config option.
> 
> I'm not sure this is a good idea.  IANAL, but a sign-off
> has some sort of legal meaning for this project (DCO)
> and that would be better decided on a patch-by-patch basis
> rather than a blanket statement.
> 
> I don't add my SoB to patches (either my own or received) until
> I'm comfortable with it; and I'd rather err on the side of
> forgetting and being prodded to resubmit rather than putting
> an SoB on the wrong patch.

This is an interesting point. I am not competent to argue about it, so I will let the Git developers decide.

-- 
Eduardo
Jacob Keller· Dec 29, 2016, 17:49 UTC · re: Eric Wong · lore

Re: [PATCH] am: add am.signoff add config variable

On December 29, 2016 12:47:01 AM PST, Eric Wong <e@80x24.org> wrote:
Show 17 quoted lines
>Eduardo Habkost <ehabkost@redhat.com> wrote:
>> git-am has options to enable --message-id and --3way by default,
>> but no option to enable --signoff by default. Add a "am.signoff"
>> config option.
>
>I'm not sure this is a good idea.  IANAL, but a sign-off
>has some sort of legal meaning for this project (DCO)
>and that would be better decided on a patch-by-patch basis
>rather than a blanket statement.
>
>I don't add my SoB to patches (either my own or received) until
>I'm comfortable with it; and I'd rather err on the side of
>forgetting and being prodded to resubmit rather than putting
>an SoB on the wrong patch.
>
>
>(I'm barely online today, no rush needed in responding)
I don't know what is true for all projects, but I can't believe making it configurable would cause problems.... If you don't want it you don't have to enable it.
I suppose your argument is that we shouldn't ever make it automatically but... I know that many people who receive email patches, apply them, then forward those to another group add their sign off as a mark of "yes I certify that this is legally OK" so I think this would be useful.

Thanks Jake

Junio C Hamano· Dec 29, 2016, 21:42 UTC · re: Eric Wong · lore

Re: [PATCH] am: add am.signoff add config variable

Eric Wong <e@80x24.org> writes:
Show 9 quoted lines
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>> git-am has options to enable --message-id and --3way by default,
>> but no option to enable --signoff by default. Add a "am.signoff"
>> config option.
>
> I'm not sure this is a good idea.  IANAL, but a sign-off
> has some sort of legal meaning for this project (DCO)
> and that would be better decided on a patch-by-patch basis
> rather than a blanket statement.
IANAL either, but we have been striving to keep output of
   $ git grep '\.signoff' Documentation
empty to keep Sign-off meaningful.

Adding more publicized ways to add SoB without thinking will make it harder to argue against one who tells the court "that log message ends with a SoB by person X but it is very plausible that it was done by inertia without person X really intending to certify what DCO says, and the SoB is meaningless".

> I don't add my SoB to patches (either my own or received) until
> I'm comfortable with it; and I'd rather err on the side of
> forgetting and being prodded to resubmit rather than putting
> an SoB on the wrong patch.
Eduardo Habkost· Dec 29, 2016, 22:16 UTC · re: Junio C Hamano · lore

Re: [PATCH] am: add am.signoff add config variable

On Thu, Dec 29, 2016 at 01:42:13PM -0800, Junio C Hamano wrote:
Show 23 quoted lines
> Eric Wong <e@80x24.org> writes:
> 
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> git-am has options to enable --message-id and --3way by default,
> >> but no option to enable --signoff by default. Add a "am.signoff"
> >> config option.
> >
> > I'm not sure this is a good idea.  IANAL, but a sign-off
> > has some sort of legal meaning for this project (DCO)
> > and that would be better decided on a patch-by-patch basis
> > rather than a blanket statement.
> 
> IANAL either, but we have been striving to keep output of
> 
>    $ git grep '\.signoff' Documentation
> 
> empty to keep Sign-off meaningful. 
> 
> Adding more publicized ways to add SoB without thinking will make it
> harder to argue against one who tells the court "that log message
> ends with a SoB by person X but it is very plausible that it was
> done by inertia without person X really intending to certify what
> DCO says, and the SoB is meaningless".

This sounds completely reasonable to me. I now see that the config option was already proposed in 2011 and the same arguments were discussed. Sorry for the noise.

Show 6 quoted lines
> 
> > I don't add my SoB to patches (either my own or received) until
> > I'm comfortable with it; and I'd rather err on the side of
> > forgetting and being prodded to resubmit rather than putting
> > an SoB on the wrong patch.
> 
-- 
Eduardo
Stefan Beller· Dec 29, 2016, 22:18 UTC · re: Junio C Hamano · lore

Re: [PATCH] am: add am.signoff add config variable

On Thu, Dec 29, 2016 at 1:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> Eric Wong <e@80x24.org> writes:
>
>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>> git-am has options to enable --message-id and --3way by default,
>>> but no option to enable --signoff by default. Add a "am.signoff"
>>> config option.
>>
>> I'm not sure this is a good idea.  IANAL, but a sign-off
>> has some sort of legal meaning for this project (DCO)
>> and that would be better decided on a patch-by-patch basis
>> rather than a blanket statement.
>
> IANAL either, but we have been striving to keep output of
>
>    $ git grep '\.signoff' Documentation

Try again with -i ;) and you'll find format.signOff

Show 8 quoted lines
>
> empty to keep Sign-off meaningful.
>
> Adding more publicized ways to add SoB without thinking will make it
> harder to argue against one who tells the court "that log message
> ends with a SoB by person X but it is very plausible that it was
> done by inertia without person X really intending to certify what
> DCO says, and the SoB is meaningless".

I think we should be symmetrical, am is the opposite of format-patch in the Git <-> email conversion.

If I were to follow your arguments here, we should revert 1d1876e930 (Add configuration variable for sign-off to format-patch)

On the other hand, I would argue that thinking and typing things is orthogonal (the more you type, doesn't imply that you think harder or even at all).

-- However I think there is a use case for such an option (in the short term?) as e.g. you as the Git maintainer being employed has the legal rights to sign off on pretty much any patch in Git. For other projects this may be different.

Which is why a per-repository configurable thing is useful to setup a default for your environment.

IIUC long term we rather want to have easily configurable trailers for am/format-patch/commit, such that you could configure to remove any "ChangeId:" footers before sending out, or adding a "Tested-by" footer by a CI system or such?

>
>> I don't add my SoB to patches (either my own or received) until
>> I'm comfortable with it;

comfortable is orthogonal to legal, specifically on the receiving side. I can understand using format-patch to send out unsigned patches (e.g. for heavy WIP things, "please to not apply literally")

>> and I'd rather err on the side of
>> forgetting and being prodded to resubmit rather than putting
>> an SoB on the wrong patch.
>
Junio C Hamano· Dec 29, 2016, 22:43 UTC · re: Stefan Beller · lore

Re: [PATCH] am: add am.signoff add config variable

Stefan Beller <sbeller@google.com> writes:
Show 9 quoted lines
>> IANAL either, but we have been striving to keep output of
>>
>>    $ git grep '\.signoff' Documentation
>
>>
>> empty to keep Sign-off meaningful.
>
> Try again with -i ;)
> and you'll find format.signOff

Mistakes happen. Finding an old mistake is not an excuse for you to make the same one again. It is an opportunity to come up with a way to correct it without hurting existing users by designing a smooth transition path.

Stefan Beller· Dec 29, 2016, 23:34 UTC · re: Junio C Hamano · lore

Re: [PATCH] am: add am.signoff add config variable

On Thu, Dec 29, 2016 at 2:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>>> IANAL either, but we have been striving to keep output of
>>>
>>>    $ git grep '\.signoff' Documentation
>>
>>>
>>> empty to keep Sign-off meaningful.
>>
>> Try again with -i ;)
>> and you'll find format.signOff
>
> Mistakes happen.  Finding an old mistake is not an excuse for you to
> make the same one again.
> It is an opportunity to come up with a way
> to correct it without hurting existing users by designing a smooth
> transition path.
>
knee jerk reaction:
1) Document why format.signoff is bad (even after a long
  office discussion I am not fully convinced it is bad. That
  may be because I am biased as I find format.signoff *very*
  useful. The cumbersome contribution process as laid out
  by SubmittingPatches just got easier for me as I have one
  step less to worry about. I haven't made a mistake so far
  sending out crap where I'll throw a temper tantrum if you
  apply it.
  So I would expect a maintainer of a project that uses
  email based workflow to write that documentation giving
  reasons. That person is currently consuming the automatically
  signed off patches, so I'd want to know their line of thinking.
2) If the config option is set, but no explicit sign off is given,
  put a different footer, e.g.
    git config format.signoff true &&
    git format-patch HEAD^
  may produce Auto-Signed-Off-By: ...
  whereas
    git -c format.signoff format-patch
  behaves the same as
    git format-patch --signoff
  that gives the Signed-Off-By as we know it.
  It is up to the upstream project to accept these new sign offs.
3) (later) warn about the option if it is set, giving the text from 1)
4) (a long time later) remove the option.

-- For 2) I am not sure what we want there, because this has to happen in collaboration with all the upstream projects that use sign offs.

We could be subtle, i.e. just use all lowercase / all uppercase letters for this differentiation. Then automated tools that check for signoff are easily adjusted. e.g. The eclipse foundation disallows pushing for review if any patch is missing a signoff; Gerrit can check for that. Gerrit is not case sensitive when checking for a footer.

I am not sure if there are any other tools out there that automatically check for that, but I would assume they are also case insensitive in such a case, as it is unclear to me how to properly capitalize the sign off.

This is an easy way forward for upstream projects., though confusing in court later on. -- We could also be non-subtle, very explicit, and each tool that can add sign offs currently, needs to be explicit about itself:

    Configured-Formatpatch-Signed-Off: (for git format.signoff with config)
    # and others:
    Explicit-Formatpatch-Signed-Off:
    Git-Gui-Button-Clicked-Signed-Off:
    Git-Gui-Button-Shortcut-Signed-Off:
Once we have that we could add much more of these:
    Configured-Commit-Signed-Off:
    etc.
You can continue to sign off via just typing it, or by
    I-typed-it-signed-Off,
--
One of the problems highlighted to me was that you could have accidentally
configured format.signoff globally, but you're only allowed/desire to sign off
in a particular repository, such that
    Repolocal-Configured-Formatpatch-Signed-Off:
    Global-Configured-Formatpatch-Signed-Off:

may be worth discussing. --

← back to recent threads