Re: [PATCH v5 1/1] config: add conditional include

From: Jeff King <peff@peff.net>
Date: Mar 7, 2017, 08:47 UTC
Message-ID: <20170307084717.i2jru77v3rhd443e@sigill.intra.peff.net>
In-Reply-To: <CAGZ79kboaKfzMEyDjg-m2oK8CX6B56i52ZcWhCaq87ECE9x2Dw@mail.gmail.com>

On Mon, Mar 06, 2017 at 02:44:27PM -0800, Stefan Beller wrote:

Show 12 quoted lines

> > +static int include_condition_is_true(const char *cond, size_t cond_len)
> > +{
> ...
> > +
> > +       error(_("unrecognized include condition: %.*s"), (int)cond_len, cond);
> > +       /* unknown conditionals are always false */
> > +       return 0;
> > +}
> 
> Thanks for putting an error message here. I was looking at what
> is currently queued as origin/nd/conditional-config-include,
> which doesn't have this error()  (yet / not any more?)

It's "not any more". It was in the original and I asked for it to be removed during the last review.

Show 6 quoted lines

> I'd strongly suggest to keep the error message here as that way
> a user can diagnose e.g. a typo in the condition easily.
> 
> If we plan to extend this list of conditions in the future, and a user
> switches between versions of git, then they may see this message
> on a regular basis (whenever they use the 'old' version).

That would make it unlike the rest of the config-include mechanism (which quietly ignores things it doesn't understand, like include.foo, or include.foo.path), as well as the config code in general (which ignores misspelt keys).

Some of that "quiet when you don't understand it" is historical necessity. Older versions _can't_ complain about not knowing include.path, because they don't yet know it's worth complaining about. Likewise here, if this ships in v2.13 and a new condition "foo:" ships in v2.14, you get:

  v2.12 - no complaint; we don't even understand includeIf at all
  v2.13 - complain; we know includeIf, but not "foo:"
  v2.14 - works as expected

Which is kind of weird and inconsistent. But maybe the typo-detection case is more important to get right than consistency across historical versions.

-Peff