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

From: Phillip Wood <phillip.wood@talktalk.net>
Date: Dec 5, 2017, 11:21 UTC
Message-ID: <632c35d3-46ca-e0bc-5b14-244647df9b19@talktalk.net>
In-Reply-To: <xmqqd13umjl7.fsf@gitster.mtv.corp.google.com>

On 04/12/17 18:30, Junio C Hamano wrote:

Show 23 quoted lines

> Phillip Wood <phillip.wood@talktalk.net> writes:
> 
>> --- 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.

If it needs fixing then doing it before it hits next makes sense.

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

I'm a bit confused by this sentence. Do you mean that when k,v is a valid configuration that is successfully handled by sequencer_config() this code unnecessarily calls default_config() as there is no need to call default_config() if k,v have already been handled?

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

The problem is that git_gpg_config() provides no indication if it has handled k,v so there's no way to avoid the call to default_config() in that case. builtin/am.c and builtin/commit.c both do something like

static int git_am_config(const char *k, const char *v, void *cb)
{
	int status;

	status = git_gpg_config(k, v, NULL);
	if (status)
		return status;

	return git_default_config(k, v, NULL);
}

Looking more generally at sequencer_config() I wonder if we should be providing a warning or an error if the config contains an invalid cleanup mode. Also should it be calling git_diff_ui_config() to set things up for print_commit_summary()? (I'm not sure if anything in that function is affected by diff config settings)

Let me know what you think. I should have time to update this patch set later in the week.

Best Wishes

Phillip

Show 13 quoted lines

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