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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 1, 2015, 20:18 UTC
Message-ID: <xmqq1tm99yhx.fsf@gitster.dls.corp.google.com>
In-Reply-To: <54CDB5C6.3020702@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 21 quoted lines

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