threads / rfc / 41524

RFC patchgit-commit: add a commit.verbose config variable

Subject: [PATCH/RFC] git-commit: add a commit.verbose config variable

## tl;dr

7 messages between Feb 25, 2016 and Feb 26, 2016. Diffs are folded; open one to read it.

replies: 6people: 5as markdown or json

Pranit Bauva· Feb 26, 2016, 02:57 UTC · lore
>From c273a02fc9cab9305cedf6e37422e257a1cc3b1e Mon Sep 17 00:00:00 2001
From: Pranit Bauva <pranit.bauva@zoho.com>
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 <pranit.bauva@zoho.com>
---
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(-)
Show changes to 3 files +9 −1

Documentation/config.txt, Documentation/git-commit.txt, builtin/commit.c

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 01cca0a..f7e9c09 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 9ec6b3c..2a72437 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. 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
diff --git a/builtin/commit.c b/builtin/commit.c
index b3bd2d4..68080fe 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -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 {
-- 
2.1.4
Matthieu Moy· Feb 25, 2016, 21:27 UTC · re: Pranit Bauva · lore

Re: [PATCH/RFC] git-commit: add a commit.verbose config variable

Pranit Bauva <pranit.bauva@zoho.com> writes:
> From c273a02fc9cab9305cedf6e37422e257a1cc3b1e Mon Sep 17 00:00:00 2001
> From: Pranit Bauva <pranit.bauva@zoho.com>
> 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.

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

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

> +commit.verbose::
> +	A boolean to specify whether to always include the verbose option
Boolean is usually written with a capital letter.
> +	with git-config.
Did you mean "git commit"?
> +	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, ..."

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

Show 7 quoted lines
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -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?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Stefan Beller· Feb 25, 2016, 21:58 UTC · re: Matthieu Moy · lore

Re: [PATCH/RFC] git-commit: add a commit.verbose config variable

On Thu, Feb 25, 2016 at 1:27 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 5 quoted lines
>
>> +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· Feb 26, 2016, 03:00 UTC · re: Pranit Bauva · lore

Re: [PATCH/RFC] git-commit: add a commit.verbose config variable

Thanks for the submission. Review comments below...
On Thu, Feb 25, 2016 at 9:57 PM, Pranit Bauva <pranit.bauva@zoho.com> wrote:
> From c273a02fc9cab9305cedf6e37422e257a1cc3b1e Mon Sep 17 00:00:00 2001
> From: Pranit Bauva <pranit.bauva@zoho.com>
> 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.

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

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

Show 10 quoted lines
> Signed-off-by: Pranit Bauva <pranit.bauva@zoho.com>
> ---
>
> 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
Show 10 quoted lines
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 01cca0a..f7e9c09 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -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.
> +       with git-config.
I guess you meant "git-commit".
> +       See linkgit:git-commit[1]
Nit: It wouldn't hurt to fold this line into the line above.
Show 8 quoted lines
> diff --git 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. 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.
Show 11 quoted lines
>  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..68080fe 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -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?

Show 5 quoted lines
>         if (get_sha1("HEAD", sha1))
>                 current_head = NULL;
>         else {
> --
> 2.1.4
Pranit Bauva· Feb 26, 2016, 13:26 UTC · re: Eric Sunshine · lore

Re: [PATCH/RFC] git-commit: add a commit.verbose config variable

Matthieu Moy:
> 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.

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

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

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

>> +	with git-config.
>
> Did you mean "git commit"?
Sorry for my carelessness. I will update this.
> 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:
Show 5 quoted lines
> On Thu, Feb 25, 2016 at 9:57 PM, Pranit Bauva <pranit.bauva@zoho.com> wrote:
>> From c273a02fc9cab9305cedf6e37422e257a1cc3b1e Mon Sep 17 00:00:00 2001
>> From: Pranit Bauva <pranit.bauva@zoho.com>
>> 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.
Show 16 quoted lines
> 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.

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.

> Some tests to consider:
> 
> * commit.verbose unset
This was working perfectly fine!
> * commit.verbose=true
This was working perfectly fine!
> * commit.verbose=false
This was working perfectly fine!
> * --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.

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

> 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· Feb 26, 2016, 16:56 UTC · re: Pranit Bauva · lore

Re: [PATCH/RFC] git-commit: add a commit.verbose config variable

Pranit Bauva <pranit.bauva@zoho.com> writes:
Show 6 quoted 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· Feb 26, 2016, 17:32 UTC · re: Junio C Hamano · lore

Re: [PATCH/RFC] git-commit: add a commit.verbose config variable

Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
> Pranit Bauva <pranit.bauva@zoho.com> writes:
>
>>> 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.

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

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

← back to recent threads