Thanks for the submission. Review comments below...
On Thu, Feb 25, 2016 at 9:57 PM, Pranit Bauva <pranit.bauva@xxxxxxxx> wrote:
> From c273a02fc9cab9305cedf6e37422e257a1cc3b1e Mon Sep 17 00:00:00 2001
> From: Pranit Bauva <pranit.bauva@xxxxxxxx>
> Date: Fri, 26 Feb 2016 07:14:18 +0530
> Subject: [PATCH/RFC] git-commit: add a commit.verbose config variable
Drop these four lines. The first is only meaningful in your
repository, and the rest are picked up automatically by git-am from
the email envelope.
> Since many people always run the command with this option, and would
> prefer not to use the argument again and again but instead specify it in
> some config file.
This seems like a sentence fragment. I suppose you meant it as a
continuation of the patch subject? Better would be to write a full
sentence instead so that the reader doesn't have to guess at its
meaning.
> The variable `verbose` is changed instead of `s.verbose` as the method
> run_status() updates the `s.verbose` with the value of `verbose`. So in
> this way the change is reflected in both of them.
Talking about this in the commit message misleads the reader into
thinking that there is some potential oddity going on where a careful
decision needs to be made about which variable to set, when that's not
in fact the case. The 'verbose' member of wt_status is just one
consumer of the "verbose" flag, not the sole consumer. Another
consumer is found in builtin/commit.c:cmd_commit():
if (verbose ||
cleanup_mode == CLEANUP_SCISSORS)
wt_status_truncate_message_at_cut_line(&sb);
So, it would not be correct for the configuration ever to set only
wt_status::verbose.
Consequently, it would be better to drop this paragraph altogether
from the commit message, so as to avoid confusing readers.
> Signed-off-by: Pranit Bauva <pranit.bauva@xxxxxxxx>
> ---
>
> Notes:
> This is a patch for the microproject of GSOC 2016. I have done the change
> under careful consideration of where to place the line. I have to yet write
> the tests for this. I have explored the config API and I am currently going
> through the tests part. I have run the test locally by manually checking.
> I currently learning about the test suite. I will update this patch
> with some tests in some time.
Some tests to consider:
* commit.verbose unset
* commit.verbose=true
* commit.verbose=false
* --verbose overrides commit.verbose
* --no-verbose overrides commit.verbose
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 01cca0a..f7e9c09 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1110,6 +1110,11 @@ commit.template::
> "`~/`" is expanded to the value of `$HOME` and "`~user/`" to the
> specified user's home directory.
>
> +commit.verbose::
> + A boolean to specify whether to always include the verbose option
It's nice to see a documentation update included in the patch.
> + with git-config.
I guess you meant "git-commit".
> + See linkgit:git-commit[1]
Nit: It wouldn't hurt to fold this line into the line above.
> 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. If this option is used always, it can
> + be set in the git-config with the boolean variable `commit.verbose`.
You could probably replace this entire added sentence with the simpler:
Also see the `commit.verbose` configuration variable.
> If specified twice, show in addition the unified diff between
> what would be committed and the worktree files, i.e. the unstaged
> diff --git a/builtin/commit.c b/builtin/commit.c
> index b3bd2d4..68080fe 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1644,6 +1644,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
> status_format = STATUS_FORMAT_NONE; /* Ignore status.short */
> s.colopts = 0;
>
> + git_config_get_bool("commit.verbose", &verbose);
I haven't fully digested builtin/commit.c, but the placement of this
new code seems suspect. My expectation would have been to see
git_commit_config() updated to recognize the new "commit.verbose"
variable. Am I missing something?
> if (get_sha1("HEAD", sha1))
> current_head = NULL;
> else {
> --
> 2.1.4
--
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