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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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.

> 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.

> 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.

> 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