On Sun, Mar 27, 2016 at 9:04 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > 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. I will 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? I will try and be more specific henceforth. >> --- >> 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). Sure. >> 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? This is because the "editor" in v9 checked for "# Changes"... While this "editor" checks for 'diff --git'. And submodules don't give a proper diff to verify (I tried this out and noticed this behavior by tweaking some parts). In fact submodules don't give diff at all. But they do give "# Changes"... So its important to setup up a little before getting started. If this seems unnecessary, then should I move all the tests which were introduced here above the submodule test? > 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. Yes, I should. I will include that. >> + >> 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