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

[Jeff King <peff@peff.net>]

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

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

> 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