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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 21, 2017, 01:52 UTC
Message-ID: <xmqqfuadgtcu.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79kbEmdJu9a-h07UvPaymMmLJ4Azm+iChSpCBwvvtOW8gog@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 20 quoted lines

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

I do not think it is a noise. I was primarily focusing on "have to explicitly" part, making it sound as if it was a flaw. I do think it is a good idea to explain a variable and/or an option is multi-valued and how multiple instances of them interact with each other. I think the status quo is:

	Both configuration and command line, these multi-valued
	things accumulate.  The "command line can be used to
	override things from the configuration" rule applies as any
	other config/option pair.

	In addition, in the configuration, there is a mechanism to
	clear the values read so far with the "an empty value
	clears" rule, because you may not have control over, or it
	may be cumbersome to modify, lower-priority configuration
	files.  There is no such thing for command line, as the
	entirety of the command line for each invocation is under
	your control.

If somebody has

	[alias] mypush = push -ofoo

then the command line argument for one invocation of "git mypush" may *not* be under your control and it might be also convenient if there were a mechanism to clear what has been accumulated from the command line. It may be worth considering, but if we were going in that direction, I suspect that it is probably a good idea to first step back a bit and introduce a mechanism to easily define pairs of option/config in a more sysmtematic way [*1*]. Once we have such a mechanism, the "clear the previous" syntax for the command line would be implemented uniformly in there.

[Footnote]

*1* E.g. right now, the fact that "push --push-option" and
    "push.pushOption" are related, or that "status -u<mode>" and
    "status.showUntrackedFiles" are related, is only known to the
    code and the documentation; I am not sure if it is even a good
    idea to add config to each and every option that exists, but for
    the ones that do exist, it would be nicer if there were a more
    uniform and systematic way for parse-options.c and config.c APIs
    to help our code, instead of writing git_config() callback and
    options[] array to give to parse_options(), making sure they
    refer to the same internal variable, and the latter overrides
    the former.