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

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

[Matthieu Moy]

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

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

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

[Stefan Beller]

On Thu, Feb 25, 2016 at 1:27 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
>
>> +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.)

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

[Pranit Bauva]

>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(-)

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

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

[Eric Sunshine]

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.

> 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

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

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

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

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

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

[Pranit Bauva]

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

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

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

[Junio C Hamano]

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.

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

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

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