# [PATCH v7] commit: add a commit.verbose config variable 9 messages from 2016-03-14 to 2016-03-15. Participants: Pranit Bauva, SZEDER Gábor, Eric Sunshine, Junio C Hamano. Thread: https://gitlist.dev/t/41686 ## Pranit Bauva, 2016-03-14 21:38 Subject: [PATCH v7] commit: add a commit.verbose config variable Message-ID: <010201537710be08-f31428b3-5df3-4694-8c4a-0e8f058131b3-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/010201537710be08-f31428b3-5df3-4694-8c4a-0e8f058131b3-000000%40eu-west-1.amazonses.com ``` Add commit.verbose configuration variable as a convenience for those who always prefer --verbose. Helped-by: Eric Sunshine Signed-off-by: Pranit Bauva --- The previous versions of this patch are: - [v6] $gmane/288811 - [v5] $gmane/288728 - [v4] $gmane/288652 - [v3] $gmane/288634 - [v2] $gmane/288569 - [v1] $gmane/287540 The changes with respect to the last version are : - Add '-c commit.verbose true' It is a mistake on my part. I was a bit sleepy. --- Documentation/config.txt | 4 ++++ Documentation/git-commit.txt | 3 ++- builtin/commit.c | 4 ++++ t/t7507-commit-verbose.sh | 29 +++++++++++++++++++++++++++++ 4 files changed, 39 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..d474226 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. See the `commit.verbose` configuration + variable in linkgit:git-config[1]. + 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..e0b96231 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..5320f1e 100755 --- a/t/t7507-commit-verbose.sh +++ b/t/t7507-commit-verbose.sh @@ -96,4 +96,33 @@ 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.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 +' + +test_expect_success 'commit.verbose true and --verbose' ' + git -c commit.verbose=true commit --amend --verbose +' + +test_expect_success 'commit.verbose false and --no-verbose' ' + test_must_fail git -c commit.verbose=false commit --amend --no-verbose +' + +test_expect_success 'status ignores commit.verbose=true' ' + git -c commit.verbose=true status >actual && + ! grep "^diff --git" actual +' + test_done -- https://github.com/git/git/pull/205 ``` ## SZEDER Gábor, 2016-03-15 11:31 Subject: Re: [PATCH v7] commit: add a commit.verbose config variable Message-ID: <1458041486-7044-1-git-send-email-szeder@ira.uka.de> URL: https://gitlist.dev/e/1458041486-7044-1-git-send-email-szeder%40ira.uka.de In-Reply-To: <010201537710be08-f31428b3-5df3-4694-8c4a-0e8f058131b3-000000@eu-west-1.amazonses.com> ``` > Add commit.verbose configuration variable as a convenience for those > who always prefer --verbose. > > Helped-by: Eric Sunshine > Signed-off-by: Pranit Bauva > > --- > The previous versions of this patch are: > - [v6] $gmane/288811 > - [v5] $gmane/288728 > - [v4] $gmane/288652 > - [v3] $gmane/288634 > - [v2] $gmane/288569 > - [v1] $gmane/287540 > > The changes with respect to the last version are : > - Add '-c commit.verbose true' > > It is a mistake on my part. I was a bit sleepy. > --- > Documentation/config.txt | 4 ++++ > Documentation/git-commit.txt | 3 ++- > builtin/commit.c | 4 ++++ > t/t7507-commit-verbose.sh | 29 +++++++++++++++++++++++++++++ > 4 files changed, 39 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]. > + You made 'commit.verbose' a boolean, so either verbose or not, ... > 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..d474226 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. See the `commit.verbose` configuration > + variable in linkgit:git-config[1]. > + > If specified twice, show in addition the unified diff between > what would be committed and the worktree files, i.e. the unstaged ... but note these context lines telling us that --verbose can be specified not just once but twice (and who knows what the future may bring). This raises some not entirely rhetorical questions: - What does 'git config commit.verbose true && git commit --verbose' do? - Should 'commit.verbose' only care about the convenience of those who always prever '--verbose', or about those who like '-v -v', too? - If the latter, i.e. 'commit.verbose' should cater for '-v -v' as well, then what should 'git config commit.verbose "" && git commit --verbose' do? > diff --git a/builtin/commit.c b/builtin/commit.c > index b3bd2d4..e0b96231 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..5320f1e 100755 > --- a/t/t7507-commit-verbose.sh > +++ b/t/t7507-commit-verbose.sh > @@ -96,4 +96,33 @@ 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.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 > +' > + > +test_expect_success 'commit.verbose true and --verbose' ' > + git -c commit.verbose=true commit --amend --verbose > +' > + > +test_expect_success 'commit.verbose false and --no-verbose' ' > + test_must_fail git -c commit.verbose=false commit --amend --no-verbose > +' > + > +test_expect_success 'status ignores commit.verbose=true' ' > + git -c commit.verbose=true status >actual && > + ! grep "^diff --git" actual > +' > + > test_done > > -- > https://github.com/git/git/pull/205 ``` ## Pranit Bauva, 2016-03-15 19:00 Subject: Re: [PATCH v7] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/CAFZEwPNAVxT2HVQyj0Wajth_y-P6Af4AcSZDzJDJP%3DvLHztwiw%40mail.gmail.com In-Reply-To: <1458041486-7044-1-git-send-email-szeder@ira.uka.de> ``` On Tue, Mar 15, 2016 at 5:01 PM, SZEDER Gábor wrote: > You made 'commit.verbose' a boolean, so either verbose or not, ... > ... but note these context lines telling us that --verbose can be > specified not just once but twice (and who knows what the future may > bring). This raises some not entirely rhetorical questions: > > - What does 'git config commit.verbose true && git commit --verbose' > do? This is a nice thought which didn't strike me! As Eric Sunshine mentioned ($gmane.org/288811), it would react according to the multiple verbosity level and since its not currently defined in `commit` it will react as it is reacting when verbosity level is 1. If let's say we incorporate this behavior now, it can lead to confusion for the user (not developer) as to whether `commit` supports multiple verbosity. > - Should 'commit.verbose' only care about the convenience of those > who always prever '--verbose', or about those who like '-v -v', > too? > > - If the latter, i.e. 'commit.verbose' should cater for '-v -v' as > well, then what should 'git config commit.verbose " two>" && git commit --verbose' do? If I was the user, I would use multiple levels of verbosity only where I would feel that there is some problem happening with the commit that is in progress. Having an "awkward" commit isn't usual and definitely not every time. Though if in future it is required, We can add edit the line namely : if(!strcmp(k, "commit.verbose")) { - verbose = git_config_bool(k, v); + verbose = git_config_int(k, v); return 0; } Additionally we will have to define scenarios and mention them in the documentation as to how `commit` will react as this cannot be known by intuition. This would complicate things for the user who is reading the man pages. Regards, Pranit Bauva ``` ## Eric Sunshine, 2016-03-15 19:24 Subject: Re: [PATCH v7] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/CAPig%2BcTRWg9yB7eXhnbgbjjYfrJyn_A-_gQmbW8NL1DEbkLpkQ%40mail.gmail.com In-Reply-To: ``` On Tue, Mar 15, 2016 at 3:00 PM, Pranit Bauva wrote: > On Tue, Mar 15, 2016 at 5:01 PM, SZEDER Gábor wrote: >> You made 'commit.verbose' a boolean, so either verbose or not, ... >> ... but note these context lines telling us that --verbose can be >> specified not just once but twice (and who knows what the future may >> bring). This raises some not entirely rhetorical questions: >> >> - What does 'git config commit.verbose true && git commit --verbose' >> do? > > This is a nice thought which didn't strike me! > > As Eric Sunshine mentioned ($gmane.org/288811), it would react > according to the multiple verbosity level and since its not currently > defined in `commit` it will react as it is reacting when verbosity > level is 1. I get the feeling that you missed SZEDER's point which was that git-commit already behaves differently when --verbose is specified multiple times. (I hadn't gotten around to reviewing that part of the code yet, so I'm glad that SZEDER saved me the effort.) The new config variable, which is boolean, doesn't mesh well with multiple verbosity levels. For instance, with a combination of commit.verbose=true and a single --verbose, the code will think that --verbose was given twice and use verbosity level 2, which is not at all intuitive and would be surprising for the user. So, SZEDER was asking how this impedance mismatch can be rectified. A possibly sane approach would be to make commit.verbose be a verbosity level rather than boolean, and behave as follows: 1. if --verbose is used (one or more), completely ignore commit.verbose. 2. else, if commit.verbose is set, use it. 3. else, default behavior. I'm not sure if this makes sense, but as a convenience, maybe also recognize "true" and "false" as aliases for 1 and 0, respectively, for values of commit.verbose. And, of course you'd want to test these behaviors. ``` ## Pranit Bauva, 2016-03-15 20:13 Subject: Re: [PATCH v7] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/CAFZEwPP5AXHCmAg62wVNy_8FuOQxPdYofeb5b%3Dm3g-ugiJkTyw%40mail.gmail.com In-Reply-To: ``` On Wed, Mar 16, 2016 at 12:54 AM, Eric Sunshine wrote: >> As Eric Sunshine mentioned ($gmane.org/288811), it would react >> according to the multiple verbosity level and since its not currently >> defined in `commit` it will react as it is reacting when verbosity >> level is 1. > > I get the feeling that you missed SZEDER's point which was that > git-commit already behaves differently when --verbose is specified > multiple times. (I hadn't gotten around to reviewing that part of the > code yet, so I'm glad that SZEDER saved me the effort.) My bad! I missed SZEDER's point. > The new config variable, which is boolean, doesn't mesh well with > multiple verbosity levels. For instance, with a combination of > commit.verbose=true and a single --verbose, the code will think that > --verbose was given twice and use verbosity level 2, which is not at > all intuitive and would be surprising for the user. So, SZEDER was > asking how this impedance mismatch can be rectified. > > A possibly sane approach would be to make commit.verbose be a > verbosity level rather than boolean, and behave as follows: > > 1. if --verbose is used (one or more), completely ignore commit.verbose. > 2. else, if commit.verbose is set, use it. > 3. else, default behavior. > > I'm not sure if this makes sense, but as a convenience, maybe also > recognize "true" and "false" as aliases for 1 and 0, respectively, for > values of commit.verbose. > > And, of course you'd want to test these behaviors. This seems a good approach to me. I have two ideas of implementing it. First one to introduce a new variable `config_verbose` to store the value read by the config. Till then the value of verbose can be set through command line options. Depending on the situation as you described, it can then make the modification. Another approach would be to swap the places where the configuration file is read and where arguments are parsed. I personally think the first approach as more appropriate as in the latter one, there might be some parts of code which can break. As for the part of alias, I can use the method git_config_bool_or_int() which takes care about aliasing for me. I will also write tests for this behavior. ``` ## Junio C Hamano, 2016-03-15 20:24 Subject: Re: [PATCH v7] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/xmqqlh5j7am8.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Pranit Bauva writes: > First one to introduce a new variable `config_verbose` to store the > value read by the config. Till then the value of verbose can be set > through command line options. Depending on the situation as you > described, it can then make the modification. Another approach would > be to swap the places where the configuration file is read and where > arguments are parsed. I personally think the first approach as more > appropriate as in the latter one, there might be some parts of code > which can break. Changing config-first-and-then-command-line is likely to break things, unless you do a lot more work to avoid breakage ;-) Wouldn't it be the simplest to do: * initialize opt-verbose to "unspecified"; * initialize config-verbosity to "unspecified"; * let git_config() update config-verbosity; * let parse_options() update opt-verbose. and then * if opt-verbose is still "unspecified", then overwrite it with config-verbosity. * if opt-verbose is still "unspecified" after that, then neither the command line nor the configuration gave you verbosity. * otherwise opt-verbose at this point has what verbosity level to use. ? ``` ## Pranit Bauva, 2016-03-15 21:09 Subject: Re: [PATCH v7] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/CAFZEwPOK1hH8CuFc6TohDc5xq7GXpp0DBnQi4QQ6m%2B8dspUbeQ%40mail.gmail.com In-Reply-To: ``` On Wed, Mar 16, 2016 at 1:54 AM, Junio C Hamano wrote: > Pranit Bauva writes: > >> First one to introduce a new variable `config_verbose` to store the >> value read by the config. Till then the value of verbose can be set >> through command line options. Depending on the situation as you >> described, it can then make the modification. Another approach would >> be to swap the places where the configuration file is read and where >> arguments are parsed. I personally think the first approach as more >> appropriate as in the latter one, there might be some parts of code >> which can break. > > Changing config-first-and-then-command-line is likely to break > things, unless you do a lot more work to avoid breakage ;-) I had guessed this correctly! :) > Wouldn't it be the simplest to do: > > * initialize opt-verbose to "unspecified"; > * initialize config-verbosity to "unspecified"; > * let git_config() update config-verbosity; > * let parse_options() update opt-verbose. > > and then > > * if opt-verbose is still "unspecified", then overwrite it with > config-verbosity. > > * if opt-verbose is still "unspecified" after that, then neither > the command line nor the configuration gave you verbosity. > > * otherwise opt-verbose at this point has what verbosity level to > use. > > ? I just realized that both of our approaches breaks the condition with no-verbose. If commit.verbose is set to true and --no-verbose is passed, then it should not give verbose output. But I suppose that is the reason you are saying "unspecified". If `opt-verbose` is 0 then it would mean --no-verbose is activated. If it is still "unspecified" then there would not be any such option. But now the question is how do you set a variable as "unspecified". I can set `opt-verbose` as -1. But then I am still not convinced for the requirement of another variable `opt-verbose` as I believe that the `verbose` and `config_verbose` are quite enough for this. First `config_verbose` will read from configuration file. Then `verbose` will read from command line options. If `verbose` is unspecified then it will use `config_verbose`, else if `verbose` is 0 it will ignore `config_verbose` else if `verbose` has a value greater than 0 then it will stay as it is. Or is there something else which I forgot to consider? ``` ## Junio C Hamano, 2016-03-15 21:16 Subject: Re: [PATCH v7] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/xmqqh9g77879.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Pranit Bauva writes: > ... But then I am still not convinced for the > requirement of another variable `opt-verbose` as I believe that the > `verbose` and `config_verbose` are quite enough for this. > ... Or is there something else which I forgot to > consider? I do not think we need three variables. If there is one "verbose" that is initialized to "unspecified" (which must be different from "no", "yes", "even more verbose"), then it is perfectly fine to reuse that as if it were "opt-verbose" in my outline. I just used that name to make it clear where the value comes from for these two entities, i.e. to contrast "opt vs config" (as opposed to "(nothing) vs config"). ``` ## Pranit Bauva, 2016-03-15 21:18 Subject: Re: [PATCH v7] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/CAFZEwPN_OTDxHpYvO2VfZEf5fG0X8fefU65XiLFy0Lt2Fs8Hbg%40mail.gmail.com In-Reply-To: ``` On Wed, Mar 16, 2016 at 2:46 AM, Junio C Hamano wrote: > Pranit Bauva writes: > >> ... But then I am still not convinced for the >> requirement of another variable `opt-verbose` as I believe that the >> `verbose` and `config_verbose` are quite enough for this. >> ... Or is there something else which I forgot to >> consider? > > I do not think we need three variables. If there is one "verbose" > that is initialized to "unspecified" (which must be different from > "no", "yes", "even more verbose"), then it is perfectly fine to > reuse that as if it were "opt-verbose" in my outline. I just used > that name to make it clear where the value comes from for these two > entities, i.e. to contrast "opt vs config" (as opposed to "(nothing) > vs config"). I just wanted to clear out the confusion! I will send the updated patch with tests soon! ```