Re: [PATCH v4 7/9] sequencer: load commit related config

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 4, 2017, 18:30 UTC
Message-ID: <xmqqd13umjl7.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171124110758.9406-8-phillip.wood@talktalk.net>

Phillip Wood <phillip.wood@talktalk.net> writes:

Show 17 quoted lines

> --- 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);
> +}
> +

Sorry for spotting the problem this late, but this code is unfortunate and we will need to revisit it later; we may want to do so sooner rather than later.

When k,v is a valid configuration that is handled by sequencer_config() successfully, this code still needs to call into default_config() with the same k,v, only to get it ignored.

The problem lies in the (mis)design of git_sequencer_config(). The function should either allow the caller to notice that (k,v) has already been handled, or be the last one in the callback by making a call to default_config() itself.

For the former, because this helper does not have to be called directly as a git_config() callback, but instead it is always meant to be called indirectly from another git_config() callback (like git_rebase_helper_config() here, and common_config() in builtin/revert.c like we see below), it does *not* have to be constrained by the function signature required for it to be a config callback. It could take a pointer to an int that stores if 'k' was handled inside the function,

    int git_sequencer_config_helper(char *k, char *v, int *handled);

for example.