Re: [GSoC][PATCH] commit: add a commit.signOff config variable
On 02/05 01:50, Stefan Beller wrote: > On Sat, Feb 3, 2018 at 6:03 PM, Chen Jingpiao wrote: > > Add the commit.signOff configuration variable to use the -s or --signoff > > option of git commit by default. > > > > Signed-off-by: Chen Jingpiao > > --- > > Welcome to the Git community! > > > > > Though we can configure signoff using format.signOff variable. Someone like > > to > > add Signed-off-by line by the committer. > > There is more discussion about this at > https://public-inbox.org/git/1482946838-28779-1-git-send-email-ehabk...@redhat.com/ > specifically > https://public-inbox.org/git/xmqqtw9m5s5m@gitster.mtv.corp.google.com/ > > Not sure if there was any other reasons and discussions brought up > since then, but that discussion seems to not favor patches that > add .signoff options. Thank you. I agree with Johannes Schindelin once said "a signoff _has_ to be a conscious act, or else it will lose its meaning." I think I shouldn't add this configuration variable. When add configuration variable for sign-off to format-patch have some discussion: https://public-inbox.org/git/20090331204338.ga88...@macbook.lan/ https://public-inbox.org/git/20090401102610.gc26...@coredump.intra.peff.net/ https://public-inbox.org/git/7veiw69p26@gitster.siamese.dyndns.org/ -- Chen Jingpiao
Re: [GSoC][PATCH] commit: add a commit.signOff config variable
On Sat, Feb 3, 2018 at 6:03 PM, Chen Jingpiao wrote: > Add the commit.signOff configuration variable to use the -s or --signoff > option of git commit by default. > > Signed-off-by: Chen Jingpiao > --- Welcome to the Git community! > > Though we can configure signoff using format.signOff variable. Someone like to > add Signed-off-by line by the committer. There is more discussion about this at https://public-inbox.org/git/1482946838-28779-1-git-send-email-ehabk...@redhat.com/ specifically https://public-inbox.org/git/xmqqtw9m5s5m@gitster.mtv.corp.google.com/ Not sure if there was any other reasons and discussions brought up since then, but that discussion seems to not favor patches that add .signoff options. Thanks, Stefan > > Documentation/config.txt | 4 +++ > Documentation/git-commit.txt | 2 ++ > builtin/commit.c | 4 +++ > t/t7501-commit.sh| 69 > > 4 files changed, 79 insertions(+) > > diff --git a/Documentation/config.txt b/Documentation/config.txt > index 0e25b2c92..5dec3f0cb 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -1303,6 +1303,10 @@ commit.gpgSign:: > convenient to use an agent to avoid typing your GPG passphrase > several times. > > +commit.signOff:: > + A boolean value which lets you enable the `-s/--signoff` option of > + `git commit` by default. See linkgit:git-commit[1]. > + > 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 f970a4342..7a28ea765 100644 > --- a/Documentation/git-commit.txt > +++ b/Documentation/git-commit.txt > @@ -166,6 +166,8 @@ 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). > + See the `commit.signOff` configuration variable in > + linkgit:git-config[1]. > > -n:: > --no-verify:: > diff --git a/builtin/commit.c b/builtin/commit.c > index 4610e3d8e..324213254 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -1548,6 +1548,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/t7501-commit.sh b/t/t7501-commit.sh > index fa61b1a4e..46733ed2a 100755 > --- a/t/t7501-commit.sh > +++ b/t/t7501-commit.sh > @@ -505,6 +505,75 @@ Myfooter: x" && > test_cmp expected actual > ' > > +test_expect_success "commit.signoff=true and --signoff omitted" ' > + echo 7 >positive && > + git add positive && > + git -c commit.signoff=true commit -m "thank you" && > + git cat-file commit HEAD | sed -e "1,/^\$/d" >actual && > + ( > + echo thank you > + echo > + git var GIT_COMMITTER_IDENT | > + sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /" > + ) >expected && > + test_cmp expected actual > +' > + > +test_expect_success "commit.signoff=true and --signoff" ' > + echo 8 >positive && > + git add positive && > + git -c commit.signoff=true commit --signoff -m "thank you" && > + git cat-file commit HEAD | sed -e "1,/^\$/d" >actual && > + ( > + echo thank you > + echo > + git var GIT_COMMITTER_IDENT | > + sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /" > + ) >expected && > + test_cmp expected actual > +' > + > +test_expect_success "commit.signoff=true and --no-signoff" ' > + echo 9 >positive && > + git add positive && > + git -c commit.signoff=true commit --no-signoff -m "thank you" && > + git cat-file commit HEAD | sed -e "1,/^\$/d" >actual && > + echo thank you >expected && > + test_cmp expected actual > +' > + > +test_expect_success "commit.signoff=false and --signoff omitted" ' > + echo 10 >positive && > + git add positive && > + git -c commit.signoff=false commit -m "thank you" && > + git cat-file commit HEAD | sed -e "1,/^\$/d" >actual && > + echo thank you >expected && > + test_cmp expected actual > +' > + > +test_expect_success "commit.signoff=false and --signoff" ' > + echo 11 >positive && > + git add positive && > + git -c commit.signoff=false commit --signoff -m "thank you" && > + git
Re: [GSoC][PATCH] commit: add a commit.signOff config variable
On Sun, Feb 04 2018, Eric Sunshine jotted: > --- >8 --- > for cfg in true false > do > for opt in '' --signoff --no-signoff > do > case "$opt:$cfg" in > --signoff:*|:true) expect= ;; > --no-signoff:*|:false) expect=! ;; > esac > test_expect_success "commit.signoff=$cfg & ${opt:---signoff omitted}" > ' > git -c commit.signoff=$cfg commit --allow-empty -m x $opt && > eval "$expect git log -1 --format=%B | grep ^Signed-off-by:" > ' > done > done > --- >8 --- > > A final consideration is that tests run slowly on Windows, and although > it's nice to be thorough by testing all six combinations, you can > probably exercise the new code sufficiently by instead testing just two > combinations. For instance, instead of all six combinations, test just > these two: > > --- >8 --- > test_expect_success 'commit.signoff=true & --signoff omitted' ' > git -c commit.signoff=true commit --allow-empty -m x && > git log -1 --format=%B | grep ^Signed-off-by: > ' > > test_expect_success 'commit.signoff=true & --no-signoff' ' > git -c commit.signoff=true commit --allow-empty -m x --no-signoff && > ! git log -1 --format=%B | grep ^Signed-off-by: > ' > --- >8 --- I just skimmed this, but just to this question. I don't think we need to worry about 2 v.s. 6 tests having an impact on Windows performance, it's just massive amounts of tests like my in-flight wildmatch test series that really matter. But if we are worring about 2 v.s. 6 there's always my in-flight EXPENSIVE_ON_WINDOWS prereq :)
Re: [GSoC][PATCH] commit: add a commit.signOff config variable
On Sun, Feb 04, 2018 at 10:03:18AM +0800, Chen Jingpiao wrote: > Add the commit.signOff configuration variable to use the -s or --signoff > option of git commit by default. > > Signed-off-by: Chen Jingpiao > --- > > Though we can configure signoff using format.signOff variable. Someone like to > add Signed-off-by line by the committer. This commentary explains why this feature is desirable, therefore it would be a good idea to include this in the commit message itself. > diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh > @@ -505,6 +505,75 @@ Myfooter: x" && > +test_expect_success "commit.signoff=true and --signoff omitted" ' > + echo 7 >positive && > + git add positive && > + git -c commit.signoff=true commit -m "thank you" && > + git cat-file commit HEAD | sed -e "1,/^\$/d" >actual && > + ( > + echo thank you > + echo > + git var GIT_COMMITTER_IDENT | > + sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /" > + ) >expected && > + test_cmp expected actual > +' The bodies of these test are quite noisy, doing a lot of work that isn't really necessary, which makes it difficult to figure out what is really being tested. Other tests in this script already check that the commit message is properly formatted when Signed-off-by: is inserted so you don't need to repeat all that boilerplate here. Instead, you are interested only in whether or not Signed-off-by: has been added to the message. For that purpose, you can use a simple 'grep' expression. The amount of copy/paste code in these six tests is also unfortunate. Rather than merely repeating the same code over and over, you could instead parameterize the test. For instance, you could run all six tests via a simple for-loop: --- >8 --- for cfg in true false do for opt in '' --signoff --no-signoff do case "$opt:$cfg" in --signoff:*|:true) expect= ;; --no-signoff:*|:false) expect=! ;; esac test_expect_success "commit.signoff=$cfg & ${opt:---signoff omitted}" ' git -c commit.signoff=$cfg commit --allow-empty -m x $opt && eval "$expect git log -1 --format=%B | grep ^Signed-off-by:" ' done done --- >8 --- A final consideration is that tests run slowly on Windows, and although it's nice to be thorough by testing all six combinations, you can probably exercise the new code sufficiently by instead testing just two combinations. For instance, instead of all six combinations, test just these two: --- >8 --- test_expect_success 'commit.signoff=true & --signoff omitted' ' git -c commit.signoff=true commit --allow-empty -m x && git log -1 --format=%B | grep ^Signed-off-by: ' test_expect_success 'commit.signoff=true & --no-signoff' ' git -c commit.signoff=true commit --allow-empty -m x --no-signoff && ! git log -1 --format=%B | grep ^Signed-off-by: ' --- >8 --- > +test_expect_success "commit.signoff=true and --signoff" ' > + echo 8 >positive && > + git add positive && > + git -c commit.signoff=true commit --signoff -m "thank you" && > + git cat-file commit HEAD | sed -e "1,/^\$/d" >actual && > + ( > + echo thank you > + echo > + git var GIT_COMMITTER_IDENT | > + sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /" > + ) >expected && > + test_cmp expected actual > +' > + > +test_expect_success "commit.signoff=true and --no-signoff" ' > + echo 9 >positive && > + git add positive && > + git -c commit.signoff=true commit --no-signoff -m "thank you" && > + git cat-file commit HEAD | sed -e "1,/^\$/d" >actual && > + echo thank you >expected && > + test_cmp expected actual > +' > + > +test_expect_success "commit.signoff=false and --signoff omitted" ' > + echo 10 >positive && > + git add positive && > + git -c commit.signoff=false commit -m "thank you" && > + git cat-file commit HEAD | sed -e "1,/^\$/d" >actual && > + echo thank you >expected && > + test_cmp expected actual > +' > + > +test_expect_success "commit.signoff=false and --signoff" ' > + echo 11 >positive && > + git add positive && > + git -c commit.signoff=false commit --signoff -m "thank you" && > + git cat-file commit HEAD | sed -e "1,/^\$/d" >actual && > + ( > + echo thank you > + echo > + git var GIT_COMMITTER_IDENT | > + sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /" > + ) >expected && > + test_cmp expected actual > +' > + > +test_expect_success "commit.signoff=false and --no-signoff" ' > + echo 12 >positive && > + git add positive && > + git -c commit.signoff=false commit --no-signoff -m "thank you" && > + git cat-file commit HEAD | sed -e "1,/^\$/d" >actual && > + echo thank you >expected && > + test_cmp expected actual > +'
[GSoC][PATCH] commit: add a commit.signOff config variable
Add the commit.signOff configuration variable to use the -s or --signoff option of git commit by default. Signed-off-by: Chen Jingpiao --- Though we can configure signoff using format.signOff variable. Someone like to add Signed-off-by line by the committer. Documentation/config.txt | 4 +++ Documentation/git-commit.txt | 2 ++ builtin/commit.c | 4 +++ t/t7501-commit.sh| 69 4 files changed, 79 insertions(+) diff --git a/Documentation/config.txt b/Documentation/config.txt index 0e25b2c92..5dec3f0cb 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1303,6 +1303,10 @@ commit.gpgSign:: convenient to use an agent to avoid typing your GPG passphrase several times. +commit.signOff:: + A boolean value which lets you enable the `-s/--signoff` option of + `git commit` by default. See linkgit:git-commit[1]. + 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 f970a4342..7a28ea765 100644 --- a/Documentation/git-commit.txt +++ b/Documentation/git-commit.txt @@ -166,6 +166,8 @@ 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). + See the `commit.signOff` configuration variable in + linkgit:git-config[1]. -n:: --no-verify:: diff --git a/builtin/commit.c b/builtin/commit.c index 4610e3d8e..324213254 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1548,6 +1548,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/t7501-commit.sh b/t/t7501-commit.sh index fa61b1a4e..46733ed2a 100755 --- a/t/t7501-commit.sh +++ b/t/t7501-commit.sh @@ -505,6 +505,75 @@ Myfooter: x" && test_cmp expected actual ' +test_expect_success "commit.signoff=true and --signoff omitted" ' + echo 7 >positive && + git add positive && + git -c commit.signoff=true commit -m "thank you" && + git cat-file commit HEAD | sed -e "1,/^\$/d" >actual && + ( + echo thank you + echo + git var GIT_COMMITTER_IDENT | + sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /" + ) >expected && + test_cmp expected actual +' + +test_expect_success "commit.signoff=true and --signoff" ' + echo 8 >positive && + git add positive && + git -c commit.signoff=true commit --signoff -m "thank you" && + git cat-file commit HEAD | sed -e "1,/^\$/d" >actual && + ( + echo thank you + echo + git var GIT_COMMITTER_IDENT | + sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /" + ) >expected && + test_cmp expected actual +' + +test_expect_success "commit.signoff=true and --no-signoff" ' + echo 9 >positive && + git add positive && + git -c commit.signoff=true commit --no-signoff -m "thank you" && + git cat-file commit HEAD | sed -e "1,/^\$/d" >actual && + echo thank you >expected && + test_cmp expected actual +' + +test_expect_success "commit.signoff=false and --signoff omitted" ' + echo 10 >positive && + git add positive && + git -c commit.signoff=false commit -m "thank you" && + git cat-file commit HEAD | sed -e "1,/^\$/d" >actual && + echo thank you >expected && + test_cmp expected actual +' + +test_expect_success "commit.signoff=false and --signoff" ' + echo 11 >positive && + git add positive && + git -c commit.signoff=false commit --signoff -m "thank you" && + git cat-file commit HEAD | sed -e "1,/^\$/d" >actual && + ( + echo thank you + echo + git var GIT_COMMITTER_IDENT | + sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /" + ) >expected && + test_cmp expected actual +' + +test_expect_success "commit.signoff=false and --no-signoff" ' + echo 12 >positive && + git add positive && + git -c commit.signoff=false commit --no-signoff -m "thank you" && + git cat-file commit HEAD | sed -e "1,/^\$/d" >actual && + echo thank you >expected && + test_cmp expected actual +' + test_expect_success 'multiple -m' ' >negative && -- 2.16.1.70.g5ccd54536