Re: [PATCH v2 2/2] config: add conditional include

[Jeff King <peff@peff.net>]

On Tue, Jun 28, 2016 at 07:26:41PM +0200, Nguyễn Thái Ngọc Duy wrote:

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 58673cf..c8ad0bf 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -91,6 +91,46 @@ found at the location of the include directive. If the value of the
>  relative to the configuration file in which the include directive was
>  found.  See below for examples.
>  
> +Included files can be grouped into subsections where subsectino name
> +is the condition when the files are included. The condition starts
> +with an condition type string, followed by a colon and a pattern.

s/subsectino/subsection/
s/an condition/a condition/

I think the first sentence may parse more easily as:

  Included files can be grouped into subsections, where the subsection
  name is a condition that must be met for the files to be included.

I wonder if it would make more sense to refer to "condition type string"
as a "condition keyword" or something.

I think we should probably also claim only that the bit to the right of
the colon is keyword-specific data. For some conditions it will not be a
glob, and may not even be a pattern at all. And then each keyword can
describe its syntax (we may end up wanting to factor out the particular
set of rules, but I think that can wait until we have a second keyword
that uses them).

> +Only "gitdir" type is supported, where files are included if
> +`$GIT_DIR` matches the specified pattern. For example,

Maybe start this as a list like:

    The currently supported keywords are:

    `gitdir`::
    ...

to make it clear that the "only" is potentially temporary. We should
probably also document that unknown keywords are always treated as
false.

> + * If the pattern starts with `~/`, `~` will be substitued with the
> +   environment variable `HOME`.

s/substitued/substituted/

> + * If the pattern starts with `./`, it is replaced with the directory
> +   where the current config file is. For example if the config file

Perhaps replace "directory where the current config file is" with
"directory containing the current config file". Also, perhaps "config
file that contains the include directive" is more descriptive than
"current" (we use similar language when describing relative include
paths).

> +static int prepare_include_condition_pattern(struct strbuf *pat)
> +{
> +	struct strbuf path = STRBUF_INIT;
> +	int prefix = 0;
> +
> +	/* TODO: maybe support ~user/ too */
> +	if (pat->buf[0] == '~' && is_dir_sep(pat->buf[1])) {
> +		const char *home = getenv("HOME");
> +
> +		if (!home)
> +			return error(_("$HOME is not defined"));
> +
> +		strbuf_add_absolute_path(&path, home);
> +		strbuf_splice(pat, 0, 1, path.buf, path.len);
> +		prefix = path.len + 1 /*slash*/;

Why did you drop expand_user_path() here?

I guess it's to do with knowing the length of the prefix portion? I'm
not sure I understand why the prefix is necessary. I thought the goal
was to construct a wildmatch pattern that could be used against
get_git_dir().

> +static int include_condition_is_true(const char *cond, int cond_len)
> +{
> +	const char *value;
> +	size_t value_len;
> +
> +	/* no condition (i.e., "include.path") is always true */
> +	if (!cond)
> +		return 1;
> +
> +	if (skip_prefix_mem(cond, cond_len, "gitdir:", &value, &value_len)) {

It's not wrong to use extra variables, but it's safe to feed the
originals, like:

  if (skip_prefix_mem(cond, cond_len, "gitdir:", &cond, &cond_len))

The variables are guaranteed to be untouched if we didn't match (so your
error() below is fine).

-Peff