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