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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 20, 2017, 02:19 UTC
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:

Show 10 quoted lines

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

Show 15 quoted lines

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

Show 5 quoted lines

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

Show 8 quoted lines

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