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

From: Jeff King <peff@peff.net>
Date: Feb 26, 2017, 06:12 UTC
Message-ID: <20170226061220.nizc4xeucx6ks6vq@sigill.intra.peff.net>
In-Reply-To: <20170224131425.32409-4-pclouds@gmail.com>

On Fri, Feb 24, 2017 at 08:14:25PM +0700, Nguyễn Thái Ngọc Duy wrote:

Show 15 quoted lines

> +static int include_condition_is_true(const char *cond, size_t cond_len)
> +{
> +	/* no condition (i.e., "include.path") is always true */
> +	if (!cond)
> +		return 1;
> +
> +	if (skip_prefix_mem(cond, cond_len, "gitdir:", &cond, &cond_len))
> +		return include_by_gitdir(cond, cond_len, 0);
> +	else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len))
> +		return include_by_gitdir(cond, cond_len, 1);
> +
> +	error(_("unrecognized include condition: %.*s"), (int)cond_len, cond);
> +	/* unknown conditionals are always false */
> +	return 0;
> +}

This "error()" is a nice warning to people who have misspelled the condition (or are surprised that an old version does not respect their condition). But it seems out of place with the rest of the compatibility strategy, which is to quietly ignore include types we don't understand.

For example, if your patch ships in v2.13 and we add the "foo:" condition in v2.14, then with config like:

  [includeif "foo:bar"]
  path = whatever

you get:

  v2.12: silently ignore
  v2.13: loudly ignore
  v2.14: works

which seems odd.

If we do keep it, it should probably be warning(). I would expect "error:" to indicate that some requested operation could not be completed, but this is just "by the way, I ignored this garbage".

-Peff

PS I know we try to avoid dashes in the section names, but "includeIf"
   and "includeif" just look really ugly to me. Maybe it is just me,
   though.