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 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)
I think the answer maybe yes so that it loads the setting for external
diff commands and textconv filters but I'm not sure, perhaps someone
more familiar with the diff code would know? Also I think we should be
loading the basic diff config for creating the patch when we stop with
conflicts?
If anyone has any thought on this, please share them!
Best Wishes
Phillip
>
> 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.
>>
>
