Re: [PATCH v4] commit: add a commit.verbose config variable
From: Eric Sunshine <hidden>
Date: 2016-06-15 23:08:42
On Thu, Mar 10, 2016 at 5:12 PM, Pranit Bauva [off-list ref] wrote:
Since many people always run the command with this option, it would be preferrable to specify it in the configuration file instead of passing the option with `git commit` again and again.
Perhaps drop the unsubstantiated "many people always" and just say:
Add commit.verbose configuration variable as a convenience
for those who always prefer --verbose.
or something.
Signed-off-by: Pranit Bauva <redacted> ---
As a convenience to reviewers, please use this area below the "---" line to provide links and explain what changed since the previous round rather than doing so in a separate email.
quoted hunk ↗ jump to hunk
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt@@ -290,7 +290,8 @@ configuration variable documented in linkgit:git-config[1]. what changes the commit has. Note that this diff output doesn't have its lines prefixed with '#'. This diff will not be a part - of the commit message. + of the commit message. To activate this option permanently, the + configuration variable `commit.verbose` can be set to true.
The "permanently" bit sounds scary. A more concise way to state this might be:
See the `commit.verbose` configuration variable in
linkgit:git-config[1].
which doesn't bother spelling out what the intelligent reader should
infer from the reference.
quoted hunk ↗ jump to hunk
diff --git a/builtin/commit.c b/builtin/commit.c@@ -1505,6 +1505,10 @@ static int git_commit_config(const char *k, const char *v, void *cb) sign_commit = git_config_bool(k, v) ? "" : NULL; return 0; } + if (!strcmp(k, "commit.verbose")){
Style: space before {
quoted hunk ↗ jump to hunk
+ verbose = git_config_bool(k, v); + return 0; + } status = git_gpg_config(k, v, NULL); if (status)diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh@@ -96,4 +96,52 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' ' test_i18ngrep "Aborting commit due to empty commit message." err ' +test_expect_success 'commit with commit.verbose true and no arguments' '
"no arguments" doesn't convey much; how about "--verbose omitted" or something? Ditto for the titles of other tests.
quoted hunk ↗ jump to hunk
+ echo content >file && + git add file && + test_config commit.verbose true && + ( + GIT_EDITOR=cat && + export GIT_EDITOR && + test_must_fail git commit >output + ) && + test_i18ngrep "diff --git" output +'
Making git-commit fail unconditionally with "aborting due to empty commit message" is a rather sneaky way to perform this test. I would have expected to see these new tests re-use the existing machinery provided by this script (the check-for-diff "editor") rather than inventing an entirely new and unintuitive mechanism. Doing so would also reduce the size of each new test. More below...
quoted hunk ↗ jump to hunk
+test_expect_success 'commit with commit.verbose true and --no-verbose' ' + echo content >file && + git add file && + test_config commit.verbose true && + ( + GIT_EDITOR=cat && + export GIT_EDITOR && + test_must_fail git commit --no-verbose >output + ) && + ! test_i18ngrep "diff --git" output +' + +test_expect_success 'commit with commit.verbose false and -v' ' + echo content >file && + git add file && + test_config commit.verbose false && + ( + GIT_EDITOR=cat && + export GIT_EDITOR && + test_must_fail git commit -v >output + ) && + test_i18ngrep "diff --git" output +' + +test_expect_success 'commit with commit.verbose false no arguments' ' + echo content >file && + git add file && + test_config commit.verbose false && + ( + GIT_EDITOR=cat && + export GIT_EDITOR && + test_must_fail git commit >output + ) && + ! test_i18ngrep "diff --git" output +'
Some additional tests[1][2] are probably warranted. [1]: http://article.gmane.org/gmane.comp.version-control.git/288648 [2]: http://article.gmane.org/gmane.comp.version-control.git/288657
+ test_done