[PATCH v4] commit: add a commit.verbose config variable

Since many people always run the command with this option, it would be
preferrable to specify it in the configuration file instead of passing
the option with `git commit` again and again.

Signed-off-by: Pranit Bauva <redacted>
---
 Documentation/config.txt     |  4 ++++
 Documentation/git-commit.txt |  3 ++-
 builtin/commit.c             |  4 ++++
 t/t7507-commit-verbose.sh    | 48 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 01cca0a..9b93f6c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -1110,6 +1110,10 @@ commit.template::
 	"`~/`" is expanded to the value of `$HOME` and "`~user/`" to the
 	specified user's home directory.
 
+commit.verbose::
+	A boolean to specify whether to always include the verbose option
+	with `git commit`. See linkgit:git-commit[1].
+
 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 9ec6b3c..3dcaac7 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt

@@ -290,7 +290,8 @@ configuration variable documented in linkgit:git-config[1].
 	what changes the commit has.
 	Note that this diff output doesn't have its
 	lines prefixed with '#'. This diff will not be a part
-	of the commit message.
+	of the commit message. To activate this option permanently, the
+	configuration variable `commit.verbose` can be set to true.
 +
 If specified twice, show in addition the unified diff between
 what would be committed and the worktree files, i.e. the unstaged

diff --git a/builtin/commit.c b/builtin/commit.c
index b3bd2d4..55e9a82 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

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

@@ -96,4 +96,52 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' '
 	test_i18ngrep "Aborting commit due to empty commit message." err
 '
 
+test_expect_success 'commit with commit.verbose true and no arguments' '
+	echo content >file &&
+	git add file &&
+	test_config commit.verbose true &&
+	(
+		GIT_EDITOR=cat &&
+		export GIT_EDITOR &&
+		test_must_fail git commit >output
+	) &&
+	test_i18ngrep "diff --git" output
+'
+
+test_expect_success 'commit with commit.verbose true and --no-verbose' '
+	echo content >file &&
+	git add file &&
+	test_config commit.verbose true &&
+	(
+		GIT_EDITOR=cat &&
+		export GIT_EDITOR &&
+		test_must_fail git commit --no-verbose >output
+	) &&
+	! test_i18ngrep "diff --git" output
+'
+
+test_expect_success 'commit with commit.verbose false and -v' '
+	echo content >file &&
+	git add file &&
+	test_config commit.verbose false &&
+	(
+		GIT_EDITOR=cat &&
+		export GIT_EDITOR &&
+		test_must_fail git commit -v >output
+	) &&
+	test_i18ngrep "diff --git" output
+'
+
+test_expect_success 'commit with commit.verbose false no arguments' '
+	echo content >file &&
+	git add file &&
+	test_config commit.verbose false &&
+	(
+		GIT_EDITOR=cat &&
+		export GIT_EDITOR &&
+		test_must_fail git commit >output
+	) &&
+	! test_i18ngrep "diff --git" output
+'
+
 test_done

--

https://github.com/git/git/pull/205

Older versions of this patch can be found at :-

[v3] : http://thread.gmane.org/gmane.comp.version-control.git/288634

[v2] : http://thread.gmane.org/gmane.comp.version-control.git/288569

[v1] : http://thread.gmane.org/gmane.comp.version-control.git/287540

The changes with respect to last version are :
 - put the code in git_commit_config() instead of git_status_config()
 - Add test to cover all possible scenarios

Regards,
Pranit Bauva
IIT Kharagpur

On Thu, Mar 10, 2016 at 5:12 PM, Pranit Bauva [off-list ref] wrote:

Since many people always run the command with this option, it would be
preferrable to specify it in the configuration file instead of passing
the option with `git commit` again and again.

Perhaps drop the unsubstantiated "many people always" and just say:

    Add commit.verbose configuration variable as a convenience
    for those who always prefer --verbose.

or something.

Signed-off-by: Pranit Bauva <redacted>
---

As a convenience to reviewers, please use this area below the "---"
line to provide links and explain what changed since the previous
round rather than doing so in a separate email.

The "permanently" bit sounds scary. A more concise way to state this might be:

    See the `commit.verbose` configuration variable in
    linkgit:git-config[1].

which doesn't bother spelling out what the intelligent reader should
infer from the reference.

Style: space before {

"no arguments" doesn't convey much; how about "--verbose omitted" or
something? Ditto for the titles of other tests.

+       echo content >file &&
+       git add file &&
+       test_config commit.verbose true &&
+       (
+               GIT_EDITOR=cat &&
+               export GIT_EDITOR &&
+               test_must_fail git commit >output
+       ) &&
+       test_i18ngrep "diff --git" output
+'

Making git-commit fail unconditionally with "aborting due to empty
commit message" is a rather sneaky way to perform this test. I would
have expected to see these new tests re-use the existing machinery
provided by this script (the check-for-diff "editor") rather than
inventing an entirely new and unintuitive mechanism. Doing so would
also reduce the size of each new test.

More below...

+test_expect_success 'commit with commit.verbose true and --no-verbose' '
+       echo content >file &&
+       git add file &&
+       test_config commit.verbose true &&
+       (
+               GIT_EDITOR=cat &&
+               export GIT_EDITOR &&
+               test_must_fail git commit --no-verbose >output
+       ) &&
+       ! test_i18ngrep "diff --git" output
+'
+
+test_expect_success 'commit with commit.verbose false and -v' '
+       echo content >file &&
+       git add file &&
+       test_config commit.verbose false &&
+       (
+               GIT_EDITOR=cat &&
+               export GIT_EDITOR &&
+               test_must_fail git commit -v >output
+       ) &&
+       test_i18ngrep "diff --git" output
+'
+
+test_expect_success 'commit with commit.verbose false no arguments' '
+       echo content >file &&
+       git add file &&
+       test_config commit.verbose false &&
+       (
+               GIT_EDITOR=cat &&
+               export GIT_EDITOR &&
+               test_must_fail git commit >output
+       ) &&
+       ! test_i18ngrep "diff --git" output
+'

Some additional tests[1][2] are probably warranted.

[1]: http://article.gmane.org/gmane.comp.version-control.git/288648
[2]: http://article.gmane.org/gmane.comp.version-control.git/288657

+
 test_done

On Fri, Mar 11, 2016 at 4:31 AM, Eric Sunshine [off-list ref] wrote:

    Add commit.verbose configuration variable as a convenience
    for those who always prefer --verbose.

or something.

Sure!

As a convenience to reviewers, please use this area below the "---"
line to provide links and explain what changed since the previous
round rather than doing so in a separate email.

Actually I am sending the patches with submitGit herokuapp because my
institute proxy does not allow IMAP/POP3 connections.

The "permanently" bit sounds scary. A more concise way to state this might be:

    See the `commit.verbose` configuration variable in
    linkgit:git-config[1].

which doesn't bother spelling out what the intelligent reader should
infer from the reference.
Style: space before {

Sure!

quoted

+test_expect_success 'commit with commit.verbose true and no arguments' '

"no arguments" doesn't convey much; how about "--verbose omitted" or
something? Ditto for the titles of other tests.

Will change the language construct.

quoted

+       echo content >file &&
+       git add file &&
+       test_config commit.verbose true &&
+       (
+               GIT_EDITOR=cat &&
+               export GIT_EDITOR &&
+               test_must_fail git commit >output
+       ) &&
+       test_i18ngrep "diff --git" output
+'

Making git-commit fail unconditionally with "aborting due to empty
commit message" is a rather sneaky way to perform this test. I would
have expected to see these new tests re-use the existing machinery
provided by this script (the check-for-diff "editor") rather than
inventing an entirely new and unintuitive mechanism. Doing so would
also reduce the size of each new test.

I agree on the fact that making git-commit fail unconditionally is not
a good way to perform the test. "check-for-diff" is not really an
"editor" and it checks for the commit message after it has been
written to the history. The verbose output is stripped when it is
written to the history so we won't be able to test whether this patch
works. This is where purposely breaking the code is required as when
the commit fails, it gives the output of the contents present at that
time (which will contain the verbose output). More over the
'check-for-diff' uses grep which is not preferred. Many tests are now
using test_i18ngrep (eg. f79ce8db). I had planned on using
'check-for-diff' before but it took me some time to figure out this
behavior and thus I began searching for another mechanism (breaking
code).

Some additional tests[1][2] are probably warranted.

[1]: http://article.gmane.org/gmane.comp.version-control.git/288648
[2]: http://article.gmane.org/gmane.comp.version-control.git/288657

I think these tests also are better included in this file as this
patch triggers it and it would not make much of a difference between
t7507 and t7502 but in fact improve its readability.

quoted

 test_done

This extra test was introduced erroneously by
f9c0181 (t7502: test commit.status, --status and
--no-status, 2010-01-13)

Signed-off-by: Pranit Bauva <redacted>
---
 t/t7502-commit.sh | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index b39e313..725687d 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh

@@ -527,11 +527,6 @@ try_commit_status_combo () {
 		test_i18ngrep "^# Changes to be committed:" .git/COMMIT_EDITMSG
 	'
 
-	test_expect_success 'commit' '
-		try_commit "" &&
-		test_i18ngrep "^# Changes to be committed:" .git/COMMIT_EDITMSG
-	'
-
 	test_expect_success 'commit --status' '
 		try_commit --status &&
 		test_i18ngrep "^# Changes to be committed:" .git/COMMIT_EDITMSG

--

https://github.com/git/git/pull/208

The older version of this patch :
 - [v1] http://thread.gmane.org/gmane.comp.version-control.git/288662

The changes between the patches :
 - Improved the language construct of the commit message
 - Provided more details about the cited commit in the commit message

Regards,
Pranit Bauva
IIT Kharagpur

[+cc:Roberto Tyley]

On Fri, Mar 11, 2016 at 05:45:27AM +0530, Pranit Bauva wrote:

On Fri, Mar 11, 2016 at 4:31 AM, Eric Sunshine [off-list ref] wrote:

quoted

As a convenience to reviewers, please use this area below the "---"
line to provide links and explain what changed since the previous
round rather than doing so in a separate email.

Actually I am sending the patches with submitGit herokuapp because my
institute proxy does not allow IMAP/POP3 connections.

That's unfortunate. Your separate "cover letter" often arrives hours
later than the patch itself. Perhaps Roberto can comment on submitGit
and per-patch commentary.

quoted

quoted

+       echo content >file &&
+       git add file &&
+       test_config commit.verbose true &&
+       (
+               GIT_EDITOR=cat &&
+               export GIT_EDITOR &&
+               test_must_fail git commit >output
+       ) &&
+       test_i18ngrep "diff --git" output
+'

Making git-commit fail unconditionally with "aborting due to empty
commit message" is a rather sneaky way to perform this test. I would
have expected to see these new tests re-use the existing machinery
provided by this script (the check-for-diff "editor") rather than
inventing an entirely new and unintuitive mechanism. Doing so would
also reduce the size of each new test.

I agree on the fact that making git-commit fail unconditionally is not
a good way to perform the test. "check-for-diff" is not really an
"editor" and it checks for the commit message after it has been
written to the history. The verbose output is stripped when it is
written to the history so we won't be able to test whether this patch
works.

It's a bit tricky if you're not used to it, but check-for-diff
actually does what you want, and does so in a more direct way. While
it's true that it's not an "editor" per se, it does get access to the
entire block of text that would normally appear in your editor during
an interactive commit. And, this is happening before the commit has
been written to history. So, check-for-diff gets a chance to look at
the full text that would appear in your editor, and can therefore
check if it contains the expected "diff --git" string.

This is where purposely breaking the code is required as when
the commit fails, it gives the output of the contents present at that
time (which will contain the verbose output). More over the
'check-for-diff' uses grep which is not preferred. Many tests are now
using test_i18ngrep (eg. f79ce8db).

'test_i18ngrep' is intended for strings which may be translated,
however, since the expected "diff --git" string should never be
translated, check-for-diff's use of 'grep' is correct, whereas
'test_i18ngrep' would be misleading (if not actively wrong).

I had planned on using
'check-for-diff' before but it took me some time to figure out this
behavior and thus I began searching for another mechanism (breaking
code).

As an experiment, I rewrote the four new tests in terms of
check-for-diff (with "test_set_editor check-for-diff" already in
effect). Here's what they look like, and they function as expected:

    test_expect_success 'commit.verbose true and --verbose omitted' '
        git -c commit.verbose=true commit --amend
    '

    test_expect_success 'commit.verbose true and --no-verbose' '
        test_must_fail git -c commit.verbose=true commit --amend --no-verbose
    '

    test_expect_success 'commit.verbose false and --verbose' '
        git -c commit.verbose=false commit --amend --verbose
    '

    test_expect_success 'commit.verbose false and --verbose omitted' '
        test_must_fail git -c commit.verbose=false commit --amend
    '

These are modeled after the "initial commit shows verbose diff" test
earlier in the script.

On Fri, Mar 11, 2016 at 11:14 AM, Eric Sunshine [off-list ref] wrote:

It's a bit tricky if you're not used to it, but check-for-diff
actually does what you want, and does so in a more direct way. While
it's true that it's not an "editor" per se, it does get access to the
entire block of text that would normally appear in your editor during
an interactive commit. And, this is happening before the commit has
been written to history. So, check-for-diff gets a chance to look at
the full text that would appear in your editor, and can therefore
check if it contains the expected "diff --git" string.

Yes, this was new to me. Thanks for explaining it in an elaborate
manner. It took me some time to actually understand the behavior of
check-for-diff (a tricky one). And it does the task pretty nicely!

'test_i18ngrep' is intended for strings which may be translated,
however, since the expected "diff --git" string should never be
translated, check-for-diff's use of 'grep' is correct, whereas
'test_i18ngrep' would be misleading (if not actively wrong).

I should have read the docs before using this method and not just
blindly using it. I will definitely take care of that next time.

As an experiment, I rewrote the four new tests in terms of
check-for-diff (with "test_set_editor check-for-diff" already in
effect). Here's what they look like, and they function as expected:

    test_expect_success 'commit.verbose true and --verbose omitted' '
        git -c commit.verbose=true commit --amend
    '

    test_expect_success 'commit.verbose true and --no-verbose' '
        test_must_fail git -c commit.verbose=true commit --amend --no-verbose
    '

    test_expect_success 'commit.verbose false and --verbose' '
        git -c commit.verbose=false commit --amend --verbose
    '

    test_expect_success 'commit.verbose false and --verbose omitted' '
        test_must_fail git -c commit.verbose=false commit --amend
    '

These are modeled after the "initial commit shows verbose diff" test
earlier in the script.

Thanks a lot for helping me with the tests. I will add the status
tests and then resend the patch. This was a nice exercise!

On 11 March 2016 at 05:44, Eric Sunshine [off-list ref] wrote:

On Fri, Mar 11, 2016 at 05:45:27AM +0530, Pranit Bauva wrote:

quoted

Actually I am sending the patches with submitGit herokuapp because my
institute proxy does not allow IMAP/POP3 connections.

Really glad to hear this is helping you Pranit - I hadn't even thought
of the blocked IMAP/POP3 connections problem, I'm not sure what other
method you could have easily used to get round this.

That's unfortunate. Your separate "cover letter" often arrives hours
later than the patch itself. Perhaps Roberto can comment on submitGit
and per-patch commentary.

This sounds like an improvement I need to make to submitGit, I've
created an issue here:

https://github.com/rtyley/submitgit/issues/30

From: "Pranit Bauva" <redacted>

On Fri, Mar 11, 2016 at 4:31 AM, Eric Sunshine [off-list ref] 
wrote:

quoted

    Add commit.verbose configuration variable as a convenience
    for those who always prefer --verbose.

or something.

Sure!

quoted

As a convenience to reviewers, please use this area below the "---"
line to provide links and explain what changed since the previous
round rather than doing so in a separate email.

Actually I am sending the patches with submitGit herokuapp because my
institute proxy does not allow IMAP/POP3 connections.

You can still include the 'three dashes' and a commentary at the end of your 
(local) regular commit message, and then when it is sent as a patch it will 
have the right format. There are carried through rebases as well.

There is a similar feature for attaching notes (though I haven't used it).

Either should get around your institute's proxy issue.

quoted

The "permanently" bit sounds scary. A more concise way to state this 
might be:

    See the `commit.verbose` configuration variable in
    linkgit:git-config[1].

which doesn't bother spelling out what the intelligent reader should
infer from the reference.
Style: space before {

Sure!

quoted

quoted

+test_expect_success 'commit with commit.verbose true and no arguments' 
'

"no arguments" doesn't convey much; how about "--verbose omitted" or
something? Ditto for the titles of other tests.

Will change the language construct.

quoted

quoted

+       echo content >file &&
+       git add file &&
+       test_config commit.verbose true &&
+       (
+               GIT_EDITOR=cat &&
+               export GIT_EDITOR &&
+               test_must_fail git commit >output
+       ) &&
+       test_i18ngrep "diff --git" output
+'

Making git-commit fail unconditionally with "aborting due to empty
commit message" is a rather sneaky way to perform this test. I would
have expected to see these new tests re-use the existing machinery
provided by this script (the check-for-diff "editor") rather than
inventing an entirely new and unintuitive mechanism. Doing so would
also reduce the size of each new test.

I agree on the fact that making git-commit fail unconditionally is not
a good way to perform the test. "check-for-diff" is not really an
"editor" and it checks for the commit message after it has been
written to the history. The verbose output is stripped when it is
written to the history so we won't be able to test whether this patch
works. This is where purposely breaking the code is required as when
the commit fails, it gives the output of the contents present at that
time (which will contain the verbose output). More over the
'check-for-diff' uses grep which is not preferred. Many tests are now
using test_i18ngrep (eg. f79ce8db). I had planned on using
'check-for-diff' before but it took me some time to figure out this
behavior and thus I began searching for another mechanism (breaking
code).

quoted

Some additional tests[1][2] are probably warranted.

[1]: http://article.gmane.org/gmane.comp.version-control.git/288648
[2]: http://article.gmane.org/gmane.comp.version-control.git/288657

I think these tests also are better included in this file as this
patch triggers it and it would not make much of a difference between
t7507 and t7502 but in fact improve its readability.

quoted

quoted

 test_done