Message ID | 20200105174127.9278-2-hji@dyntopia.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | commit: make the sign-off trailer configurable | expand |
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
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.
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/
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::
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::
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 &&
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(+)