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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 27, 2016, 03:34 UTC
Message-ID: <CAPig+cTBxH7BAs2M+PBwTHfVJvH0ADL1OSEKdHQY59L-WiYosA@mail.gmail.com>
In-Reply-To: <01020153b478cfab-5309f834-7617-4a7f-a3e9-93f0dcf4a6be-000000@eu-west-1.amazonses.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.

Show 6 quoted lines

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

Show 9 quoted lines

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

Show 11 quoted lines

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

Show 12 quoted lines

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

Show 39 quoted lines

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