Re: [PATCH v12 5/5] commit: add a commit.verbose config variable

[Eric Sunshine <sunshine@sunshineco.com>]

On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> Add commit.verbose configuration variable as a convenience for those
> who always prefer --verbose.
>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -1110,6 +1110,10 @@ commit.template::
> +commit.verbose::
> +       A boolean or int to specify the level of verbose with `git commit`.
> +       See linkgit:git-commit[1].

s/level of verbose with/verbosity level of/

> diff --git a/builtin/commit.c b/builtin/commit.c
> @@ -1505,6 +1509,11 @@ static int git_commit_config(const char *k, const char *v, void *cb)
> +       if (!strcmp(k, "commit.verbose")) {
> +               int is_bool;
> +               config_verbose = git_config_bool_or_int(k, v, &is_bool);
> +               return 0;
> +       }
> @@ -1654,6 +1663,9 @@ 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 < 0) ? 0 : config_verbose;

Okay, so this version compares 'config_verbose' with "< 0" rather than
"== -1" so it won't misbehave when someone sets config.verbose=-2 as
pointed out in [1]. Good.

> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> @@ -98,4 +98,179 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' '
> +test_expect_success 'set up -v -v' '

Style: "setup -v -v", not "set up -v -v"

> +       echo dirty >file &&
> +       echo dirty >file2 &&
> +       git add file2
> +'

Why this extra complexity when all you need for the "-v -v" case is:

    test_expect_success 'setup -v -v' '
        echo dirty >file
    '

as suggested by [1]? Try to keep unnecessary gunk out of the tests
(and code, in general) to avoid misleading readers into wondering if
something unusual is going on. Which leads to...

> +test_expect_success 'commit.verbose true and --verbose omitted' '
> +       git -c commit.verbose=true commit -F message &&
> +       test_line_count = 1 out
> +'

Why is this new test different (by using "-F message") from all other
new tests which simply use "--amend"? The answer is that it is
performing a "setup" action which should have been done in the "setup"
test just above. But, as noted, this extra setup is unnecessary, thus
also misleading and confusing for readers. Better would be to use the
minimal "setup" shown above (from [1]), and restore this test to use
"--amend" like all other tests.

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
> +'
> [...large number of almost identical tests omitted...]
> +
> +test_expect_success 'commit.verbose=-2 and --no-verbose' '
> +       git -c commit.verbose=-2 commit --amend --no-verbose &&
> +       test_line_count = 0 out
> +'
> +
> +test_expect_success 'commit.verbose=-1 and --no-verbose' '
> +       git -c commit.verbose=-1 commit --amend --no-verbose &&
> +       test_line_count = 0 out
> +'
> +
> +test_expect_success 'commit.verbose=0 and --no-verbose' '
> +       git -c commit.verbose=0 commit --amend --no-verbose &&
> +       test_line_count = 0 out
> +'
> +
> +test_expect_success 'commit.verbose=1 and --no-verbose' '
> +       git -c commit.verbose=1 commit --amend --no-verbose &&
> +       test_line_count = 0 out
> +'

The fact that the 32 new tests are nearly identical suggests strongly
that the testing should instead either be table-driven or be done via
for-loops to systematically cover all cases. Not only would either of
these approaches be easier to digest, but they would make it easy to
tell at a glance if any cases were missing. See [2] for an example of
how the tests can be table-driven, and see the bottom of [3] for an
example of using for-loops to test systematically (though you'd need
to use nested for-loops rather than just the one in the example).

I'm leaning toward systematic testing via nested for-loops as the more
suitable of the two approach for this particular application.

[1]: http://article.gmane.org/gmane.comp.version-control.git/290000
[2]: http://article.gmane.org/gmane.comp.version-control.git/290344
[3]: http://article.gmane.org/gmane.comp.version-control.git/289860