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

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

[Eduardo Habkost]

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(+)

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

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

[Stefan Beller]

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

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

[Eduardo Habkost]

On Wed, Dec 28, 2016 at 09:45:24AM -0800, Stefan Beller wrote:
> 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

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

[Eric Wong]

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)

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

[Eduardo Habkost]

On Thu, Dec 29, 2016 at 08:47:01AM +0000, Eric Wong wrote:
> 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

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

[Jacob Keller]

On December 29, 2016 12:47:01 AM PST, Eric Wong <e@80x24.org> wrote:
>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

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

[Junio C Hamano]

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".

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

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

[Eduardo Habkost]

On Thu, Dec 29, 2016 at 01:42:13PM -0800, Junio C Hamano wrote:
> 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.

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

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

[Stefan Beller]

On Thu, Dec 29, 2016 at 1:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

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

[Junio C Hamano]

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.

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

[Stefan Beller]

On Thu, Dec 29, 2016 at 2:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.
--