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

[Jeff King <peff@peff.net>]

On Sun, Feb 01, 2015 at 06:12:38AM +0100, Michael Haggerty 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 had the same reaction on reading this.

We should be striving to break config options down as much as possible
to single scalar values, because that is the only format that is
understood systematically by the config code.

If a config option's value is a list, then we have to come up with an
ad-hoc syntax for the list, which we parse in the config callback. And
that leaves users of "git config" to reinvent that parsing themselves
when they want to do simple things like "remove item B from the list".
I think the examples you gave over on the fsck thread all make the same
point.

> > + [...] Use
> > +   subsection names or variable values, like existing variables
> > +   branch.<name>.description and core.whitespace do, instead.
> 
> But there is also a precedent for the opposite approach: "advice.*".

The pager.*, color.* (and color.$program.*) examples come to mind. For
example, we did not add:

  [core]
  usePagerFor = log, diff, -status

but instead:

  [pager]
  log = true
  diff = true
  status = false

Not only is the latter easier to manipulate and examine with the
existing config tools, I think it is more flexible in the long run. We
later extended the syntax to allow:

  [pager]
  log = diff-highlight | less

which would have been even more awkward in the "userPagerFor" format
(you could use "log=...", of course, but now you need to get into
whitespace quoting and other complexities, all of which are handled
already by the config code in the latter case).

-Peff