Re: [PATCH v1 7/8] sequencer: load commit related config

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 7, 2017, 01:02 UTC
Message-ID: <alpine.DEB.2.21.1.1711070202040.6482@virtualbox>
In-Reply-To: <20171106112709.2121-8-phillip.wood@talktalk.net>

Hi Phillip,

On Mon, 6 Nov 2017, Phillip Wood wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> Load default values for message cleanup and gpg signing of commits in
> preparation for committing without forking 'git commit'.

Nicely explained.

Show 17 quoted lines

> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index f8519363a393862b6857acab037e74367c7f2134..68194d3aed950f327a8bc624fa1991478dfea01e 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -9,6 +9,17 @@ static const char * const builtin_rebase_helper_usage[] = {
>  	NULL
>  };
>  
> +static int git_rebase_helper_config(const char *k, const char *v, void *cb)
> +{
> +	int status;
> +
> +	status = git_sequencer_config(k, v, NULL);
> +	if (status)
> +		return status;
> +
> +	return git_default_config(k, v, NULL);

It's more a matter of taste than anything else, but this one would be a little bit shorter:

	return git_sequencer_config(k, v, NULL) ||
		git_default_config(k, v, NULL);

A more important question would be whether this `git_default_config()` call could be folded into `git_sequencer_config()` right away, so that the same pattern does not have to be repeated in rebase--helper as well as in revert/cherry-pick.

Show 9 quoted lines

> diff --git a/builtin/revert.c b/builtin/revert.c
> index b9d927eb09c9ed87c84681df1396f4e6d9b13c97..b700dc7f7fd8657ed8cd2450a8537fe98371783f 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -31,6 +31,17 @@ static const char * const cherry_pick_usage[] = {
>  	NULL
>  };
>  
> +static int git_revert_config(const char *k, const char *v, void *cb)

Seeing as it is used also by `cmd_cherry_pick()`, and that it is file-local anyway, maybe `common_config()` is a better name?

This point is moot if we can call `git_default_config()` in `git_sequencer_config()` directly, though.

Show 10 quoted lines

> diff --git a/sequencer.c b/sequencer.c
> index 3e4c3bbb265db58df22cfcb5a321fb74d822327e..b8cf679751449591d6f97102904e060ebee9d7a1 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -688,6 +688,39 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
>  	return run_command(&cmd);
>  }
>  
> +static enum cleanup_mode default_msg_cleanup = CLEANUP_NONE;
> +static char *default_gpg_sign;

I was ready to shout about global state not meshing well with libified code, but as long as we're sure that these values are set only while Git executes single-threaded, still, it is the correct way to do it: these settings reflect the config, and therefore *are* kinda global (at least until the day when the submodule fans try to call `git commit` in a submodule using the `struct repository` data structure).

In short: this code is good (and I was just describing a little bit of my thinking, to demonstrate that I tried to be a diligent reviewer :-)).

Thanks, Dscho