[GSoC][PATCH] commit: add a commit.signOff config variable

[GSoC][PATCH] commit: add a commit.signOff config variable

[Chen Jingpiao]

Add the commit.signOff configuration variable to use the -s or --signoff
option of git commit by default.

Signed-off-by: Chen Jingpiao <chenjingpiao@gmail.com>
---

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

Re: [GSoC][PATCH] commit: add a commit.signOff config variable

[Eric Sunshine]

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 <chenjingpiao@gmail.com>
> ---
> 
> 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
> +'

Re: [GSoC][PATCH] commit: add a commit.signOff config variable

[Ævar Arnfjörð Bjarmason]

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

[Stefan Beller]

On Sat, Feb 3, 2018 at 6:03 PM, Chen Jingpiao <chenjingpiao@gmail.com> wrote:
> Add the commit.signOff configuration variable to use the -s or --signoff
> option of git commit by default.
>
> Signed-off-by: Chen Jingpiao <chenjingpiao@gmail.com>
> ---

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-ehabkost@redhat.com/
specifically
https://public-inbox.org/git/xmqqtw9m5s5m.fsf@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 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
>

Re: [GSoC][PATCH] commit: add a commit.signOff config variable

[Chen Jingpiao]

On 02/05 01:50, Stefan Beller wrote:
> On Sat, Feb 3, 2018 at 6:03 PM, Chen Jingpiao <chenjingpiao@gmail.com> wrote:
> > Add the commit.signOff configuration variable to use the -s or --signoff
> > option of git commit by default.
> >
> > Signed-off-by: Chen Jingpiao <chenjingpiao@gmail.com>
> > ---
> 
> 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-ehabkost@redhat.com/
> specifically
> https://public-inbox.org/git/xmqqtw9m5s5m.fsf@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.GA88381@macbook.lan/
	https://public-inbox.org/git/20090401102610.GC26181@coredump.intra.peff.net/
	https://public-inbox.org/git/7veiw69p26.fsf@gitster.siamese.dyndns.org/

--
Chen Jingpiao