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

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

[Caleb Thompson]

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

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.

Since the last version of this patch
(http://marc.info/?l=git&m=140251155830422&w=2), I've made the following
changes:

* Revert change to flags, as --no-verbose already existed and worked as
  expected with the commit.verbose configuration. Thanks to  René Scharfe.
* Fix <<-'EOS' style for check-for-no-diff script. Thanks to Mike Burns.

Additionally, this set of patches was generated by format-patch, so it
should work correctly with git-am.

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

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

[PATCH v5 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 protects us from potential shell meta-characters in the
name of our trash directory, which would be interpreted if we set
$EDITOR directly.

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

Improve robustness against global state changes by having only tests
which intend to use the $EDITOR to check for presence of a diff in the
editor set up the test-editor to use check-for-diff rather than relying
upon the editor set once at script start.

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

Signed-off-by: Caleb Thompson <caleb@calebthompson.io>
---
 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 v5 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 cd2d651..ec51e1c 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 99c2044..c782388 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1489,6 +1489,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 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 35a4d06..402d6a1 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

@@ -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 v5 0/4] commit: Add commit.verbose configuration

[Jeremiah Mahler]

On Thu, Jun 12, 2014 at 02:38:58PM -0500, Caleb Thompson wrote:
> This patch allows people to set commit.verbose to implicitly send
> --verbose to git-commit.
> 
> 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.
> 
> Since the last version of this patch
> (http://marc.info/?l=git&m=140251155830422&w=2), I've made the following
> changes:
> 
> * Revert change to flags, as --no-verbose already existed and worked as
>   expected with the commit.verbose configuration. Thanks to  René Scharfe.
> * Fix <<-'EOS' style for check-for-no-diff script. Thanks to Mike Burns.
> 
> Additionally, this set of patches was generated by format-patch, so it
> should work correctly with git-am.
> 
> ------------------------------------------------------
> 
> 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
> 

The patches look good, they apply clean ('git am'), and all tests pass.

Reviewed-by: Jeremiah Mahler <jmmahler@gmail.com>
-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler

Re: [PATCH v5 2/4] commit test: Use write_script

[Jeff King]

On Thu, Jun 12, 2014 at 02:39:00PM -0500, Caleb Thompson wrote:

> Use write_script from t/test-lib-functions.sh instead of cat, shebang,
> and chmod. This protects us from potential shell meta-characters in the
> name of our trash directory, which would be interpreted if we set
> $EDITOR directly.

I'm not sure about this last sentence; isn't that what test_set_editor
is doing, which was already there? I think the real rationale is
readability: since $SHELL_PATH is handled for us, you can turn off
interpolation in the here-doc containing the helper script. That avoids
an extra layer of quoting.

-Peff

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

[Jeff King]

On Thu, Jun 12, 2014 at 02:39:01PM -0500, Caleb Thompson wrote:

> t/t7507-commit-verbose.sh was using a global test_set_editor call to
> build its environment.
> 
> Improve robustness against global state changes by having only tests
> which intend to use the $EDITOR to check for presence of a diff in the
> editor set up the test-editor to use check-for-diff rather than relying
> upon the editor set once at script start.

This implies to me that EDITOR is unset after leaving these tests. I
don't think that is how it works, though.  The tests themselves run in
the main environment of the test script. A call to test_set_editor from
one of them will still affect the other tests[1].

I think it works anyway because every subsequent test that cares
actually sets the editor itself.

Or did you just mean that the new rule is "every test sets the editor as
they need", which means that we do not have to worry anymore about
polluting the environment for other tests?

-Peff

[1] It might make sense for test_set_editor, when run from within a
    test, to behave more like test_config, and do:

      test_when_finished '
        sane_unset FAKE_EDITOR &&
        sane_unset EDITOR
      '

    I don't know if there would be fallouts with other test scripts,
    though.

Re: [PATCH v5 2/4] commit test: Use write_script

[Caleb Thompson]

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

You're very right - I may have confused this commit message and the one
to switch to test_set_editor. I'll rewrite this commit message.

What do you think of something like this for the description:

    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.

On Fri, Jun 13, 2014 at 02:50:37AM -0400, Jeff King wrote:
> On Thu, Jun 12, 2014 at 02:39:00PM -0500, Caleb Thompson wrote:
>
> > Use write_script from t/test-lib-functions.sh instead of cat, shebang,
> > and chmod. This protects us from potential shell meta-characters in the
> > name of our trash directory, which would be interpreted if we set
> > $EDITOR directly.
>
> I'm not sure about this last sentence; isn't that what test_set_editor
> is doing, which was already there? I think the real rationale is
> readability: since $SHELL_PATH is handled for us, you can turn off
> interpolation in the here-doc containing the helper script. That avoids
> an extra layer of quoting.
>
> -Peff

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

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

[Caleb Thompson]

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

On Fri, Jun 13, 2014 at 02:59:42AM -0400, Jeff King wrote:
> On Thu, Jun 12, 2014 at 02:39:01PM -0500, Caleb Thompson wrote:
>
> > t/t7507-commit-verbose.sh was using a global test_set_editor call to
> > build its environment.
> >
> > Improve robustness against global state changes by having only tests
> > which intend to use the $EDITOR to check for presence of a diff in the
> > editor set up the test-editor to use check-for-diff rather than relying
> > upon the editor set once at script start.
>
> This implies to me that EDITOR is unset after leaving these tests. I
> don't think that is how it works, though.  The tests themselves run in
> the main environment of the test script. A call to test_set_editor from
> one of them will still affect the other tests[1].
>
> I think it works anyway because every subsequent test that cares
> actually sets the editor itself.
>
> Or did you just mean that the new rule is "every test sets the editor as
> they need", which means that we do not have to worry anymore about
> polluting the environment for other tests?

That's exactly what I meant. We can stop relying on the global state *as
it is initially set* and instead move the setup into the tests which
rely on it.

>
> -Peff
>
> [1] It might make sense for test_set_editor, when run from within a
>     test, to behave more like test_config, and do:
>
>       test_when_finished '
>         sane_unset FAKE_EDITOR &&
>         sane_unset EDITOR
>       '

It might, but it's a little out of scope in addition to your concern
about other test scripts.

>
>     I don't know if there would be fallouts with other test scripts,
>     though.

How is this for a reword of that commit description:

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

Caleb Thompson

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

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

[Caleb Thompson]

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

On Thu, Jun 12, 2014 at 01:30:10PM -0700, Jeremiah Mahler wrote:
> On Thu, Jun 12, 2014 at 02:38:58PM -0500, Caleb Thompson wrote:
> > This patch allows people to set commit.verbose to implicitly send
> > --verbose to git-commit.
> >
> > 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.
> >
> > Since the last version of this patch
> > (http://marc.info/?l=git&m=140251155830422&w=2), I've made the following
> > changes:
> >
> > * Revert change to flags, as --no-verbose already existed and worked as
> >   expected with the commit.verbose configuration. Thanks to  René Scharfe.
> > * Fix <<-'EOS' style for check-for-no-diff script. Thanks to Mike Burns.
> >
> > Additionally, this set of patches was generated by format-patch, so it
> > should work correctly with git-am.
> >
> > ------------------------------------------------------
> >
> > 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
> >
>
> The patches look good, they apply clean ('git am'), and all tests pass.
>
> Reviewed-by: Jeremiah Mahler <jmmahler@gmail.com>

So that I'm clear on the etiquitte, is it appropriate for me to add this
Reviewed-by line to the commit messages at this point, provided that the
patches don't change?

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

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

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

[Jakub Narębski]

W dniu 2014-06-13 18:36, Caleb Thompson pisze:
> On Fri, Jun 13, 2014 at 02:59:42AM -0400, Jeff King wrote:

>> [1] It might make sense for test_set_editor, when run from within a
>>      test, to behave more like test_config, and do:
>>
>>        test_when_finished '
>>          sane_unset FAKE_EDITOR &&
>>          sane_unset EDITOR
>>        '
>
> It might, but it's a little out of scope in addition to your concern
> about other test scripts.
>
>>
>>      I don't know if there would be fallouts with other test scripts,
>>      though.
>
> How is this for a reword of that commit description:
>
>      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, 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.

This also allows to run only specified subset of tests
with TEST_SKIP without requiring to remember which tests
are setup tests and have to be not skipped, isn't it?

-- 
Jakub Narębski

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> [1] It might make sense for test_set_editor, when run from within a
>     test, to behave more like test_config, and do:
>
>       test_when_finished '
>         sane_unset FAKE_EDITOR &&
>         sane_unset EDITOR
>       '
>
>     I don't know if there would be fallouts with other test scripts,
>     though.

The default environment for tests is to set EDITOR=: to avoid
accidentally triggering interactive cruft and interfering with
automated tests, I thought.

If the above sane-unset is changed to EDITOR=: then I think that is
probably sensible.

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

[Caleb Thompson]

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

On Fri, Jun 13, 2014 at 07:16:53PM +0200, Jakub Narębski wrote:
> W dniu 2014-06-13 18:36, Caleb Thompson pisze:
> >On Fri, Jun 13, 2014 at 02:59:42AM -0400, Jeff King wrote:
>
> >>[1] It might make sense for test_set_editor, when run from within a
> >>     test, to behave more like test_config, and do:
> >>
> >>       test_when_finished '
> >>         sane_unset FAKE_EDITOR &&
> >>         sane_unset EDITOR
> >>       '
> >
> >It might, but it's a little out of scope in addition to your concern
> >about other test scripts.
> >
> >>
> >>     I don't know if there would be fallouts with other test scripts,
> >>     though.
> >
> >How is this for a reword of that commit description:
> >
> >     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, 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.
>
> This also allows to run only specified subset of tests
> with TEST_SKIP without requiring to remember which tests
> are setup tests and have to be not skipped, isn't it?

I don't see any references to TEST_SKIP in the code. Do you mean
test_skip() from t/test_lib.sh? If so, it isn't clear to me what the use
case would be for that, so I'd have to take your word.

Caleb Thompson

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

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

[Junio C Hamano]

Caleb Thompson <caleb@calebthompson.io> writes:

> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> index 35a4d06..402d6a1 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

This lets grep show all lines that are not "diff --git" in the
input, and as usual grep exits success if it has any line in the
output.

    $ grep -v '^diff --git' <<\EOF ; echo $?
    diff --git
    a
    EOF
    a
    0
    $ exit

What are we testing, exactly?

> @@ -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 v5 3/4] commit test: test_set_editor in each test

[Jakub Narębski]

W dniu 2014-06-13 19:47, Caleb Thompson pisze:
> On Fri, Jun 13, 2014 at 07:16:53PM +0200, Jakub Narębski wrote:
>> W dniu 2014-06-13 18:36, Caleb Thompson pisze:

>>>      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, 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.
>>
>> This also allows to run only specified subset of tests
>> with TEST_SKIP without requiring to remember which tests
>> are setup tests and have to be not skipped, isn't it?
>
> I don't see any references to TEST_SKIP in the code. Do you mean
> test_skip() from t/test_lib.sh? If so, it isn't clear to me what the use
> case would be for that, so I'd have to take your word.

I meant here GIT_SKIP_TESTS, but I see that test_set_editor was not in 
first test i.e. test_expect_success 'setup', so it wouldn't matter.
Before and after both work correctly with GIT_SKIP_TESTS, before because
of global setup.

I'm sorry for the noise, then.

-- 
Jakub Narębski

Re: [PATCH v5 2/4] commit test: Use write_script

[Jeff King]

On Fri, Jun 13, 2014 at 11:26:07AM -0500, Caleb Thompson wrote:

> You're very right - I may have confused this commit message and the one
> to switch to test_set_editor. I'll rewrite this commit message.
> 
> What do you think of something like this for the description:
> 
>     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.

That looks fine to me. Thanks.

-Peff

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

[Jeff King]

On Fri, Jun 13, 2014 at 11:36:44AM -0500, Caleb Thompson wrote:

> > Or did you just mean that the new rule is "every test sets the editor as
> > they need", which means that we do not have to worry anymore about
> > polluting the environment for other tests?
> 
> That's exactly what I meant. We can stop relying on the global state *as
> it is initially set* and instead move the setup into the tests which
> rely on it.

Ah, OK, it was just me mis-reading, then.

The rewording you included below is clearer to me. Thanks.

> > [1] It might make sense for test_set_editor, when run from within a
> >     test, to behave more like test_config, and do:
> >
> >       test_when_finished '
> >         sane_unset FAKE_EDITOR &&
> >         sane_unset EDITOR
> >       '
> 
> It might, but it's a little out of scope in addition to your concern
> about other test scripts.

Yeah, I agree it does not need to block this series.

-Peff

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

[Jeff King]

On Fri, Jun 13, 2014 at 10:42:26AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > [1] It might make sense for test_set_editor, when run from within a
> >     test, to behave more like test_config, and do:
> >
> >       test_when_finished '
> >         sane_unset FAKE_EDITOR &&
> >         sane_unset EDITOR
> >       '
> >
> >     I don't know if there would be fallouts with other test scripts,
> >     though.
> 
> The default environment for tests is to set EDITOR=: to avoid
> accidentally triggering interactive cruft and interfering with
> automated tests, I thought.

Ah, yeah, that would make more sense.

> If the above sane-unset is changed to EDITOR=: then I think that is
> probably sensible.

I think the trick is that other scripts may be relying on the global
side-effect, and would need to be fixed up (and it is not always obvious
which spots will need it; they might fail the tests, or they might start
silently passing for the wrong reason).

-Peff

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

[Jeremiah Mahler]

On Fri, Jun 13, 2014 at 11:49:10AM -0500, Caleb Thompson wrote:
> On Thu, Jun 12, 2014 at 01:30:10PM -0700, Jeremiah Mahler wrote:
> > On Thu, Jun 12, 2014 at 02:38:58PM -0500, Caleb Thompson wrote:
> > > This patch allows people to set commit.verbose to implicitly send
> > > --verbose to git-commit.
> > >
> > > 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.
> > >
> > > Since the last version of this patch
> > > (http://marc.info/?l=git&m=140251155830422&w=2), I've made the following
> > > changes:
> > >
> > > * Revert change to flags, as --no-verbose already existed and worked as
> > >   expected with the commit.verbose configuration. Thanks to  René Scharfe.
> > > * Fix <<-'EOS' style for check-for-no-diff script. Thanks to Mike Burns.
> > >
> > > Additionally, this set of patches was generated by format-patch, so it
> > > should work correctly with git-am.
> > >
> > > ------------------------------------------------------
> > >
> > > 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
> > >
> >
> > The patches look good, they apply clean ('git am'), and all tests pass.
> >
> > Reviewed-by: Jeremiah Mahler <jmmahler@gmail.com>
> 
> So that I'm clear on the etiquitte, is it appropriate for me to add this
> Reviewed-by line to the commit messages at this point, provided that the
> patches don't change?
> 

I am not sure of the etiquette either.  But personally, since I hardly
contributed anything, I don't think it is necessary to put my tag in
your patches.

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

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

[Caleb Thompson]

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

On Fri, Jun 13, 2014 at 07:41:29PM -0400, Jeff King wrote:
> On Fri, Jun 13, 2014 at 10:42:26AM -0700, Junio C Hamano wrote:
>
> > Jeff King <peff@peff.net> writes:
> >
> > > [1] It might make sense for test_set_editor, when run from within a
> > >     test, to behave more like test_config, and do:
> > >
> > >       test_when_finished '
> > >         sane_unset FAKE_EDITOR &&
> > >         sane_unset EDITOR
> > >       '
> > >
> > >     I don't know if there would be fallouts with other test scripts,
> > >     though.
> >
> > The default environment for tests is to set EDITOR=: to avoid
> > accidentally triggering interactive cruft and interfering with
> > automated tests, I thought.
>
> Ah, yeah, that would make more sense.
>
> > If the above sane-unset is changed to EDITOR=: then I think that is
> > probably sensible.
>
> I think the trick is that other scripts may be relying on the global
> side-effect, and would need to be fixed up (and it is not always obvious
> which spots will need it; they might fail the tests, or they might start
> silently passing for the wrong reason).

For this reason, and that the scope of this change has already ballooned, I'd
rather not make this change in this patch if that's alright.

Caleb Thompson

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

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

[Junio C Hamano]

Caleb Thompson <caleb@calebthompson.io> writes:

> On Fri, Jun 13, 2014 at 07:41:29PM -0400, Jeff King wrote:
>> On Fri, Jun 13, 2014 at 10:42:26AM -0700, Junio C Hamano wrote:
>>
>> > Jeff King <peff@peff.net> writes:
>> >
>> > > [1] It might make sense for test_set_editor, when run from within a
>> > >     test, to behave more like test_config, and do:
>> > >
>> > >       test_when_finished '
>> > >         sane_unset FAKE_EDITOR &&
>> > >         sane_unset EDITOR
>> > >       '
>> > >
>> > >     I don't know if there would be fallouts with other test scripts,
>> > >     though.
>> >
>> > The default environment for tests is to set EDITOR=: to avoid
>> > accidentally triggering interactive cruft and interfering with
>> > automated tests, I thought.
>>
>> Ah, yeah, that would make more sense.
>>
>> > If the above sane-unset is changed to EDITOR=: then I think that is
>> > probably sensible.
>>
>> I think the trick is that other scripts may be relying on the global
>> side-effect, and would need to be fixed up (and it is not always obvious
>> which spots will need it; they might fail the tests, or they might start
>> silently passing for the wrong reason).
>
> For this reason, and that the scope of this change has already ballooned, I'd
> rather not make this change in this patch if that's alright.
>
> Caleb Thompson

My comment was not about your series, but "if we were to update
test_set_editor, unsetting EDITOR is not the right thing to do".

I do not think it is reasonable to include such a change to
test_set_editor in this series.

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

[Caleb Thompson]

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

On Fri, Jun 13, 2014 at 10:48:55AM -0700, Junio C Hamano wrote:
> Caleb Thompson <caleb@calebthompson.io> writes:
>
> > diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> > index 35a4d06..402d6a1 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
>
> This lets grep show all lines that are not "diff --git" in the
> input, and as usual grep exits success if it has any line in the
> output.
>
>     $ grep -v '^diff --git' <<\EOF ; echo $?
>     diff --git
>     a
>     EOF
>     a
>     0
>     $ exit
>
> What are we testing, exactly?

Good catch. It worked when I switched check-for-diff from
check-for-no-diff, but I didn't try to make check-for-no-diff fail
independently, so I apologize.

This version removes the the beginning of a line starting with
"diff --git" from the string, then checks that the result and the
original string are not the same. Switching the != logic to = makes the
tests using check-for-no-diff fail.

	write_script check-for-no-diff <<-'EOF'
		exec test "${1#*^diff --git} != $1
	EOF

Another option is to replace the parameter substitution with a call to
grep:

	write_script check-for-no-diff <<-'EOF'
		exec test "`grep -v '^diff --git' \"$1\"` != "$1
	EOF

I think that the former reads nicer, and requires less escaping, but I'm
open to feedback.


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

Caleb Thompson

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

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

[Caleb Thompson]

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

On Mon, Jun 16, 2014 at 02:50:57PM -0500, Caleb Thompson wrote:
> On Fri, Jun 13, 2014 at 10:48:55AM -0700, Junio C Hamano wrote:
> > Caleb Thompson <caleb@calebthompson.io> writes:
> >
> > > diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> > > index 35a4d06..402d6a1 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
> >
> > This lets grep show all lines that are not "diff --git" in the
> > input, and as usual grep exits success if it has any line in the
> > output.
> >
> >     $ grep -v '^diff --git' <<\EOF ; echo $?
> >     diff --git
> >     a
> >     EOF
> >     a
> >     0
> >     $ exit
> >
> > What are we testing, exactly?
>
> Good catch. It worked when I switched check-for-diff from
> check-for-no-diff, but I didn't try to make check-for-no-diff fail
> independently, so I apologize.
>
> This version removes the the beginning of a line starting with
> "diff --git" from the string, then checks that the result and the
> original string are not the same. Switching the != logic to = makes the
> tests using check-for-no-diff fail.
>
>	write_script check-for-no-diff <<-'EOF'
>		exec test "${1#*^diff --git} != $1
>	EOF
>
> Another option is to replace the parameter substitution with a call to
> grep:
>
>	write_script check-for-no-diff <<-'EOF'
>		exec test "`grep -v '^diff --git' \"$1\"` != "$1
>	EOF
>
> I think that the former reads nicer, and requires less escaping, but I'm
> open to feedback.

Correction: the first variant does not work, only the second. Sorry for the
confustion.

	write_script check-for-no-diff <<-'EOF'
		exec test "`grep -v '^diff --git' \"$1\"`" != "$1"
	EOF

> > > @@ -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
>
> Caleb Thompson



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

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

[Junio C Hamano]

Caleb Thompson <caleb@calebthompson.io> writes:

> On Fri, Jun 13, 2014 at 10:48:55AM -0700, Junio C Hamano wrote:
>> Caleb Thompson <caleb@calebthompson.io> writes:
>>
>> > diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
>> > index 35a4d06..402d6a1 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
>>
>> This lets grep show all lines that are not "diff --git" in the
>> input, and as usual grep exits success if it has any line in the
>> output.
>>
>>     $ grep -v '^diff --git' <<\EOF ; echo $?
>>     diff --git
>>     a
>>     EOF
>>     a
>>     0
>>     $ exit
>>
>> What are we testing, exactly?
>
> Good catch. It worked when I switched check-for-diff from
> check-for-no-diff, but I didn't try to make check-for-no-diff fail
> independently, so I apologize.

No need to apologize at all.  None of us (including this reviewer)
is perfect and that is why we review patches by each other.

> This version removes the the beginning of a line starting with
> "diff --git" from the string,...

Again, what are we testing, exactly?

We do not want to see "^diff --git" in the output file, in other
words, we want to make sure "^diff --git" does not appear in the
output.

So

        write_script check-for-no-diff <<-\EOF
        ! grep '^diff --git' "$@"
	EOF

should be the most natural way to express what we are testing, no?

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

[Caleb Thompson]

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

On Mon, Jun 16, 2014 at 01:06:45PM -0700, Junio C Hamano wrote:
> Caleb Thompson <caleb@calebthompson.io> writes:
>
> > On Fri, Jun 13, 2014 at 10:48:55AM -0700, Junio C Hamano wrote:
> >> Caleb Thompson <caleb@calebthompson.io> writes:
> >>
> >> > diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> >> > index 35a4d06..402d6a1 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
> >>
> >> This lets grep show all lines that are not "diff --git" in the
> >> input, and as usual grep exits success if it has any line in the
> >> output.
> >>
> >>     $ grep -v '^diff --git' <<\EOF ; echo $?
> >>     diff --git
> >>     a
> >>     EOF
> >>     a
> >>     0
> >>     $ exit
> >>
> >> What are we testing, exactly?
> >
> > Good catch. It worked when I switched check-for-diff from
> > check-for-no-diff, but I didn't try to make check-for-no-diff fail
> > independently, so I apologize.
>
> No need to apologize at all.  None of us (including this reviewer)
> is perfect and that is why we review patches by each other.
>
> > This version removes the the beginning of a line starting with
> > "diff --git" from the string,...
>
> Again, what are we testing, exactly?
>
> We do not want to see "^diff --git" in the output file, in other
> words, we want to make sure "^diff --git" does not appear in the
> output.
>
> So
>
>         write_script check-for-no-diff <<-\EOF
>         ! grep '^diff --git' "$@"
>	EOF
>
> should be the most natural way to express what we are testing, no?

I did consider that. The reason I didn't propose that is that it doesn't catch
the unlikely case that the $1 only contains a "diff --git" line or that $1 is
empty.

Those are rather unreasonable concerns, so I'm happy to use the much more
readable version as you propose.

Caleb Thompson

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

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

[Junio C Hamano]

Jeremiah Mahler <jmmahler@gmail.com> writes:

> On Fri, Jun 13, 2014 at 11:49:10AM -0500, Caleb Thompson wrote:
> ...
>> > The patches look good, they apply clean ('git am'), and all tests pass.
>> >
>> > Reviewed-by: Jeremiah Mahler <jmmahler@gmail.com>
>> 
>> So that I'm clear on the etiquitte, is it appropriate for me to add this
>> Reviewed-by line to the commit messages at this point, provided that the
>> patches don't change?
>
> I am not sure of the etiquette either.  But personally, since I hardly
> contributed anything, I don't think it is necessary to put my tag in
> your patches.

It is correct that it is not necessary for Caleb to resend these
patches if it is done only to add "Reviewed-by:" from you (but it
does not hurt to do so, either).

In the review process, "Reviewed-by:" sent for a patch by one of the
trusted reviewers will reduce workload from other reviewers because
there will be one less reason to read such a patch [*1*].  A
corollary to this is that other reviewers will ignore an extra
"Reviewed-by:" by somebody whose review quality is still unknown, so
it will not hurt.

Your reading others patches and commenting on is appreciated very
much, and sending a "Reviewed-by:" is perfectly fine.  As you gain
experience and as others see your review comments more and more,
people will start trusting your reviews.  Everybody begins at a
"novice" state, after all ;-)

By the way, speaking of netiquette, please refrain from using
"Mail-Followup-To:" while working on this list [*2*].

Thanks.

[Footnotes]

*1* We usually read a patch for one of the two reasons: either the
reviewer personally finds what the patch wants to do interesting and
worthwhile, and wants to make sure it is done in the right way.  Or
the reviewer thinks that applying the patch is detrimental to the
overall project, perhaps the design and/or the implementation is
wrong, and point the problems out.  A "Reviewed-by" from a trusted
reviewer will allow other reviewers to simply skip/ignore the patch
if what it does is "Meh" to them, saying "I am not particularly
interested, but as long as it does not hurt, I would not be opposed,
and the other guy reviewed and says it would not hurt, and I tend to
trust his judgment."  The maintainer does not have the luxury of
skip/ignore such a patch because he needs to at least apply and test
the integration result, though ;-).

Now, who are the "trusted reviewers"?  Anybody who is knows s/he is
one of them ;-)

*2* http://thread.gmane.org/gmane.comp.version-control.git/165477/focus=165549

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

[Junio C Hamano]

Caleb Thompson <caleb@calebthompson.io> writes:

>> Again, what are we testing, exactly?
>>
>> We do not want to see "^diff --git" in the output file, in other
>> words, we want to make sure "^diff --git" does not appear in the
>> output.
>>
>> So
>>
>>         write_script check-for-no-diff <<-\EOF
>>         ! grep '^diff --git' "$@"
>>	EOF
>>
>> should be the most natural way to express what we are testing, no?
>
> I did consider that. The reason I didn't propose that is that it doesn't catch
> the unlikely case that the $1 only contains a "diff --git" line or that $1 is
> empty.
>
> Those are rather unreasonable concerns, so I'm happy to use the much more
> readable version as you propose.

If it only has "diff --git", then the grep will find a hit, exits
with success, the script yields the opposite and "git commit" will
fail, which is what we want, so that is OK.  "$1 is empty" may or
may not be an error, depending on your settings, I guess (i.e. can't
we squelch the "# helpful instruction" lines altogether?)?

If the editor input is expected to be very stable, we could even do
something like:

	write_script check-editor-input <<-\EOF
	diff expect "$1" >&2
        EOF

and then catch any deviation from the norm with something like:

	cat >expect <<-\EOF &&
        ... expected editor input comes here ...
        EOF
        test_set_editor "$PWD/check-editor-input &&
	git commit --amend

but if the editor input may easily be affected by volatile things
like blob object names given in the diff output by "commit -v" or
untracked cruft in the working tree listed in the status output,
then it would add unnecessary maintenance burden (every time earlier
parts of the test scripts are updated, the expected output may have
to change to adjust to these non-essential details), so it is a
judgment call.

Thanks.