From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 20 Oct 2017 02:19:44 GMT
Subject: Re: [PATCH] builtin/push.c: add push.pushOption config
Message-ID: <xmqqbml2imrj.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79kaSU+w0=zb61=5pEzhtd4U5Hzae4C2bUgpchNHAL_mzMA@mail.gmail.com>

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

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