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

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

[Caleb Thompson]

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

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 <caleb@calebthompson.io>
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 <caleb@calebthompson.io>
    Reviewed-by: Duy Nguyen <pclouds@gmail.com>
    Reviewed-by: Jeremiah Mahler <jmmahler@gmail.com>

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.

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

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

[Jeremiah Mahler]

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.

> 
> @@ -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
> +'
> +
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

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

[Jeremiah Mahler]

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

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

[Duy Nguyen]

On Sun, May 25, 2014 at 1:24 PM, Caleb Thompson <cjaysson@gmail.com> 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

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

[Eric Sunshine]

On Sun, May 25, 2014 at 2:24 AM, Caleb Thompson <cjaysson@gmail.com> 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 <caleb@calebthompson.io>
> 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 <caleb@calebthompson.io>
>     Reviewed-by: Duy Nguyen <pclouds@gmail.com>
>     Reviewed-by: Jeremiah Mahler <jmmahler@gmail.com>

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

> 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

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 /

>  -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"

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.

>  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 &&

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.

[PATCH v3 0/5] commit: support commit.verbose and --no-verbose

[Caleb Thompson]

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(-)

[PATCH v3 1/5] 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>
---
 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

[PATCH v3 2/5] commit test: Change $PWD to $(pwd)

[Caleb Thompson]

Signed-off-by: Caleb Thompson <caleb@calebthompson.io>
---
 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

[PATCH v3 3/5] commit test: Use write_script

[Caleb Thompson]

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

Signed-off-by: Caleb Thompson <caleb@calebthompson.io>
---
 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

[PATCH v3 4/5] 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.

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 <caleb@calebthompson.io>
---
 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

[PATCH v3 5/5] commit: support commit.verbose and --no-verbose

[Caleb Thompson]

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 <caleb@calebthompson.io>
---
 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

Re: [PATCH v3 5/5] commit: support commit.verbose and --no-verbose

[Jeremiah Mahler]

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

Re: [PATCH v3 5/5] commit: support commit.verbose and --no-verbose

[Caleb Thompson]

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

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:
> > 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

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

Re: [PATCH v3 5/5] commit: support commit.verbose and --no-verbose

[Jeremiah Mahler]

On Mon, May 26, 2014 at 03:39:55PM -0500, Caleb Thompson wrote:
> The editors, `check-for-diff` and `check-for-no-diffs`, are grepping for
> the output and lack thereof, respectively.
...
> >
> > It appears that these tests still aren't checking to see if the
> > "verbose" output appears in the commit message.
> >
> >

OK, got it.  The editor, set by test_set_editor, is run as part of
the commit.  Thanks for explaining that.

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

Re: [PATCH v3 5/5] commit: support commit.verbose and --no-verbose

[Jeremiah Mahler]

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 <caleb@calebthompson.io>
> ---
>  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

Re: [PATCH v3 0/5] commit: support commit.verbose and --no-verbose

[Jeremiah Mahler]

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

Re: [PATCH v3 0/5] commit: support commit.verbose and --no-verbose

[Caleb Thompson]

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

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:
> > 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

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

Re: [PATCH v3 2/5] commit test: Change $PWD to $(pwd)

[Johannes Sixt]

Am 5/26/2014 20:56, schrieb Caleb Thompson:
> Signed-off-by: Caleb Thompson <caleb@calebthompson.io>
> ---
>  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

Re: [PATCH v3 2/5] commit test: Change $PWD to $(pwd)

[Eric Sunshine]

On Tue, May 27, 2014 at 1:46 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 5/26/2014 20:56, schrieb Caleb Thompson:
>> Signed-off-by: Caleb Thompson <caleb@calebthompson.io>
>> ---
>>  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.

Re: [PATCH v3 2/5] commit test: Change $PWD to $(pwd)

[Jeremiah Mahler]

On Tue, May 27, 2014 at 07:46:59AM +0200, Johannes Sixt wrote:
> Am 5/26/2014 20:56, schrieb Caleb Thompson:
> > Signed-off-by: Caleb Thompson <caleb@calebthompson.io>
> > ---
> >  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

Re: [PATCH v3 2/5] commit test: Change $PWD to $(pwd)

[Johannes Sixt]

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:
>> Am 5/26/2014 20:56, schrieb Caleb Thompson:
>>> Signed-off-by: Caleb Thompson <caleb@calebthompson.io>
>>> ---
>>>  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

Re: [PATCH v3 2/5] commit test: Change $PWD to $(pwd)

[David Kastrup]

Johannes Sixt <j.sixt@viscovery.net> writes:

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

Any shell that knows $(...) is pretty sure to have pwd as a built-in.
I don't think Git will run on those kind of ancient shells reverting to
/bin/pwd here.

The autoconf manual (info "(autoconf) Limitations of Builtins") states

'pwd'
     With modern shells, plain 'pwd' outputs a "logical" directory name,
     some of whose components may be symbolic links.  These directory
     names are in contrast to "physical" directory names, whose
     components are all directories.

     Posix 1003.1-2001 requires that 'pwd' must support the '-L'
     ("logical") and '-P' ("physical") options, with '-L' being the
     default.  However, traditional shells do not support these options,
     and their 'pwd' command has the '-P' behavior.

     Portable scripts should assume neither option is supported, and
     should assume neither behavior is the default.  Also, on many hosts
     '/bin/pwd' is equivalent to 'pwd -P', but Posix does not require
     this behavior and portable scripts should not rely on it.

     Typically it's best to use plain 'pwd'.  On modern hosts this
     outputs logical directory names, which have the following
     advantages:

        * Logical names are what the user specified.
        * Physical names may not be portable from one installation host
          to another due to network file system gymnastics.
        * On modern hosts 'pwd -P' may fail due to lack of permissions
          to some parent directory, but plain 'pwd' cannot fail for this
          reason.

     Also please see the discussion of the 'cd' command.

So $PWD is pretty much guaranteed to be the same as $(pwd) and pretty
much guaranteed to _not_ be "unnecessarily slow" when not run in an
inner loop.

However, looking at (info "(autoconf) Special Shell Variables") I see

'PWD'
     Posix 1003.1-2001 requires that 'cd' and 'pwd' must update the
     'PWD' environment variable to point to the logical name of the
     current directory, but traditional shells do not support this.
     This can cause confusion if one shell instance maintains 'PWD' but
     a subsidiary and different shell does not know about 'PWD' and
     executes 'cd'; in this case 'PWD' points to the wrong directory.
     Use '`pwd`' rather than '$PWD'.

Ok, probably Git relies on Posix 1003.1-2001 in other respects so it's
likely not much of an actual issue.

-- 
David Kastrup

Re: [PATCH v3 3/5] commit test: Use write_script

[Eric Sunshine]

On Mon, May 26, 2014 at 2:56 PM, Caleb Thompson <cjaysson@gmail.com> wrote:
> Use write_script from t/test-lib-functions instead of cat, shebang, and
> chmod.
>
> Signed-off-by: Caleb Thompson <caleb@calebthompson.io>
> ---
>  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"

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

Re: [PATCH v3 3/5] commit test: Use write_script

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> -cat >check-for-diff <<EOF
>> -#!$SHELL_PATH
>> -exec grep '^diff --git' "\$1"
>> +write_script check-for-diff <<-EOF
>> +       exec grep '^diff --git' "\$1"
>
> 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.

Very good comments.  Thanks.

Re: [PATCH v3 4/5] commit test: test_set_editor in each test

[Eric Sunshine]

On Mon, May 26, 2014 at 2:56 PM, Caleb Thompson <cjaysson@gmail.com> 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.

> Signed-off-by: Caleb Thompson <caleb@calebthompson.io>
> ---
>  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

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

> @@ -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
>  '

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'.

>         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

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
>