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

> On 04/12/17 18:30, Junio C Hamano wrote:
>> 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
> 
>> 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.
>>
>