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

From: Nori, Sekhar <nsekhar@ti.com>
Date: Jun 1, 2011, 18:27 UTC
Message-ID: <B85A65D85D7EB246BE421B3FB0FBB593024D2D22DE@dbde02.ent.ti.com>
In-Reply-To: <7voc2hh3t5.fsf@alter.siamese.dyndns.org>

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