{"thread":{"id":"52572","subject":"[PATCH 0/1] commit: make the sign-off trailer configurable","startedAt":"2020-01-05T17:41:37Z","lastAt":"2020-01-06T20:45:33Z","messageCount":7,"participants":["Hans Jerry Illikainen","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"389254","messageId":"20200105174127.9278-1-hji@dyntopia.com","threadId":"52572","inReplyTo":null,"subject":"[PATCH 0/1] commit: make the sign-off trailer configurable","fromName":"Hans Jerry Illikainen","fromEmail":"hji@dyntopia.com","sentAt":"2020-01-05T17:41:26Z","receivedAt":"2020-01-05T17:41:37Z","isPatch":true,"sender":{"key":"hji@dyntopia.com","avatar":"https://avatars.githubusercontent.com/u/8175517?v=4"},"body":"This patch that adds commit.signOff as a configuration option.  The\nconfig/commit.txt documentation where the significance of the\nsigned-off-by trailer is highlighted is copy-pasted from\nconfig/format.txt.\n\nEven with the note in config/{commit,format}.txt, I still think it's\nworthwhile to have the trailer as a configuration option for\ncontributors that always have the rights to send patches.\n\nHans Jerry Illikainen (1):\n  commit: make the sign-off trailer configurable\n\n Documentation/config/commit.txt |  8 ++++++++\n Documentation/git-commit.txt    |  4 ++++\n builtin/commit.c                |  4 ++++\n t/t7502-commit-porcelain.sh     | 36 +++++++++++++++++++++++++++++++++\n 4 files changed, 52 insertions(+)\n\n--\n2.25.0.rc1.298.g45d5f025e1\n"},{"id":"389255","messageId":"20200105174127.9278-2-hji@dyntopia.com","threadId":"52572","inReplyTo":"20200105174127.9278-1-hji@dyntopia.com","subject":"[PATCH 1/1] commit: make the sign-off trailer configurable","fromName":"Hans Jerry Illikainen","fromEmail":"hji@dyntopia.com","sentAt":"2020-01-05T17:41:27Z","receivedAt":"2020-01-05T17:41:40Z","isPatch":true,"sender":{"key":"hji@dyntopia.com","avatar":"https://avatars.githubusercontent.com/u/8175517?v=4"},"body":"The commit builtin did not previously have a configuration option to\nenable the 'Signed-off-by' trailer by default.\n\nFor some use-cases (namely, when the user doesn't always have the right\nto contribute patches to a project) it makes sense to make it a\nconscientious decision to add the signoff trailer.  However, others'\nmight always have the right to ship patches -- in which case it makes\nsense to have an option to add the trailer by default for projects that\nrequire it.\n\nThis patch introduces a commit.signOff configuration option that\ndetermine whether the trailer should be added for commits.  It can be\noverridden with the --(no-)signoff command-line option.\n\nSigned-off-by: Hans Jerry Illikainen <hji@dyntopia.com>\n---\n Documentation/config/commit.txt |  8 ++++++++\n Documentation/git-commit.txt    |  4 ++++\n builtin/commit.c                |  4 ++++\n t/t7502-commit-porcelain.sh     | 36 +++++++++++++++++++++++++++++++++\n 4 files changed, 52 insertions(+)\n\ndiff --git a/Documentation/config/commit.txt b/Documentation/config/commit.txt\nindex 2c95573930..6ebfe384ac 100644\n--- a/Documentation/config/commit.txt\n+++ b/Documentation/config/commit.txt\n@@ -15,6 +15,14 @@ commit.gpgSign::\n \tconvenient to use an agent to avoid typing your GPG passphrase\n \tseveral times.\n \n+commit.signOff::\n+\tA boolean to specify whether commits should enable the\n+\t`-s/--signoff` option by default.  *Note:* Adding the\n+\tSigned-off-by: line to a commit message should be a conscious\n+\tact and means that you certify you have the rights to submit the\n+\twork under the same open source license.  Please see the\n+\t'SubmittingPatches' document for further discussion.\n+\n commit.status::\n \tA boolean to enable/disable inclusion of status information in the\n \tcommit message template when using an editor to prepare the commit\ndiff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\nindex ced5a9beab..61a362770d 100644\n--- a/Documentation/git-commit.txt\n+++ b/Documentation/git-commit.txt\n@@ -165,12 +165,16 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.\n \n -s::\n --signoff::\n+--no-signoff::\n \tAdd Signed-off-by line by the committer at the end of the commit\n \tlog message.  The meaning of a signoff depends on the project,\n \tbut it typically certifies that committer has\n \tthe rights to submit this work under the same license and\n \tagrees to a Developer Certificate of Origin\n \t(see http://developercertificate.org/ for more information).\n+\tThis option can be enabled by default with the `commit.signOff`\n+\tconfiguration option, in which case it can be disabled\n+\ttemporarily with `--no-signoff`.\n \n -n::\n --no-verify::\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex c70ad01cc9..497e29c58c 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1474,6 +1474,10 @@ static int git_commit_config(const char *k, const char *v, void *cb)\n \t\tsign_commit = git_config_bool(k, v) ? \"\" : NULL;\n \t\treturn 0;\n \t}\n+\tif (!strcmp(k, \"commit.signoff\")) {\n+\t\tsignoff = git_config_bool(k, v);\n+\t\treturn 0;\n+\t}\n \tif (!strcmp(k, \"commit.verbose\")) {\n \t\tint is_bool;\n \t\tconfig_commit_verbose = git_config_bool_or_int(k, v, &is_bool);\ndiff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh\nindex 14c92e4c25..7510325698 100755\n--- a/t/t7502-commit-porcelain.sh\n+++ b/t/t7502-commit-porcelain.sh\n@@ -151,6 +151,42 @@ test_expect_success 'sign off' '\n \n '\n \n+test_expect_success 'commit.signOff=true' '\n+\ttest_config commit.signOff true &&\n+\techo 1 >>positive &&\n+\tgit add positive &&\n+\tgit commit -m \"thank you\" &&\n+\tgit cat-file commit HEAD >commit.msg &&\n+\tsed -ne \"s/Signed-off-by: //p\" commit.msg >actual &&\n+\tgit var GIT_COMMITTER_IDENT >ident &&\n+\tsed -e \"s/>.*/>/\" ident >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'commit.signOff=true and --no-signoff' '\n+\ttest_config commit.signOff true &&\n+\techo 2 >>positive &&\n+\tgit add positive &&\n+\tgit commit --no-signoff -m \"thank you\" &&\n+\tgit cat-file commit HEAD >commit.msg &&\n+\tsed -ne \"s/Signed-off-by: //p\" commit.msg >actual &&\n+\tgit var GIT_COMMITTER_IDENT >ident &&\n+\tsed -e \"s/>.*/>/\" ident >expected &&\n+\t! test_cmp expected actual\n+'\n+\n+test_expect_success 'commit.signOff=false and --signoff' '\n+\ttest_config commit.signOff false &&\n+\techo 1 >>positive &&\n+\tgit add positive &&\n+\tgit commit --signoff -m \"thank you\" &&\n+\tgit cat-file commit HEAD >commit.msg &&\n+\tsed -ne \"s/Signed-off-by: //p\" commit.msg >actual &&\n+\tgit var GIT_COMMITTER_IDENT >ident &&\n+\tsed -e \"s/>.*/>/\" ident >expected &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'multiple -m' '\n \n \t>negative &&\n-- \n2.25.0.rc1.298.g45d5f025e1\n\n"},{"id":"389261","messageId":"71a718a7-2be7-391c-dc8f-0626a0a21aac@gmail.com","threadId":"52572","inReplyTo":"20200105174127.9278-2-hji@dyntopia.com","subject":"Re: [PATCH 1/1] commit: make the sign-off trailer configurable","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-01-06T13:38:23Z","receivedAt":"2020-01-06T13:38:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/5/2020 12:41 PM, Hans Jerry Illikainen wrote:\n> The commit builtin did not previously have a configuration option to\n> enable the 'Signed-off-by' trailer by default.\n> \n> For some use-cases (namely, when the user doesn't always have the right\n> to contribute patches to a project) it makes sense to make it a\n> conscientious decision to add the signoff trailer.  However, others'\n> might always have the right to ship patches -- in which case it makes\n> sense to have an option to add the trailer by default for projects that\n> require it.\n\nMy initial thought was that the sign-off was supposed to be a purposeful\ndecision, but then I also realized that I never do anything in the Git\ncodebase that I _can't_ put online under the GPL. (It may not make it\nupstream, but it will always be put online somewhere.)\n\n> This patch introduces a commit.signOff configuration option that\n> determine whether the trailer should be added for commits.  It can be\n> overridden with the --(no-)signoff command-line option.\n\nWith that in mind, I think this is a valuable feature.\n \n> Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>\n\nDid you generate this with your config option? ;)\n\n> +commit.signOff::\n> +\tA boolean to specify whether commits should enable the\n> +\t`-s/--signoff` option by default.  *Note:* Adding the\n> +\tSigned-off-by: line to a commit message should be a conscious\n> +\tact and means that you certify you have the rights to submit the\n> +\twork under the same open source license.  Please see the\n> +\t'SubmittingPatches' document for further discussion.\n> +\n\nI wonder about the language of \"should be a conscious act\" here. It's\nas if you are trying to convince someone to not use this feature. Perhaps\nswitch it to \"is a deliberate act\" and add something such as \"Enable this\nvalue only in repos where you are the only user and always have these\nrights.\"\n\nThe multi-user scenario may be worth clarifying explicitly here. If there\nis any chance that another user will join the machine and use that same\nrepo, then they would have a different user.name and user.email in their\nglobal config (probably) but this as a local setting would provide their\nsign-off.\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index c70ad01cc9..497e29c58c 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1474,6 +1474,10 @@ static int git_commit_config(const char *k, const char *v, void *cb)\n>  \t\tsign_commit = git_config_bool(k, v) ? \"\" : NULL;\n>  \t\treturn 0;\n>  \t}\n> +\tif (!strcmp(k, \"commit.signoff\")) {\n> +\t\tsignoff = git_config_bool(k, v);\n> +\t\treturn 0;\n> +\t}\n\nSince we are directly modifying the same global used by the --[no-]signoff\noption, I verified that the config options are checked before the arguments\nare parsed. Thus, --no-signoff will override commit.signOff=true...\n\n> +test_expect_success 'commit.signOff=true' '\n> +\ttest_config commit.signOff true &&\n> +\techo 1 >>positive &&\n> +\tgit add positive &&\n> +\tgit commit -m \"thank you\" &&\n> +\tgit cat-file commit HEAD >commit.msg &&\n> +\tsed -ne \"s/Signed-off-by: //p\" commit.msg >actual &&\n> +\tgit var GIT_COMMITTER_IDENT >ident &&\n> +\tsed -e \"s/>.*/>/\" ident >expected &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> +test_expect_success 'commit.signOff=true and --no-signoff' '\n> +\ttest_config commit.signOff true &&\n> +\techo 2 >>positive &&\n> +\tgit add positive &&\n> +\tgit commit --no-signoff -m \"thank you\" &&\n> +\tgit cat-file commit HEAD >commit.msg &&\n> +\tsed -ne \"s/Signed-off-by: //p\" commit.msg >actual &&\n> +\tgit var GIT_COMMITTER_IDENT >ident &&\n> +\tsed -e \"s/>.*/>/\" ident >expected &&\n> +\t! test_cmp expected actual\n> +'\n\n...which you test here, too. Excellent.\n\n> +test_expect_success 'commit.signOff=false and --signoff' '\n> +\ttest_config commit.signOff false &&\n> +\techo 1 >>positive &&\n> +\tgit add positive &&\n> +\tgit commit --signoff -m \"thank you\" &&\n\nPerhaps it is worth adding an explicit \"-c commit.signOff=false\" here?\n\n> +\tgit cat-file commit HEAD >commit.msg &&\n> +\tsed -ne \"s/Signed-off-by: //p\" commit.msg >actual &&\n> +\tgit var GIT_COMMITTER_IDENT >ident &&\n> +\tsed -e \"s/>.*/>/\" ident >expected &&\n> +\ttest_cmp expected actual\n> +'\n> +\n\nI wonder if the boilerplate for these tests could be simplified or\nshared across the tests?\n\nFor example: could we not just use test_i18ngrep to see if commit.msg\ncontains (or does not contain) the string \"Signed-off-by\"?\n\nI believe this patch delivers the stated feature well. Hopefully others\ncan add commentary on its usefulness or possible issues in using it.\n\nThanks,\n-Stolee\n"},{"id":"389266","messageId":"xmqqtv58a5m2.fsf@gitster-ct.c.googlers.com","threadId":"52572","inReplyTo":"71a718a7-2be7-391c-dc8f-0626a0a21aac@gmail.com","subject":"Re: [PATCH 1/1] commit: make the sign-off trailer configurable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-06T16:53:41Z","receivedAt":"2020-01-06T16:53:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> My initial thought was that the sign-off was supposed to be a purposeful\n> decision, but then I also realized that I never do anything in the Git\n> codebase that I _can't_ put online under the GPL. (It may not make it\n> upstream, but it will always be put online somewhere.)\n\nHmm...\n\nSorry, but I do not quite understand the flow of your logic here,\nespecially, how \"but then I also realized\" trumps \"signing off a\npatch is a conscious act---it would weaken the legal meaning if you\nautomate it\", which was why we deliberately avoided adding this\nconfiguration variable for the last 10+ years.\n\nSo, I dunno.\n"},{"id":"389282","messageId":"cfa40ca5-98a1-fc9c-9ccc-f14b81119e60@gmail.com","threadId":"52572","inReplyTo":"xmqqtv58a5m2.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] commit: make the sign-off trailer configurable","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-01-06T19:53:58Z","receivedAt":"2020-01-06T19:54:03Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/6/2020 11:53 AM, Junio C Hamano wrote:\n> Derrick Stolee <stolee@gmail.com> writes:\n> \n>> My initial thought was that the sign-off was supposed to be a purposeful\n>> decision, but then I also realized that I never do anything in the Git\n>> codebase that I _can't_ put online under the GPL. (It may not make it\n>> upstream, but it will always be put online somewhere.)\n> \n> Hmm...\n> \n> Sorry, but I do not quite understand the flow of your logic here,\n> especially, how \"but then I also realized\" trumps \"signing off a\n> patch is a conscious act---it would weaken the legal meaning if you\n> automate it\", which was why we deliberately avoided adding this\n> configuration variable for the last 10+ years.\n> \n> So, I dunno.\n\nI guess I meant that enabling this config for a repo is also a\nconscious act, making me think this is not completely unreasonable.\n\nBut if we've already discussed and rejected this feature in the past,\nthen that's sufficient.\n\nSince I started the line of \"this isn't a bad idea\" I'll follow up with\nthe historical search. Here are previous attempts from 2018 [1], 2015 [2],\n2010 [3].\n\nThanks,\n-Stolee\n\n[1] https://lore.kernel.org/git/20180204020318.4363-1-chenjingpiao@gmail.com/\n[2] https://lore.kernel.org/git/1435217454-5718-1-git-send-email-cmarcelo@gmail.com/\n[3] https://lore.kernel.org/git/alpine.LNX.2.00.1001131635510.16395@vqena.qenxr.bet.am/\n"},{"id":"389283","messageId":"xmqqpnfw8gyn.fsf@gitster-ct.c.googlers.com","threadId":"52572","inReplyTo":"cfa40ca5-98a1-fc9c-9ccc-f14b81119e60@gmail.com","subject":"Re: [PATCH 1/1] commit: make the sign-off trailer configurable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-06T20:31:28Z","receivedAt":"2020-01-06T20:31:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> Since I started the line of \"this isn't a bad idea\" I'll follow up with\n> the historical search. Here are previous attempts from 2018 [1], 2015 [2],\n> 2010 [3].\n>\n> Thanks,\n> -Stolee\n>\n> [1] https://lore.kernel.org/git/20180204020318.4363-1-chenjingpiao@gmail.com/\n> [2] https://lore.kernel.org/git/1435217454-5718-1-git-send-email-cmarcelo@gmail.com/\n> [3] https://lore.kernel.org/git/alpine.LNX.2.00.1001131635510.16395@vqena.qenxr.bet.am/\n\nThanks.  In an earlier thread, we did\n\nhttps://lore.kernel.org/git/20090401175153.GA90421@macbook.lan/\n\nto which I said \"... the wording we can update if somebody can come\nup with a better one\" in its follow-up.  Perhaps it's time for us to\nbe that somebody to help everybody to be on the same page.\n\nHere is my attempt, starting from what I wrote in\n\nhttps://lore.kernel.org/git/xmqqtw9m5s5m.fsf@gitster.mtv.corp.google.com/\n\n-- >8 --\nSubject: commit -s: document why commit.signoff option will not be added\n\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n As I said in https://lore.kernel.org/git/7veiw69p26.fsf@gitster.siamese.dyndns.org/\n I have a mixed feeling about this.  To projects that use the same\n definition of what SoB means, not adding the configuration ever is\n the right thing to do, but Git is to be used by other projects, and\n some of them may use SoB with a completely different meaning that\n has no legal weight---and to them, lack of such an automation may\n be a bug.  So ...\n\n Documentation/git-commit.txt | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\nindex ced5a9beab..1909551087 100644\n--- a/Documentation/git-commit.txt\n+++ b/Documentation/git-commit.txt\n@@ -171,6 +171,13 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.\n \tthe rights to submit this work under the same license and\n \tagrees to a Developer Certificate of Origin\n \t(see http://developercertificate.org/ for more information).\n++\n+As it makes it harder to argue against one who tells the court \"that\n+log message ends with a SoB by person X but it is very plausible\n+that it was done by inertia without person X really intending to\n+certify what DCO says, and the SoB is meaningless.\" to more\n+publicized ways to add SoB automatically, Git does not (and will not)\n+have a configuration variable to enable it by default.\n \n -n::\n --no-verify::\n\n"},{"id":"389285","messageId":"xmqqeewc8gbc.fsf@gitster-ct.c.googlers.com","threadId":"52572","inReplyTo":"xmqqpnfw8gyn.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] commit: make the sign-off trailer configurable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-06T20:45:27Z","receivedAt":"2020-01-06T20:45:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> ++\n> +As it makes it harder to argue against one who tells the court \"that\n> +log message ends with a SoB by person X but it is very plausible\n> +that it was done by inertia without person X really intending to\n> +certify what DCO says, and the SoB is meaningless.\" to more\n\ns/to more/to have more/; otherwise the sentence does not make any\nsense.  Sorry about the noise.\n\n> +publicized ways to add SoB automatically, Git does not (and will not)\n> +have a configuration variable to enable it by default.\n>  \n>  -n::\n>  --no-verify::\n"}]}