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

On Tue, Mar 7, 2017 at 12:47 AM, Jeff King [off-list ref] wrote:

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

quoted

quoted

+static int include_condition_is_true(const char *cond, size_t cond_len)
+{

...

quoted

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

Okay. The joys of contradicting opinions on a mailing list. :)

quoted

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.

agreed

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.

Oh, I see. I was contemplating a future in which 2.12 is not used anymore.

When looking at other examples, such as url.<...>.insteadOf we also do not
warn about typos (well we can't actually).

In diff.<driver>.(command/binary/..) we know the limited set of drivers,
which is similar to the situation we have here.

Maybe a compromise between typo checking (edit distance < 2 -> warn;
silent for larger distances) and the consistency over time is desired.
But this is even more code to write.

So for now I retract my strong opinion and be happy with what is
presented as-is for the reasons Peff gave.

Thanks,
Stefan