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

[Stefan Beller <sbeller@google.com>]

Being late to the review party here.

> +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?)

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

In that case it may be only a warning() instead of an error(),
but I have no strong opinion on that.

---
Reason why I am reviewing this series now:

I thought about extending the config system for submodule
usage (see debate at [1]).

My gut reaction was to have a condition for "if a superproject
exists" and then include a special config (e.g. "config.super"
in the superprojects $GIT_DIR).

However while reviewing these patches I realized
I am not interested in conditional includes, but when setting
up the submodules we know for a fact that we have a superproject,
so no conditional needed. Instead we need a special markup
of paths, i.e. we want to have an easy way to say
"$GIT_DIR of superproject". Ideas how to do that?

Thanks,
Stefan

[1] https://public-inbox.org/git/84fcb0bd-85dc-0142-dd58-47a04eaa7c2b@durchholz.org/