[PATCH 0/1] commit: make the sign-off trailer configurable

[PATCH 0/1] commit: make the sign-off trailer configurable

[Hans Jerry Illikainen]

This patch that adds commit.signOff as a configuration option.  The
config/commit.txt documentation where the significance of the
signed-off-by trailer is highlighted is copy-pasted from
config/format.txt.

Even with the note in config/{commit,format}.txt, I still think it's
worthwhile to have the trailer as a configuration option for
contributors that always have the rights to send patches.

Hans Jerry Illikainen (1):
  commit: make the sign-off trailer configurable

 Documentation/config/commit.txt |  8 ++++++++
 Documentation/git-commit.txt    |  4 ++++
 builtin/commit.c                |  4 ++++
 t/t7502-commit-porcelain.sh     | 36 +++++++++++++++++++++++++++++++++
 4 files changed, 52 insertions(+)

--
2.25.0.rc1.298.g45d5f025e1

[PATCH 1/1] commit: make the sign-off trailer configurable

[Hans Jerry Illikainen]

The commit builtin did not previously have a configuration option to
enable the 'Signed-off-by' trailer by default.

For some use-cases (namely, when the user doesn't always have the right
to contribute patches to a project) it makes sense to make it a
conscientious decision to add the signoff trailer.  However, others'
might always have the right to ship patches -- in which case it makes
sense to have an option to add the trailer by default for projects that
require it.

This patch introduces a commit.signOff configuration option that
determine whether the trailer should be added for commits.  It can be
overridden with the --(no-)signoff command-line option.

Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
 Documentation/config/commit.txt |  8 ++++++++
 Documentation/git-commit.txt    |  4 ++++
 builtin/commit.c                |  4 ++++
 t/t7502-commit-porcelain.sh     | 36 +++++++++++++++++++++++++++++++++
 4 files changed, 52 insertions(+)

diff --git a/Documentation/config/commit.txt b/Documentation/config/commit.txt
index 2c95573930..6ebfe384ac 100644
--- a/Documentation/config/commit.txt
+++ b/Documentation/config/commit.txt
@@ -15,6 +15,14 @@ commit.gpgSign::
 	convenient to use an agent to avoid typing your GPG passphrase
 	several times.
 
+commit.signOff::
+	A boolean to specify whether commits should enable the
+	`-s/--signoff` option by default.  *Note:* Adding the
+	Signed-off-by: line to a commit message should be a conscious
+	act and means that you certify you have the rights to submit the
+	work under the same open source license.  Please see the
+	'SubmittingPatches' document for further discussion.
+
 commit.status::
 	A boolean to enable/disable inclusion of status information in the
 	commit message template when using an editor to prepare the commit
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index ced5a9beab..61a362770d 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -165,12 +165,16 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.
 
 -s::
 --signoff::
+--no-signoff::
 	Add Signed-off-by line by the committer at the end of the commit
 	log message.  The meaning of a signoff depends on the project,
 	but it typically certifies that committer has
 	the rights to submit this work under the same license and
 	agrees to a Developer Certificate of Origin
 	(see http://developercertificate.org/ for more information).
+	This option can be enabled by default with the `commit.signOff`
+	configuration option, in which case it can be disabled
+	temporarily with `--no-signoff`.
 
 -n::
 --no-verify::
diff --git a/builtin/commit.c b/builtin/commit.c
index c70ad01cc9..497e29c58c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1474,6 +1474,10 @@ static int git_commit_config(const char *k, const char *v, void *cb)
 		sign_commit = git_config_bool(k, v) ? "" : NULL;
 		return 0;
 	}
+	if (!strcmp(k, "commit.signoff")) {
+		signoff = git_config_bool(k, v);
+		return 0;
+	}
 	if (!strcmp(k, "commit.verbose")) {
 		int is_bool;
 		config_commit_verbose = git_config_bool_or_int(k, v, &is_bool);
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index 14c92e4c25..7510325698 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -151,6 +151,42 @@ test_expect_success 'sign off' '
 
 '
 
+test_expect_success 'commit.signOff=true' '
+	test_config commit.signOff true &&
+	echo 1 >>positive &&
+	git add positive &&
+	git commit -m "thank you" &&
+	git cat-file commit HEAD >commit.msg &&
+	sed -ne "s/Signed-off-by: //p" commit.msg >actual &&
+	git var GIT_COMMITTER_IDENT >ident &&
+	sed -e "s/>.*/>/" ident >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'commit.signOff=true and --no-signoff' '
+	test_config commit.signOff true &&
+	echo 2 >>positive &&
+	git add positive &&
+	git commit --no-signoff -m "thank you" &&
+	git cat-file commit HEAD >commit.msg &&
+	sed -ne "s/Signed-off-by: //p" commit.msg >actual &&
+	git var GIT_COMMITTER_IDENT >ident &&
+	sed -e "s/>.*/>/" ident >expected &&
+	! test_cmp expected actual
+'
+
+test_expect_success 'commit.signOff=false and --signoff' '
+	test_config commit.signOff false &&
+	echo 1 >>positive &&
+	git add positive &&
+	git commit --signoff -m "thank you" &&
+	git cat-file commit HEAD >commit.msg &&
+	sed -ne "s/Signed-off-by: //p" commit.msg >actual &&
+	git var GIT_COMMITTER_IDENT >ident &&
+	sed -e "s/>.*/>/" ident >expected &&
+	test_cmp expected actual
+'
+
 test_expect_success 'multiple -m' '
 
 	>negative &&
-- 
2.25.0.rc1.298.g45d5f025e1

Re: [PATCH 1/1] commit: make the sign-off trailer configurable

[Derrick Stolee]

On 1/5/2020 12:41 PM, Hans Jerry Illikainen wrote:
> The commit builtin did not previously have a configuration option to
> enable the 'Signed-off-by' trailer by default.
> 
> For some use-cases (namely, when the user doesn't always have the right
> to contribute patches to a project) it makes sense to make it a
> conscientious decision to add the signoff trailer.  However, others'
> might always have the right to ship patches -- in which case it makes
> sense to have an option to add the trailer by default for projects that
> require it.

My initial thought was that the sign-off was supposed to be a purposeful
decision, but then I also realized that I never do anything in the Git
codebase that I _can't_ put online under the GPL. (It may not make it
upstream, but it will always be put online somewhere.)

> This patch introduces a commit.signOff configuration option that
> determine whether the trailer should be added for commits.  It can be
> overridden with the --(no-)signoff command-line option.

With that in mind, I think this is a valuable feature.
 
> Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>

Did you generate this with your config option? ;)

> +commit.signOff::
> +	A boolean to specify whether commits should enable the
> +	`-s/--signoff` option by default.  *Note:* Adding the
> +	Signed-off-by: line to a commit message should be a conscious
> +	act and means that you certify you have the rights to submit the
> +	work under the same open source license.  Please see the
> +	'SubmittingPatches' document for further discussion.
> +

I wonder about the language of "should be a conscious act" here. It's
as if you are trying to convince someone to not use this feature. Perhaps
switch it to "is a deliberate act" and add something such as "Enable this
value only in repos where you are the only user and always have these
rights."

The multi-user scenario may be worth clarifying explicitly here. If there
is any chance that another user will join the machine and use that same
repo, then they would have a different user.name and user.email in their
global config (probably) but this as a local setting would provide their
sign-off.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index c70ad01cc9..497e29c58c 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1474,6 +1474,10 @@ static int git_commit_config(const char *k, const char *v, void *cb)
>  		sign_commit = git_config_bool(k, v) ? "" : NULL;
>  		return 0;
>  	}
> +	if (!strcmp(k, "commit.signoff")) {
> +		signoff = git_config_bool(k, v);
> +		return 0;
> +	}

Since we are directly modifying the same global used by the --[no-]signoff
option, I verified that the config options are checked before the arguments
are parsed. Thus, --no-signoff will override commit.signOff=true...

> +test_expect_success 'commit.signOff=true' '
> +	test_config commit.signOff true &&
> +	echo 1 >>positive &&
> +	git add positive &&
> +	git commit -m "thank you" &&
> +	git cat-file commit HEAD >commit.msg &&
> +	sed -ne "s/Signed-off-by: //p" commit.msg >actual &&
> +	git var GIT_COMMITTER_IDENT >ident &&
> +	sed -e "s/>.*/>/" ident >expected &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_success 'commit.signOff=true and --no-signoff' '
> +	test_config commit.signOff true &&
> +	echo 2 >>positive &&
> +	git add positive &&
> +	git commit --no-signoff -m "thank you" &&
> +	git cat-file commit HEAD >commit.msg &&
> +	sed -ne "s/Signed-off-by: //p" commit.msg >actual &&
> +	git var GIT_COMMITTER_IDENT >ident &&
> +	sed -e "s/>.*/>/" ident >expected &&
> +	! test_cmp expected actual
> +'

...which you test here, too. Excellent.

> +test_expect_success 'commit.signOff=false and --signoff' '
> +	test_config commit.signOff false &&
> +	echo 1 >>positive &&
> +	git add positive &&
> +	git commit --signoff -m "thank you" &&

Perhaps it is worth adding an explicit "-c commit.signOff=false" here?

> +	git cat-file commit HEAD >commit.msg &&
> +	sed -ne "s/Signed-off-by: //p" commit.msg >actual &&
> +	git var GIT_COMMITTER_IDENT >ident &&
> +	sed -e "s/>.*/>/" ident >expected &&
> +	test_cmp expected actual
> +'
> +

I wonder if the boilerplate for these tests could be simplified or
shared across the tests?

For example: could we not just use test_i18ngrep to see if commit.msg
contains (or does not contain) the string "Signed-off-by"?

I believe this patch delivers the stated feature well. Hopefully others
can add commentary on its usefulness or possible issues in using it.

Thanks,
-Stolee

Re: [PATCH 1/1] commit: make the sign-off trailer configurable

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

> My initial thought was that the sign-off was supposed to be a purposeful
> decision, but then I also realized that I never do anything in the Git
> codebase that I _can't_ put online under the GPL. (It may not make it
> upstream, but it will always be put online somewhere.)

Hmm...

Sorry, but I do not quite understand the flow of your logic here,
especially, how "but then I also realized" trumps "signing off a
patch is a conscious act---it would weaken the legal meaning if you
automate it", which was why we deliberately avoided adding this
configuration variable for the last 10+ years.

So, I dunno.

Re: [PATCH 1/1] commit: make the sign-off trailer configurable

[Derrick Stolee]

On 1/6/2020 11:53 AM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
> 
>> My initial thought was that the sign-off was supposed to be a purposeful
>> decision, but then I also realized that I never do anything in the Git
>> codebase that I _can't_ put online under the GPL. (It may not make it
>> upstream, but it will always be put online somewhere.)
> 
> Hmm...
> 
> Sorry, but I do not quite understand the flow of your logic here,
> especially, how "but then I also realized" trumps "signing off a
> patch is a conscious act---it would weaken the legal meaning if you
> automate it", which was why we deliberately avoided adding this
> configuration variable for the last 10+ years.
> 
> So, I dunno.

I guess I meant that enabling this config for a repo is also a
conscious act, making me think this is not completely unreasonable.

But if we've already discussed and rejected this feature in the past,
then that's sufficient.

Since I started the line of "this isn't a bad idea" I'll follow up with
the historical search. Here are previous attempts from 2018 [1], 2015 [2],
2010 [3].

Thanks,
-Stolee

[1] https://lore.kernel.org/git/20180204020318.4363-1-chenjingpiao@gmail.com/
[2] https://lore.kernel.org/git/1435217454-5718-1-git-send-email-cmarcelo@gmail.com/
[3] https://lore.kernel.org/git/alpine.LNX.2.00.1001131635510.16395@vqena.qenxr.bet.am/

Re: [PATCH 1/1] commit: make the sign-off trailer configurable

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

> Since I started the line of "this isn't a bad idea" I'll follow up with
> the historical search. Here are previous attempts from 2018 [1], 2015 [2],
> 2010 [3].
>
> Thanks,
> -Stolee
>
> [1] https://lore.kernel.org/git/20180204020318.4363-1-chenjingpiao@gmail.com/
> [2] https://lore.kernel.org/git/1435217454-5718-1-git-send-email-cmarcelo@gmail.com/
> [3] https://lore.kernel.org/git/alpine.LNX.2.00.1001131635510.16395@vqena.qenxr.bet.am/

Thanks.  In an earlier thread, we did

https://lore.kernel.org/git/20090401175153.GA90421@macbook.lan/

to which I said "... the wording we can update if somebody can come
up with a better one" in its follow-up.  Perhaps it's time for us to
be that somebody to help everybody to be on the same page.

Here is my attempt, starting from what I wrote in

https://lore.kernel.org/git/xmqqtw9m5s5m.fsf@gitster.mtv.corp.google.com/

-- >8 --
Subject: commit -s: document why commit.signoff option will not be added


Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 As I said in https://lore.kernel.org/git/7veiw69p26.fsf@gitster.siamese.dyndns.org/
 I have a mixed feeling about this.  To projects that use the same
 definition of what SoB means, not adding the configuration ever is
 the right thing to do, but Git is to be used by other projects, and
 some of them may use SoB with a completely different meaning that
 has no legal weight---and to them, lack of such an automation may
 be a bug.  So ...

 Documentation/git-commit.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index ced5a9beab..1909551087 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -171,6 +171,13 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.
 	the rights to submit this work under the same license and
 	agrees to a Developer Certificate of Origin
 	(see http://developercertificate.org/ for more information).
++
+As it makes 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." to more
+publicized ways to add SoB automatically, Git does not (and will not)
+have a configuration variable to enable it by default.
 
 -n::
 --no-verify::

Re: [PATCH 1/1] commit: make the sign-off trailer configurable

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> ++
> +As it makes 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." to more

s/to more/to have more/; otherwise the sentence does not make any
sense.  Sorry about the noise.

> +publicized ways to add SoB automatically, Git does not (and will not)
> +have a configuration variable to enable it by default.
>  
>  -n::
>  --no-verify::