On Thu, Mar 10, 2016 at 5:12 PM, Pranit Bauva <pranit.bauva@xxxxxxxxx> 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 <pranit.bauva@xxxxxxxxx>
> ---
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.
> 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.
> 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 {
> + 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.
> + 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...
> +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
--
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