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

[Eric Sunshine <sunshine@sunshineco.com>]

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.

> +
>  test_done