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