[PATCH v2] commit: support commit.verbose and --no-verbose

Incorporated changes from Duy Nguyen and Jeremiah Mahler.

Jeremiah, I didn't make the changes about `<<-EOF` or `test_expect_success`
because I'm guessing that keeping the local style of the code intact is more
important than using those. Do you think it makes sense to refactor the rest of
the test file (t/t7507-commit-verbose.sh) to use those? I could also change the
other `git config` calls to use `test_config`.

Duy, you were right about `-V`. Do you know of a simple way to add that
shortened flag? `OPT_BOOL('v', "verbose", ...)` gives me `-v`, `--verbose`, and
`--no-verbose`, but no `-V` as a shortened form of `--no-verbose`.

commit 1a49356b87c9028e68e731f34790c11a3075f736
Author: Caleb Thompson [off-list ref]
Date:   Fri May 23 11:47:44 2014 -0500

    commit: support commit.verbose and --no-verbose

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

    Signed-off-by: Caleb Thompson [off-list ref]
    Reviewed-by: Duy Nguyen [off-list ref]
    Reviewed-by: Jeremiah Mahler [off-list ref]

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1932e9b..a245928 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -1009,6 +1009,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..d7b50e2 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 9cfef6c..7978d7f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -1417,6 +1417,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)

@@ -1484,7 +1488,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	static struct wt_status s;
 	static struct option builtin_commit_options[] = {
 		OPT__QUIET(&quiet, N_("suppress summary after successful commit")),
-		OPT__VERBOSE(&verbose, N_("show diff in commit message template")),
+		OPT_BOOL('v', "verbose", &verbose, N_("show diff in commit message template")),
 
 		OPT_GROUP(N_("Commit message options")),
 		OPT_FILENAME('F', "file", &logfile, N_("read message from file")),

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 2c59a76..b8f4b94 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash

@@ -1976,6 +1976,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 2ddf28c..bea5d88 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh

@@ -10,6 +10,12 @@ EOF
 chmod +x check-for-diff
 test_set_editor "$PWD/check-for-diff"
 
+cat >check-for-no-diff <<EOF
+#!$SHELL_PATH
+exec grep -v '^diff --git' "\$1"
+EOF
+chmod +x check-for-no-diff
+
 cat >message <<'EOF'
 subject

@@ -48,6 +54,21 @@ test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '
 	check_message message
 '

+test_expect_success 'commit shows verbose diff with set commit.verbose' '
+	echo morecontent >file &&
+	git add file &&
+	test_config commit.verbose true &&
+	check_message message
+'
+
+test_expect_success 'commit does not show verbose diff with --no-verbose' '
+	echo morecontent >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.

On Sun, May 25, 2014 at 01:24:27AM -0500, Caleb Thompson wrote:
...

 	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.

Why is there two spaces between "diff  at"?
Needs a space between "the`comm" -> "the `comm".

+cat >check-for-no-diff <<EOF
+#!$SHELL_PATH
+exec grep -v '^diff --git' "\$1"
+EOF
+chmod +x check-for-no-diff
+

Me personally, I would leave it like that for now, since that is
the style being used nearby.  We'll see what others have to say.

I certainly wouldn't convert all the other cases to use
test_expect_success.  Leave that for another patch.

I like those better with 'test_config' instead of 'git config', good.

Keep working on it, it is looking better :-)

-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler

On Sun, May 25, 2014 at 01:24:27AM -0500, Caleb Thompson wrote:

Incorporated changes from Duy Nguyen and Jeremiah Mahler.

...

+test_expect_success 'commit shows verbose diff with set commit.verbose' '
+	echo morecontent >file &&
+	git add file &&
+	test_config commit.verbose true &&
+	check_message message
+'

This test case doesn't appear to be checking for the verbose output.
No commit is made so it can't check for the presence of a diff.
"check_message message" passes as it did in the test above this (not shown).

-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler

On Sun, May 25, 2014 at 1:24 PM, Caleb Thompson [off-list ref] wrote:

Duy, you were right about `-V`. Do you know of a simple way to add that
shortened flag? `OPT_BOOL('v', "verbose", ...)` gives me `-v`, `--verbose`, and
`--no-verbose`, but no `-V` as a shortened form of `--no-verbose`.

No, I don't think parse_options() allows something like that. And we
probably don't want -V for --no-verbose unless it's very often used.
-- 
Duy

On Sun, May 25, 2014 at 2:24 AM, Caleb Thompson [off-list ref] wrote:

Incorporated changes from Duy Nguyen and Jeremiah Mahler.

As a courtesy to reviewers, it is helpful to provide a pointer to the
previous submission to give context for the new submission. For
instance, like this [1].

[1]: http://git.661346.n2.nabble.com/commit-support-commit-verbose-and-no-verbose-td7611617.html

Jeremiah, I didn't make the changes about `<<-EOF` or `test_expect_success`
because I'm guessing that keeping the local style of the code intact is more
important than using those. Do you think it makes sense to refactor the rest of
the test file (t/t7507-commit-verbose.sh) to use those? I could also change the
other `git config` calls to use `test_config`.

Generally speaking, it is important to respect local style, however,
it is also appropriate to include one or more cleanup patches before
your primary changes in order to bring the code in line with current
practices. Conversion to test_config could be such a cleanup patch.

Duy, you were right about `-V`. Do you know of a simple way to add that
shortened flag? `OPT_BOOL('v', "verbose", ...)` gives me `-v`, `--verbose`, and
`--no-verbose`, but no `-V` as a shortened form of `--no-verbose`.

At this point, after your email commentary but before the actual
patch, you should have a scissor line -->8-- so that "git am" can
extract your patch automatically from the email.

commit 1a49356b87c9028e68e731f34790c11a3075f736

Drop this line. It has no meaning outside of your local repository.

Author: Caleb Thompson [off-list ref]
Date:   Fri May 23 11:47:44 2014 -0500

Ditto for the date.

    commit: support commit.verbose and --no-verbose

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

The commit message would read just as well or better without the backquotes.

    Signed-off-by: Caleb Thompson [off-list ref]
    Reviewed-by: Duy Nguyen [off-list ref]
    Reviewed-by: Jeremiah Mahler [off-list ref]

Considering that the code in this patch has changed since v1, it's
probably not appropriate to add these Reviewed-by: lines.

Already mentioned by Jeremiah: s/diff\s+/diff /

+       template.  This is the default behavior, but can be used to override
+       the`commit.verbose` configuration variable.

Also already mentioned: s/the/the /

This is not a new problem, but since you copied and modified the
test_set_editor invocation for your own test (below), it can be
mentioned that $(pwd) should be used rather than $PWD. See discussion
of $(pwd) in t/README. A preparatory patch which fixes this would not
be unwelcome.

+cat >check-for-no-diff <<EOF
+#!$SHELL_PATH
+exec grep -v '^diff --git' "\$1"
+EOF
+chmod +x check-for-no-diff

write_script (from test-lib-functions.sh) would be a more appropriate
and modern way to compose this script. If you're concerned about style
consistency, a cleanup patch before this one could employ write_script
for the check-for-diff script, as well.

Also, since this script is used by only the one test, current practice
suggests that script creation should be done within the test itself.

Is your intention to add more content to 'file'? If so, use '>>'.

+       git add file &&
+       test_config commit.verbose true &&
+       check_message message

As Jeremiah pointed out, this test is not actually testing if
commit.verbose=true worked since it's not invoking git-commit. In
fact, check_message is testing something unrelated. You probably meant
"git commit --amend" rather than "check_message message"

+'
+
+test_expect_success 'commit does not show verbose diff with --no-verbose' '

As this is the only test which needs check-for-no-diff, it would be
appropriate to move script creation (via write_script) here into the
test itself (unless you plan on adding more tests which invoke the
script).

+       echo morecontent >file &&
+       git add file &&

Again, since you're using '>' rather than '>>', you haven't actually
changed the content of the file since the last test, so this code
serves no purpose.

+       test_config commit.verbose true &&
+       test_set_editor "$PWD/check-for-no-diff" &&

As noted above, use $(pwd) rather than $PWD.

This invocation of test_set_editor potentially breaks tests following
this one (including tests which may be added in the future) since it
changes the global state established by test_set_editor near the top
of the script. To avoid such a problem, you could invoke
test_set_editor and git-commit in a subshell.

Alternately, current practice would suggest that each test which
requires a particular editor should be responsible for setting it. As
such, a preparatory patch could drop the global test_set_editor and
invoke it instead in each test which requires it. (In fact, there are
a couple tests which are still setting EDITOR manually, and these
could be converted to test_set_editor.)

+       git commit --amend --no-verbose
+'

You're missing some potential tests, such as:

commit.verbose = <unset> (optional)
commit.verbose = false
--verbose overrides commit.verbose=false

 cat >diff <<'EOF'
 This is an example commit message that contains a diff.

t/t7507-commit-verbose.sh was using a global test_set_editor call to
build its environment.

Rather than building global state with test_set_editor at the beginning
of the file, move test_set_editor calls into each test.

Besides being inline with current practices, it also allows the tests
which required GIT_EDITOR=cat to avoid using a subshell and simplify
their logic.

Signed-off-by: Caleb Thompson <redacted>
---
 t/t7507-commit-verbose.sh | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index e62d921..310b68b 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,10 +20,12 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'initial commit shows verbose diff' '
+	test_set_editor "$(pwd)/check-for-diff" &&
 	git commit --amend -v
 '
 
 test_expect_success 'second commit' '
+	test_set_editor "$(pwd)/check-for-diff" &&
 	echo content modified >file &&
 	git add file &&
 	git commit -F message

@@ -36,11 +37,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

@@ -59,16 +62,19 @@ index 0000000..f95c11d
 EOF
 
 test_expect_success 'diff in message is retained without -v' '
+	test_set_editor "$(pwd)/check-for-diff" &&
 	git commit --amend -F diff &&
 	check_message diff
 '
 
 test_expect_success 'diff in message is retained with -v' '
+	test_set_editor "$(pwd)/check-for-diff" &&
 	git commit --amend -F diff -v &&
 	check_message diff
 '
 
 test_expect_success 'submodule log is stripped out too with -v' '
+	test_set_editor "$(pwd)/check-for-diff" &&
 	test_config diff.submodule log &&
 	git submodule add ./. sub &&
 	git commit -m "sub added" &&

@@ -77,20 +83,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
 '
 

-- 
1.9.3

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 <redacted>
---
 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" &&
 	(

-- 
1.9.3

Signed-off-by: Caleb Thompson <redacted>
---
 t/t7507-commit-verbose.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index 6d778ed..3b06d73 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh

@@ -8,7 +8,7 @@ cat >check-for-diff <<EOF
 exec grep '^diff --git' "\$1"
 EOF
 chmod +x check-for-diff
-test_set_editor "$PWD/check-for-diff"
+test_set_editor "$(pwd)/check-for-diff"
 
 cat >message <<'EOF'
 subject

-- 
1.9.3

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

Signed-off-by: Caleb Thompson <redacted>
---
 Documentation/config.txt               |  5 +++++
 Documentation/git-commit.txt           |  8 +++++++-
 builtin/commit.c                       |  6 +++++-
 contrib/completion/git-completion.bash |  1 +
 t/t7507-commit-verbose.sh              | 36 ++++++++++++++++++++++++++++++++++
 5 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1932e9b..a245928 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -1009,6 +1009,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 9cfef6c..7978d7f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -1417,6 +1417,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)

@@ -1484,7 +1488,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	static struct wt_status s;
 	static struct option builtin_commit_options[] = {
 		OPT__QUIET(&quiet, N_("suppress summary after successful commit")),
-		OPT__VERBOSE(&verbose, N_("show diff in commit message template")),
+		OPT_BOOL('v', "verbose", &verbose, N_("show diff in commit message template")),
 
 		OPT_GROUP(N_("Commit message options")),
 		OPT_FILENAME('F', "file", &logfile, N_("read message from file")),

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 2c59a76..b8f4b94 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash

@@ -1976,6 +1976,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 310b68b..b9eb317 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
+	exec grep -v '^diff --git' "\$1"
+EOF
+
 cat >message <<'EOF'
 subject
 

@@ -49,6 +53,38 @@ test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '
 	check_message message
 '
 
+test_expect_success 'commit shows verbose diff with set 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 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.
 

-- 
1.9.3

Use write_script from t/test-lib-functions instead of cat, shebang, and
chmod.

Signed-off-by: Caleb Thompson <redacted>
---
 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 3b06d73..e62d921 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'

-- 
1.9.3

This patch allows people to set commit.verbose to implicitly send
--verbose to git-commit. It also introduces --no-verbose to
override the configuration setting.

This version incorporates changes suggested by Eric Sunshine, Duy
Nguyen, and Jeremiah Mahler.

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

Caleb Thompson (5):
      commit test: Use test_config instead of git-config
      commit test: Change $PWD to $(pwd)
      commit test: Use write_script
      commit test: test_set_editor in each test
      commit: support commit.verbose and --no-verbose

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

j
On Mon, May 26, 2014 at 01:56:26PM -0500, Caleb Thompson wrote:

Add a new configuration variable commit.verbose to implicitly pass
 

...

+test_expect_success 'commit shows verbose diff with set 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 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
+'
+

...

It appears that these tests still aren't checking to see if the
"verbose" output appears in the commit message.

-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler

The editors, `check-for-diff` and `check-for-no-diffs`, are grepping for the
output and lack thereof, respectively.

On Mon, May 26, 2014 at 01:33:04PM -0700, Jeremiah Mahler wrote:

j
On Mon, May 26, 2014 at 01:56:26PM -0500, Caleb Thompson wrote:

quoted

Add a new configuration variable commit.verbose to implicitly pass
 

...

quoted

+test_expect_success 'commit shows verbose diff with set 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 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
+'
+

...

quoted

It appears that these tests still aren't checking to see if the
"verbose" output appears in the commit message.

-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler

Caleb,

On Mon, May 26, 2014 at 01:56:26PM -0500, Caleb Thompson wrote:

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

Signed-off-by: Caleb Thompson <redacted>
---
 Documentation/config.txt               |  5 +++++
 '

...

 
+test_expect_success 'commit shows verbose diff with set commit.verbose=true' '
+	echo morecontent >>file &&

...

+'
+
+test_expect_success 'commit --verbose overrides verbose=false' '
+	echo evenmorecontent >>file &&

...

+
+test_expect_success 'commit does not show verbose diff with commit.verbose=false' '
+	echo evenmorecontent >>file &&

...

+'
+
+test_expect_success 'commit --no-verbose overrides commit.verbose=true' '
+	echo evenmorecontent >>file &&

...

+'
+
 

Some minor style nits...

Use a consistent naming convention for your tests.  verbose=false looks
different than commit.verbose=false at first glance.  Also, since
"commit.verbose=false" is an invalid syntax for a config option, I would
remove the '=' and just make it "commit.verbose false".

-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler

Caleb,

On Mon, May 26, 2014 at 01:56:21PM -0500, Caleb Thompson wrote:

This patch allows people to set commit.verbose to implicitly send
--verbose to git-commit. It also introduces --no-verbose to
override the configuration setting.

This version incorporates changes suggested by Eric Sunshine, Duy
Nguyen, and Jeremiah Mahler.

...

Other than the minor style issue I pointed out in another email, it looks
good, and the patch set works properly on my machine.

-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler

Great, thanks Jeremiah!

I made that change, and will send up another patch version in the next day or so
while I wait on others who may have input.

I'm really appreciative of everyone's feedback!

Caleb

------------------------------------>8----------------------------------

diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index b9eb317..88de624 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh

@@ -53,7 +53,7 @@ test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '
        check_message message
 '
 
-test_expect_success 'commit shows verbose diff with set commit.verbose=true' '
+test_expect_success 'commit shows verbose diff with commit.verbose true' '
        echo morecontent >>file &&
        git add file &&
        test_config commit.verbose true &&

@@ -61,7 +61,7 @@ test_expect_success 'commit shows verbose diff with set commit.verbose=true' '
        git commit --amend
 '
 
-test_expect_success 'commit --verbose overrides verbose=false' '
+test_expect_success 'commit --verbose overrides commit.verbose false' '
        echo evenmorecontent >>file &&
        git add file &&
        test_config commit.verbose false  &&

@@ -69,7 +69,7 @@ test_expect_success 'commit --verbose overrides verbose=false' '
        git commit --amend --verbose
 '
 
-test_expect_success 'commit does not show verbose diff with commit.verbose=false' '
+test_expect_success 'commit does not show verbose diff with commit.verbose false' '
        echo evenmorecontent >>file &&
        git add file &&
        test_config commit.verbose false &&

@@ -77,7 +77,7 @@ test_expect_success 'commit does not show verbose diff with commit.verbose=false
        git commit --amend
 '
 
-test_expect_success 'commit --no-verbose overrides commit.verbose=true' '
+test_expect_success 'commit --no-verbose overrides commit.verbose true' '
        echo evenmorecontent >>file &&
        git add file &&
        test_config commit.verbose true &&

On Mon, May 26, 2014 at 03:34:20PM -0700, Jeremiah Mahler wrote:

Caleb,

On Mon, May 26, 2014 at 01:56:21PM -0500, Caleb Thompson wrote:

quoted

This patch allows people to set commit.verbose to implicitly send
--verbose to git-commit. It also introduces --no-verbose to
override the configuration setting.

This version incorporates changes suggested by Eric Sunshine, Duy
Nguyen, and Jeremiah Mahler.

...

quoted

Other than the minor style issue I pointed out in another email, it looks
good, and the patch set works properly on my machine.

-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler

Am 5/26/2014 20:56, schrieb Caleb Thompson:

Why? I see no benefit. Both $PWD and $(pwd) work fine everywhere,
including Windows, and the former is faster, particularly on Windows.

-- Hannes

On Tue, May 27, 2014 at 1:46 AM, Johannes Sixt [off-list ref] wrote:

Am 5/26/2014 20:56, schrieb Caleb Thompson:

quoted

Signed-off-by: Caleb Thompson <redacted>
---
 t/t7507-commit-verbose.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index 6d778ed..3b06d73 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh

@@ -8,7 +8,7 @@ cat >check-for-diff <<EOF
 exec grep '^diff --git' "\$1"
 EOF
 chmod +x check-for-diff
-test_set_editor "$PWD/check-for-diff"
+test_set_editor "$(pwd)/check-for-diff"

 cat >message <<'EOF'
 subject

Why? I see no benefit. Both $PWD and $(pwd) work fine everywhere,
including Windows, and the former is faster, particularly on Windows.

Poor advice on my part when reviewing the previous round. When I had
read in git/t/README (in the distant past):

    When a test checks for an absolute path that a git command
    generated, construct the expected value using $(pwd) rather than
    $PWD, $TEST_DIRECTORY, or $TRASH_DIRECTORY. It makes a difference
    on Windows, where the shell (MSYS bash) mangles absolute path
    names.  For details, see the commit message of 4114156ae9.

I must have missed the word "check" in the first sentence.

On Tue, May 27, 2014 at 07:46:59AM +0200, Johannes Sixt wrote:

Am 5/26/2014 20:56, schrieb Caleb Thompson:

quoted

Signed-off-by: Caleb Thompson <redacted>
---
 t/t7507-commit-verbose.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index 6d778ed..3b06d73 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh

@@ -8,7 +8,7 @@ cat >check-for-diff <<EOF
 exec grep '^diff --git' "\$1"
 EOF
 chmod +x check-for-diff
-test_set_editor "$PWD/check-for-diff"
+test_set_editor "$(pwd)/check-for-diff"
 
 cat >message <<'EOF'
 subject

Why? I see no benefit. Both $PWD and $(pwd) work fine everywhere,
including Windows, and the former is faster, particularly on Windows.

-- Hannes

I don't know the technical details of why this change is needed.
But someone felt it was important enough to put in t/README.

  - When a test checks for an absolute path that a git command generated,
    construct the expected value using $(pwd) rather than $PWD,
    $TEST_DIRECTORY, or $TRASH_DIRECTORY. It makes a difference on
    Windows, where the shell (MSYS bash) mangles absolute path names.
    For details, see the commit message of 4114156ae9.

-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler

Please do not cull the Cc list.

Am 5/27/2014 8:14, schrieb Jeremiah Mahler:

On Tue, May 27, 2014 at 07:46:59AM +0200, Johannes Sixt wrote:

quoted

Am 5/26/2014 20:56, schrieb Caleb Thompson:

quoted

Signed-off-by: Caleb Thompson <redacted>
---
 t/t7507-commit-verbose.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index 6d778ed..3b06d73 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh

@@ -8,7 +8,7 @@ cat >check-for-diff <<EOF
 exec grep '^diff --git' "\$1"
 EOF
 chmod +x check-for-diff
-test_set_editor "$PWD/check-for-diff"
+test_set_editor "$(pwd)/check-for-diff"
 
 cat >message <<'EOF'
 subject

Why? I see no benefit. Both $PWD and $(pwd) work fine everywhere,
including Windows, and the former is faster, particularly on Windows.

I don't know the technical details of why this change is needed.
But someone felt it was important enough to put in t/README.

  - When a test checks for an absolute path that a git command generated,
    construct the expected value using $(pwd) rather than $PWD,
    $TEST_DIRECTORY, or $TRASH_DIRECTORY. It makes a difference on
    Windows, where the shell (MSYS bash) mangles absolute path names.
    For details, see the commit message of 4114156ae9.

That someone was I. I appreciate that people study t/README and do not
ignore the sentence.

However, it does not apply to the situation because the path to the editor
is not "generated by a git command and checked for by a test".

That said, it is not wrong to use $(pwd) with test_set_editor, it's just
unnecessarily slow.

-- Hannes

On Mon, May 26, 2014 at 2:56 PM, Caleb Thompson [off-list ref] wrote:

Food for thought:

The original code used <<EOF since it needed $SHELL_PATH to be
evaluated at script creation time, and took special care to escape $1
in the 'grep' invocation since $1 should be evaluated only at script
execution time.

With the change to write_script(), nothing within the here-doc
requires evaluation, yet you are still using the evaluating <<-EOF
form (and manually escaping $1). The intent might be clearer if you
switch to <<-\EOF which suppresses evaluation (and drop the manual
escaping of $1).

The same observation applies to the new write_script() invocation to
create check-for-no-diff in patch 5.

 EOF
-chmod +x check-for-diff
 test_set_editor "$(pwd)/check-for-diff"

 cat >message <<'EOF'
--
1.9.3

On Mon, May 26, 2014 at 2:56 PM, Caleb Thompson [off-list ref] wrote:

t/t7507-commit-verbose.sh was using a global test_set_editor call to
build its environment.

Rather than building global state with test_set_editor at the beginning
of the file, move test_set_editor calls into each test.

Rather than repeating in prose what the patch itself says more
concisely and precisely, explain the reason for this change. For
instance, you might replace the above two sentences with something
like this (or better):

    Improve robustness against global state changes by having each
    test set up the test-editor it requires rather than relying upon
    the editor set once at script start.

Besides being inline with current practices, it also allows the tests

s/inline/in line/

which required GIT_EDITOR=cat to avoid using a subshell and simplify
their logic.

"required" sounds odd here. Perhaps:

    ...which set GIT_EDITOR=cat manually...

More below.

This test does not invoke the test-editor at all, so it's misleading
to insert test_set_editor here.

Also misleading, unnecessary test_set_editor.

 test_expect_success 'diff in message is retained with -v' '
+       test_set_editor "$(pwd)/check-for-diff" &&
        git commit --amend -F diff -v &&
        check_message diff
 '

Ditto.

 test_expect_success 'submodule log is stripped out too with -v' '
+       test_set_editor "$(pwd)/check-for-diff" &&

Unnecessary. The editor isn't invoked until the test_must_fail line,
and by then you've already overridden it with 'test_set_editor cat'.

Broken &&-chain.

        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

Broken &&-chain.

        test_i18ngrep "Aborting commit due to empty commit message." err
 '

--
1.9.3