[PATCH 1/1] git-am: provide configuration to enable signoff by default

[PATCH 1/1] git-am: provide configuration to enable signoff by default

[Sekhar Nori]

Provide a git config option to enable --signoff a
default when using git-am. This should be handy
for maintainers who regularly apply patches from
mailing lists to send them upstream and want to
be on the sign-off path.

Signed-off-by: Sekhar Nori <nsekhar@ti.com>
---
 Documentation/config.txt |    7 +++++++
 Documentation/git-am.txt |    3 ++-
 git-am.sh                |    5 +++++
 3 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6b93777..5da7ca8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -597,6 +597,13 @@ am.keepcr::
 	by giving '--no-keep-cr' from the command line.
 	See linkgit:git-am[1], linkgit:git-mailsplit[1].
 
+am.signoff::
+	A boolean value which lets you enable the `-s/--signoff` option of
+	am by default. *Note:* Adding the Signed-off-by: line to a patch
+	should be a conscious act and means that you certify you have
+	the rights to submit this work under the same open source license.
+	Please see the 'SubmittingPatches' document for further discussion.
+
 apply.ignorewhitespace::
 	When set to 'change', tells 'git apply' to ignore changes in
 	whitespace, in the same way as the '--ignore-space-change'
diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index 6b1b5af..6b2c51a 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -33,7 +33,8 @@ OPTIONS
 -s::
 --signoff::
 	Add a `Signed-off-by:` line to the commit message, using
-	the committer identity of yourself.
+	the committer identity of yourself. `am.signoff` configuration
+	variable can be used to make this the defaut.
 
 -k::
 --keep::
diff --git a/git-am.sh b/git-am.sh
index 6cdd591..8e2a693 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -328,6 +328,11 @@ then
     keepcr=t
 fi
 
+if test "$(git config --bool --get am.signoff)" = true
+then
+    sign=t
+fi
+
 while test $# != 0
 do
 	case "$1" in
-- 
1.7.3.2

Re: [PATCH 1/1] git-am: provide configuration to enable signoff by default

[Sverre Rabbelier]

Heya,

On Wed, Jun 1, 2011 at 03:42, Sekhar Nori <nsekhar@ti.com> wrote:
> Provide a git config option to enable --signoff a
> default when using git-am. This should be handy
> for maintainers who regularly apply patches from
> mailing lists to send them upstream and want to
> be on the sign-off path.

Wouldn't this need tests?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 1/1] git-am: provide configuration to enable signoff by default

[Jeff King]

On Wed, Jun 01, 2011 at 02:12:31PM +0530, Sekhar Nori wrote:

> Provide a git config option to enable --signoff a
> default when using git-am. This should be handy
> for maintainers who regularly apply patches from
> mailing lists to send them upstream and want to
> be on the sign-off path.

I think people have complained about auto-signoff features before,
because it was supposed to be a conscious act. I don't recall if it was
ever resolved, and I don't have an opinion myself, but it may be worth
searching the archives for past arguments.

> diff --git a/git-am.sh b/git-am.sh
> index 6cdd591..8e2a693 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -328,6 +328,11 @@ then
>      keepcr=t
>  fi
>  
> +if test "$(git config --bool --get am.signoff)" = true
> +then
> +    sign=t
> +fi
> +
>  while test $# != 0
>  do
>  	case "$1" in

Shouldn't this be turned off if --rebasing is passed? Otherwise you
would introduce signoffs during rebase (I didn't check, though, so maybe
there is some other mechanism preventing it).

-Peff

Re: [PATCH 1/1] git-am: provide configuration to enable signoff by default

[Junio C Hamano]

Sekhar Nori <nsekhar@ti.com> writes:

> +am.signoff::
> +	A boolean value which lets you enable the `-s/--signoff` option of
> +	am by default. *Note:* Adding the Signed-off-by: line to a patch
> +	should be a conscious act and means that you certify you have
> +	the rights to submit this work under the same open source license.
> +	Please see the 'SubmittingPatches' document for further discussion.

I see you already have done your homework, which is much better than
previous attempts by other people to add an option to add sign-off to
various commands (commit and format-patch), but there are at least three
problems here:

 * This is an end-user document, not a document for git developers, so
   they are not bound by SubmittingPatches at all. They do not necessarily
   have SubmittingPatches document in the first place;

 * Once you set this configuration variable, I do not see a way to
   countermand "This is not signed-off" from the command line for a single
   invocation of "am" in your patch;

 * If it should be a "conscious act", it shouldn't be "set once and forget
   about it" configuration option. You should be making the conscious
   decision for each piece of e-mail if it is in line with the project's
   Sign-off policy.

The last one means that the patch is internally inconsistent. In a project
(not this project) where sign-off is used but it does not require signing
off to be a conscious act, I see it may make sense to make it easier to
sign-off a patch when generating (i.e. "commit -s"), preparing to send
(i.e. "format-patch -s"), or accepting (i.e. "am -s"). And this option
might be a possible way to do so. But in that case, the option description
wouldn't say "it should be a conscious act".

Also if it doesn't have to be a conscious act, what value does such a
boilerplate have? Such a project may be better off not using sign-off at
all to begin with.

RE: [PATCH 1/1] git-am: provide configuration to enable signoff by default

[Nori, Sekhar]

Hi Junio,

On Wed, Jun 01, 2011 at 22:44:30, Junio C Hamano wrote:
> Sekhar Nori <nsekhar@ti.com> writes:
> 
> > +am.signoff::
> > +	A boolean value which lets you enable the `-s/--signoff` option of
> > +	am by default. *Note:* Adding the Signed-off-by: line to a patch
> > +	should be a conscious act and means that you certify you have
> > +	the rights to submit this work under the same open source license.
> > +	Please see the 'SubmittingPatches' document for further discussion.
> 
> I see you already have done your homework, which is much better than
> previous attempts by other people to add an option to add sign-off to
> various commands (commit and format-patch), but there are at least three
> problems here:

Yes, I guessed there would have been previous attempts at this
and actually did search the mail archives a bit before sending
the patch. I actually didn't hit anything so looks like I probably
used the wrong keywords.

>  * This is an end-user document, not a document for git developers, so
>    they are not bound by SubmittingPatches at all. They do not necessarily
>    have SubmittingPatches document in the first place;

I will be honest here, I just took this note from the existing documentation 
for format.signoff config option. May be I can instead say: "Please check the 
guidelines on submitting patches for your project for further discussion on 
this. 'SubmittingPatches' document in Linux kernel or 'git' sources is an 
example of such a document."

>  * Once you set this configuration variable, I do not see a way to
>    countermand "This is not signed-off" from the command line for a single
>    invocation of "am" in your patch;

Okay. I guess adding an --no-signoff to git am should help here?

> 
>  * If it should be a "conscious act", it shouldn't be "set once and forget
>    about it" configuration option. You should be making the conscious
>    decision for each piece of e-mail if it is in line with the project's
>    Sign-off policy.
> 
> The last one means that the patch is internally inconsistent. In a project
> (not this project) where sign-off is used but it does not require signing
> off to be a conscious act, I see it may make sense to make it easier to
> sign-off a patch when generating (i.e. "commit -s"), preparing to send
> (i.e. "format-patch -s"), or accepting (i.e. "am -s"). And this option
> might be a possible way to do so. But in that case, the option description
> wouldn't say "it should be a conscious act".
> 
> Also if it doesn't have to be a conscious act, what value does such a
> boilerplate have? Such a project may be better off not using sign-off at
> all to begin with.

Its hard to argue against this. All I would say is it is probably
much safer to enable sign off by default when accepting a patch
than when preparing to send it. Yet, we have format.signoff but
no am.signoff. On any project with conscious sign-off rules, one
would never accept a patch without a sign-off from the original
developer.

So, just making it easier to accept patches which are already
signed-off should be okay, I guess? May be we should look for
an existing sign-off in the mbox and only then have this option
come into play?

Thanks,
Sekhar

Re: [PATCH 1/1] git-am: provide configuration to enable signoff by default

[Jonathan Nieder]

Nori, Sekhar wrote:
> On Wed, Jun 01, 2011 at 22:44:30, Junio C Hamano wrote:

>> Also if it doesn't have to be a conscious act, what value does such a
>> boilerplate have? Such a project may be better off not using sign-off at
>> all to begin with.
>
> Its hard to argue against this. All I would say is it is probably
> much safer to enable sign off by default when accepting a patch
> than when preparing to send it. Yet, we have format.signoff but
> no am.signoff. On any project with conscious sign-off rules, one
> would never accept a patch without a sign-off from the original
> developer.

In that case, just the first sign-off should be enough and there is no
need to record the later ones, no?

In practice, with format.signoff hopefully people read the patches
they are sending out before mailing them.  It is very easy to remove
the sign-off in an MUA or by editing patch files before running
"git send-email" if it was inappropriate.  On the contrary, importing
patches en masse with "git am" is a common operation and I suspect
looking over the new history in detail before a "git push" is rare, so
the possibility of someone forgetting that an "[am] signOff" variable
is in effect is much more dangerous.

That said, I am all for giving people rope as long as there is some
legitimate use case documented to explain how not to hang themselves.
Could you say more about the example that motivated this?  What
benefit would this provide to motivate not using "git am -3sc" (which
contains the -s on the commandline as a reminder)?

> So, just making it easier to accept patches which are already
> signed-off should be okay, I guess?

A related idea seems interesting: would a --check-sign-off option that
prevents applying a patch unless the last sign-off matches the email
sender be useful?  Unfortunately individual people sometimes use
different addresses for the From and Signed-off-by lines in some
projects (like git and the Linux kernel) so I don't think I'd use such
a thing but in a more controlled environment I can imagine it might be
nice.

Thanks.

Sorry for the longwinded message; still, hope that helps.

Regards,
Jonathan

RE: [PATCH 1/1] git-am: provide configuration to enable signoff by default

[Nori, Sekhar]

Hi Jonathan,

On Thu, Jun 02, 2011 at 00:53:57, Jonathan Nieder wrote:
> Nori, Sekhar wrote:
> > On Wed, Jun 01, 2011 at 22:44:30, Junio C Hamano wrote:
> 
> >> Also if it doesn't have to be a conscious act, what value does such a
> >> boilerplate have? Such a project may be better off not using sign-off at
> >> all to begin with.
> >
> > Its hard to argue against this. All I would say is it is probably
> > much safer to enable sign off by default when accepting a patch
> > than when preparing to send it. Yet, we have format.signoff but
> > no am.signoff. On any project with conscious sign-off rules, one
> > would never accept a patch without a sign-off from the original
> > developer.
> 
> In that case, just the first sign-off should be enough and there is no
> need to record the later ones, no?

Well, it is required to keep track of who was in the upstream
path. This way all relevant folks can be contacted in case
the patch introduces a problem.

> 
> In practice, with format.signoff hopefully people read the patches
> they are sending out before mailing them.  It is very easy to remove
> the sign-off in an MUA or by editing patch files before running
> "git send-email" if it was inappropriate.  On the contrary, importing
> patches en masse with "git am" is a common operation and I suspect
> looking over the new history in detail before a "git push" is rare, so
> the possibility of someone forgetting that an "[am] signOff" variable
> is in effect is much more dangerous.
> 
> That said, I am all for giving people rope as long as there is some
> legitimate use case documented to explain how not to hang themselves.
> Could you say more about the example that motivated this?  What
> benefit would this provide to motivate not using "git am -3sc" (which
> contains the -s on the commandline as a reminder)?

Okay, here is the issue that prompted me to work on this:

http://linux.davincidsp.com/pipermail/davinci-linux-open-source/2011-May/022832.html

I think this option is useful for maintainers who usually do not
apply patches en-masse, but accept patches after review and want to
pass along to other upstream maintainers after adding their sign-off.

Granted you can always rely on -s option to be added, but people do
forget (like I did).

> > So, just making it easier to accept patches which are already
> > signed-off should be okay, I guess?
> 
> A related idea seems interesting: would a --check-sign-off option that
> prevents applying a patch unless the last sign-off matches the email
> sender be useful?  Unfortunately individual people sometimes use
> different addresses for the From and Signed-off-by lines in some
> projects (like git and the Linux kernel) so I don't think I'd use such
> a thing but in a more controlled environment I can imagine it might be
> nice.

If you received any patch with a sign-off, I think it is usually
safe to attach your own sign-off since then the origin of the
patch is already certified by someone else (you would be adding
your sign-off under Developer's Certificate of Origin 1.1 section
(c))

Thanks,
Sekhar

RE: [PATCH 1/1] git-am: provide configuration to enable signoff by default

[Nori, Sekhar]

Hi Sverre,

On Wed, Jun 01, 2011 at 21:33:14, Sverre Rabbelier wrote:
> Heya,
> 
> On Wed, Jun 1, 2011 at 03:42, Sekhar Nori <nsekhar@ti.com> wrote:
> > Provide a git config option to enable --signoff a
> > default when using git-am. This should be handy
> > for maintainers who regularly apply patches from
> > mailing lists to send them upstream and want to
> > be on the sign-off path.
> 
> Wouldn't this need tests?

Yes, I did test this (by applying mailbox with and
without this option turned on). Didn't actually try
with --rebasing that Jeff asked for in another e-mail.

I should have included test information in the patch
description.

At this point I am not sure if this patch will be
considered for inclusion. If I get to submit another
version of it, I will surely add this info.

Thanks,
Sekhar

Re: [PATCH 1/1] git-am: provide configuration to enable signoff by default

[Thiago Farina]

On Thu, Jun 2, 2011 at 4:10 PM, Nori, Sekhar <nsekhar@ti.com> wrote:
> At this point I am not sure if this patch will be
> considered for inclusion. If I get to submit another
> version of it, I will surely add this info.
>
I think he is talking about the tests under the t/ directory (the git
test suite). You can take a look at many examples there.