Re: [PATCH] commit: make default of "cleanup" option configurable
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 8, 2013, 21:18 UTC
- Message-ID
- <7vlic3cr1g.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1357676176-30019-1-git-send-email-ralf.thielow@gmail.com>
Ralf Thielow <ralf.thielow@gmail.com> writes:
Show 10 quoted lines
> The default of the "cleanup" option in "git commit" > is not configurable. Users who don't want to use the > default have to pass this option on every commit since > there's no way to configure it. This commit introduces > a new config option "commit.cleanup" which can be used > to change the default of the "cleanup" option in > "git commit". > > Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com> > ---
The idea makes sense, but I am not sure the implementation is correct. Does the code already know the final value of use_editor at the point where it calls git_commit_config? I think you do not know at least until you call parse_and_validate_options().
Once you got the implementation right, we would want to make sure that future changes will not break it by adding some tests that verify:
- Without configuration and without option, the command keeps the built-in default;
- Without configuration but with option, the command uses the command line option (we may already have this test, I didn't check);
- With configuration and without option, the command uses the configured default (what this patch adds); and
- With configuration and with option, the command uses the command line option.
The latter two probably needs a few variants, one with --edit (with EDITOR set to something like "true"), another with --no-edit, one without --edit nor --no-edit but with -m "$msg" to implicitly turn use_editor off, and one without --edit, --no-edit, nor -m but with EDITOR set to a scriptlet that writes a message to the file to implicitly turn use_editor on.
Also, the readers of the documentation for "commit --cleanup" will wonder the same thing you wondered before you wrote this patch; mentioning the configuration variable in its documentation will help them.
Thanks.
Show 82 quoted lines
> Documentation/config.txt | 4 ++++
> builtin/commit.c | 29 ++++++++++++++++++-----------
> 2 files changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 53c4ca1..3f76cd1 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -917,6 +917,10 @@ column.tag::
> Specify whether to output tag listing in `git tag` in columns.
> See `column.ui` for details.
>
> +commit.cleanup::
> + This setting overrides the default of the `--cleanup` option in
> + `git commit`. See linkgit:git-commit[1] for details.
> +
> commit.status::
> A boolean to enable/disable inclusion of status information in the
> commit message template when using an editor to prepare the commit
> diff --git a/builtin/commit.c b/builtin/commit.c
> index d6dd3df..42acde7 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -103,7 +103,7 @@ static enum {
> CLEANUP_NONE,
> CLEANUP_ALL
> } cleanup_mode;
> -static char *cleanup_arg;
> +const static char *cleanup_arg;
>
> static enum commit_whence whence;
> static int use_editor = 1, include_status = 1;
> @@ -966,6 +966,20 @@ static const char *read_commit_message(const char *name)
> return out;
> }
>
> +static void parse_cleanup_arg()
> +{
> + if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
> + cleanup_mode = use_editor ? CLEANUP_ALL : CLEANUP_SPACE;
> + else if (!strcmp(cleanup_arg, "verbatim"))
> + cleanup_mode = CLEANUP_NONE;
> + else if (!strcmp(cleanup_arg, "whitespace"))
> + cleanup_mode = CLEANUP_SPACE;
> + else if (!strcmp(cleanup_arg, "strip"))
> + cleanup_mode = CLEANUP_ALL;
> + else
> + die(_("Invalid cleanup mode %s"), cleanup_arg);
> +}
> +
> static int parse_and_validate_options(int argc, const char *argv[],
> const struct option *options,
> const char * const usage[],
> @@ -1044,18 +1058,9 @@ static int parse_and_validate_options(int argc, const char *argv[],
> only_include_assumed = _("Clever... amending the last one with dirty index.");
> if (argc > 0 && !also && !only)
> only_include_assumed = _("Explicit paths specified without -i nor -o; assuming --only paths...");
> - if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
> - cleanup_mode = use_editor ? CLEANUP_ALL : CLEANUP_SPACE;
> - else if (!strcmp(cleanup_arg, "verbatim"))
> - cleanup_mode = CLEANUP_NONE;
> - else if (!strcmp(cleanup_arg, "whitespace"))
> - cleanup_mode = CLEANUP_SPACE;
> - else if (!strcmp(cleanup_arg, "strip"))
> - cleanup_mode = CLEANUP_ALL;
> - else
> - die(_("Invalid cleanup mode %s"), cleanup_arg);
>
> handle_untracked_files_arg(s);
> + parse_cleanup_arg();
>
> if (all && argc > 0)
> die(_("Paths with -a does not make sense."));
> @@ -1320,6 +1325,8 @@ static int git_commit_config(const char *k, const char *v, void *cb)
> include_status = git_config_bool(k, v);
> return 0;
> }
> + if (!strcmp(k, "commit.cleanup"))
> + return git_config_string(&cleanup_arg, k, v);
>
> status = git_gpg_config(k, v, NULL);
> if (status)