On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva <pranit.bauva@xxxxxxxxx> 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@xxxxxxxxx> > > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html