Re: [PATCH] builtin/push.c: add push.pushOption config

[Stefan Beller <sbeller@google.com>]

On Thu, Oct 19, 2017 at 7:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> @@ -161,6 +161,9 @@ already exists on the remote side.
>>>         Transmit the given string to the server, which passes them to
>>>         the pre-receive as well as the post-receive hook. The given string
>>>         must not contain a NUL or LF character.
>>> +       When no `--push-option <option>` is given from the command
>>> +       line, the values of configuration variable `push.pushOption`
>>> +       are used instead.
>>
>> We'd also want to document how push.pushOption works in
>> Documentation/config.txt (that contains all the configs)
>
> Perhaps.
>
>> So in the config, we have to explicitly give an empty option to
>> clear the previous options, but on the command line we do not need
>> that, but instead we'd have to repeat any push options that we desire
>> that were configured?
>
> It is not wrong per-se to phrase it like so, but I think that is
> making it unnecessarily confusing by conflating two things.  (1)
> configured values are overridden from the command line, just like
> any other --option/config.variable pair

because they are single value options (usually).

> and (2) unlike usual single
> value variables where "last one wins" rule is simple enough to
> explain,, multi-value variables need a way to "forget everything we
> said so far and start from scratch" syntax, especially when multiple
> input files are involved.

ok, my view of how that should be done is clashing once again
with the projects established standards. Sorry for the noise.

>> Example:
>>
>>   /etc/gitconfig
>>   push.pushoption = a
>>   push.pushoption = b
>>
>>   ~/.gitconfig
>>   push.pushoption = c
>>
>>   repo/.git/config
>>   push.pushoption =
>>   push.pushoption = b
>>
>> will result in only b as a and c are
>> cleared.
>
> The above is correct, and it might be worth giving it as an example
> in the doc, because not just "give an empty entry to clear what has
> been accumulated so far" but a multi-valued option in general is a
> rather rare thing.
>
>> If I were to run
>>   git -c push.pushOption=d push ... (in repo)
>> it would be b and d, but
>>   git push --push-option=d
>> would be d only?
>
>>> @@ -584,12 +599,13 @@ int cmd_push(int argc, const char **argv, const char *prefix)
>>>                 set_refspecs(argv + 1, argc - 1, repo);
>>>         }
>>>
>>> -       for_each_string_list_item(item, &push_options)
>>> +       for_each_string_list_item(item, push_options)
>>
>> We have to do the same for _cmdline here, too?
>
> I do not think so.  The point of these lines that appear before this
> loop:
>
>         git_config(git_push_config, &flags);
>         argc = parse_options(argc, argv, prefix, options, push_usage, 0);
> +       push_options = (push_options_cmdline.nr
> +               ? &push_options_cmdline
> +               : &push_options_config);
>
> is that the command line overrides configured values, just like any
> other configuration.  Adding _cmdline variant here is doubly wrong
> when command line options are given in that it (1) duplicates what
> was obtained from the command line, and (2) does not clear the
> configured values.

Oh, ok. Sorry for the noise once again.

Thanks,
Stefan