[PATCH v6 0/4] commit: Add commit.verbose configuration

[PATCH v6 0/4] commit: Add commit.verbose configuration

[Caleb Thompson]

This patch allows people to set commit.verbose to implicitly send
--verbose to git-commit.

It introduces several cleanup patches to t/t7505-commit-verbose.sh to
bring it closer to the current state of the tests as they have been
explained to me, then adds the verbose config and --no-verbose flag.

Since the last version of this patch
(http://thread.gmane.org/gmane.comp.version-control.git/251486), there
have been a couple of commit message clarifications and the body of the
check-for-no-diff script in the last patch was changed to simply negate
check-for-diff.

Caleb Thompson

------------------------------------------------------


Caleb Thompson (4):
  commit test: Use test_config instead of git-config
  commit test: Use write_script
  commit test: test_set_editor in each test
  commit: Add commit.verbose configuration

 Documentation/config.txt               |  5 +++
 Documentation/git-commit.txt           |  8 ++++-
 builtin/commit.c                       |  4 +++
 contrib/completion/git-completion.bash |  1 +
 t/t7507-commit-verbose.sh              | 64 +++++++++++++++++++++++++---------
 5 files changed, 64 insertions(+), 18 deletions(-)

--
2.0.0

[PATCH v6 1/4] commit test: Use test_config instead of git-config

[Caleb Thompson]

Some of the tests in t/t7507-commit-verbose.sh were still using
git-config to set configuration. Change them to use the test_config
helper.

Signed-off-by: Caleb Thompson <caleb@calebthompson.io>
Reviewed-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 t/t7507-commit-verbose.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index 2ddf28c..6d778ed 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -43,7 +43,7 @@ test_expect_success 'verbose diff is stripped out' '
 '
 
 test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '
-	git config diff.mnemonicprefix true &&
+	test_config diff.mnemonicprefix true &&
 	git commit --amend -v &&
 	check_message message
 '
@@ -71,7 +71,7 @@ test_expect_success 'diff in message is retained with -v' '
 '
 
 test_expect_success 'submodule log is stripped out too with -v' '
-	git config diff.submodule log &&
+	test_config diff.submodule log &&
 	git submodule add ./. sub &&
 	git commit -m "sub added" &&
 	(
-- 
2.0.0

Re: [PATCH v6 1/4] commit test: Use test_config instead of git-config

[Junio C Hamano]

Caleb Thompson <caleb@calebthompson.io> writes:

> Some of the tests in t/t7507-commit-verbose.sh were still using
> git-config to set configuration. Change them to use the test_config
> helper.

"were still using" is only a half of the story, and we need to be
more careful than that, though.

> Signed-off-by: Caleb Thompson <caleb@calebthompson.io>
> Reviewed-by: Jeremiah Mahler <jmmahler@gmail.com>
> ---
>  t/t7507-commit-verbose.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> index 2ddf28c..6d778ed 100755
> --- a/t/t7507-commit-verbose.sh
> +++ b/t/t7507-commit-verbose.sh
> @@ -43,7 +43,7 @@ test_expect_success 'verbose diff is stripped out' '
>  '
>  
>  test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '
> -	git config diff.mnemonicprefix true &&
> +	test_config diff.mnemonicprefix true &&

By switching to test_config, you unconfigure the diff.mnemonicprefix
configuration after this test piece is done.  The next one, "diff in
message is retained with -v", used to show the change using c/ and
i/ as prefixes in the log editor, but now it should be showing a/
and b/.

Have you thought about the reason why the log message used in that
test uses c/ and i/ prefixes as a sample patch to be retained,
instead of a/ and b/ prefixes?  It is to make sure that this
in-message patch looks similar to the diff for "--verbose" option,
and we used to do the latter under mnemonicprefix.  That way, we
would be more sure that an incorrect implementation that cuts the
result given from the editor at "diff --git $srcprefix/" will fail
the test.

This is the kind of thing I mean by "we need to be more careful".

If somebody who is doing "git config to test_config" conversion
understands what he is doing, I would have expected to see that
these c/ and i/ prefixes in the sample log message are replaced
to use a/ and b/ prefixes.

>  	git commit --amend -v &&
>  	check_message message
>  '
> @@ -71,7 +71,7 @@ test_expect_success 'diff in message is retained with -v' '
>  '
>  
>  test_expect_success 'submodule log is stripped out too with -v' '
> -	git config diff.submodule log &&
> +	test_config diff.submodule log &&
>  	git submodule add ./. sub &&
>  	git commit -m "sub added" &&
>  	(

Can we make a similar reasoning on possible fallout from this
change?  An expected answer would be something like:

    The remaining test pieces after this one does not make any
    change before running "commit -a -v", so having diff.submodule
    set to log or unset does not make any difference to them.

Re: [PATCH v6 1/4] commit test: Use test_config instead of git-config

[Junio C Hamano]

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

> Caleb Thompson <caleb@calebthompson.io> writes:
>
>> Some of the tests in t/t7507-commit-verbose.sh were still using
>> git-config to set configuration. Change them to use the test_config
>> helper.
>
> "were still using" is only a half of the story, and we need to be
> more careful than that, though.

I'd rephrase it like so if I were doing this patch myself:

    Subject: t7507: use test_config instead of git-config
    
    Some of the tests in t/t7507-commit-verbose.sh uses git-config
    to set configuration, but the updated variable setting does not
    have to be used in later tests.
    
    Change them to use the test_config helper.

[PATCH v6 2/4] commit test: Use write_script

[Caleb Thompson]

Use write_script from t/test-lib-functions.sh instead of cat, shebang,
and chmod. This aids in readability for creating the script by using the
named function and allows us to turn off interpolation in the heredoc of
the script body to avoid extra escaping, since $SHELL_PATH is handled
for us.

Signed-off-by: Caleb Thompson <caleb@calebthompson.io>
Reviewed-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 t/t7507-commit-verbose.sh | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index 6d778ed..db09107 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -3,11 +3,9 @@
 test_description='verbose commit template'
 . ./test-lib.sh
 
-cat >check-for-diff <<EOF
-#!$SHELL_PATH
-exec grep '^diff --git' "\$1"
+write_script check-for-diff <<-'EOF'
+	exec grep '^diff --git' "$1"
 EOF
-chmod +x check-for-diff
 test_set_editor "$PWD/check-for-diff"
 
 cat >message <<'EOF'
-- 
2.0.0

[PATCH v6 3/4] commit test: test_set_editor in each test

[Caleb Thompson]

t/t7507-commit-verbose.sh was using a global test_set_editor call to
build its environment. The $EDITOR being used was not necessary for
all tests, and was in fact circumvented using subshells in some
cases.

To improve robustness against global state changes and avoid the use of
subshells to temporarily switch the editor as some were with
GIT_EDITOR=cat, set the editor explicitly wherever it will be important.

Specifically, in tests that need to check for the presence of a diff in the
editor, make calls to set_test_editor to set $EDITOR to check-for-diff
rather than relying on that editor being configured globally. This also
helps readers grok the tests as the setup is closer to the verification.

Signed-off-by: Caleb Thompson <caleb@calebthompson.io>
Reviewed-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 t/t7507-commit-verbose.sh | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index db09107..35a4d06 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -6,7 +6,6 @@ test_description='verbose commit template'
 write_script check-for-diff <<-'EOF'
 	exec grep '^diff --git' "$1"
 EOF
-test_set_editor "$PWD/check-for-diff"
 
 cat >message <<'EOF'
 subject
@@ -21,6 +20,7 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'initial commit shows verbose diff' '
+	test_set_editor "$PWD/check-for-diff" &&
 	git commit --amend -v
 '
 
@@ -36,11 +36,13 @@ check_message() {
 }
 
 test_expect_success 'verbose diff is stripped out' '
+	test_set_editor "$PWD/check-for-diff" &&
 	git commit --amend -v &&
 	check_message message
 '
 
 test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '
+	test_set_editor "$PWD/check-for-diff" &&
 	test_config diff.mnemonicprefix true &&
 	git commit --amend -v &&
 	check_message message
@@ -77,20 +79,14 @@ test_expect_success 'submodule log is stripped out too with -v' '
 		echo "more" >>file &&
 		git commit -a -m "submodule commit"
 	) &&
-	(
-		GIT_EDITOR=cat &&
-		export GIT_EDITOR &&
-		test_must_fail git commit -a -v 2>err
-	) &&
+	test_set_editor cat &&
+	test_must_fail git commit -a -v 2>err &&
 	test_i18ngrep "Aborting commit due to empty commit message." err
 '
 
 test_expect_success 'verbose diff is stripped out with set core.commentChar' '
-	(
-		GIT_EDITOR=cat &&
-		export GIT_EDITOR &&
-		test_must_fail git -c core.commentchar=";" commit -a -v 2>err
-	) &&
+	test_set_editor cat &&
+	test_must_fail git -c core.commentchar=";" commit -a -v 2>err &&
 	test_i18ngrep "Aborting commit due to empty commit message." err
 '
 
-- 
2.0.0

[PATCH v6 4/4] commit: Add commit.verbose configuration

[Caleb Thompson]

Add a new configuration variable commit.verbose to implicitly pass
--verbose to git-commit. Ensure that --no-verbose to git-commit
negates that setting.

Signed-off-by: Caleb Thompson <caleb@calebthompson.io>
---
 Documentation/config.txt               |  5 +++++
 Documentation/git-commit.txt           |  8 +++++++-
 builtin/commit.c                       |  4 ++++
 contrib/completion/git-completion.bash |  1 +
 t/t7507-commit-verbose.sh              | 36 ++++++++++++++++++++++++++++++++++
 5 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9f467d3..85ef016 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1017,6 +1017,11 @@ commit.template::
 	"`~/`" is expanded to the value of `$HOME` and "`~user/`" to the
 	specified user's home directory.
 
+commit.verbose::
+	A boolean to enable/disable inclusion of diff information in the
+	commit message template when using an editor to prepare the commit
+	message.  Defaults to false.
+
 credential.helper::
 	Specify an external helper to be called when a username or
 	password credential is needed; the helper may consult external
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 0bbc8f5..8cb3439 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -282,7 +282,13 @@ configuration variable documented in linkgit:git-config[1].
 	Show unified diff between the HEAD commit and what
 	would be committed at the bottom of the commit message
 	template.  Note that this diff output doesn't have its
-	lines prefixed with '#'.
+	lines prefixed with '#'.  The `commit.verbose` configuration
+	variable can be set to true to implicitly send this option.
+
+--no-verbose::
+	Do not show the unified diff at the bottom of the commit message
+	template.  This is the default behavior, but can be used to override
+	the `commit.verbose` configuration variable.
 
 -q::
 --quiet::
diff --git a/builtin/commit.c b/builtin/commit.c
index 5e2221c..b746dc8 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1503,6 +1503,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.verbose")) {
+		verbose = git_config_bool(k, v);
+		return 0;
+	}
 
 	status = git_gpg_config(k, v, NULL);
 	if (status)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 019026e..b686985 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1980,6 +1980,7 @@ _git_config ()
 		color.ui
 		commit.status
 		commit.template
+		commit.verbose
 		core.abbrev
 		core.askpass
 		core.attributesfile
diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index 35a4d06..471bd8f 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -7,6 +7,10 @@ write_script check-for-diff <<-'EOF'
 	exec grep '^diff --git' "$1"
 EOF
 
+write_script check-for-no-diff <<-'EOF'
+	! check-for-diff
+EOF
+
 cat >message <<'EOF'
 subject
 
@@ -48,6 +52,38 @@ test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '
 	check_message message
 '
 
+test_expect_success 'commit shows verbose diff with commit.verbose true' '
+	echo morecontent >>file &&
+	git add file &&
+	test_config commit.verbose true &&
+	test_set_editor "$PWD/check-for-diff" &&
+	git commit --amend
+'
+
+test_expect_success 'commit --verbose overrides commit.verbose false' '
+	echo evenmorecontent >>file &&
+	git add file &&
+	test_config commit.verbose false  &&
+	test_set_editor "$PWD/check-for-diff" &&
+	git commit --amend --verbose
+'
+
+test_expect_success 'commit does not show verbose diff with commit.verbose false' '
+	echo evenmorecontent >>file &&
+	git add file &&
+	test_config commit.verbose false &&
+	test_set_editor "$PWD/check-for-no-diff" &&
+	git commit --amend
+'
+
+test_expect_success 'commit --no-verbose overrides commit.verbose true' '
+	echo evenmorecontent >>file &&
+	git add file &&
+	test_config commit.verbose true &&
+	test_set_editor "$PWD/check-for-no-diff" &&
+	git commit --amend --no-verbose
+'
+
 cat >diff <<'EOF'
 This is an example commit message that contains a diff.
 
-- 
2.0.0

Re: [PATCH v6 4/4] commit: Add commit.verbose configuration

[Jeff King]

On Tue, Jun 17, 2014 at 02:39:00PM -0500, Caleb Thompson wrote:

> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> index 35a4d06..471bd8f 100755
> --- a/t/t7507-commit-verbose.sh
> +++ b/t/t7507-commit-verbose.sh
> @@ -7,6 +7,10 @@ write_script check-for-diff <<-'EOF'
>  	exec grep '^diff --git' "$1"
>  EOF
>  
> +write_script check-for-no-diff <<-'EOF'
> +	! check-for-diff
> +EOF

Don't you need "$PWD/" or similar here, since check-for-diff is not in our path?

Otherwise, your tests will always pass ("! check-for-diff" is always
true, since we cannot find it). I get (running with "-v"):

expecting success: 
        echo evenmorecontent >>file &&
        git add file &&
        test_config commit.verbose true &&
        test_set_editor "$PWD/check-for-no-diff" &&
        git commit --amend --no-verbose

/home/peff/compile/git/t/trash directory.t7507-commit-verbose/check-for-no-diff: 2: /home/peff/compile/git/t/trash directory.t7507-commit-verbose/check-for-no-diff: check-for-diff: not found

Other than that, the whole series looks OK to me.

-Peff

Re: [PATCH v6 4/4] commit: Add commit.verbose configuration

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Jun 17, 2014 at 02:39:00PM -0500, Caleb Thompson wrote:
>
>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
>> index 35a4d06..471bd8f 100755
>> --- a/t/t7507-commit-verbose.sh
>> +++ b/t/t7507-commit-verbose.sh
>> @@ -7,6 +7,10 @@ write_script check-for-diff <<-'EOF'
>>  	exec grep '^diff --git' "$1"
>>  EOF
>>  
>> +write_script check-for-no-diff <<-'EOF'
>> +	! check-for-diff
>> +EOF
>
> Don't you need "$PWD/" or similar here, since check-for-diff is not in our path?

I actually would prefer a simpler

	! grep '^diff --git' "$1"

that does not depend on what other parts of the tests have done.

Re: [PATCH v6 4/4] commit: Add commit.verbose configuration

[Jeff King]

On Tue, Jun 17, 2014 at 02:37:45PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Tue, Jun 17, 2014 at 02:39:00PM -0500, Caleb Thompson wrote:
> >
> >> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> >> index 35a4d06..471bd8f 100755
> >> --- a/t/t7507-commit-verbose.sh
> >> +++ b/t/t7507-commit-verbose.sh
> >> @@ -7,6 +7,10 @@ write_script check-for-diff <<-'EOF'
> >>  	exec grep '^diff --git' "$1"
> >>  EOF
> >>  
> >> +write_script check-for-no-diff <<-'EOF'
> >> +	! check-for-diff
> >> +EOF
> >
> > Don't you need "$PWD/" or similar here, since check-for-diff is not in our path?
> 
> I actually would prefer a simpler
> 
> 	! grep '^diff --git' "$1"
> 
> that does not depend on what other parts of the tests have done.

Yeah, if it is that simple, I would agree (I did not even look at the
definition of check-for-diff, and assumed it was more complicated :) ).

-Peff

Re: [PATCH v6 0/4] commit: Add commit.verbose configuration

[Torstein Hegge]

On Tue, Jun 17, 2014 at 14:38:56 -0500, Caleb Thompson wrote:
> This patch allows people to set commit.verbose to implicitly send
> --verbose to git-commit.
> 
> It introduces several cleanup patches to t/t7505-commit-verbose.sh to
> bring it closer to the current state of the tests as they have been
> explained to me, then adds the verbose config and --no-verbose flag.
> 
> Since the last version of this patch
> (http://thread.gmane.org/gmane.comp.version-control.git/251486), there
> have been a couple of commit message clarifications and the body of the
> check-for-no-diff script in the last patch was changed to simply negate
> check-for-diff.

Hi Caleb,

Do you intend to work further on this topic? I have been using a similar
(but less polished) patch, and would like to see this included in Git.

If you don't have time to work on it, I can incorporate the comments on
this version and submit a v7.


Torstein

Re: [PATCH v6 0/4] commit: Add commit.verbose configuration

[Caleb Thompson]

[-- Attachment #1: Type: text/plain, Size: 1350 bytes --]

Hey Torstein.

I'd planned to come back with a heavily simplified version of this that
didn't include any of the related changes, just the feature and the new
test, since a lot of that seemed controversial.

That said, I haven't done so and you're welcome to take a whack at this
feedback if you'd like.

Caleb Thompson

On Fri, Feb 27, 2015 at 10:13:36AM +0100, Torstein Hegge wrote:
> On Tue, Jun 17, 2014 at 14:38:56 -0500, Caleb Thompson wrote:
> > This patch allows people to set commit.verbose to implicitly send
> > --verbose to git-commit.
> >
> > It introduces several cleanup patches to t/t7505-commit-verbose.sh to
> > bring it closer to the current state of the tests as they have been
> > explained to me, then adds the verbose config and --no-verbose flag.
> >
> > Since the last version of this patch
> > (http://thread.gmane.org/gmane.comp.version-control.git/251486), there
> > have been a couple of commit message clarifications and the body of the
> > check-for-no-diff script in the last patch was changed to simply negate
> > check-for-diff.
>
> Hi Caleb,
>
> Do you intend to work further on this topic? I have been using a similar
> (but less polished) patch, and would like to see this included in Git.
>
> If you don't have time to work on it, I can incorporate the comments on
> this version and submit a v7.
>
>
> Torstein

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]