[PATCH v5 0/1] Conditional config include

[PATCH v5 0/1] Conditional config include

[Nguyễn Thái Ngọc Duy]

Let's try this again. v4 and before can be found in the original
thread [1]. The remaining issues of v4 were

On Fri, Aug 19, 2016 at 8:54 PM, Jeff King <peff@peff.net> wrote:
> On Sat, Aug 13, 2016 at 03:40:59PM +0700, Duy Nguyen wrote:
>
>> Ping..
>
> 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")

Fixed

>
>   - the syntax might be more clear as:
>
>        [include-if "gitdir:..."]
>
>     or
>
>        [include "gitdir-is:..."]

I'll go with include-if. It feels a bit awkward to add -is to every
new type of condition.

>   - there is an open question of whether we would like to go this route,
>     maintaining backwards compatibility syntactically (and thus having
>     these includes quietly skipped in older versions), or introduce a
>     breaking syntax that could be more convenient, like:
>
>       [if-gitdir(...):section]
>       conditional-but-no-include-necessary = true
>
>     I do not have a strong opinion myself, though I had written the
>     original [include] assuming syntactic compatibility, and I think it
>     has worked out (e.g., you can manipulate include.path via "git
>     config" just as you would any other key).

I'll go with the what we have done, at least it's proven not provoking
too bad effects. So no breaking syntax.

[1] http://public-inbox.org/git/20160714153311.2166-1-pclouds@gmail.com/

Nguyễn Thái Ngọc Duy (1):
  config: add conditional include

 Documentation/config.txt  |  54 +++++++++++++++++++++++++
 config.c                  | 100 ++++++++++++++++++++++++++++++++++++++++++++++
 t/t1305-config-include.sh |  56 ++++++++++++++++++++++++++
 3 files changed, 210 insertions(+)

-- 
2.11.0.157.gd943d85

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

[Nguyễn Thái Ngọc Duy]

This allows some more flexibility in managing configuration across
repositories. The most often seen use case on the mailing list is when
the user needs to use different email addresses on different
repositories. If these repositories share something that we can use to
group them up, then we can set the same configuration for groups
automatically.

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.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/config.txt  |  54 +++++++++++++++++++++++++
 config.c                  | 100 ++++++++++++++++++++++++++++++++++++++++++++++
 t/t1305-config-include.sh |  56 ++++++++++++++++++++++++++
 3 files changed, 210 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 015346c417..8cadf2b776 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -91,6 +91,49 @@ 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.
 
+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`.
+
+The condition starts with a keyword, followed by a colon and a
+pattern. The interpretation of the pattern depends on the keyword.
+Supported keywords are:
+
+`gitdir`::
+	The environment variable `GIT_DIR` must match the following
+	pattern for files to be included. The pattern can contain
+	standard globbing wildcards and two additional ones, `**/` and
+	`/**`, that can match multiple path components. Please refer
+	to linkgit:gitignore[5] for details. For convenience:
+
+ * If the pattern starts with `~/`, `~` will be substituted with the
+   content of the environment variable `HOME`.
+
+ * If the pattern starts with `./`, it is replaced with the directory
+   containing the current config file.
+
+ * If the pattern does not start with either `~/`, `./` or `/`, `**/`
+   will be automatically prepended. For example, the pattern `foo/bar`
+   becomes `**/foo/bar` and would match `/any/path/to/foo/bar`.
+
+ * 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:
+
+ * Symlinks in `$GIT_DIR` are not resolved before matching.
+
+ * Note that "../" is not special and will match literally, which is
+   unlikely what you want.
 
 Example
 ~~~~~~~
@@ -119,6 +162,17 @@ Example
 		path = foo ; expand "foo" relative to the current file
 		path = ~/foo ; expand "foo" in your `$HOME` directory
 
+	; include if $GIT_DIR is /path/to/foo/.git
+	[include-if "gitdir:/path/to/foo/.git"]
+		path = /path/to/foo.inc
+
+	; include for all repositories inside /path/to/group
+	[include-if "gitdir:/path/to/group/"]
+		path = /path/to/foo.inc
+
+	; include for all repositories inside $HOME/to/group
+	[include-if "gitdir:~/to/group/"]
+		path = /path/to/foo.inc
 
 Values
 ~~~~~~
diff --git a/config.c b/config.c
index c6b874a7bf..3090fbf1a8 100644
--- a/config.c
+++ b/config.c
@@ -13,6 +13,7 @@
 #include "hashmap.h"
 #include "string-list.h"
 #include "utf8.h"
+#include "dir.h"
 
 struct config_source {
 	struct config_source *prev;
@@ -170,9 +171,101 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 	return ret;
 }
 
+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*/;
+	} else if (pat->buf[0] == '.' && is_dir_sep(pat->buf[1])) {
+		const char *slash;
+
+		if (!cf || !cf->path)
+			return error(_("relative config include "
+				       "conditionals must come from files"));
+
+		/* TODO: escape wildcards */
+		strbuf_add_absolute_path(&path, cf->path);
+		slash = find_last_dir_sep(path.buf);
+		if (!slash)
+			die("BUG: how is this possible?");
+		strbuf_splice(pat, 0, 1, path.buf, slash - path.buf);
+		prefix = slash - path.buf + 1 /* slash */;
+	} else if (!is_absolute_path(pat->buf))
+		strbuf_insert(pat, 0, "**/", 3);
+
+	if (pat->len && is_dir_sep(pat->buf[pat->len - 1]))
+		strbuf_addstr(pat, "**");
+
+	strbuf_release(&path);
+	return prefix;
+}
+
+static int include_by_gitdir(const char *cond, size_t cond_len, int icase)
+{
+	struct strbuf text = STRBUF_INIT;
+	struct strbuf pattern = STRBUF_INIT;
+	int ret = 0, prefix;
+
+	strbuf_add_absolute_path(&text, get_git_dir());
+	strbuf_add(&pattern, cond, cond_len);
+	prefix = prepare_include_condition_pattern(&pattern);
+
+	if (prefix < 0)
+		goto done;
+
+	if (prefix > 0) {
+		/*
+		 * perform literal matching on the prefix part so that
+		 * any wildcard character in it can't create side effects.
+		 */
+		if (text.len < prefix)
+			goto done;
+		if (!icase && strncmp(pattern.buf, text.buf, prefix))
+			goto done;
+		if (icase && strncasecmp(pattern.buf, text.buf, prefix))
+			goto done;
+	}
+
+	ret = !wildmatch(pattern.buf + prefix, text.buf + prefix,
+			 icase ? WM_CASEFOLD : 0, NULL);
+
+done:
+	strbuf_release(&pattern);
+	strbuf_release(&text);
+	return ret;
+}
+
+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;
+}
+
 int git_config_include(const char *var, const char *value, void *data)
 {
 	struct config_include_data *inc = data;
+	const char *cond, *key;
+	int cond_len;
 	int ret;
 
 	/*
@@ -185,6 +278,13 @@ int git_config_include(const char *var, const char *value, void *data)
 
 	if (!strcmp(var, "include.path"))
 		ret = handle_path_include(value, inc);
+
+	if (!parse_config_key(var, "include-if", &cond, &cond_len, &key) &&
+	    include_condition_is_true(cond, cond_len)) {
+		if (!strcmp(key, "path"))
+			ret = handle_path_include(value, inc);
+		/* else we do not know about this type of include; ignore */
+	}
 	return ret;
 }
 
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index 9ba2ba11c3..d5b586e270 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -152,6 +152,62 @@ test_expect_success 'relative includes from stdin line fail' '
 	test_must_fail git config --file - test.one
 '
 
+test_expect_success 'conditional include, both unanchored' '
+	git init foo &&
+	(
+		cd foo &&
+		echo "[include-if \"gitdir:foo/\"]path=bar" >>.git/config &&
+		echo "[test]one=1" >.git/bar &&
+		echo 1 >expect &&
+		git config test.one >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'conditional include, $HOME expansion' '
+	(
+		cd foo &&
+		echo "[include-if \"gitdir:~/foo/\"]path=bar2" >>.git/config &&
+		echo "[test]two=2" >.git/bar2 &&
+		echo 2 >expect &&
+		git config test.two >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'conditional include, full pattern' '
+	(
+		cd foo &&
+		echo "[include-if \"gitdir:**/foo/**\"]path=bar3" >>.git/config &&
+		echo "[test]three=3" >.git/bar3 &&
+		echo 3 >expect &&
+		git config test.three >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'conditional include, relative path' '
+	echo "[include-if \"gitdir:./foo/.git\"]path=bar4" >>.gitconfig &&
+	echo "[test]four=4" >bar4 &&
+	(
+		cd foo &&
+		echo 4 >expect &&
+		git config test.four >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'conditional include, both unanchored, icase' '
+	(
+		cd foo &&
+		echo "[include-if \"gitdir/i:FOO/\"]path=bar5" >>.git/config &&
+		echo "[test]five=5" >.git/bar5 &&
+		echo 5 >expect &&
+		git config test.five >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_expect_success 'include cycles are detected' '
 	cat >.gitconfig <<-\EOF &&
 	[test]value = gitconfig
-- 
2.11.0.157.gd943d85

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

[Junio C Hamano]

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.

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

[Duy Nguyen]

On Fri, Feb 24, 2017 at 2:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

Yeah. I was thinking "value" and writing "variable" instead (or
perhaps I meant to write "variable value" and accidentally'd the last
word again).

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

This comment came up in previous iterations. I perform explicit
expansion to make sure we don't apply wildcards on the expanded "~".
But I guess going with expand_user_path() is better (less confusion
for future readers). We can come back to it when that "don't apply
wildcards" concern becomes real.

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

Yeah I don't see a third include coming soon and did not go with that.
Let's way for it and refactor then.
-- 
Duy

[PATCH v6 0/1] Conditional config include

[Nguyễn Thái Ngọc Duy]

v6 has lots of text changes, most of which I shamelessly copied from
Junio's, with some minor edits. The biggest edit is the mention of .git
files, which is probably a good idea since .git files are used by
multi worktrees and submodules.

I could not move the "notes about matching" block into the gitdir
block though. I needed to indent it once (not twice like the "for
convenience" block) but my asciidoc-foo did not manage it so I settled
for "a few more notes on matching _with gitdir_" without moving it.

Code changes include renaming include-if to includeIf, and using
expand_user_path() instead of rolling my own.

Nguyễn Thái Ngọc Duy (1):
  config: add conditional include

 Documentation/config.txt  | 61 +++++++++++++++++++++++++++++
 config.c                  | 97 +++++++++++++++++++++++++++++++++++++++++++++++
 t/t1305-config-include.sh | 56 +++++++++++++++++++++++++++
 3 files changed, 214 insertions(+)

-- 
2.11.0.157.gd943d85

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

[Nguyễn Thái Ngọc Duy]

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.

Setting $GIT_DIR/.config works, but if the penalty of forgetting to
update $GIT_DIR/.config is high (especially when you end up cloning
often), it may not be the best way to go. Having the settings in
~/.gitconfig, which would work for just one set of repositories, would
not well in such a situation. Having separate ${HOME}s may add more
problems than it solves.

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 includeIf.<condition>.path to make it clearer that a condition is
required. The new config has the same backward compatibility approach as
include.path: older git versions that don't understand includeIf will
simply ignore them.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/config.txt  | 61 +++++++++++++++++++++++++++++
 config.c                  | 97 +++++++++++++++++++++++++++++++++++++++++++++++
 t/t1305-config-include.sh | 56 +++++++++++++++++++++++++++
 3 files changed, 214 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 015346c417..6c0cd2a273 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -91,6 +91,56 @@ 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.
 
+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.
++
+The .git location which may be auto-discovered, or come from
+`$GIT_DIR` environment variable. If the repository auto discovered via
+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.
++
+The pattern can contain standard globbing wildcards and two additional
+ones, `**/` and `/**`, that can match multiple path components. Please
+refer to linkgit:gitignore[5] for details. For convenience:
+
+ * If the pattern starts with `~/`, `~` will be substituted with the
+   content of the environment variable `HOME`.
+
+ * If the pattern starts with `./`, it is replaced with the directory
+   containing the current config file.
+
+ * If the pattern does not start with either `~/`, `./` or `/`, `**/`
+   will be automatically prepended. For example, the pattern `foo/bar`
+   becomes `**/foo/bar` and would match `/any/path/to/foo/bar`.
+
+ * 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 with `gitdir` and `gitdir/i`:
+
+ * Symlinks in `$GIT_DIR` are not resolved before matching.
+
+ * Note that "../" is not special and will match literally, which is
+   unlikely what you want.
 
 Example
 ~~~~~~~
@@ -119,6 +169,17 @@ Example
 		path = foo ; expand "foo" relative to the current file
 		path = ~/foo ; expand "foo" in your `$HOME` directory
 
+	; include if $GIT_DIR is /path/to/foo/.git
+	[include-if "gitdir:/path/to/foo/.git"]
+		path = /path/to/foo.inc
+
+	; include for all repositories inside /path/to/group
+	[include-if "gitdir:/path/to/group/"]
+		path = /path/to/foo.inc
+
+	; include for all repositories inside $HOME/to/group
+	[include-if "gitdir:~/to/group/"]
+		path = /path/to/foo.inc
 
 Values
 ~~~~~~
diff --git a/config.c b/config.c
index c6b874a7bf..ad16802c8a 100644
--- a/config.c
+++ b/config.c
@@ -13,6 +13,7 @@
 #include "hashmap.h"
 #include "string-list.h"
 #include "utf8.h"
+#include "dir.h"
 
 struct config_source {
 	struct config_source *prev;
@@ -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)
+{
+	struct strbuf path = STRBUF_INIT;
+	char *expanded;
+	int prefix = 0;
+
+	expanded = expand_user_path(pat->buf);
+	if (expanded) {
+		strbuf_reset(pat);
+		strbuf_addstr(pat, expanded);
+		free(expanded);
+	}
+
+	if (pat->buf[0] == '.' && is_dir_sep(pat->buf[1])) {
+		const char *slash;
+
+		if (!cf || !cf->path)
+			return error(_("relative config include "
+				       "conditionals must come from files"));
+
+		/* TODO: escape wildcards */
+		strbuf_add_absolute_path(&path, cf->path);
+		slash = find_last_dir_sep(path.buf);
+		if (!slash)
+			die("BUG: how is this possible?");
+		strbuf_splice(pat, 0, 1, path.buf, slash - path.buf);
+		prefix = slash - path.buf + 1 /* slash */;
+	} else if (!is_absolute_path(pat->buf))
+		strbuf_insert(pat, 0, "**/", 3);
+
+	if (pat->len && is_dir_sep(pat->buf[pat->len - 1]))
+		strbuf_addstr(pat, "**");
+
+	strbuf_release(&path);
+	return prefix;
+}
+
+static int include_by_gitdir(const char *cond, size_t cond_len, int icase)
+{
+	struct strbuf text = STRBUF_INIT;
+	struct strbuf pattern = STRBUF_INIT;
+	int ret = 0, prefix;
+
+	strbuf_add_absolute_path(&text, get_git_dir());
+	strbuf_add(&pattern, cond, cond_len);
+	prefix = prepare_include_condition_pattern(&pattern);
+
+	if (prefix < 0)
+		goto done;
+
+	if (prefix > 0) {
+		/*
+		 * perform literal matching on the prefix part so that
+		 * any wildcard character in it can't create side effects.
+		 */
+		if (text.len < prefix)
+			goto done;
+		if (!icase && strncmp(pattern.buf, text.buf, prefix))
+			goto done;
+		if (icase && strncasecmp(pattern.buf, text.buf, prefix))
+			goto done;
+	}
+
+	ret = !wildmatch(pattern.buf + prefix, text.buf + prefix,
+			 icase ? WM_CASEFOLD : 0, NULL);
+
+done:
+	strbuf_release(&pattern);
+	strbuf_release(&text);
+	return ret;
+}
+
+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;
+}
+
 int git_config_include(const char *var, const char *value, void *data)
 {
 	struct config_include_data *inc = data;
+	const char *cond, *key;
+	int cond_len;
 	int ret;
 
 	/*
@@ -185,6 +276,12 @@ int git_config_include(const char *var, const char *value, void *data)
 
 	if (!strcmp(var, "include.path"))
 		ret = handle_path_include(value, inc);
+
+	if (!parse_config_key(var, "includeif", &cond, &cond_len, &key) &&
+	    include_condition_is_true(cond, cond_len) &&
+	    !strcmp(key, "path"))
+		ret = handle_path_include(value, inc);
+
 	return ret;
 }
 
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index 9ba2ba11c3..f0cd2056ba 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -152,6 +152,62 @@ test_expect_success 'relative includes from stdin line fail' '
 	test_must_fail git config --file - test.one
 '
 
+test_expect_success 'conditional include, both unanchored' '
+	git init foo &&
+	(
+		cd foo &&
+		echo "[includeIf \"gitdir:foo/\"]path=bar" >>.git/config &&
+		echo "[test]one=1" >.git/bar &&
+		echo 1 >expect &&
+		git config test.one >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'conditional include, $HOME expansion' '
+	(
+		cd foo &&
+		echo "[includeIf \"gitdir:~/foo/\"]path=bar2" >>.git/config &&
+		echo "[test]two=2" >.git/bar2 &&
+		echo 2 >expect &&
+		git config test.two >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'conditional include, full pattern' '
+	(
+		cd foo &&
+		echo "[includeIf \"gitdir:**/foo/**\"]path=bar3" >>.git/config &&
+		echo "[test]three=3" >.git/bar3 &&
+		echo 3 >expect &&
+		git config test.three >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'conditional include, relative path' '
+	echo "[includeIf \"gitdir:./foo/.git\"]path=bar4" >>.gitconfig &&
+	echo "[test]four=4" >bar4 &&
+	(
+		cd foo &&
+		echo 4 >expect &&
+		git config test.four >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'conditional include, both unanchored, icase' '
+	(
+		cd foo &&
+		echo "[includeIf \"gitdir/i:FOO/\"]path=bar5" >>.git/config &&
+		echo "[test]five=5" >.git/bar5 &&
+		echo 5 >expect &&
+		git config test.five >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_expect_success 'include cycles are detected' '
 	cat >.gitconfig <<-\EOF &&
 	[test]value = gitconfig
-- 
2.11.0.157.gd943d85

[PATCH v6 0/1] Conditional config include

[Nguyễn Thái Ngọc Duy]

v6 has lots of text changes, most of which I shamelessly copied from
Junio's, with some minor edits. The biggest edit is the mention of .git
files, which is probably a good idea since .git files are used by
multi worktrees and submodules.

I could not move the "notes about matching" block into the gitdir
block though. I needed to indent it once (not twice like the "for
convenience" block) but my asciidoc-foo did not manage it so I settled
for "a few more notes on matching _with gitdir_" without moving it.

Code changes include renaming include-if to includeIf, and using
expand_user_path() instead of rolling my own.

Nguyễn Thái Ngọc Duy (1):
  config: add conditional include

 Documentation/config.txt  | 61 +++++++++++++++++++++++++++++
 config.c                  | 97 +++++++++++++++++++++++++++++++++++++++++++++++
 t/t1305-config-include.sh | 56 +++++++++++++++++++++++++++
 3 files changed, 214 insertions(+)

-- 
2.11.0.157.gd943d85

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

[Nguyễn Thái Ngọc Duy]

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.

Setting $GIT_DIR/.config works, but if the penalty of forgetting to
update $GIT_DIR/.config is high (especially when you end up cloning
often), it may not be the best way to go. Having the settings in
~/.gitconfig, which would work for just one set of repositories, would
not well in such a situation. Having separate ${HOME}s may add more
problems than it solves.

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 includeIf.<condition>.path to make it clearer that a condition is
required. The new config has the same backward compatibility approach as
include.path: older git versions that don't understand includeIf will
simply ignore them.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/config.txt  | 61 +++++++++++++++++++++++++++++
 config.c                  | 97 +++++++++++++++++++++++++++++++++++++++++++++++
 t/t1305-config-include.sh | 56 +++++++++++++++++++++++++++
 3 files changed, 214 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 015346c417..6c0cd2a273 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -91,6 +91,56 @@ 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.
 
+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.
++
+The .git location which may be auto-discovered, or come from
+`$GIT_DIR` environment variable. If the repository auto discovered via
+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.
++
+The pattern can contain standard globbing wildcards and two additional
+ones, `**/` and `/**`, that can match multiple path components. Please
+refer to linkgit:gitignore[5] for details. For convenience:
+
+ * If the pattern starts with `~/`, `~` will be substituted with the
+   content of the environment variable `HOME`.
+
+ * If the pattern starts with `./`, it is replaced with the directory
+   containing the current config file.
+
+ * If the pattern does not start with either `~/`, `./` or `/`, `**/`
+   will be automatically prepended. For example, the pattern `foo/bar`
+   becomes `**/foo/bar` and would match `/any/path/to/foo/bar`.
+
+ * 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 with `gitdir` and `gitdir/i`:
+
+ * Symlinks in `$GIT_DIR` are not resolved before matching.
+
+ * Note that "../" is not special and will match literally, which is
+   unlikely what you want.
 
 Example
 ~~~~~~~
@@ -119,6 +169,17 @@ Example
 		path = foo ; expand "foo" relative to the current file
 		path = ~/foo ; expand "foo" in your `$HOME` directory
 
+	; include if $GIT_DIR is /path/to/foo/.git
+	[include-if "gitdir:/path/to/foo/.git"]
+		path = /path/to/foo.inc
+
+	; include for all repositories inside /path/to/group
+	[include-if "gitdir:/path/to/group/"]
+		path = /path/to/foo.inc
+
+	; include for all repositories inside $HOME/to/group
+	[include-if "gitdir:~/to/group/"]
+		path = /path/to/foo.inc
 
 Values
 ~~~~~~
diff --git a/config.c b/config.c
index c6b874a7bf..ad16802c8a 100644
--- a/config.c
+++ b/config.c
@@ -13,6 +13,7 @@
 #include "hashmap.h"
 #include "string-list.h"
 #include "utf8.h"
+#include "dir.h"
 
 struct config_source {
 	struct config_source *prev;
@@ -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)
+{
+	struct strbuf path = STRBUF_INIT;
+	char *expanded;
+	int prefix = 0;
+
+	expanded = expand_user_path(pat->buf);
+	if (expanded) {
+		strbuf_reset(pat);
+		strbuf_addstr(pat, expanded);
+		free(expanded);
+	}
+
+	if (pat->buf[0] == '.' && is_dir_sep(pat->buf[1])) {
+		const char *slash;
+
+		if (!cf || !cf->path)
+			return error(_("relative config include "
+				       "conditionals must come from files"));
+
+		/* TODO: escape wildcards */
+		strbuf_add_absolute_path(&path, cf->path);
+		slash = find_last_dir_sep(path.buf);
+		if (!slash)
+			die("BUG: how is this possible?");
+		strbuf_splice(pat, 0, 1, path.buf, slash - path.buf);
+		prefix = slash - path.buf + 1 /* slash */;
+	} else if (!is_absolute_path(pat->buf))
+		strbuf_insert(pat, 0, "**/", 3);
+
+	if (pat->len && is_dir_sep(pat->buf[pat->len - 1]))
+		strbuf_addstr(pat, "**");
+
+	strbuf_release(&path);
+	return prefix;
+}
+
+static int include_by_gitdir(const char *cond, size_t cond_len, int icase)
+{
+	struct strbuf text = STRBUF_INIT;
+	struct strbuf pattern = STRBUF_INIT;
+	int ret = 0, prefix;
+
+	strbuf_add_absolute_path(&text, get_git_dir());
+	strbuf_add(&pattern, cond, cond_len);
+	prefix = prepare_include_condition_pattern(&pattern);
+
+	if (prefix < 0)
+		goto done;
+
+	if (prefix > 0) {
+		/*
+		 * perform literal matching on the prefix part so that
+		 * any wildcard character in it can't create side effects.
+		 */
+		if (text.len < prefix)
+			goto done;
+		if (!icase && strncmp(pattern.buf, text.buf, prefix))
+			goto done;
+		if (icase && strncasecmp(pattern.buf, text.buf, prefix))
+			goto done;
+	}
+
+	ret = !wildmatch(pattern.buf + prefix, text.buf + prefix,
+			 icase ? WM_CASEFOLD : 0, NULL);
+
+done:
+	strbuf_release(&pattern);
+	strbuf_release(&text);
+	return ret;
+}
+
+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;
+}
+
 int git_config_include(const char *var, const char *value, void *data)
 {
 	struct config_include_data *inc = data;
+	const char *cond, *key;
+	int cond_len;
 	int ret;
 
 	/*
@@ -185,6 +276,12 @@ int git_config_include(const char *var, const char *value, void *data)
 
 	if (!strcmp(var, "include.path"))
 		ret = handle_path_include(value, inc);
+
+	if (!parse_config_key(var, "includeif", &cond, &cond_len, &key) &&
+	    include_condition_is_true(cond, cond_len) &&
+	    !strcmp(key, "path"))
+		ret = handle_path_include(value, inc);
+
 	return ret;
 }
 
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index 9ba2ba11c3..f0cd2056ba 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -152,6 +152,62 @@ test_expect_success 'relative includes from stdin line fail' '
 	test_must_fail git config --file - test.one
 '
 
+test_expect_success 'conditional include, both unanchored' '
+	git init foo &&
+	(
+		cd foo &&
+		echo "[includeIf \"gitdir:foo/\"]path=bar" >>.git/config &&
+		echo "[test]one=1" >.git/bar &&
+		echo 1 >expect &&
+		git config test.one >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'conditional include, $HOME expansion' '
+	(
+		cd foo &&
+		echo "[includeIf \"gitdir:~/foo/\"]path=bar2" >>.git/config &&
+		echo "[test]two=2" >.git/bar2 &&
+		echo 2 >expect &&
+		git config test.two >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'conditional include, full pattern' '
+	(
+		cd foo &&
+		echo "[includeIf \"gitdir:**/foo/**\"]path=bar3" >>.git/config &&
+		echo "[test]three=3" >.git/bar3 &&
+		echo 3 >expect &&
+		git config test.three >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'conditional include, relative path' '
+	echo "[includeIf \"gitdir:./foo/.git\"]path=bar4" >>.gitconfig &&
+	echo "[test]four=4" >bar4 &&
+	(
+		cd foo &&
+		echo 4 >expect &&
+		git config test.four >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'conditional include, both unanchored, icase' '
+	(
+		cd foo &&
+		echo "[includeIf \"gitdir/i:FOO/\"]path=bar5" >>.git/config &&
+		echo "[test]five=5" >.git/bar5 &&
+		echo 5 >expect &&
+		git config test.five >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_expect_success 'include cycles are detected' '
 	cat >.gitconfig <<-\EOF &&
 	[test]value = gitconfig
-- 
2.11.0.157.gd943d85

Re: [PATCH v6 0/1] Conditional config include

[Duy Nguyen]

And sorry for duplicates :P Somehow I managed to --dry-run correctly
the first time, then had _two_ 000*.patch in the command line. And
send-email happily let me shoot myself in the foot.


-- 
Duy

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

>>> +     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.
>
> Yeah I don't see a third include coming soon and did not go with that.
> Let's way for it and refactor then.

I would have said exactly that in my message if you already had
include_by_gitdir() and include_by_gitdir_i() as separate functions.

But I didn't, because the above code gives an excuse to the person
who adds the third one to be lazy and add another "else if" for
expediency, because making it table-driven would require an extra
work to add two wrapper functions that do not have anything to do
with the third one being added.

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

[Junio C Hamano]

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

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

> @@ -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.

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

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

[Ramsay Jones]

On 24/02/17 13:14, Nguyễn Thái Ngọc Duy wrote:
[snip] 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Documentation/config.txt  | 61 +++++++++++++++++++++++++++++
>  config.c                  | 97 +++++++++++++++++++++++++++++++++++++++++++++++
>  t/t1305-config-include.sh | 56 +++++++++++++++++++++++++++
>  3 files changed, 214 insertions(+)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 015346c417..6c0cd2a273 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -91,6 +91,56 @@ 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.
>  
> +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.
> ++
> +The .git location which may be auto-discovered, or come from
> +`$GIT_DIR` environment variable. If the repository auto discovered via
> +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.
> ++
> +The pattern can contain standard globbing wildcards and two additional
> +ones, `**/` and `/**`, that can match multiple path components. Please
> +refer to linkgit:gitignore[5] for details. For convenience:
> +
> + * If the pattern starts with `~/`, `~` will be substituted with the
> +   content of the environment variable `HOME`.
> +
> + * If the pattern starts with `./`, it is replaced with the directory
> +   containing the current config file.
> +
> + * If the pattern does not start with either `~/`, `./` or `/`, `**/`
> +   will be automatically prepended. For example, the pattern `foo/bar`
> +   becomes `**/foo/bar` and would match `/any/path/to/foo/bar`.
> +
> + * 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 with `gitdir` and `gitdir/i`:
> +
> + * Symlinks in `$GIT_DIR` are not resolved before matching.
> +
> + * Note that "../" is not special and will match literally, which is
> +   unlikely what you want.
>  
>  Example
>  ~~~~~~~
> @@ -119,6 +169,17 @@ Example
>  		path = foo ; expand "foo" relative to the current file
>  		path = ~/foo ; expand "foo" in your `$HOME` directory
>  
> +	; include if $GIT_DIR is /path/to/foo/.git
> +	[include-if "gitdir:/path/to/foo/.git"]

s/include-if/includeIf/

> +		path = /path/to/foo.inc
> +
> +	; include for all repositories inside /path/to/group
> +	[include-if "gitdir:/path/to/group/"]

ditto

> +		path = /path/to/foo.inc
> +
> +	; include for all repositories inside $HOME/to/group
> +	[include-if "gitdir:~/to/group/"]

ditto

ATB,
Ramsay Jones

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

[Philip Oakley]

From: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
> 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.
>
> Setting $GIT_DIR/.config works, but if the penalty of forgetting to
> update $GIT_DIR/.config is high (especially when you end up cloning
> often), it may not be the best way to go. Having the settings in
> ~/.gitconfig, which would work for just one set of repositories, would
> not well in such a situation. Having separate ${HOME}s may add more
> problems than it solves.
>
> 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 includeIf.<condition>.path to make it clearer that a condition is
> required. The new config has the same backward compatibility approach as
> include.path: older git versions that don't understand includeIf will
> simply ignore them.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> Documentation/config.txt  | 61 +++++++++++++++++++++++++++++
> config.c                  | 97 
> +++++++++++++++++++++++++++++++++++++++++++++++
> t/t1305-config-include.sh | 56 +++++++++++++++++++++++++++
> 3 files changed, 214 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 015346c417..6c0cd2a273 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -91,6 +91,56 @@ 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.
>
> +Conditional includes
> +~~~~~~~~~~~~~~~~~~~~
> +
> +You can include one config file from another conditionally by setting

On first reading I thought this implied you can only have one `includeIf` 
within the config file.
I think it is meant to mean that each `includeIf`could include one other 
file, and that users can have multiple `includeIf` lines.


> +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`.
> +

--
Philip 

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

[Duy Nguyen]

On Sat, Feb 25, 2017 at 5:08 AM, Philip Oakley <philipoakley@iee.org> wrote:
>> +Conditional includes
>> +~~~~~~~~~~~~~~~~~~~~
>> +
>> +You can include one config file from another conditionally by setting
>
>
> On first reading I thought this implied you can only have one `includeIf`
> within the config file.
> I think it is meant to mean that each `includeIf`could include one other
> file, and that users can have multiple `includeIf` lines.

Yes. Not sure how to put it better though (I basically copied the
first paragraph from the unconditional include section above, which
shares the same confusion). Perhaps just write "the variable can be
specified multiple times"? Or "multiple variables include multiple
times, the last variable does not override the previous ones"?
-- 
Duy

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

[Jeff King]

On Fri, Feb 24, 2017 at 09:46:17AM -0800, Junio C Hamano wrote:

> Duy Nguyen <pclouds@gmail.com> writes:
> 
> >>> +     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.
> >
> > Yeah I don't see a third include coming soon and did not go with that.
> > Let's way for it and refactor then.
> 
> I would have said exactly that in my message if you already had
> include_by_gitdir() and include_by_gitdir_i() as separate functions.
> 
> But I didn't, because the above code gives an excuse to the person
> who adds the third one to be lazy and add another "else if" for
> expediency, because making it table-driven would require an extra
> work to add two wrapper functions that do not have anything to do
> with the third one being added.

I don't think driving that with a two-entry table is the right thing
here. We are as likely to add another "foobar:" entry as we are to add
another modifier "/i" modifier to "gitdir:", and it is unclear whether
that modifier would be mutually exclusive with "/i".

E.g., imagine we add "/re" for a regex, but allow a case-insensitive
regex with an "i", giving something like "gitdir/i/re:".  Now you would
want to drive it by matching "gitdir" first, and then collecting the
"/i/re" independently, to avoid an explosion of matches.

Driving that with a table is much more complex. I'd just as soon keep
things as simple as possible for now and worry about flagging it in
review when something new gets added.

-Peff

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

[Jeff King]

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

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

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

[Philip Oakley]

From: "Duy Nguyen" <pclouds@gmail.com>
> On Sat, Feb 25, 2017 at 5:08 AM, Philip Oakley <philipoakley@iee.org> 
> wrote:
>>> +Conditional includes
>>> +~~~~~~~~~~~~~~~~~~~~
>>> +
>>> +You can include one config file from another conditionally by setting
>>
>>
>> On first reading I thought this implied you can only have one `includeIf`
>> within the config file.
>> I think it is meant to mean that each `includeIf`could include one other
>> file, and that users can have multiple `includeIf` lines.
>
> Yes. Not sure how to put it better though (I basically copied the
> first paragraph from the unconditional include section above, which
> shares the same confusion). Perhaps just write "the variable can be
> specified multiple times"? Or "multiple variables include multiple
> times, the last variable does not override the previous ones"?
> -- 

My attempt, based on updating the `Includes` section would be something 
like:

`You can include a config file from another by setting the special 
`include.path` variable to the name of the file to be included. The variable 
takes a pathname as its value, and is subject to tilde expansion. 
`include.path` supports multiple key values.`

The subtle change was to s/one/a/ at the start, and then add the final short 
sentence that states that the section's variables can have multiple key 
values.

I copied the 'multiple key values' phrase from the man page intro for 
consitency, though 'multivalued' could just as easily be used as it is the 
term used by the 'Configuration File' section that this is part of 
https://git-scm.com/docs/git-config#_configuration_file.

Even shorter may be:
`You can include a config file from another by setting the special 
`include.path` variable to the name of the file to be included. The variable 
(can be multivalued) takes a pathname as its value, and is subject to tilde 
expansion.`


The Conditional Includes would follow suit.

Philip

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I don't think driving that with a two-entry table is the right thing
> here. We are as likely to add another "foobar:" entry as we are to add
> another modifier "/i" modifier to "gitdir:", and it is unclear whether
> that modifier would be mutually exclusive with "/i".

OK, I didn't take /i as something that was meant as a modifier; I
took the "gitdir:" and "gitdir/i:" are totally different tests that
are spelled similarly, but for the implementation expediency, called
into a single helper function without having a layer that presents
the same function signature in the middle to make it drivable by a
table.

Let's leave it to the review of a future patch that wants to add a
third condition then.  At that time, we will have more things to
look at to make a better decision.

[PATCH v7 0/3] Conditional config include

[Nguyễn Thái Ngọc Duy]

v7 changes:

 - typo fix here and there
 - delete a dead TODO comment
 - delete the error on unrecognized condition (we probably want a
   better, general mechanism for troubleshooting config keys anyway)
 - some fix up on include.path document because the "one or many"
   confusion started from there

I don't have a good answer for Jeff's PS about includeIf ugliness. I
agree that includeif is ugly but includeIf looks a bit better. I don't
see a better option (if only "include" does not start or end with a
vowel...). Maybe includewith? Suggestions are welcome.

Interdiff below (just on patch 3, not the first two)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index aab3df04fb..2a41e84bab 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -93,9 +93,11 @@ was found.  See below for examples.
 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`.
+You can include a 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`. `includeIf.<condition>path` supports multiple key
+values.
 
 The condition starts with a keyword followed by a colon and some data
 whose format and meaning depends on the keyword. Supported keywords
@@ -104,13 +106,14 @@ 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. If the location of the .git directory matches the
 	pattern, the include condition is met.
 +
-The .git location which may be auto-discovered, or come from
-`$GIT_DIR` environment variable. If the repository auto discovered via
-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.
+The .git location may be auto-discovered, or come from `$GIT_DIR`
+environment variable. If the repository is auto discovered via a .git
+file (e.g. from submodules, or a linked worktree), the .git location
+would be the final location where the .git directory is, not where the
+.git file is.
 +
 The pattern can contain standard globbing wildcards and two additional
 ones, `**/` and `/**`, that can match multiple path components. Please
@@ -169,15 +172,15 @@ Example
 		path = ~/foo ; expand "foo" in your `$HOME` directory
 
 	; include if $GIT_DIR is /path/to/foo/.git
-	[include-if "gitdir:/path/to/foo/.git"]
+	[includeIf "gitdir:/path/to/foo/.git"]
 		path = /path/to/foo.inc
 
 	; include for all repositories inside /path/to/group
-	[include-if "gitdir:/path/to/group/"]
+	[includeIf "gitdir:/path/to/group/"]
 		path = /path/to/foo.inc
 
 	; include for all repositories inside $HOME/to/group
-	[include-if "gitdir:~/to/group/"]
+	[includeIf "gitdir:~/to/group/"]
 		path = /path/to/foo.inc
 
 Values
diff --git a/config.c b/config.c
index ad16802c8a..0dac0f4cb2 100644
--- a/config.c
+++ b/config.c
@@ -191,7 +191,6 @@ static int prepare_include_condition_pattern(struct strbuf *pat)
 			return error(_("relative config include "
 				       "conditionals must come from files"));
 
-		/* TODO: escape wildcards */
 		strbuf_add_absolute_path(&path, cf->path);
 		slash = find_last_dir_sep(path.buf);
 		if (!slash)
@@ -245,16 +244,12 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase)
 
 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;
 }
@@ -278,7 +273,7 @@ int git_config_include(const char *var, const char *value, void *data)
 		ret = handle_path_include(value, inc);
 
 	if (!parse_config_key(var, "includeif", &cond, &cond_len, &key) &&
-	    include_condition_is_true(cond, cond_len) &&
+	    (cond && include_condition_is_true(cond, cond_len)) &&
 	    !strcmp(key, "path"))
 		ret = handle_path_include(value, inc);

Nguyễn Thái Ngọc Duy (3):
  config.txt: clarify multiple key values in include.path
  config.txt: reflow the second include.path paragraph
  config: add conditional include

 Documentation/config.txt  | 77 +++++++++++++++++++++++++++++++++++----
 config.c                  | 92 +++++++++++++++++++++++++++++++++++++++++++++++
 t/t1305-config-include.sh | 56 +++++++++++++++++++++++++++++
 3 files changed, 218 insertions(+), 7 deletions(-)

-- 
2.11.0.157.gd943d85

[PATCH v7 1/3] config.txt: clarify multiple key values in include.path

[Nguyễn Thái Ngọc Duy]

The phrasing in this paragraph may give an impression that you can only
use it once. Rephrase it a bit.

Helped-by: Philip Oakley <philipoakley@iee.org>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/config.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 015346c417..4748efbf36 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -79,10 +79,10 @@ escape sequences) are invalid.
 Includes
 ~~~~~~~~
 
-You can include one config file from another by setting the special
+You can include a config file from another by setting the special
 `include.path` variable to the name of the file to be included. The
 variable takes a pathname as its value, and is subject to tilde
-expansion.
+expansion. `include.path` supports multiple key values.
 
 The
 included file is expanded immediately, as if its contents had been
-- 
2.11.0.157.gd943d85

[PATCH v7 2/3] config.txt: reflow the second include.path paragraph

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/config.txt | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4748efbf36..1fad746efd 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -84,12 +84,11 @@ You can include a config file from another by setting the special
 variable takes a pathname as its value, and is subject to tilde
 expansion. `include.path` supports multiple key values.
 
-The
-included file is expanded immediately, as if its contents had been
+The included file is expanded immediately, as if its contents had been
 found at the location of the include directive. If the value of the
-`include.path` variable is a relative path, the path is considered to be
-relative to the configuration file in which the include directive was
-found.  See below for examples.
+`include.path` variable is a relative path, the path is considered to
+be relative to the configuration file in which the include directive
+was found.  See below for examples.
 
 
 Example
-- 
2.11.0.157.gd943d85

[PATCH v7 3/3] config: add conditional include

[Nguyễn Thái Ngọc Duy]

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.

Setting $GIT_DIR/.config works, but if the penalty of forgetting to
update $GIT_DIR/.config is high (especially when you end up cloning
often), it may not be the best way to go. Having the settings in
~/.gitconfig, which would work for just one set of repositories, would
not well in such a situation. Having separate ${HOME}s may add more
problems than it solves.

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 includeIf.<condition>.path to make it clearer that a condition is
required. The new config has the same backward compatibility approach as
include.path: older git versions that don't understand includeIf will
simply ignore them.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/config.txt  | 64 +++++++++++++++++++++++++++++++++
 config.c                  | 92 +++++++++++++++++++++++++++++++++++++++++++++++
 t/t1305-config-include.sh | 56 +++++++++++++++++++++++++++++
 3 files changed, 212 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1fad746efd..2a41e84bab 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -90,6 +90,59 @@ found at the location of the include directive. If the value of the
 be relative to the configuration file in which the include directive
 was found.  See below for examples.
 
+Conditional includes
+~~~~~~~~~~~~~~~~~~~~
+
+You can include a 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`. `includeIf.<condition>path` supports multiple key
+values.
+
+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 matches the
+	pattern, the include condition is met.
++
+The .git location may be auto-discovered, or come from `$GIT_DIR`
+environment variable. If the repository is auto discovered via a .git
+file (e.g. from submodules, or a linked worktree), the .git location
+would be the final location where the .git directory is, not where the
+.git file is.
++
+The pattern can contain standard globbing wildcards and two additional
+ones, `**/` and `/**`, that can match multiple path components. Please
+refer to linkgit:gitignore[5] for details. For convenience:
+
+ * If the pattern starts with `~/`, `~` will be substituted with the
+   content of the environment variable `HOME`.
+
+ * If the pattern starts with `./`, it is replaced with the directory
+   containing the current config file.
+
+ * If the pattern does not start with either `~/`, `./` or `/`, `**/`
+   will be automatically prepended. For example, the pattern `foo/bar`
+   becomes `**/foo/bar` and would match `/any/path/to/foo/bar`.
+
+ * 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 via `gitdir` and `gitdir/i`:
+
+ * Symlinks in `$GIT_DIR` are not resolved before matching.
+
+ * Note that "../" is not special and will match literally, which is
+   unlikely what you want.
 
 Example
 ~~~~~~~
@@ -118,6 +171,17 @@ Example
 		path = foo ; expand "foo" relative to the current file
 		path = ~/foo ; expand "foo" in your `$HOME` directory
 
+	; include if $GIT_DIR is /path/to/foo/.git
+	[includeIf "gitdir:/path/to/foo/.git"]
+		path = /path/to/foo.inc
+
+	; include for all repositories inside /path/to/group
+	[includeIf "gitdir:/path/to/group/"]
+		path = /path/to/foo.inc
+
+	; include for all repositories inside $HOME/to/group
+	[includeIf "gitdir:~/to/group/"]
+		path = /path/to/foo.inc
 
 Values
 ~~~~~~
diff --git a/config.c b/config.c
index c6b874a7bf..0dac0f4cb2 100644
--- a/config.c
+++ b/config.c
@@ -13,6 +13,7 @@
 #include "hashmap.h"
 #include "string-list.h"
 #include "utf8.h"
+#include "dir.h"
 
 struct config_source {
 	struct config_source *prev;
@@ -170,9 +171,94 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 	return ret;
 }
 
+static int prepare_include_condition_pattern(struct strbuf *pat)
+{
+	struct strbuf path = STRBUF_INIT;
+	char *expanded;
+	int prefix = 0;
+
+	expanded = expand_user_path(pat->buf);
+	if (expanded) {
+		strbuf_reset(pat);
+		strbuf_addstr(pat, expanded);
+		free(expanded);
+	}
+
+	if (pat->buf[0] == '.' && is_dir_sep(pat->buf[1])) {
+		const char *slash;
+
+		if (!cf || !cf->path)
+			return error(_("relative config include "
+				       "conditionals must come from files"));
+
+		strbuf_add_absolute_path(&path, cf->path);
+		slash = find_last_dir_sep(path.buf);
+		if (!slash)
+			die("BUG: how is this possible?");
+		strbuf_splice(pat, 0, 1, path.buf, slash - path.buf);
+		prefix = slash - path.buf + 1 /* slash */;
+	} else if (!is_absolute_path(pat->buf))
+		strbuf_insert(pat, 0, "**/", 3);
+
+	if (pat->len && is_dir_sep(pat->buf[pat->len - 1]))
+		strbuf_addstr(pat, "**");
+
+	strbuf_release(&path);
+	return prefix;
+}
+
+static int include_by_gitdir(const char *cond, size_t cond_len, int icase)
+{
+	struct strbuf text = STRBUF_INIT;
+	struct strbuf pattern = STRBUF_INIT;
+	int ret = 0, prefix;
+
+	strbuf_add_absolute_path(&text, get_git_dir());
+	strbuf_add(&pattern, cond, cond_len);
+	prefix = prepare_include_condition_pattern(&pattern);
+
+	if (prefix < 0)
+		goto done;
+
+	if (prefix > 0) {
+		/*
+		 * perform literal matching on the prefix part so that
+		 * any wildcard character in it can't create side effects.
+		 */
+		if (text.len < prefix)
+			goto done;
+		if (!icase && strncmp(pattern.buf, text.buf, prefix))
+			goto done;
+		if (icase && strncasecmp(pattern.buf, text.buf, prefix))
+			goto done;
+	}
+
+	ret = !wildmatch(pattern.buf + prefix, text.buf + prefix,
+			 icase ? WM_CASEFOLD : 0, NULL);
+
+done:
+	strbuf_release(&pattern);
+	strbuf_release(&text);
+	return ret;
+}
+
+static int include_condition_is_true(const char *cond, size_t cond_len)
+{
+
+	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);
+
+	/* unknown conditionals are always false */
+	return 0;
+}
+
 int git_config_include(const char *var, const char *value, void *data)
 {
 	struct config_include_data *inc = data;
+	const char *cond, *key;
+	int cond_len;
 	int ret;
 
 	/*
@@ -185,6 +271,12 @@ int git_config_include(const char *var, const char *value, void *data)
 
 	if (!strcmp(var, "include.path"))
 		ret = handle_path_include(value, inc);
+
+	if (!parse_config_key(var, "includeif", &cond, &cond_len, &key) &&
+	    (cond && include_condition_is_true(cond, cond_len)) &&
+	    !strcmp(key, "path"))
+		ret = handle_path_include(value, inc);
+
 	return ret;
 }
 
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index 9ba2ba11c3..f0cd2056ba 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -152,6 +152,62 @@ test_expect_success 'relative includes from stdin line fail' '
 	test_must_fail git config --file - test.one
 '
 
+test_expect_success 'conditional include, both unanchored' '
+	git init foo &&
+	(
+		cd foo &&
+		echo "[includeIf \"gitdir:foo/\"]path=bar" >>.git/config &&
+		echo "[test]one=1" >.git/bar &&
+		echo 1 >expect &&
+		git config test.one >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'conditional include, $HOME expansion' '
+	(
+		cd foo &&
+		echo "[includeIf \"gitdir:~/foo/\"]path=bar2" >>.git/config &&
+		echo "[test]two=2" >.git/bar2 &&
+		echo 2 >expect &&
+		git config test.two >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'conditional include, full pattern' '
+	(
+		cd foo &&
+		echo "[includeIf \"gitdir:**/foo/**\"]path=bar3" >>.git/config &&
+		echo "[test]three=3" >.git/bar3 &&
+		echo 3 >expect &&
+		git config test.three >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'conditional include, relative path' '
+	echo "[includeIf \"gitdir:./foo/.git\"]path=bar4" >>.gitconfig &&
+	echo "[test]four=4" >bar4 &&
+	(
+		cd foo &&
+		echo 4 >expect &&
+		git config test.four >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'conditional include, both unanchored, icase' '
+	(
+		cd foo &&
+		echo "[includeIf \"gitdir/i:FOO/\"]path=bar5" >>.git/config &&
+		echo "[test]five=5" >.git/bar5 &&
+		echo 5 >expect &&
+		git config test.five >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_expect_success 'include cycles are detected' '
 	cat >.gitconfig <<-\EOF &&
 	[test]value = gitconfig
-- 
2.11.0.157.gd943d85

Re: [PATCH v7 3/3] config: add conditional include

[Ramsay Jones]

On 01/03/17 11:26, Nguyễn Thái Ngọc Duy wrote:
> 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.
> 
> Setting $GIT_DIR/.config works, but if the penalty of forgetting to
> update $GIT_DIR/.config is high (especially when you end up cloning
> often), it may not be the best way to go. Having the settings in
> ~/.gitconfig, which would work for just one set of repositories, would
> not well in such a situation. Having separate ${HOME}s may add more
> problems than it solves.
> 
> 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 includeIf.<condition>.path to make it clearer that a condition is
> required. The new config has the same backward compatibility approach as
> include.path: older git versions that don't understand includeIf will
> simply ignore them.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Documentation/config.txt  | 64 +++++++++++++++++++++++++++++++++
>  config.c                  | 92 +++++++++++++++++++++++++++++++++++++++++++++++
>  t/t1305-config-include.sh | 56 +++++++++++++++++++++++++++++
>  3 files changed, 212 insertions(+)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 1fad746efd..2a41e84bab 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -90,6 +90,59 @@ found at the location of the include directive. If the value of the
>  be relative to the configuration file in which the include directive
>  was found.  See below for examples.
>  
> +Conditional includes
> +~~~~~~~~~~~~~~~~~~~~
> +
> +You can include a 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`. `includeIf.<condition>path` supports multiple key
----------------------------------------^^
s/<condition>path/<condition>.path/

ATB,
Ramsay Jones

Re: [PATCH v7 1/3] config.txt: clarify multiple key values in include.path

[Junio C Hamano]

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

> The phrasing in this paragraph may give an impression that you can only
> use it once. Rephrase it a bit.
>
> Helped-by: Philip Oakley <philipoakley@iee.org>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Documentation/config.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 015346c417..4748efbf36 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -79,10 +79,10 @@ escape sequences) are invalid.
>  Includes
>  ~~~~~~~~
>  
> -You can include one config file from another by setting the special
> +You can include a config file from another by setting the special
>  `include.path` variable to the name of the file to be included. The
>  variable takes a pathname as its value, and is subject to tilde
> -expansion.
> +expansion. `include.path` supports multiple key values.

Multiple key values or just simply multiple values?  I think it is
the latter.

But to the intended target audience, I think

	`include.path` can be given multiple times.

is easier to understand.  It's not like you can (or want to)
enumerate with "git config --get-all include.path" to learn all the
values (for the single key "include.value"), and it is better not to
lead readers to think of this in terms of <key,value> in the first
place (which is already clarified in the text that follows).

>  The
>  included file is expanded immediately, as if its contents had been

Re: [PATCH v7 3/3] config: add conditional include

[Junio C Hamano]

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

>  int git_config_include(const char *var, const char *value, void *data)
>  {
>  	struct config_include_data *inc = data;
> +	const char *cond, *key;
> +	int cond_len;
>  	int ret;
>  
>  	/*
> @@ -185,6 +271,12 @@ int git_config_include(const char *var, const char *value, void *data)
>  
>  	if (!strcmp(var, "include.path"))
>  		ret = handle_path_include(value, inc);
> +
> +	if (!parse_config_key(var, "includeif", &cond, &cond_len, &key) &&
> +	    (cond && include_condition_is_true(cond, cond_len)) &&
> +	    !strcmp(key, "path"))
> +		ret = handle_path_include(value, inc);
> +
>  	return ret;
>  }

So "includeif.path" (misspelled one without any condition) falls
through to "return ret" and gives the value we got from inc->fn().
I am OK with that (i.e. "missing condition is false").

Or we can make it go to handle_path_include(), effectively making
the "include.path" a short-hand for "includeIf.path".  I am also OK
with that (i.e. "missing condition is true").

Or we could even have "include.[<condition>.]path" without
"includeIf"?  I am not sure if it is a bad idea that paints
ourselves in a corner, but somehow I find it tempting.

Re: [PATCH v7 0/3] Conditional config include

[Junio C Hamano]

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

> Nguyễn Thái Ngọc Duy (3):
>   config.txt: clarify multiple key values in include.path
>   config.txt: reflow the second include.path paragraph
>   config: add conditional include

Thanks.  The primary change looked good (it looked good already in
the previous round).

Here is a list of minor suggestions on top.

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2a41e84bab..5faabc7934 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -82,7 +82,7 @@ Includes
 You can include a config file from another by setting the special
 `include.path` variable to the name of the file to be included. The
 variable takes a pathname as its value, and is subject to tilde
-expansion. `include.path` supports multiple key values.
+expansion. `include.path` can be given multiple times.
 
 The included file is expanded immediately, as if its contents had been
 found at the location of the include directive. If the value of the
@@ -96,8 +96,7 @@ Conditional includes
 You can include a 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`. `includeIf.<condition>path` supports multiple key
-values.
+`include.path`. `includeIf.<condition>.path` can be given multiple times.
 
 The condition starts with a keyword followed by a colon and some data
 whose format and meaning depends on the keyword. Supported keywords

Re: [PATCH v7 3/3] config: add conditional include

[Jeff King]

On Wed, Mar 01, 2017 at 09:47:40AM -0800, Junio C Hamano wrote:

> > @@ -185,6 +271,12 @@ int git_config_include(const char *var, const char *value, void *data)
> >  
> >  	if (!strcmp(var, "include.path"))
> >  		ret = handle_path_include(value, inc);
> > +
> > +	if (!parse_config_key(var, "includeif", &cond, &cond_len, &key) &&
> > +	    (cond && include_condition_is_true(cond, cond_len)) &&
> > +	    !strcmp(key, "path"))
> > +		ret = handle_path_include(value, inc);
> > +
> >  	return ret;
> >  }
> 
> So "includeif.path" (misspelled one without any condition) falls
> through to "return ret" and gives the value we got from inc->fn().
> I am OK with that (i.e. "missing condition is false").

Yeah, I think that is sensible, just as not-understood nonsense like
"include.foobar" would be ignored, as well.

> Or we can make it go to handle_path_include(), effectively making
> the "include.path" a short-hand for "includeIf.path".  I am also OK
> with that (i.e. "missing condition is true").

I think we want "missing condition is false". Certainly an empty
condition like "includeIf..path" is false, as are conditions we don't
understand.

> Or we could even have "include.[<condition>.]path" without
> "includeIf"?  I am not sure if it is a bad idea that paints
> ourselves in a corner, but somehow I find it tempting.

That was how I had originally envisioned the namespace working when I
introduced include.path long ago. And I think Duy's v1 used that, but
the feedback was that it was not sufficiently obvious that the
subsection was a conditional.

-Peff

Re: [PATCH v7 0/3] Conditional config include

[Jeff King]

On Wed, Mar 01, 2017 at 06:26:28PM +0700, Nguyễn Thái Ngọc Duy wrote:

> I don't have a good answer for Jeff's PS about includeIf ugliness. I
> agree that includeif is ugly but includeIf looks a bit better. I don't
> see a better option (if only "include" does not start or end with a
> vowel...). Maybe includewith? Suggestions are welcome.

I actually think "include-if" _looks_ better, although maybe the
inconsistency with "-" is something we don't want to encourage (though I
also think the implicit include.<cond>.path was OK, too). Feel free to
just ignore me. I will live with it either way.

For those following on the mailing list, there is some discussion at:

  https://github.com/git/git/commit/484f78e46d00c6d35f20058671a8c76bb924fb33

I think that is mostly focused around another failing in the
error-handling of the config code, and that does not need to be
addressed by this series (though of course I'd welcome any fixes).

But there's a test failure that probably does need to be dealt with
before this graduates to 'next'.

-Peff

Re: [PATCH v7 0/3] Conditional config include

[Duy Nguyen]

On Fri, Mar 03, 2017 at 01:33:29AM -0500, Jeff King wrote:
> For those following on the mailing list, there is some discussion at:
> 
>   https://github.com/git/git/commit/484f78e46d00c6d35f20058671a8c76bb924fb33
> 
> I think that is mostly focused around another failing in the
> error-handling of the config code, and that does not need to be
> addressed by this series (though of course I'd welcome any fixes).
> 
> But there's a test failure that probably does need to be dealt with
> before this graduates to 'next'.

The patch to fix it is this

diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index f0cd2056ba..e833939320 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -102,7 +102,7 @@ test_expect_success 'config modification does not affect includes' '
 
 test_expect_success 'missing include files are ignored' '
 	cat >.gitconfig <<-\EOF &&
-	[include]path = foo
+	[include]path = non-existent
 	[test]value = yes
 	EOF
 	echo yes >expect &&

Junio could you squash this in?

A lot more explanation is in another series [1] I just sent. Not sure
if we want to repeat the same in this commit's message.

And this series without that squash, when combined with [1], will fail
this test even on Linux. Good thing imo.

[1] http://public-inbox.org/git/20170303094252.11706-1-pclouds@gmail.com/
--
Duy

Re: [PATCH v7 0/3] Conditional config include

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> For those following on the mailing list, there is some discussion at:
>
>   https://github.com/git/git/commit/484f78e46d00c6d35f20058671a8c76bb924fb33
>
> I think that is mostly focused around another failing in the
> error-handling of the config code, and that does not need to be
> addressed by this series (though of course I'd welcome any fixes).

Thanks.  Without a message like this, the list may have never known
about the discussion taken elsewhere.  I'd appreciate such a report
to appear on list the next time much earlier ;-)

When built with FREAD_READS_DIRECTORIES=Yes on Linux, the error in
the test can easily reproduce.

In early days of UNIX it was sometimes handy to be able to read the
bytes off of directory to "investigate", but we are not a filesystem
application, and I do not offhand see any reason why we should be
relying on being able to successfully fopen() a directory for
reading.  A FILE * successfully opened that just returns EOF when
read is totally useless for any purpose anyway.  

When the path to be opened from the end user (either from the
command line or in a configuration file) is a directory, it is
better to diagnose it as a user error, and if the path was computed
by our code, it may be a bug.

I am wondering if we should enable this on Linux, at least in
DEVELOPER builds but possibly even on the release builds, to catch
these problems more easily.

Re: [PATCH v7 3/3] config: add conditional include

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> That was how I had originally envisioned the namespace working when I
> introduced include.path long ago. And I think Duy's v1 used that, but
> the feedback was that it was not sufficiently obvious that the
> subsection was a conditional.

I am not sure about "obviousness", but I agree that we do not know
that "conditional include" would be the only thing we want for the
second level for "include.path" directive.  "include-if.<cond>.path"
is better for that reason.

I presume that you could still do

	[include "if:gitdir=$path"]
		path = ...

i.e. design the second level to begin with a token that tells
readers what it means (and assign "if:" token for "conditional
include"), but I do not think it is worth it.

I also imagine that

	[include]
		condition = ...
		path = ...

is easier to read and write by end-users, but it probably is not
feasible because it requires too invasive change to the current code
to teach it to grok such construct.

Between "include-if" and "includeIf", if people find the latter not
too ugly, I'd prefer to keep it the way Duy posted.  Because of the
way "include.path" and "include-if.<cond>.path" work, we can declare
that they are not like ordinary configuration variable definition
at all but are higher-level directives and that may be a sufficient
justification to allow "-" in its name, though, if people find
"includeIf" too ugly a name to live.

Re: [PATCH v7 0/3] Conditional config include

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> The patch to fix it is this
>
> diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
> index f0cd2056ba..e833939320 100755
> --- a/t/t1305-config-include.sh
> +++ b/t/t1305-config-include.sh
> @@ -102,7 +102,7 @@ test_expect_success 'config modification does not affect includes' '
>  
>  test_expect_success 'missing include files are ignored' '
>  	cat >.gitconfig <<-\EOF &&
> -	[include]path = foo
> +	[include]path = non-existent
>  	[test]value = yes
>  	EOF
>  	echo yes >expect &&
>
> Junio could you squash this in?

Sure.

Re: [PATCH v7 0/3] Conditional config include

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Fri, Mar 03, 2017 at 01:33:29AM -0500, Jeff King wrote:
>> For those following on the mailing list, there is some discussion at:
>> 
>>   https://github.com/git/git/commit/484f78e46d00c6d35f20058671a8c76bb924fb33
>> 
>> I think that is mostly focused around another failing in the
>> error-handling of the config code, and that does not need to be
>> addressed by this series (though of course I'd welcome any fixes).
>> 
>> But there's a test failure that probably does need to be dealt with
>> before this graduates to 'next'.
>
> The patch to fix it is this
>
> diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
> index f0cd2056ba..e833939320 100755
> --- a/t/t1305-config-include.sh
> +++ b/t/t1305-config-include.sh
> @@ -102,7 +102,7 @@ test_expect_success 'config modification does not affect includes' '
>  
>  test_expect_success 'missing include files are ignored' '
>  	cat >.gitconfig <<-\EOF &&
> -	[include]path = foo
> +	[include]path = non-existent
>  	[test]value = yes
>  	EOF
>  	echo yes >expect &&
>
> Junio could you squash this in?
>

I said yes, but I won't, as the series already has SQUASH??? for
other things, so I'll just queue this at the tip.  I _might_ get
inclined to do the squashing and cleaning up myself later, but
please don't hold your breath.

Re: [PATCH v7 0/3] Conditional config include

[Jeff King]

On Fri, Mar 03, 2017 at 10:24:05AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > For those following on the mailing list, there is some discussion at:
> >
> >   https://github.com/git/git/commit/484f78e46d00c6d35f20058671a8c76bb924fb33
> >
> > I think that is mostly focused around another failing in the
> > error-handling of the config code, and that does not need to be
> > addressed by this series (though of course I'd welcome any fixes).
> 
> Thanks.  Without a message like this, the list may have never known
> about the discussion taken elsewhere.  I'd appreciate such a report
> to appear on list the next time much earlier ;-)
> 
> When built with FREAD_READS_DIRECTORIES=Yes on Linux, the error in
> the test can easily reproduce.

Heh. I had no idea we had FREAD_READS_DIRECTORIES. I think Duy and I
reinvented it in another thread. ;)

I agree that may be worth setting on Linux (though note that we _do_
ignore other stdio read errors in the rest of the code path, which may
be something we want to address).

-Peff

Re: [PATCH v7 3/3] config: add conditional include

[Jeff King]

On Fri, Mar 03, 2017 at 11:05:20AM -0800, Junio C Hamano wrote:

> I am not sure about "obviousness", but I agree that we do not know
> that "conditional include" would be the only thing we want for the
> second level for "include.path" directive.  "include-if.<cond>.path"
> is better for that reason.
> 
> I presume that you could still do
> 
> 	[include "if:gitdir=$path"]
> 		path = ...
> 
> i.e. design the second level to begin with a token that tells
> readers what it means (and assign "if:" token for "conditional
> include"), but I do not think it is worth it.

Yep, all true.

> I also imagine that
> 
> 	[include]
> 		condition = ...
> 		path = ...
> 
> is easier to read and write by end-users, but it probably is not
> feasible because it requires too invasive change to the current code
> to teach it to grok such construct.

I am against that as it introduces a dependency in the presence and
ordering between two config variables, which can yield some surprises.

> Between "include-if" and "includeIf", if people find the latter not
> too ugly, I'd prefer to keep it the way Duy posted.  Because of the
> way "include.path" and "include-if.<cond>.path" work, we can declare
> that they are not like ordinary configuration variable definition
> at all but are higher-level directives and that may be a sufficient
> justification to allow "-" in its name, though, if people find
> "includeIf" too ugly a name to live.

OK. I can live with includeIf.

-Peff

Re: [PATCH v7 0/3] Conditional config include

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Heh. I had no idea we had FREAD_READS_DIRECTORIES. I think Duy and I
> reinvented it in another thread. ;)

You two were not alone.  I was planning to reinvent it before I went
to bed last night and then found it already was there this morning ;-)

> I agree that may be worth setting on Linux (though note that we _do_
> ignore other stdio read errors in the rest of the code path, which may
> be something we want to address).

Yes, we need an error check on fopen() in git_config_from_file()
regardless.

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

[Stefan Beller]

Being late to the review party here.

> +static int include_condition_is_true(const char *cond, size_t cond_len)
> +{
...
> +
> +       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/

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

[Jeff King]

On Mon, Mar 06, 2017 at 02:44:27PM -0800, Stefan Beller wrote:

> > +static int include_condition_is_true(const char *cond, size_t cond_len)
> > +{
> ...
> > +
> > +       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?)

It's "not any more". It was in the original and I asked for it to be
removed during the last review.

> 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).

That would make it unlike the rest of the config-include mechanism
(which quietly ignores things it doesn't understand, like include.foo,
or include.foo.path), as well as the config code in general (which
ignores misspelt keys).

Some of that "quiet when you don't understand it" is historical
necessity. Older versions _can't_ complain about not knowing
include.path, because they don't yet know it's worth complaining about.
Likewise here, if this ships in v2.13 and a new condition "foo:" ships
in v2.14, you get:

  v2.12 - no complaint; we don't even understand includeIf at all
  v2.13 - complain; we know includeIf, but not "foo:"
  v2.14 - works as expected

Which is kind of weird and inconsistent. But maybe the typo-detection
case is more important to get right than consistency across historical
versions.

-Peff

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

[Stefan Beller]

On Tue, Mar 7, 2017 at 12:47 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Mar 06, 2017 at 02:44:27PM -0800, Stefan Beller wrote:
>
>> > +static int include_condition_is_true(const char *cond, size_t cond_len)
>> > +{
>> ...
>> > +
>> > +       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?)
>
> It's "not any more". It was in the original and I asked for it to be
> removed during the last review.

Okay. The joys of contradicting opinions on a mailing list. :)

>
>> 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).
>
> That would make it unlike the rest of the config-include mechanism
> (which quietly ignores things it doesn't understand, like include.foo,
> or include.foo.path), as well as the config code in general (which
> ignores misspelt keys).
>
> Some of that "quiet when you don't understand it" is historical
> necessity. Older versions _can't_ complain about not knowing
> include.path, because they don't yet know it's worth complaining about.

agreed

> Likewise here, if this ships in v2.13 and a new condition "foo:" ships
> in v2.14, you get:
>
>   v2.12 - no complaint; we don't even understand includeIf at all
>   v2.13 - complain; we know includeIf, but not "foo:"
>   v2.14 - works as expected
>
> Which is kind of weird and inconsistent. But maybe the typo-detection
> case is more important to get right than consistency across historical
> versions.

Oh, I see. I was contemplating a future in which 2.12 is not used anymore.

When looking at other examples, such as url.<...>.insteadOf we also do not
warn about typos (well we can't actually).

In diff.<driver>.(command/binary/..) we know the limited set of drivers,
which is similar to the situation we have here.

Maybe a compromise between typo checking (edit distance < 2 -> warn;
silent for larger distances) and the consistency over time is desired.
But this is even more code to write.

So for now I retract my strong opinion and be happy with what is
presented as-is for the reasons Peff gave.

Thanks,
Stefan