Subject: Re: [PATCH 3/3] CodingGuidelines: describe naming rules for configuration variables

From: Junio C Hamano <[email-masked]>
Date: Sun, 01 Feb 2015 12:18:34 -0800

Michael Haggerty <[email-masked]> writes:

> On 01/28/2015 11:33 PM, Junio C Hamano wrote:
>> +   When choosing the variable namespace, do not use variable name for
>> +   specifying possibly unbounded set of things, most notably anything
>> +   an end user can freely come up with (e.g. branch names), but also
>> +   large fixed set defined by the system that can grow over time
>> +   (e.g. what kind of common whitespace problems to notice).
>
> I think we can all agree with this rule for "anything an end user can
> freely come up with". Such sets are truly unbounded.
>
> But what is the justification for applying it to "large fixed set
> defined by the system that can grow over time"? Any set of items that
> needs to be programmed one by one is not unbounded in the same sense. It
> is true that it can grow over time, but there is a practical limit on
> how many such options we would ever implement, and at any given time the
> set has a well-defined, finite number of members.
>
> I suppose that this is a reaction to Johannes's proposal [1] to add
> configuration settings like
>
>     git config fsck.badDate ignore

Not really.

This started after I looked at the add.ignore-errors issue discussed
and I was trying to codify existing practices by running greps,
blames and logs on 'master' to see what potential pitfalls are, and
what good existing practices to follow suit we already had.

The fsck changes were not in my mind at all.

I can see it argued that for things that are completely independent
(e.g. the consequence of setting fsck.badDate can never be affected
by how fsck.missingTagger is configured), separate configuration
variables may not be wrong per-se, but I can see that a set of knobs
that would have been originally independent, as the operation grow
richer, gain more semantics to make them related, and at that point,
I doubt "they are internally independent; expose them as independent
to the end users" argument would hold water that well.

A good example is "core.whitespace", where you would have started
with a simple set of booleans ("blank-at-eol" and "space-before-tab"
are conceptually independent and will stay to be), but once you
start supporting "indent-with-non-tab" and "tab-in-indent" you
cannot pretend that they are independent.

And that is the "existing practice" I primarily had in mind.  We
didn't do

	whitespace.tabInIndent = true
        whitespace.indentWithNonTab = true

to pretend they are independent and still internally having to make
sure the consistency of the setting.  We structured the syntax for
ease of the end user (not scripter) to shorter

	core.whitespace = tab-in-indent,indent-with-non-tab

as we need the consistency thing either way (and it is easier to see
the consistency mistakes when they appear next to each other).

And I am happy that we chose wisely in an early version that didn't
use one-variable-per-knob but used list-of-features-as-value instead.

> [2] which you didn't like [3] but I did [4]. (Did you miss [4], in which
> I think I made some good arguments for Johannes's proposal? I don't
> think you responded to it.)

I saw it, found it as an argument between "not good" and "weak" (see
above), kept in my mailbox while trying to decide if it was worth
responding, and then forgot about it after I got busy dealing with
other topics that have more relevance to the upcoming release ;-)

> I think it would be more productive to finish the concrete discussion
> about the "fsck" proposal,...

Surely.

 (0) fsck.bad-date is out.

 (1) fsck.badDate is OK.  I expect there is very high chance for
     them to stay independent forever.

 (2) fsck.ignore = bad-date,... is still my preference, only if it
     aligns with existing core.whitespace precedence that allows
     users to leverage the familiarity.

I see Peff cites "pager.<cmd>", but I think it was something that we
would rather shouldn't have done, similar to "alias.<cmd>".  They
are bad precedents we shouldn't encourage new things to mimic.

But that is not from "one-variable-with-list-is-better" (it is not
better for these "independent" ones) but is purely from the syntax
point of view.  There is no good reason to force case insensitivity
to the command names used as the key in these cases, but we do
because we made them the last-level variable names, and we have to
avoid hyphens in the command names while at it if we want to be
consistent (and the consistency was the point of the original
add.ignore-errors discussion that started all this, after all).

If they were

    pager.diff.enable = true
    alias.co.command = checkout

the world would have been a lot better place.