threads / patch / 36902

v5, 4 partscommit: Add commit.verbose configuration

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

## tl;dr

28 messages between Jun 12, 2014 and Jun 16, 2014. Diffs are folded; open one to read it.

replies: 27people: 5as markdown or json

Caleb Thompson· Jun 12, 2014, 19:38 UTC · lore

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

Caleb Thompson· Jun 12, 2014, 19:38 UTC · re: Caleb Thompson · lore

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

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(-)
Show changes to t/t7507-commit-verbose.sh +2 −2
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
Caleb Thompson· Jun 12, 2014, 19:39 UTC · re: Caleb Thompson · lore

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

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(-)
Show changes to t/t7507-commit-verbose.sh +2 −4
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
Jeff King· Jun 13, 2014, 06:50 UTC · re: Caleb Thompson · lore

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

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
Caleb Thompson· Jun 13, 2014, 16:26 UTC · re: Jeff King · lore

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

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:
Show 14 quoted lines
> 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
Jeff King· Jun 13, 2014, 23:28 UTC · re: Caleb Thompson · lore

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

On Fri, Jun 13, 2014 at 11:26:07AM -0500, Caleb Thompson wrote:
Show 10 quoted lines
> 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
Caleb Thompson· Jun 12, 2014, 19:39 UTC · re: Caleb Thompson · lore

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

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(-)
Show changes to t/t7507-commit-verbose.sh +7 −11
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
Jeff King· Jun 13, 2014, 06:59 UTC · re: Caleb Thompson · lore

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

On Thu, Jun 12, 2014 at 02:39:01PM -0500, Caleb Thompson wrote:
Show 7 quoted lines
> 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.
Caleb Thompson· Jun 13, 2014, 16:36 UTC · re: Jeff King · lore

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

On Fri, Jun 13, 2014 at 02:59:42AM -0400, Jeff King wrote:
Show 21 quoted lines
> 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.

Show 10 quoted lines
>
> -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
Jakub Narębski· Jun 13, 2014, 17:16 UTC · re: Caleb Thompson · lore

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

W dniu 2014-06-13 18:36, Caleb Thompson pisze:
> On Fri, Jun 13, 2014 at 02:59:42AM -0400, Jeff King wrote:
Show 30 quoted lines
>> [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
Caleb Thompson· Jun 13, 2014, 17:47 UTC · re: Jakub Narębski · lore

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

On Fri, Jun 13, 2014 at 07:16:53PM +0200, Jakub Narębski wrote:
Show 37 quoted lines
> 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
Jakub Narębski· Jun 13, 2014, 18:52 UTC · re: Caleb Thompson · lore

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

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:
Show 21 quoted lines
>>>      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
Jeff King· Jun 13, 2014, 23:39 UTC · re: Caleb Thompson · lore

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

On Fri, Jun 13, 2014 at 11:36:44AM -0500, Caleb Thompson wrote:
Show 7 quoted lines
> > 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.
Show 10 quoted lines
> > [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
Junio C Hamano· Jun 13, 2014, 17:42 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 10 quoted lines
> [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.

Jeff King· Jun 13, 2014, 23:41 UTC · re: Junio C Hamano · lore

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

On Fri, Jun 13, 2014 at 10:42:26AM -0700, Junio C Hamano wrote:
Show 16 quoted lines
> 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
Caleb Thompson· Jun 16, 2014, 17:46 UTC · re: Jeff King · lore

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

On Fri, Jun 13, 2014 at 07:41:29PM -0400, Jeff King wrote:
Show 28 quoted lines
> 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
Junio C Hamano· Jun 16, 2014, 18:58 UTC · re: Caleb Thompson · lore

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

Caleb Thompson <caleb@calebthompson.io> writes:
Show 34 quoted lines
> 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.

Caleb Thompson· Jun 12, 2014, 20:00 UTC · re: Caleb Thompson · lore

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

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(-)
Show changes to 5 files +53 −2

Documentation/config.txt, Documentation/git-commit.txt, builtin/commit.c, contrib/completion/git-completion.bash, t/t7507-commit-verbose.sh

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
Junio C Hamano· Jun 13, 2014, 17:48 UTC · re: Caleb Thompson · lore

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

Caleb Thompson <caleb@calebthompson.io> writes:
Show 11 quoted lines
> 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?
Show 41 quoted lines
> @@ -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· Jun 16, 2014, 19:50 UTC · re: Junio C Hamano · lore

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

On Fri, Jun 13, 2014 at 10:48:55AM -0700, Junio C Hamano wrote:
Show 27 quoted lines
> 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.

Show 41 quoted lines
> > @@ -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
Caleb Thompson· Jun 16, 2014, 20:05 UTC · re: Caleb Thompson · lore

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

On Mon, Jun 16, 2014 at 02:50:57PM -0500, Caleb Thompson wrote:
Show 51 quoted lines
> 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
Show 43 quoted lines
> > > @@ -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
Junio C Hamano· Jun 16, 2014, 20:06 UTC · re: Caleb Thompson · lore

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

Caleb Thompson <caleb@calebthompson.io> writes:
Show 32 quoted lines
> 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?
Caleb Thompson· Jun 16, 2014, 20:10 UTC · re: Junio C Hamano · lore

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

On Mon, Jun 16, 2014 at 01:06:45PM -0700, Junio C Hamano wrote:
Show 54 quoted lines
> 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
Junio C Hamano· Jun 16, 2014, 22:25 UTC · re: Caleb Thompson · lore

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

Caleb Thompson <caleb@calebthompson.io> writes:
Show 20 quoted lines
>> 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.
Jeremiah Mahler· Jun 12, 2014, 20:30 UTC · re: Caleb Thompson · lore

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

On Thu, Jun 12, 2014 at 02:38:58PM -0500, Caleb Thompson wrote:
Show 39 quoted lines
> 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
Caleb Thompson· Jun 13, 2014, 16:49 UTC · re: Jeremiah Mahler · lore

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

On Thu, Jun 12, 2014 at 01:30:10PM -0700, Jeremiah Mahler wrote:
Show 44 quoted lines
> 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
Jeremiah Mahler· Jun 14, 2014, 04:14 UTC · re: Caleb Thompson · lore

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

On Fri, Jun 13, 2014 at 11:49:10AM -0500, Caleb Thompson wrote:
Show 50 quoted lines
> 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
Junio C Hamano· Jun 16, 2014, 20:28 UTC · re: Jeremiah Mahler · lore

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

Jeremiah Mahler <jmmahler@gmail.com> writes:
Show 13 quoted lines
> 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

← back to recent threads