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

[Junio C Hamano <gitster@pobox.com>]

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

>> There was some discussion after v4. I think the open issues are:
>>
>>   - the commit message is rather terse (it should describe motivation,
>>     and can refer to the docs for the "how")

> This allows some more flexibility in managing configuration across
> repositories. 

That is not an ideal opening to describe motivation without people
knowing what "this" is ;-) Of course, the person who wrote the
sentence know it already after writing the patch and the subject,
but others don't.

	Sometimes a set of repositories want to share configuration
	settings among themselves that are distinct from other such
	sets of repositories.  A user may work on two projects, each
	of which have multiple repositories, and use one user.email
	for one project while using another for the other.  Having
	the settings in ~/.gitconfig, which would work for just one
	set of repositories, would not well in such a situation.

	Extend the include.path mechanism that lets a config file
	include another config file, so that the inclusion can be
	done only when some conditions hold.  Then ~/.gitconfig can
	say "include config-project-A only when working on project-A"
        for each project A the user works on.

        In this patch, the only supported grouping is based on
        $GIT_DIR (in absolute path), so you would need to group
        repositories by directory, or something like that to take
        advantage of it.

        We already have include.path for unconditional
        includes. This patch goes with include-if.xxx.path to make
        it clearer that a condition is required.

        Similar to include.path, older git versions that don't
        understand include-if will simply ignore them.

or something along that line?

> +Conditional includes
> +~~~~~~~~~~~~~~~~~~~~
> +
> +You can include one config file from another conditionally by setting
> +a special `include-if.<condition>.path` variable to the name of the
> +file to be included. The variable is treated the same way as
> +`include.path`.

Drop "special", as all configuration variables are "special" in their
own sense, it does not add any useful information.

I think we avoid '-' in Git-native variable and section names, so
"include-if" would become an odd-man-out.

The variable is obviously not treated the same way as include.path ;-)

    When includeIf.<condition>.path variable is set in a
    configuration file, the configuration file named by that
    variable is included (in a way similar to how include.path
    works) only if the <condition> holds true.

> +The condition starts with a keyword, followed by a colon and a
> +pattern. The interpretation of the pattern depends on the keyword.

"a pattern"?  I think it is "some data whose format and meaning
depends on the keyword".

Hence...

> +Supported keywords are:
> +
> +`gitdir`::
> +	The environment variable `GIT_DIR` must match the following
> +	pattern for files to be included. The pattern can contain

	The data that follows the keyword `gitdir:` is used as a
	glob pattern.  If the location of the .git directory (which
	may be auto-discovered, or come from `GIT_DIR` environment
	variable) match the pattern, the `<condition>` becomes true.

> + ...
> + * If the pattern ends with `/`, `**` will be automatically added. For
> +   example, the pattern `foo/` becomes `foo/**`. In other words, it
> +   matches "foo" and everything inside, recursively.
> +
> +`gitdir/i`::
> +	This is the same as `gitdir` except that matching is done
> +	case-insensitively (e.g. on case-insensitive file sytems)
> +
> +A few more notes on matching:

As future <keywords> may not be about path or matching at all, this
belongs to `gitdir`:: section (and it would be obvious that that
applies to `gitdir/i`:: because we say "this is the same as...").

> + * Symlinks in `$GIT_DIR` are not resolved before matching.
> +
> + * Note that "../" is not special and will match literally, which is
> +   unlikely what you want.

> +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"));

Instead of half-duplicating it here yourself, can't we let
expand_user_path() do its thing?

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

This may be OK for now, but it should be trivial to start from a
table with two entries, i.e.

	struct include_cond {
		const char *keyword;
		int (*fn)(const char *, size_t);
	};

and will show a better way to do things to those who follow your
footsteps.