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

From: Stefan Beller <sbeller@google.com>
Date: Mar 6, 2017, 22:44 UTC
Message-ID: <CAGZ79kboaKfzMEyDjg-m2oK8CX6B56i52ZcWhCaq87ECE9x2Dw@mail.gmail.com>
In-Reply-To: <20170223122346.12222-2-pclouds@gmail.com>

Being late to the review party here.

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

...

Show 5 quoted lines

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