On 05/12/17 11:21, Phillip Wood wrote:
> On 04/12/17 18:30, Junio C Hamano wrote:
>> Phillip Wood <phillip.wood@xxxxxxxxxxxx> 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 through the callers of gpg_config() it should be fairly easy to
convert it to take an extra parameter in the same way as you suggested
for sequencer_config()
> 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.
>>
>
