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

From: Jeff King <peff@peff.net>
Date: Feb 1, 2015, 21:57 UTC
Message-ID: <20150201215729.GA19692@peff.net>
In-Reply-To: <xmqq1tm99yhx.fsf@gitster.dls.corp.google.com>

On Sun, Feb 01, 2015 at 12:18:34PM -0800, Junio C Hamano wrote:

Show 8 quoted lines

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

I'm not sure I buy this argument. There are two syntaxes being discussed here. Let's look at each.

Let's call this "type 1":

  [core]
  foo = bar -moof xyzzy=whatever

We have a single variable, but the value of that variable is essentially a list of sub-variables. The sub-variables are either booleans (like "bar" or "-moof") or possibly have values of their own (with an "="). The sub-variables are tied together logically by being part of a single "core.foo". Presumably the user can infer some relationship between them through this.

Type 2 is more like:

  [foo]
  bar = true
  moof = false
  xyzzy = whatever

So we still have our same set of sub-variables, except now each is a first-class config variable. They are tied together logically by being part of a single section (I called it "foo" here, which drops the "core"; but clearly we could give it whatever descriptive name we wanted).

>From the user's perspective, I don't see how the implied relationships

are significantly different. In type 1, they are placed inside a single value and in type 2, they are not. Both are a form of grouping.

Moreover, I am not even sure that the syntax is an important element in communicating semantic relationships. In these examples, are "bar" and "moof" dependent? Clearly they are _related_ by our grouping. But does one depend on the other? Similarly, with the existing core.whitespace, what tells you which of the sub-variables are related. The "blank-at-eol" and "space-before-tab" variables are both whitespace-related, but do not depend on each other at all. But "indent-with-non-tab" and "tab-in-indent" are. Yet those two pairs are represented the same way syntactically.

I don't think you can infer semantic independence from syntax. It's simply too blunt an instrument (and as you noted, it may even change over time).

Show 7 quoted lines

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

I agree that this provides a slight advantage to "type 1", because it requires syntactically that the definitions are near each other (whereas with split variables, they can literally be in different files). And if everything else were equal, that would be enough. But I think "type 1" has a lot of other disadvantages. For example:

  1. I'm a user who has set my preferred core.whitespace in my
     ~/.gitconfig. A particular project I am working on uses an
     alternate tabwidth. How do I set that in the repo config without
     repeating my defaults?

  2. I'm writing a hook whose behavior depends on the whitespace
     settings. How do I ask git whether blank-at-eol is enabled?

  3. I'm a user who wants to set whitespace config. I prefer using "git
     config" to editing the file manually. How do I turn off
     blank-at-eol without disrupting my existing settings?

An astute reader will notice that case (1) is a double-edged sword. If your defaults have "blank-at-eol" and you want to set "indent-with-non-tab" in the repo, that's fine. If the default is "tab-in-indent" and you want to set "indent-with-non-tab", then those are in conflict (i.e., this is the exact thing you were complaining about above).

But is it such a bad thing to have them in conflict? Can't we define a set of rules that does what people expects? For example, by the "last one wins" principle, any time we see "whitespace.tab-in-indent", it clears the setting of "whitespace.indent-with-non-tab", and vice versa. This isn't represented syntactically in the config file, but it is an easy rule, does what people would want, and can be documented in config.txt (which is where we have to talk about such semantic conflicts anyway).

By the way, this is the exact case I am concerned about for fsck.*. Our use case at GitHub would be something like:

  a. We set up sane defaults for fsck.* in /etc/gitconfig

  b. User complains that we will not accept their push, which contains
     objects with malformed committers.

  c. Support investigates, determines that the malformed objects are
     part of a well-established history, and that they are OK to enter.

  d. We relax fsck.committerIdent in that repo's $GIT_DIR/config file.

Copy-and-pasting the rest of the rules from (a) into the repo config file in step (d) is not ideal.

Show 7 quoted lines

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

Yeah, I'd agree that the problem there is orthogonal to the type 1/2 thing above. I don't think it has been a big deal in practice, just because people with good taste do not name their commands with uppercase anyway.

I'd be happy to transition to pager.*.enabled, etc, if we care. That is a much more flexible system anyway, and I do not think there is any backwards compatibility problem (we'd continue with alias.X as a synonym for alias.X.command indefinitely). It would not even be that much work; it is mostly that I have never seen anybody complain about it in the real world.

-Peff