Re: [PATCH v10 3/3] commit: add a commit.verbose config variable

> On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Add commit.verbose configuration variable as a convenience for those
>> who always prefer --verbose taking care of multiple levels of verbosity.
>
> What does "taking care of multiple levels of verbosity" mean? I
> suppose you mean that commit.verbose specifies a verbosity level
> (rather than being merely boolean), but that phrase is difficult to
> decipher. And, since it's obvious from the patch itself that verbosity
> is not a mere boolean, there isn't really a need to mention it here.
> The commit message would be perfectly fine without that bit, so
> perhaps just drop it.

>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>>
>> ---
>> Changes with respect to the previous patch:
>>  - Fixed a status related bug pointed out by SZEDER
>>  - Change some tests
>
> Please help reviewers out by being a bit more verbose when describing
> the changes. For instance, what bug did you fix pointed out by SZEDER?
> And, "change some tests" says nothing useful. What did you change in
> the tests?

>> ---
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> @@ -1355,6 +1357,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)
>>         finalize_colopts(&s.colopts, -1);
>>         finalize_deferred_config(&s);
>>
>> +       if (verbose == -1)
>> +               verbose = 0;
>> +
>
> Nit: It might be good to drop the blank line above this new
> conditional to make it clear that it is part of the
> init_config/parse_options processing (the tail of which is visible in
> the context above).

>>         handle_untracked_files_arg(&s);
>>         if (show_ignored_in_status)
>>                 s.show_ignored_files = 1;
>> @@ -1654,6 +1664,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>         argc = parse_and_validate_options(argc, argv, builtin_commit_options,
>>                                           builtin_commit_usage,
>>                                           prefix, current_head, &s);
>> +
>> +       if (verbose == -1)
>> +               verbose = (config_verbose == -1) ? 0 : config_verbose;
>> +
>
> Nit: For consistency, dropping the blank line above this new
> conditional wouldn't hurt either.
>
>>         if (dry_run)
>>                 return dry_run_commit(argc, argv, prefix, current_head, &s);
>>         index_file = prepare_index(argc, argv, prefix, current_head, 0);
>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
>> @@ -101,4 +101,52 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' '
>> +test_expect_success 'commit.verbose true and --verbose omitted' '
>> +       echo content >file2 &&
>> +       echo content >>file &&
>> +       git add file2 &&
>> +       git -c commit.verbose=true commit -F message &&
>> +       test_line_count = 1 out
>> +'
>
> Why is this test so utterly different than it was in v9 (even though
> the title is the same), and why is it so different from other tests
> below?

> More below...
>
>> +test_expect_success 'commit.verbose true and --verbose' '
>> +       git -c commit.verbose=true commit --amend --verbose &&
>> +       test_line_count = 1 out
>> +'
>> +
>> +test_expect_success 'commit.verbose true and -v -v' '
>> +       git -c commit.verbose=true commit --amend -v -v &&
>> +       test_line_count = 2 out
>> +'
>> +
>> +test_expect_success 'commit.verbose true and --no-verbose' '
>> +       git -c commit.verbose=true commit --amend --no-verbose &&
>> +       ! test -s out
>> +'
>> +
>> +test_expect_success 'commit.verbose false and --verbose' '
>> +       git -c commit.verbose=false commit --amend --verbose &&
>> +       test_line_count = 1 out
>> +'
>> +
>> +test_expect_success 'commit.verbose false and -v -v' '
>> +       git -c commit.verbose=false commit --amend -v -v &&
>> +       test_line_count = 2 out
>> +'
>> +
>> +test_expect_success 'commit.verbose false and --verbose omitted' '
>> +       git -c commit.verbose=false commit --amend &&
>> +       ! test -s out
>> +'
>> +
>> +test_expect_success 'commit.verbose false and --no-verbose' '
>> +       git -c commit.verbose=false commit --amend --no-verbose &&
>> +       ! test -s out
>> +'
>> +
>> +test_expect_success 'status ignores commit.verbose=true' '
>> +       git -c commit.verbose=true status >actual &&
>> +       ! grep "^diff --git" actual
>> +'
>
> The fact that v9 broke a number of tests in other scripts which use
> git-status (as SZEDER pointed out[1]), due to initializing 'verbose'
> to -1 in builtin/commit.c, suggests that you probably ought to add
> another test here to protect against that sort of breakage, as well.
> Specifically, git-status should remain non-verbose when neither
> commit.verbose nor --verbose is specified.