threads / patch / 27519

patchgit-am: provide configuration to enable signoff by default

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

## tl;dr

9 messages between Jun 1, 2011 and Jun 2, 2011. Diffs are folded; open one to read it.

replies: 8people: 6as markdown or json

Sekhar Nori· Jun 1, 2011, 08:42 UTC · lore

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(-)
Show changes to 3 files +14 −1

Documentation/config.txt, Documentation/git-am.txt, git-am.sh

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
Sverre Rabbelier· Jun 1, 2011, 16:03 UTC · re: Sekhar Nori · lore

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

Heya,
On Wed, Jun 1, 2011 at 03:42, Sekhar Nori <nsekhar@ti.com> wrote:
Show 5 quoted lines
> 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
Nori, Sekhar· Jun 2, 2011, 19:10 UTC · re: Sverre Rabbelier · lore

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

Hi Sverre,
On Wed, Jun 01, 2011 at 21:33:14, Sverre Rabbelier wrote:
Show 10 quoted lines
> 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

Thiago Farina· Jun 2, 2011, 19:41 UTC · re: Nori, Sekhar · lore

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

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.

Jeff King· Jun 1, 2011, 16:39 UTC · re: Sekhar Nori · lore

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

On Wed, Jun 01, 2011 at 02:12:31PM +0530, Sekhar Nori wrote:
Show 5 quoted lines
> 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.

Show 16 quoted lines
> 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
Junio C Hamano· Jun 1, 2011, 17:14 UTC · re: Sekhar Nori · lore

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

Sekhar Nori <nsekhar@ti.com> writes:
Show 6 quoted lines
> +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.

Nori, Sekhar· Jun 1, 2011, 18:27 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On Wed, Jun 01, 2011 at 22:44:30, Junio C Hamano wrote:
Show 13 quoted lines
> 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?
Show 17 quoted lines
> 
>  * 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

Jonathan Nieder· Jun 1, 2011, 19:23 UTC · re: Nori, Sekhar · lore

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

Nori, Sekhar wrote:
> On Wed, Jun 01, 2011 at 22:44:30, Junio C Hamano wrote:
Show 10 quoted lines
>> 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

Nori, Sekhar· Jun 2, 2011, 19:05 UTC · re: Jonathan Nieder · lore

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

Hi Jonathan,
On Thu, Jun 02, 2011 at 00:53:57, Jonathan Nieder wrote:
Show 16 quoted lines
> 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.

Show 15 quoted lines
> 
> 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).

Show 10 quoted lines
> > 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

← back to recent threads