git-commit: add a commit.verbose config variable

PATCH/RFC Pranit Bauva 5 participants 7 messages Jun 15, 2016 Documentation

lore

Share & tools

lore

Pranit Bauva

author

2016-06-15 23:08 UTC #0 lore raw

Quoted 1 line

From c273a02fc9cab9305cedf6e37422e257a1cc3b1e Mon Sep 17 00:00:00 2001

From: Pranit Bauva <[email protected]>
Date: Fri, 26 Feb 2016 07:14:18 +0530
Subject: [PATCH/RFC] git-commit: add a commit.verbose config variable

Since many people always run the command with this option, and would prefer not to use the argument again and again but instead specify it in some config file.

The variable verbose is changed instead of s.verbose as the method run_status() updates the s.verbose with the value of verbose. So in this way the change is reflected in both of them.

Signed-off-by Pranit Bauva <[email protected]>

---

Notes:
This is a patch for the microproject of GSOC 2016. I have done the change under careful consideration of where to place the line. I have to yet write the tests for this. I have explored the config API and I am currently going through the tests part. I have run the test locally by manually checking. I currently learning about the test suite. I will update this patch with some tests in some time.

 Documentation/config.txt     | 5 +++++
 Documentation/git-commit.txt | 3 ++-
 builtin/commit.c             | 2 ++
 3 files changed, 9 insertions(+), 1 deletion(-)

Documentation/config.txt +5 -0

index 01cca0a..f7e9c09 100644
@@ -1110,6 +1110,11 @@ 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-config.
+	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

Documentation/git-commit.txt +2 -1

index 9ec6b3c..2a72437 100644
@@ -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. If this option is used always, it can
+	be set in the git-config with the boolean variable `commit.verbose`.
 +
 If specified twice, show in addition the unified diff between
 what would be committed and the worktree files, i.e. the unstaged

builtin/commit.c +2 -0

index b3bd2d4..68080fe 100644
@@ -1644,6 +1644,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	status_format = STATUS_FORMAT_NONE; /* Ignore status.short */
 	s.colopts = 0;
 
+	git_config_get_bool("commit.verbose", &verbose);
+
 	if (get_sha1("HEAD", sha1))
 		current_head = NULL;
 	else {

Signature · 1 line

2.1.4

Matthieu Moy

2016-06-15 23:08 UTC #1 lore raw

Quoted 4 lines · Pranit Bauva

From c273a02fc9cab9305cedf6e37422e257a1cc3b1e Mon Sep 17 00:00:00 2001
From: Pranit Bauva <[email protected]>
Date: Fri, 26 Feb 2016 07:14:18 +0530
Subject: [PATCH/RFC] git-commit: add a commit.verbose config variable

These should not appear in the body of your message, but they should be the actual headers of your email. Try sending such message to yourself, and compare with what you see on the list.

Using "git send-email" normally does the right thing. You may want to look at https://submitgit.herokuapp.com/ too.

Quoted 3 lines

The variable verbose is changed instead of s.verbose as the method run_status() updates the s.verbose with the value of verbose. So in this way the change is reflected in both of them.

The commit message should not try to rephrase what the patch aleady says.

Quoted 3 lines

This is a patch for the microproject of GSOC 2016. I have done the change under careful consideration of where to place the line. I have to yet write the tests for this.

If you know you haven't finished, you may use WIP (work in progress) instead of RFC in the title.

Documentation/config.txt commit.template:: · line 1114

Quoted 2 lines

Documentation/config.txt +2 -0

+commit.verbose::
+	A boolean to specify whether to always include the verbose option

Boolean is usually written with a capital letter.

Documentation/config.txt commit.template:: · line 1115

Quoted 1 line

Documentation/config.txt +1 -0
+	with git-config.

Did you mean "git commit"?

Documentation/git-commit.txt configuration variable documented in linkgit:git-config[1]. · line 293

Quoted 1 line

Documentation/git-commit.txt +1 -0
+	of the commit message. If this option is used always, it can

"If this option is used always" does not sound right. I'd write "To activate this option permanently, ..."

Documentation/git-commit.txt configuration variable documented in linkgit:git-config[1]. · line 294

Quoted 1 line

Documentation/git-commit.txt +1 -0
+	be set in the git-config with the boolean variable `commit.verbose`.

"the git-config" is not proper English. You mean "a configuration file". I'd write "the configuration variable commit.verbose can be set to true".

builtin/commit.c int cmd_commit(int argc, const char **argv, const char *prefix) · line 1647

Quoted 7 lines

builtin/commit.c +1 -0

@@ -1644,6 +1644,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	status_format = STATUS_FORMAT_NONE; /* Ignore status.short */
 	s.colopts = 0;
 
+	git_config_get_bool("commit.verbose", &verbose);

Doesn't this override any value that --verbose or --no-verbose may have set before?

Signature · 2 lines

Matthieu Moy
http://www-verimag.imag.fr/~moy/

Stefan Beller

2016-06-15 23:08 UTC #2 lore raw

Quoted 5 lines · Matthieu Moy

Documentation/config.txt commit.template:: · line 1114
Quoted 3 lines
Documentation/config.txt +2 -0
+commit.verbose::
+     A boolean to specify whether to always include the verbose option
Boolean is usually written with a capital letter.

I disagree here, ("grep -riI boolean" suggestes to only capitalize boolean at the beginning of a sentence.)

Eric Sunshine

2016-06-15 23:08 UTC #3 lore raw

In reply to Pranit Bauva

Thanks for the submission. Review comments below...

Quoted 4 lines · Pranit Bauva

From c273a02fc9cab9305cedf6e37422e257a1cc3b1e Mon Sep 17 00:00:00 2001
From: Pranit Bauva <[email protected]>
Date: Fri, 26 Feb 2016 07:14:18 +0530
Subject: [PATCH/RFC] git-commit: add a commit.verbose config variable

Drop these four lines. The first is only meaningful in your repository, and the rest are picked up automatically by git-am from the email envelope.

Quoted 3 lines

Since many people always run the command with this option, and would prefer not to use the argument again and again but instead specify it in some config file.

This seems like a sentence fragment. I suppose you meant it as a continuation of the patch subject? Better would be to write a full sentence instead so that the reader doesn't have to guess at its meaning.

Quoted 3 lines

The variable verbose is changed instead of s.verbose as the method run_status() updates the s.verbose with the value of verbose. So in this way the change is reflected in both of them.

Talking about this in the commit message misleads the reader into thinking that there is some potential oddity going on where a careful decision needs to be made about which variable to set, when that's not in fact the case. The 'verbose' member of wt_status is just one consumer of the "verbose" flag, not the sole consumer. Another consumer is found in builtin/commit.c:cmd_commit():

    if (verbose ||
        cleanup_mode == CLEANUP_SCISSORS)
            wt_status_truncate_message_at_cut_line(&sb);

So, it would not be correct for the configuration ever to set only wt_status::verbose.

Consequently, it would be better to drop this paragraph altogether from the commit message, so as to avoid confusing readers.

Quoted 10 lines

Signed-off-by Pranit Bauva <[email protected]>
---
Notes:
This is a patch for the microproject of GSOC 2016. I have done the change under careful consideration of where to place the line. I have to yet write the tests for this. I have explored the config API and I am currently going through the tests part. I have run the test locally by manually checking. I currently learning about the test suite. I will update this patch with some tests in some time.

Some tests to consider:

* commit.verbose unset
* commit.verbose=true
* commit.verbose=false
* --verbose overrides commit.verbose
* --no-verbose overrides commit.verbose

Documentation/config.txt commit.template:: · line 1114

Quoted 10 lines

Documentation/config.txt +2 -0

index 01cca0a..f7e9c09 100644
@@ -1110,6 +1110,11 @@ 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

It's nice to see a documentation update included in the patch.

Documentation/config.txt commit.template:: · line 1115

Quoted 1 line

Documentation/config.txt +1 -0
+       with git-config.

I guess you meant "git-commit".

Documentation/config.txt commit.template:: · line 1116

Quoted 1 line

Documentation/config.txt +1 -0
+       See linkgit:git-commit[1]

Nit: It wouldn't hurt to fold this line into the line above.

Documentation/git-commit.txt configuration variable documented in linkgit:git-config[1]. · line 294

Quoted 8 lines

Documentation/git-commit.txt +2 -1

@@ -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. If this option is used always, it can
+       be set in the git-config with the boolean variable `commit.verbose`.

You could probably replace this entire added sentence with the simpler:

Also see the commit.verbose configuration variable.

builtin/commit.c int cmd_commit(int argc, const char **argv, const char *prefix) · line 1647

Quoted 11 lines

Documentation/git-commit.txt +0 -0

 If specified twice, show in addition the unified diff between
 what would be committed and the worktree files, i.e. the unstaged

builtin/commit.c +1 -0

index b3bd2d4..68080fe 100644
@@ -1644,6 +1644,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
        status_format = STATUS_FORMAT_NONE; /* Ignore status.short */
        s.colopts = 0;
 
+       git_config_get_bool("commit.verbose", &verbose);

I haven't fully digested builtin/commit.c, but the placement of this new code seems suspect. My expectation would have been to see git_commit_config() updated to recognize the new "commit.verbose" variable. Am I missing something?

Quoted 5 lines

builtin/commit.c +0 -1

        if (get_sha1("HEAD", sha1))
                current_head = NULL;
        else {
--

2.1.4

Pranit Bauva

author

2016-06-15 23:08 UTC #4 lore raw

Matthieu Moy:

Quoted 2 lines

Using "git send-email" normally does the right thing. You may want to look at https://submitgit.herokuapp.com/ too.

git send-email does not work as my institute proxy does not allow IMAP/SMTP connections. But I will take care of this from hence forward.

Quoted 2 lines

The commit message should not try to rephrase what the patch aleady says.

I am dropping this statement off because of one another reason which Eric Sunshine gave.

Quoted 2 lines

If you know you haven't finished, you may use WIP (work in progress) instead of RFC in the title.

I wasn't familiar with this tag. I will keep it in mind. And this is not included in Documentation/SubmittingPatches , so I will send a patch to include WIP tag.

Quoted 3 lines

"the git-config" is not proper English. You mean "a configuration file". I'd write "the configuration variable commit.verbose can be set to true".

I actually was facing problem in phrasing it. Thanks for your suggestion. I will update this.

Quoted 3 lines

Documentation/config.txt commit.template:: · line 1115
Quoted 1 line
Documentation/config.txt +1 -0
+	with git-config.
Did you mean "git commit"?

Sorry for my carelessness. I will update this.

Quoted 1 line

Doesn't this override any value that --verbose or --no-verbose may >

have set before?
Yes, this was the problem. I have fixed it now. But there is a glitch. See below.

Eric Sunshine:

Quoted 19 lines · Pranit Bauva

Quoted 4 lines
From c273a02fc9cab9305cedf6e37422e257a1cc3b1e Mon Sep 17 00:00:00 2001
From: Pranit Bauva <[email protected]>
Date: Fri, 26 Feb 2016 07:14:18 +0530
Subject: [PATCH/RFC] git-commit: add a commit.verbose config variable
From hence forth I will take that into consideration.
Talking about this in the commit message misleads the reader into thinking that there is some potential oddity going on where a careful decision needs to be made about which variable to set, when that's not in fact the case. The 'verbose' member of wt_status is just one consumer of the "verbose" flag, not the sole consumer. Another consumer is found in builtin/commit.c:cmd_commit():
    if (verbose ||
        cleanup_mode == CLEANUP_SCISSORS)
            wt_status_truncate_message_at_cut_line(&sb);

Quoted 5 lines

So, it would not be correct for the configuration ever to set only wt_status::verbose.
Consequently, it would be better to drop this paragraph altogether from the commit message, so as to avoid confusing readers.

I guessed this parent-child behavior and I wanted the commit to sound like that but now that I read it again, I can understand that it might confuse readers who aren't familiar with the code base.

Quoted 3 lines

Some tests to consider:
* commit.verbose unset

This was working perfectly fine!

Quoted 1 line

* commit.verbose=true

This was working perfectly fine!

Quoted 1 line

* commit.verbose=false

This was working perfectly fine!

Quoted 1 line

* --verbose overrides commit.verbose

This was showing errors. So I now included an if statement and then placed the whole code after the method parse_and_validate_options() otherwise it would just ignore the behavior. Now even this is working perfectly fine.

Quoted 1 line

* --no-verbose overrides commit.verbose

This is a problematic one as currently git-commit does not have such a behavior. I tried printing value of verbose in both the cases ie. with git commit and git commit --no-verbose and in both of them it printed the value 0. So currently, the program won't understand the "overriding" nature. I have an idea for implementing this. We can keep the default nature to point out to 0 and --no-verbose to point to -1. But I guess this problem would have been faced/implemented in other part of the code. I will have to look in different parts of code and see how this has been done in those so as to maintain the practices followed with git. I am currently not quite familiar with parse-options.c so I will read about it. But you could help by pointing out specific commits or email threads which are related to --no-verbose option in other git commands to speed up the process.

Quoted 4 lines

I haven't fully digested builtin/commit.c, but the placement of this new code seems suspect. My expectation would have been to see git_commit_config() updated to recognize the new "commit.verbose" variable. Am I missing something?

I too had a lot of confusing regarding this. But it seems to me that the method git_commit_config() shows a "different" behavior and I don't think such "complicated" behavior is required for reading the boolean variable commit.verbose.

Junio C Hamano

2016-06-15 23:08 UTC #5 lore raw

Quoted 6 lines · Pranit Bauva

Quoted 2 lines
If you know you haven't finished, you may use WIP (work in progress) instead of RFC in the title.
I wasn't familiar with this tag. I will keep it in mind. And this is not included in Documentation/SubmittingPatches , so I will send a patch to include WIP tag.

Please don't. It is OK to say WIP or RFC or some people even say [not a patch] there; we do not need to enumerate all of them, and your original RFC was just fine.

Matthieu Moy

2016-06-15 23:08 UTC #6 lore raw

Quoted 5 lines · Junio C Hamano

Quoted 3 lines · Pranit Bauva
Quoted 2 lines
If you know you haven't finished, you may use WIP (work in progress) instead of RFC in the title.

Quoted 7 lines

Quoted 3 lines
I wasn't familiar with this tag. I will keep it in mind. And this is not included in Documentation/SubmittingPatches , so I will send a patch to include WIP tag.
Please don't. It is OK to say WIP or RFC or some people even say [not a patch] there; we do not need to enumerate all of them, and your original RFC was just fine.

In case it was unclear: my "you may" here was really meant to be "you may", not "you should" ;-).

Signature · 2 lines

Matthieu Moy
http://www-verimag.imag.fr/~moy/