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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 24, 2017, 18:35 UTC
Message-ID: <xmqqfuj34etq.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170224131425.32409-2-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 16 quoted lines

> +Conditional includes
> +~~~~~~~~~~~~~~~~~~~~
> +
> +You can include one config file from another conditionally by setting
> +a `includeIf.<condition>.path` variable to the name of the file to be
> +included. The variable's value is treated the same way as `include.path`.
> +
> +The condition starts with a keyword followed by a colon and some data
> +whose format and meaning depends on the keyword. Supported keywords
> +are:
> +
> +`gitdir`::
> +
> +	The data that follows the keyword `gitdir:` is used as a glob
> +	pattern. If the location of the .git directory match the
> +	pattern, the include condition is met.

s/match/&es/, I think.

> ++
> +The .git location which may be auto-discovered, or come from

s/ which//?

> +`$GIT_DIR` environment variable. If the repository auto discovered via

s/If the /In a/?

> +a .git file (e.g. from submodules, or a linked worktree), the .git
> +location would be the final location, not where the .git file is.

OK.

Show 9 quoted lines

> @@ -170,9 +171,99 @@ static int handle_path_include(const char *path, struct config_include_data *inc
>  	return ret;
>  }
>  
> +static int prepare_include_condition_pattern(struct strbuf *pat)
> +{
> + ...
> +		/* TODO: escape wildcards */
> +		strbuf_add_absolute_path(&path, cf->path);

Is this still TODO? As this one returns the prefix length (which probably wants to be commented before the function) and this codepath computes the prefix to cover the path to here, doesn't caller already do the right thing?

> +static int include_condition_is_true(const char *cond, size_t cond_len)
> +{
> +	/* no condition (i.e., "include.path") is always true */

Does this want to say "includeIf.path" instead? "include.path" is done by handle_path_include() without involving a call to this function.

Show 12 quoted lines

> +	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;
> +}

Thanks.