[PATCH] config: add conditional include

[PATCH] config: add conditional include

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

If the path argument in "include" starts with "gitdir:", it is
followed by a wildmatch pattern. The include is only effective if
$GIT_DIR matches the pattern. This is very useful to add configuration
to a group of repositories.

For convenience

 - "~" is expanded to $USER

 - if the pattern ends with '/', "**" will be appended (e.g. foo/
   becomes foo/**). In other words, "foo/" automatically matches
   everything in starting with "foo/".

 - if the pattern contains no slashes, it's wrapped around by "**/"
   and "/**" (e.g. "foo" becomes "**/foo/**"). In other words, "foo"
   matches any directory component in $GIT_DIR.

The combination of the first two is used to group repositories by
path. While the last one could be used to match worktree's basename.

This code is originally written by Jeff King [1]. All genius designs
are his. All bugs are mine (claiming bugs is just more fun :).

[1] http://thread.gmane.org/gmane.comp.version-control.git/273811/focus=273825

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Original thread is [1]. Sebastian may not need it but I do and not
 just for user.* stuff. So I'm bringing it back. I deleted Jeff's
 de-anchoring and replaced with something a bit more restrictive. Once
 we settle that, I'll add tests and stuff.

 config.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 55 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index f51c56b..dd55a5f 100644
--- a/config.c
+++ b/config.c
@@ -140,9 +140,58 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 	return ret;
 }
 
+static int include_condition_is_true(const char *cond, int cond_len)
+{
+	const char *value;
+
+	/* no condition (i.e., "include.path") is always true */
+	if (!cond)
+		return 1;
+
+	/*
+	 * It's OK to run over cond_len in our checks here, as that just pushes
+	 * us past the final ".", which cannot match any of our prefixes.
+	 */
+	if (skip_prefix(cond, "gitdir:", &value)) {
+		struct strbuf text = STRBUF_INIT;
+		struct strbuf pattern = STRBUF_INIT;
+		char *buf;
+		int ret;
+
+		strbuf_add_absolute_path(&text, get_git_dir());
+
+		strbuf_add(&pattern, value, cond_len - (value - cond));
+		buf = expand_user_path(pattern.buf);
+		if (buf) {
+			strbuf_reset(&pattern);
+			strbuf_addstr(&pattern, buf);
+			free(buf);
+		}
+
+		if (pattern.len && pattern.buf[pattern.len - 1] == '/') {
+			/* foo/ matches recursively */
+			strbuf_addstr(&pattern, "**");
+		} else if (!strchr(pattern.buf, '/')) {
+			/* no slashes match one directory component */
+			strbuf_insert(&pattern, 0, "**/", 3);
+			strbuf_addstr(&pattern, "/**");
+		}
+
+		ret = !wildmatch(pattern.buf, text.buf, 0, NULL);
+		strbuf_release(&pattern);
+		strbuf_release(&text);
+		return ret;
+	}
+
+	/* 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;
 
 	/*
@@ -153,8 +202,12 @@ int git_config_include(const char *var, const char *value, void *data)
 	if (ret < 0)
 		return ret;
 
-	if (!strcmp(var, "include.path"))
-		ret = handle_path_include(value, inc);
+	if (!parse_config_key(var, "include", &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;
 }
 
-- 
2.8.2.526.g02eed6d

Re: [PATCH] config: add conditional include

[Jeff King]

On Sun, Jun 26, 2016 at 09:06:17AM +0200, Nguyễn Thái Ngọc Duy wrote:

> If the path argument in "include" starts with "gitdir:", it is
> followed by a wildmatch pattern. The include is only effective if
> $GIT_DIR matches the pattern. This is very useful to add configuration
> to a group of repositories.

I think this needs some more introduction to the concept. When you say
"path argument" here, I assumed you meant the value of include.path. But
you really mean: we are introducing a new concept for the "subsection"
field of include.*, which is to provide restrictions for conditional
includes.

It also may be worth discussing the motivation or examples.

> For convenience
> 
>  - "~" is expanded to $USER
> 
>  - if the pattern ends with '/', "**" will be appended (e.g. foo/
>    becomes foo/**). In other words, "foo/" automatically matches
>    everything in starting with "foo/".
> 
>  - if the pattern contains no slashes, it's wrapped around by "**/"
>    and "/**" (e.g. "foo" becomes "**/foo/**"). In other words, "foo"
>    matches any directory component in $GIT_DIR.
>
> The combination of the first two is used to group repositories by
> path. While the last one could be used to match worktree's basename.

This is a nice description, but it probably belongs in the
documentation.

I don't have any real opinion on the rules themselves, though they seem
reasonable to me (though in the first one I assume you mean $HOME).

> This code is originally written by Jeff King [1]. All genius designs
> are his. All bugs are mine (claiming bugs is just more fun :).

Heh. I have written this code in a "something like this" form at least 3
times over the years.  Conditional includes were always something I
planned into the original scheme, but had never actually found a good
use for it. ;)

> +	/*
> +	 * It's OK to run over cond_len in our checks here, as that just pushes
> +	 * us past the final ".", which cannot match any of our prefixes.
> +	 */
> +	if (skip_prefix(cond, "gitdir:", &value)) {

This would benefit from the skip_prefix_mem I proposed in:

  http://article.gmane.org/gmane.comp.version-control.git/298050

(and which is ae989a61dad98debe9899823ca987305f8e8020d in Junio's tree,
though it is only in pu so far, I think).

That eliminates the need for the comment, and auto-update cond_len, so
that later:

> +		strbuf_add(&pattern, value, cond_len - (value - cond));

...you do not have to do extra computation to get the correct length.

This is a tangent, but I wonder if expand_user_path() should take a
buf/len. It always puts the result into a new strbuf anyway, so it would
not be a big deal to do so. Skimming the output of grep, though, it
looks like this might be the only caller that would be helped.

> +		buf = expand_user_path(pattern.buf);
> +		if (buf) {
> +			strbuf_reset(&pattern);
> +			strbuf_addstr(&pattern, buf);
> +			free(buf);
> +		}

Maybe strbuf_attach() would be shorter here?

> +		} else if (!strchr(pattern.buf, '/')) {
> +			/* no slashes match one directory component */
> +			strbuf_insert(&pattern, 0, "**/", 3);
> +			strbuf_addstr(&pattern, "/**");
> +		}

I guess it's a little funny that "foo" and "foo/bar" are matched quite
differently. I wonder if a simpler rule would just be: relative paths
are unanchored.

-Peff

Re: [PATCH] config: add conditional include

[Duy Nguyen]

On Sun, Jun 26, 2016 at 8:27 PM, Jeff King <peff@peff.net> wrote:
> On Sun, Jun 26, 2016 at 09:06:17AM +0200, Nguyễn Thái Ngọc Duy wrote:
>
>> If the path argument in "include" starts with "gitdir:", it is
>> followed by a wildmatch pattern. The include is only effective if
>> $GIT_DIR matches the pattern. This is very useful to add configuration
>> to a group of repositories.
>
> I think this needs some more introduction to the concept. When you say
> "path argument" here, I assumed you meant the value of include.path. But
> you really mean: we are introducing a new concept for the "subsection"
> field of include.*, which is to provide restrictions for conditional
> includes.

Yep.

>
> It also may be worth discussing the motivation or examples.
>
>> For convenience
>>
>>  - "~" is expanded to $USER
>>
>>  - if the pattern ends with '/', "**" will be appended (e.g. foo/
>>    becomes foo/**). In other words, "foo/" automatically matches
>>    everything in starting with "foo/".
>>
>>  - if the pattern contains no slashes, it's wrapped around by "**/"
>>    and "/**" (e.g. "foo" becomes "**/foo/**"). In other words, "foo"
>>    matches any directory component in $GIT_DIR.
>>
>> The combination of the first two is used to group repositories by
>> path. While the last one could be used to match worktree's basename.
>
> This is a nice description, but it probably belongs in the
> documentation.

Yeah.. just too lazy for proper documentation at this stage.

>
> I don't have any real opinion on the rules themselves, though they seem
> reasonable to me (though in the first one I assume you mean $HOME).

Yep $HOME, what was I thinking...

(skipping all the technical suggestions, will do.. most likely)

>> +             } else if (!strchr(pattern.buf, '/')) {
>> +                     /* no slashes match one directory component */
>> +                     strbuf_insert(&pattern, 0, "**/", 3);
>> +                     strbuf_addstr(&pattern, "/**");
>> +             }
>
> I guess it's a little funny that "foo" and "foo/bar" are matched quite
> differently. I wonder if a simpler rule would just be: relative paths
> are unanchored.

I modeled it after .gitignore patterns, but that's probably not a good
fit here. Making all relative paths un-anchored means I can't say
"paths that end with this suffix". How useful that statement is, I
can't say though. Or if you mean only prepend "**/" to relative paths,
not "/**" then that door is still open.

(after a couple more minutes..) hmm.. I think that "paths that end
with ..." may have its use. The degenerated case is "match $(basename
<path>)", not very useful for gitdir when most of the time $(basename)
is ".git". It could be useful for "worktree:" matching and would
reduce false positives a bit (compared to un-anchoring both ends).
Conditionally including some config per worktree this way is also an
interesting way to deal with per-worktree config, but I need more time
to think about that...
-- 
Duy

Re: [PATCH] config: add conditional include

[Jeff King]

On Mon, Jun 27, 2016 at 06:14:17PM +0200, Duy Nguyen wrote:

> >> +             } else if (!strchr(pattern.buf, '/')) {
> >> +                     /* no slashes match one directory component */
> >> +                     strbuf_insert(&pattern, 0, "**/", 3);
> >> +                     strbuf_addstr(&pattern, "/**");
> >> +             }
> >
> > I guess it's a little funny that "foo" and "foo/bar" are matched quite
> > differently. I wonder if a simpler rule would just be: relative paths
> > are unanchored.
> 
> I modeled it after .gitignore patterns, but that's probably not a good
> fit here. Making all relative paths un-anchored means I can't say
> "paths that end with this suffix". How useful that statement is, I
> can't say though. Or if you mean only prepend "**/" to relative paths,
> not "/**" then that door is still open.

I didn't really mean anything, as I had not thought about it that
carefully. :)

You do allow distinguishing the suffix thing with "/" at the end in the
rule above, though. So between the two rules:

  - slash at the end is a shorthand for "/**"

  - no-slash at the beginning (i.e., a non-absolute path) is a shorthand
    for "**/" at the beginning

you should be able to specify anything.

I do agree that there's value in trying to make the rules consistent
with other parts of git, though. I don't know the corner cases of
gitignore and gitattributes well enough to compare off the top of my
head, though (though I suspect you do. :) ).

-Peff

Re: [PATCH] config: add conditional include

[Duy Nguyen]

On Mon, Jun 27, 2016 at 6:20 PM, Jeff King <peff@peff.net> wrote:
> You do allow distinguishing the suffix thing with "/" at the end in the
> rule above, though. So between the two rules:
>
>   - slash at the end is a shorthand for "/**"
>
>   - no-slash at the beginning (i.e., a non-absolute path) is a shorthand
>     for "**/" at the beginning

Neither slash or "./" at the beginning...

> you should be able to specify anything.

...then yeah it looks pretty good. With the exception of  "./" we can
still have paths relative to where the the config file is. For
$HOME/.gitconfig that  eliminates the need for expanding "~"
($HOME/.config/git/config may still need it, unless we allow "../"
too, but that complicates matching).

> I do agree that there's value in trying to make the rules consistent
> with other parts of git, though. I don't know the corner cases of
> gitignore and gitattributes well enough to compare off the top of my
> head, though (though I suspect you do. :) ).

Naah the complication of .gitignore and .gitattributes have long
exceeded my brain's capacity.
-- 
Duy

Re: [PATCH] config: add conditional include

[Jeff King]

On Mon, Jun 27, 2016 at 06:32:39PM +0200, Duy Nguyen wrote:

> On Mon, Jun 27, 2016 at 6:20 PM, Jeff King <peff@peff.net> wrote:
> > You do allow distinguishing the suffix thing with "/" at the end in the
> > rule above, though. So between the two rules:
> >
> >   - slash at the end is a shorthand for "/**"
> >
> >   - no-slash at the beginning (i.e., a non-absolute path) is a shorthand
> >     for "**/" at the beginning
> 
> Neither slash or "./" at the beginning...
> 
> > you should be able to specify anything.
> 
> ...then yeah it looks pretty good. With the exception of  "./" we can
> still have paths relative to where the the config file is. For
> $HOME/.gitconfig that  eliminates the need for expanding "~"
> ($HOME/.config/git/config may still need it, unless we allow "../"
> too, but that complicates matching).

Yeah, I didn't think actual relative paths were that interesting, but if
we declare that they are relative to the config file, I agree that
works pretty well. And I agree that "./" is a convenient way to anchor
them.

-Peff

[PATCH v2 0/2] Config conditional include

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

Second try. The anchoring rules now look a lot better. I cherry picked
Jeff's skip_prefix_mem() for now to avoid dependency.

There's a surprise about core.ignorecase. We are matching paths, so we
should match case-insensitively if core.ignorecase tells us so. And it
gets a bit tricky if core.ignorecase is defined in the same config
file. I don't think we have ever told the user that keys are processed
from top down. We do now.

It makes me wonder if we should allow users the option to match case
insensitively regardless of filesystem too. Something close to
pathspec magic. But that probably has little use in real world for a
lot more work.

The '/' vs '\\' battle on Windows, I'll leave it to Windows guys again.

I'm also not adding "worktree:" condition. If that happens, it
probably happens in the per-worktree config file series.

To leave room for future expansion, perhaps we should declare that ':'
can't appear in the pattern, so we can add more prefix later, and even
stack them, e.g. "regexp:gitdir:<pattern>". The prefix can't be one char
long. That should be enough for windows to specify full path
including the drive letter.

Jeff King (1):
  add skip_prefix_mem helper

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

 Documentation/config.txt  | 40 +++++++++++++++++++++
 config.c                  | 89 +++++++++++++++++++++++++++++++++++++++++++++--
 git-compat-util.h         | 17 +++++++++
 t/t1305-config-include.sh | 45 ++++++++++++++++++++++++
 4 files changed, 189 insertions(+), 2 deletions(-)

-- 
2.8.2.531.gd073806

[PATCH v2 1/2] add skip_prefix_mem helper

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

From: Jeff King <peff@peff.net>

The skip_prefix function has been very useful for
simplifying pointer arithmetic and avoiding repeated magic
numbers, but we have no equivalent for length-limited
buffers. So we're stuck with:

  if (3 <= len && skip_prefix(buf, "foo", &buf))
	  len -= 3;

That's not that complicated, but it needs to use magic
numbers for the length of the prefix (or else write out
strlen("foo"), repeating the string). By using a helper, we
can get the string length behind the scenes (and often at
compile time for string literals).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 git-compat-util.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 49d4029..c99cddc 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -473,6 +473,23 @@ static inline int skip_prefix(const char *str, const char *prefix,
 	return 0;
 }
 
+/*
+ * Like skip_prefix, but promises never to read past "len" bytes of the input
+ * buffer, and returns the remaining number of bytes in "out" via "outlen".
+ */
+static inline int skip_prefix_mem(const char *buf, size_t len,
+				  const char *prefix,
+				  const char **out, size_t *outlen)
+{
+	size_t prefix_len = strlen(prefix);
+	if (prefix_len <= len && !memcmp(buf, prefix, prefix_len)) {
+		*out = buf + prefix_len;
+		*outlen = len - prefix_len;
+		return 1;
+	}
+	return 0;
+}
+
 /*
  * If buf ends with suffix, return 1 and subtract the length of the suffix
  * from *len. Otherwise, return 0 and leave *len untouched.
-- 
2.8.2.531.gd073806

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

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

Main description is already in config.txt. Here is a dev-only note
about Windows support.

While prepare_include_condition_pattern() is Windows-friendly (because
it does not hard code '/'). The reality could be uglier because
internally get_git_dir() may return a path with '/' only or worse, a
mix of '/' and '\\'.

At some point, we need to teach wildmatch() that '/' and '\' should be
treated the same way (via a flag) as well. Then we could care less
about '/' vs '\\'. But a Windows dev probably has to do it.

Helped-by: Jeff King <peff@peff.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/config.txt  | 40 +++++++++++++++++++++
 config.c                  | 89 +++++++++++++++++++++++++++++++++++++++++++++--
 t/t1305-config-include.sh | 45 ++++++++++++++++++++++++
 3 files changed, 172 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 58673cf..c8ad0bf 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -91,6 +91,46 @@ found at the location of the include directive. If the value of the
 relative to the configuration file in which the include directive was
 found.  See below for examples.
 
+Included files can be grouped into subsections where subsectino name
+is the condition when the files are included. The condition starts
+with an condition type string, followed by a colon and a pattern.
+
+Only "gitdir" type is supported, where files are included if
+`$GIT_DIR` matches the specified pattern. For example,
+
+
+	[include "gitdir:/path/to/foo.git"]
+		path = /path/to/foo.inc
+
+would only include "/path/to/foo.inc" if `$GIT_DIR` is
+/path/to/foo.git.
+
+The following pattern is a wildcard pattern with two additional
+wildcards `**/` and `/**`. See linkgit:gitignore[5] for more
+information. For convenience:
+
+ * 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.
+
+ * If the pattern starts with `~/`, `~` will be substitued with the
+   environment variable `HOME`.
+
+ * If the pattern starts with `./`, it is replaced with the directory
+   where the current config file is. For example if the config file
+   that contains the "include" subsection is `$HOME/.gitconfig` then
+   the pattern `./foo` would match the path `$HOME/foo`
+
+A few more notes:
+
+ * Symlinks in `$GIT_DIR` are not resolved before matching.
+
+ * Note that "../" is not special and will match literally, which is
+   unlikely what you want.
+
+ * On case-insensitive file systems, you may need to specify
+   core.ignoreCase before the `include` subsections in order to match
+   case-insensitively if core.ignoreCase is declared in the same file.
 
 Example
 ~~~~~~~
diff --git a/config.c b/config.c
index f51c56b..97c450e 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;
@@ -140,9 +141,89 @@ 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_condition_is_true(const char *cond, int cond_len)
+{
+	const char *value;
+	size_t value_len;
+
+	/* no condition (i.e., "include.path") is always true */
+	if (!cond)
+		return 1;
+
+	if (skip_prefix_mem(cond, cond_len, "gitdir:", &value, &value_len)) {
+		struct strbuf text = STRBUF_INIT;
+		struct strbuf pattern = STRBUF_INIT;
+		int ret, prefix;
+
+		strbuf_add_absolute_path(&text, get_git_dir());
+		strbuf_add(&pattern, value, value_len);
+		prefix = prepare_include_condition_pattern(&pattern);
+
+		if (prefix < 0)
+			return 0;
+
+		if (prefix > 0 &&
+		    (text.len < prefix ||
+		     fspathncmp(pattern.buf, text.buf, prefix)))
+			return 0;
+
+		ret = !wildmatch(pattern.buf + prefix, text.buf + prefix,
+				 ignore_case ? WM_CASEFOLD : 0,
+				 NULL);
+		strbuf_release(&pattern);
+		strbuf_release(&text);
+		return ret;
+	}
+
+	error(_("unrecognized include condition: %.*s"), 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;
 
 	/*
@@ -153,8 +234,12 @@ int git_config_include(const char *var, const char *value, void *data)
 	if (ret < 0)
 		return ret;
 
-	if (!strcmp(var, "include.path"))
-		ret = handle_path_include(value, inc);
+	if (!parse_config_key(var, "include", &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 9ba2ba1..30351f2 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -152,6 +152,51 @@ 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 \"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 \"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 \"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 \"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 'include cycles are detected' '
 	cat >.gitconfig <<-\EOF &&
 	[test]value = gitconfig
-- 
2.8.2.531.gd073806

Re: [PATCH v2 0/2] Config conditional include

[Jeff King]

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

> There's a surprise about core.ignorecase. We are matching paths, so we
> should match case-insensitively if core.ignorecase tells us so. And it
> gets a bit tricky if core.ignorecase is defined in the same config
> file. I don't think we have ever told the user that keys are processed
> from top down. We do now.

Hrm. I'm not excited about introducing ordering issues into the config
parsing. But I think it's actually even more complicated than that.

core.ignorecase is generally about the working tree, not the git
repository directory, which may reside on another filesystem entirely
(though I would not be surprised if we've blurred that line already).

I wonder if it would be that bad to just punt on the issue, and say that
these include-match globs are always case-insensitive, or something.
True, that does not allow one to distinguish between config for "foo"
and "Foo" directories, but I find it unlikely anybody would ever want
to. And if we define it that way from day one, then nobody expects it to
work.

> It makes me wonder if we should allow users the option to match case
> insensitively regardless of filesystem too. Something close to
> pathspec magic. But that probably has little use in real world for a
> lot more work.

Yeah, if we had a builtin syntax for "add these options to the
wildmatch", it wouldn't be so bad, but I think we'd be inventing that
syntax.

> The '/' vs '\\' battle on Windows, I'll leave it to Windows guys again.

Oof. Me too. :)

> To leave room for future expansion, perhaps we should declare that ':'
> can't appear in the pattern, so we can add more prefix later, and even
> stack them, e.g. "regexp:gitdir:<pattern>". The prefix can't be one char
> long. That should be enough for windows to specify full path
> including the drive letter.

It seems unnecessarily restrictive to place rules about what can go in
the pattern provided by the user, when we can easily restrict the
characters on the keyword side. For example "gitdir/regexp:<pattern>" is
a natural extension of "gitdir:<pattern>", and is backwards compatible
as long as we use something besides ":" for the option separator.

-Peff

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

[Jeff King]

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

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

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

I think the first sentence may parse more easily as:

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

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

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

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

Maybe start this as a list like:

    The currently supported keywords are:

    `gitdir`::
    ...

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

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

s/substitued/substituted/

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

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

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

Why did you drop expand_user_path() here?

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

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

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

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

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

-Peff

Re: [PATCH v2 0/2] Config conditional include

[Matthieu Moy]

Jeff King <peff@peff.net> writes:

> On Tue, Jun 28, 2016 at 07:26:39PM +0200, Nguyễn Thái Ngọc Duy wrote:
>
>> There's a surprise about core.ignorecase. We are matching paths, so we
>> should match case-insensitively if core.ignorecase tells us so. And it
>> gets a bit tricky if core.ignorecase is defined in the same config
>> file. I don't think we have ever told the user that keys are processed
>> from top down. We do now.
>
> Hrm. I'm not excited about introducing ordering issues into the config
> parsing.

There's already at least one case of ordering-sensitive variables, that
we encountered when writting the config cache during Tanay Abhra's GSoC:
diff.<driver>.funcname Vs diff.<driver>.xfuncname. Git applies the "last
one wins" policy, which is the normal rule for a single-valued variable,
but in this case, a "funcname" definition can override an "xfuncname"
def. To preserve this behavior we had to introduce ordering in the
cache, but to me this was a design mistake to rely on order.

In short: we already have one, but I'm not excited either about
introducing new ones.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 0/2] Config conditional include

[Jeff King]

On Tue, Jun 28, 2016 at 10:51:15PM +0200, Matthieu Moy wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Tue, Jun 28, 2016 at 07:26:39PM +0200, Nguyễn Thái Ngọc Duy wrote:
> >
> >> There's a surprise about core.ignorecase. We are matching paths, so we
> >> should match case-insensitively if core.ignorecase tells us so. And it
> >> gets a bit tricky if core.ignorecase is defined in the same config
> >> file. I don't think we have ever told the user that keys are processed
> >> from top down. We do now.
> >
> > Hrm. I'm not excited about introducing ordering issues into the config
> > parsing.
> 
> There's already at least one case of ordering-sensitive variables, that
> we encountered when writting the config cache during Tanay Abhra's GSoC:
> diff.<driver>.funcname Vs diff.<driver>.xfuncname. Git applies the "last
> one wins" policy, which is the normal rule for a single-valued variable,
> but in this case, a "funcname" definition can override an "xfuncname"
> def. To preserve this behavior we had to introduce ordering in the
> cache, but to me this was a design mistake to rely on order.
> 
> In short: we already have one, but I'm not excited either about
> introducing new ones.

I still see funcname versus xfuncname as fundamentally a "last one wins"
scenario; it's just that the two options are sort-of synonyms. But we
are still talking about the same linear-ish parsing scheme, and I think
it just makes the implementation a little more complicated.

I'm much more worried about something that impacts how we parse the
config, and is set up in a possibly unrelated config-parsing sequence.
So whether ignorecase will work depends on more variables:

 - are we doing our config parse before or after somebody has called
   git_config() at the start of a program?

 - if before (or during), does our callback call git_default_core_config()?

 - if so, did core.ignorecase appear before our include? (Almost
   certainly not, if our include is in ~/.gitconfig, because we parse
   from least-specific to most-specific).

So here it is not the implementation that is complicated, but the
user-facing behavior. It's very difficult to predict when your include
will kick in, and there is a good chance it will behave differently for
different programs.

In general I think the best bet here is to lazy-load such values from
the config-cache (so we _know_ that we got a complete parse before we
look at the value). But that creates a recursion problem when we try to
lazy-load from inside the config parser itself.

-Peff

Re: [PATCH v2 0/2] Config conditional include

[Junio C Hamano]

Honestly, I'd really prefer to see those with topics in 'pu' that
has seen reviews but not yet updated to go back to and polish them
to help move things forward, with the goal to have them in 'next'
sooner so that we can have fixes and features that are sufficiently
vetted and tested in the next release, before starting yet another
new topic that will be left hanging in 'pu'.

In your case, I am talking about nd/icase, nd/fetch-ref-summary, and
nd/connect-ssh-command-config topics.

Thanks.

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

[Eric Sunshine]

On Tue, Jun 28, 2016 at 1:26 PM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/config.c b/config.c
> @@ -140,9 +141,89 @@ static int handle_path_include(const char *path, struct config_include_data *inc
> +static int include_condition_is_true(const char *cond, int cond_len)
> +{
> +       const char *value;
> +       size_t value_len;
> +
> +       /* no condition (i.e., "include.path") is always true */
> +       if (!cond)
> +               return 1;
> +
> +       if (skip_prefix_mem(cond, cond_len, "gitdir:", &value, &value_len)) {
> +               struct strbuf text = STRBUF_INIT;
> +               struct strbuf pattern = STRBUF_INIT;
> +               int ret, prefix;
> +
> +               strbuf_add_absolute_path(&text, get_git_dir());
> +               strbuf_add(&pattern, value, value_len);
> +               prefix = prepare_include_condition_pattern(&pattern);
> +
> +               if (prefix < 0)
> +                       return 0;
> +
> +               if (prefix > 0 &&
> +                   (text.len < prefix ||
> +                    fspathncmp(pattern.buf, text.buf, prefix)))
> +                       return 0;

Are the above two "return"s leaking 'text' and 'pattern' strbufs?

> +
> +               ret = !wildmatch(pattern.buf + prefix, text.buf + prefix,
> +                                ignore_case ? WM_CASEFOLD : 0,
> +                                NULL);
> +               strbuf_release(&pattern);
> +               strbuf_release(&text);
> +               return ret;
> +       }
> +
> +       error(_("unrecognized include condition: %.*s"), cond_len, cond);
> +       /* unknown conditionals are always false */
> +       return 0;
> +}

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

[Duy Nguyen]

On Tue, Jun 28, 2016 at 10:49 PM, Jeff King <peff@peff.net> wrote:
>> +static int prepare_include_condition_pattern(struct strbuf *pat)
>> +{
>> +     struct strbuf path = STRBUF_INIT;
>> +     int prefix = 0;
>> +
>> +     /* TODO: maybe support ~user/ too */
>> +     if (pat->buf[0] == '~' && is_dir_sep(pat->buf[1])) {
>> +             const char *home = getenv("HOME");
>> +
>> +             if (!home)
>> +                     return error(_("$HOME is not defined"));
>> +
>> +             strbuf_add_absolute_path(&path, home);
>> +             strbuf_splice(pat, 0, 1, path.buf, path.len);
>> +             prefix = path.len + 1 /*slash*/;
>
> Why did you drop expand_user_path() here?
>
> I guess it's to do with knowing the length of the prefix portion? I'm
> not sure I understand why the prefix is necessary. I thought the goal
> was to construct a wildmatch pattern that could be used against
> get_git_dir().

We need to make sure any '*', '?' and '[' in the $HOME (or `pwd`)
portion are not automatically promoted to wildcards. One option is
split the pattern in two parts, the prefix part is matched literally
then the rest passed to wildmatch(). The other option is escape
$HOME/`pwd`, but I think that's more complicated (or at least slower).
-- 
Duy

Re: [PATCH v2 0/2] Config conditional include

[Duy Nguyen]

On Tue, Jun 28, 2016 at 10:28 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jun 28, 2016 at 07:26:39PM +0200, Nguyễn Thái Ngọc Duy wrote:
>
>> There's a surprise about core.ignorecase. We are matching paths, so we
>> should match case-insensitively if core.ignorecase tells us so. And it
>> gets a bit tricky if core.ignorecase is defined in the same config
>> file. I don't think we have ever told the user that keys are processed
>> from top down. We do now.
>
> Hrm. I'm not excited about introducing ordering issues into the config
> parsing. But I think it's actually even more complicated than that.
>
> core.ignorecase is generally about the working tree, not the git
> repository directory, which may reside on another filesystem entirely
> (though I would not be surprised if we've blurred that line already).
>
> I wonder if it would be that bad to just punt on the issue, and say that
> these include-match globs are always case-insensitive, or something.
> True, that does not allow one to distinguish between config for "foo"
> and "Foo" directories, but I find it unlikely anybody would ever want
> to. And if we define it that way from day one, then nobody expects it to
> work.

You already opened a path for this with your gitdir/regexp suggestion:
we could support case-sensitive match with "gitdir:" then
case-insensitive match with "gitdir/i:". Everybody is happy.
-- 
Duy

[PATCH v3] config: add conditional include

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

Helped-by: Jeff King <peff@peff.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 v3 fixes some memory leak and typos. Most importantly it no longer
 depends on core.ignorecase for case-insensitive matching. The choice
 must be explicitly made by the user when they choose "gitdir" or
 "gitdir/i" keyword.

 Documentation/config.txt  |  48 ++++++++++++++++++++++
 config.c                  | 102 +++++++++++++++++++++++++++++++++++++++++++++-
 t/t1305-config-include.sh |  56 +++++++++++++++++++++++++
 3 files changed, 204 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index db05dec..18623ee 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -91,6 +91,43 @@ found at the location of the include directive. If the value of the
 relative to the configuration file in which the include directive was
 found.  See below for examples.
 
+Included files can be grouped into subsections where the subsection
+name is the condition that must be met for the files to be included.
+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 +156,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 "gitdir:/path/to/foo/.git"]
+		path = /path/to/foo.inc
+
+	; include for all repositories inside /path/to/group
+	[include "gitdir:/path/to/group/"]
+		path = /path/to/foo.inc
+
+	; include for all repositories inside $HOME/to/group
+	[include "gitdir:~/to/group/"]
+		path = /path/to/foo.inc
 
 Values
 ~~~~~~
diff --git a/config.c b/config.c
index bea937e..ff44e00 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;
@@ -168,9 +169,102 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 	return ret;
 }
 
+static int prepare_include_condition_pattern(struct strbuf *pat)
+{
+	int prefix = 0;
+
+	/* TODO: maybe support ~user/ too */
+	if (pat->buf[0] == '~' && is_dir_sep(pat->buf[1])) {
+		struct strbuf path = STRBUF_INIT;
+		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*/;
+		strbuf_release(&path);
+	} else if (pat->buf[0] == '.' && is_dir_sep(pat->buf[1])) {
+		struct strbuf path = STRBUF_INIT;
+		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 */;
+		strbuf_release(&path);
+	} 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, "**");
+
+	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;
 
 	/*
@@ -181,8 +275,12 @@ int git_config_include(const char *var, const char *value, void *data)
 	if (ret < 0)
 		return ret;
 
-	if (!strcmp(var, "include.path"))
-		ret = handle_path_include(value, inc);
+	if (!parse_config_key(var, "include", &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 9ba2ba1..83501ec 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 \"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 \"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 \"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 \"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 \"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.9.1.564.gb2f7278

Re: [PATCH v3] config: add conditional include

[Matthieu Moy]

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

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

It's unclear to me whether the whole content of GIT_DIR must match, or
whether the pattern should be included (or a be prefix) of $GIT_DIR.
From this text, I read it as "the whole content", but ...

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

... here it seems it only has to be a prefix.

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

expand_user_path in path.c seems to do the same as you're doing (but
does deal with ~user). Any reason not to use it?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v3] config: add conditional include

[Jeff King]

On Wed, Jul 13, 2016 at 09:21:37AM +0200, Matthieu Moy wrote:

> > +static int prepare_include_condition_pattern(struct strbuf *pat)
> > +{
> > +	int prefix = 0;
> > +
> > +	/* TODO: maybe support ~user/ too */
> > +	if (pat->buf[0] == '~' && is_dir_sep(pat->buf[1])) {
> > +		struct strbuf path = STRBUF_INIT;
> > +		const char *home = getenv("HOME");
> > +
> > +		if (!home)
> > +			return error(_("$HOME is not defined"));
> 
> expand_user_path in path.c seems to do the same as you're doing (but
> does deal with ~user). Any reason not to use it?

I had a similar question, which Duy answered in:

  http://article.gmane.org/gmane.comp.version-control.git/298528

It does feel pretty hacky, though (especially for a case that seems
unlikely to come up: people having wildcard patterns in the name of
their home directory).

-Peff

Re: [PATCH v3] config: add conditional include

[Matthieu Moy]

Jeff King <peff@peff.net> writes:

> On Wed, Jul 13, 2016 at 09:21:37AM +0200, Matthieu Moy wrote:
>
>> > +static int prepare_include_condition_pattern(struct strbuf *pat)
>> > +{
>> > +	int prefix = 0;
>> > +
>> > +	/* TODO: maybe support ~user/ too */
>> > +	if (pat->buf[0] == '~' && is_dir_sep(pat->buf[1])) {
>> > +		struct strbuf path = STRBUF_INIT;
>> > +		const char *home = getenv("HOME");
>> > +
>> > +		if (!home)
>> > +			return error(_("$HOME is not defined"));
>> 
>> expand_user_path in path.c seems to do the same as you're doing (but
>> does deal with ~user). Any reason not to use it?
>
> I had a similar question, which Duy answered in:
>
>   http://article.gmane.org/gmane.comp.version-control.git/298528
>
> It does feel pretty hacky, though (especially for a case that seems
> unlikely to come up: people having wildcard patterns in the name of
> their home directory).

Ah, OK. Then I'd suggest at least an improvement in the comment:

 		/*
-		 * perform literal matching on the prefix part so that
- 		 * any wildcard character in it can't create side effects.
+		 * perform literal matching on the expanded prefix part
+		 * so that any wildcard character in it (e.g in the
+		 * expansion of ~) can't create side effects.
 		 */

I would also rename the variable prefix -> expanded_prefix. As-is, the
code is really hard to understand IMHO.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v3] config: add conditional include

[Duy Nguyen]

On Wed, Jul 13, 2016 at 9:21 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> +`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:
>
> It's unclear to me whether the whole content of GIT_DIR must match, or
> whether the pattern should be included (or a be prefix) of $GIT_DIR.
> From this text, I read it as "the whole content", but ...
>
>> +     ; include for all repositories inside /path/to/group
>> +     [include "gitdir:/path/to/group/"]
>> +             path = /path/to/foo.inc
>> +
>> +     ; include for all repositories inside $HOME/to/group
>> +     [include "gitdir:~/to/group/"]
>> +             path = /path/to/foo.inc
>
> ... here it seems it only has to be a prefix.

I should have written "with two additional ones... and a few
exceptions"., One of the bullet point below would say the trailing
slash is rewritten to "/**" so it becomes prefix match. If it proves
confusing, I will probably just get rid of that.
-- 
Duy

[PATCH v4] config: add conditional include

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

Helped-by: Jeff King <peff@peff.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 The diff from v3 is mostly clarification in code and document.

    diff --git a/Documentation/config.txt b/Documentation/config.txt
    index 18623ee..d971334 100644
    --- a/Documentation/config.txt
    +++ b/Documentation/config.txt
    @@ -99,10 +99,9 @@ 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:
    +	pattern for files to be included. The pattern shares the same
    +	syntax as patterns in link:gitignore[5] with a few exceptions
    +	below:
     
      * If the pattern starts with `~/`, `~` will be substituted with the
        content of the environment variable `HOME`.
    diff --git a/config.c b/config.c
    index ff44e00..690f3d5 100644
    --- a/config.c
    +++ b/config.c
    @@ -183,6 +183,11 @@ static int prepare_include_condition_pattern(struct strbuf *pat)
     
     		strbuf_add_absolute_path(&path, home);
     		strbuf_splice(pat, 0, 1, path.buf, path.len);
    +		/*
    +		 * This part, path.buf[0..len], should be considered
    +		 * a literal string even if it has wildcards in it,
    +		 * because those wildcards are not wanted by the user.
    +		 */
     		prefix = path.len + 1 /*slash*/;
     		strbuf_release(&path);
     	} else if (pat->buf[0] == '.' && is_dir_sep(pat->buf[1])) {
    @@ -198,6 +203,11 @@ static int prepare_include_condition_pattern(struct strbuf *pat)
     		if (!slash)
     			die("BUG: how is this possible?");
     		strbuf_splice(pat, 0, 1, path.buf, slash - path.buf);
    +		/*
    +		 * This part, path.buf[0..slash], should be consider
    +		 * a literal string even if it has wildcards in it,
    +		 * because those wildcards are not wanted by the user.
    +		 */
     		prefix = slash - path.buf + 1 /* slash */;
     		strbuf_release(&path);
     	} else if (!is_absolute_path(pat->buf))
    @@ -224,8 +234,9 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase)
     
     	if (prefix > 0) {
     		/*
    -		 * perform literal matching on the prefix part so that
    -		 * any wildcard character in it can't create side effects.
    +		 * perform literal matching on the expanded prefix
    +		 * part so that any wildcard character in it (e.g in
    +		 * the expansion of ~) can't create side effects.
     		 */
     		if (text.len < prefix)
     			goto done;
 Documentation/config.txt  |  47 +++++++++++++++++++
 config.c                  | 113 +++++++++++++++++++++++++++++++++++++++++++++-
 t/t1305-config-include.sh |  56 +++++++++++++++++++++++
 3 files changed, 214 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index db05dec..d971334 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -91,6 +91,42 @@ found at the location of the include directive. If the value of the
 relative to the configuration file in which the include directive was
 found.  See below for examples.
 
+Included files can be grouped into subsections where the subsection
+name is the condition that must be met for the files to be included.
+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 shares the same
+	syntax as patterns in link:gitignore[5] with a few exceptions
+	below:
+
+ * 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 +155,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 "gitdir:/path/to/foo/.git"]
+		path = /path/to/foo.inc
+
+	; include for all repositories inside /path/to/group
+	[include "gitdir:/path/to/group/"]
+		path = /path/to/foo.inc
+
+	; include for all repositories inside $HOME/to/group
+	[include "gitdir:~/to/group/"]
+		path = /path/to/foo.inc
 
 Values
 ~~~~~~
diff --git a/config.c b/config.c
index bea937e..690f3d5 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;
@@ -168,9 +169,113 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 	return ret;
 }
 
+static int prepare_include_condition_pattern(struct strbuf *pat)
+{
+	int prefix = 0;
+
+	/* TODO: maybe support ~user/ too */
+	if (pat->buf[0] == '~' && is_dir_sep(pat->buf[1])) {
+		struct strbuf path = STRBUF_INIT;
+		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);
+		/*
+		 * This part, path.buf[0..len], should be considered
+		 * a literal string even if it has wildcards in it,
+		 * because those wildcards are not wanted by the user.
+		 */
+		prefix = path.len + 1 /*slash*/;
+		strbuf_release(&path);
+	} else if (pat->buf[0] == '.' && is_dir_sep(pat->buf[1])) {
+		struct strbuf path = STRBUF_INIT;
+		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);
+		/*
+		 * This part, path.buf[0..slash], should be consider
+		 * a literal string even if it has wildcards in it,
+		 * because those wildcards are not wanted by the user.
+		 */
+		prefix = slash - path.buf + 1 /* slash */;
+		strbuf_release(&path);
+	} 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, "**");
+
+	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 expanded prefix
+		 * part so that any wildcard character in it (e.g in
+		 * the expansion of ~) 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;
 
 	/*
@@ -181,8 +286,12 @@ int git_config_include(const char *var, const char *value, void *data)
 	if (ret < 0)
 		return ret;
 
-	if (!strcmp(var, "include.path"))
-		ret = handle_path_include(value, inc);
+	if (!parse_config_key(var, "include", &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 9ba2ba1..83501ec 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 \"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 \"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 \"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 \"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 \"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.9.1.566.gbd532d4

Re: [PATCH v4] config: add conditional include

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 788 bytes --]

Hi Duy,

On Thu, 14 Jul 2016, Nguyễn Thái Ngọc Duy wrote:

> Helped-by: Jeff King <peff@peff.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

This commit message is quite a bit short on details. I still fail to see
what use case this would benefit, and I already start to suspect that the
change would open a can of worms that might not be desired.

> +	; include if $GIT_DIR is /path/to/foo/.git
> +	[include "gitdir:/path/to/foo/.git"]
> +		path = /path/to/foo.inc

I find this way to specify a conditional unintuitive. Reading
"gitdir:/path/to/foo/.git" suggests to me that the Git dir is *re-set*,
not that this is a condition.

I would have expected something like

	[include "if-gitdir-is:/path/to/foo/.git"]

instead.

Ciao,
Dscho

Re: [PATCH v4] config: add conditional include

[Duy Nguyen]

On Thu, Jul 14, 2016 at 5:53 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Duy,
>
> On Thu, 14 Jul 2016, Nguyễn Thái Ngọc Duy wrote:
>
>> Helped-by: Jeff King <peff@peff.com>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> This commit message is quite a bit short on details.

Because it's fully documented in config.txt.

> I still fail to see what use case this would benefit,

It allows you to group repos together. The first mail that started all
this [1] is one of the use cases: you may want to use one identity in
a group of repos, and another identity in some other. I want some
more, to use a private key one some repos and another private key on
some other. Some more settings may be shareable this way, like coding
style-related (trailing spaces or not...)

[1] http://thread.gmane.org/gmane.comp.version-control.git/273811

> and I already start to suspect that the change would open a can of worms that might not be desired.

You can choose not to use it, I guess. From what I see it's nothing
super tricky. The config hierarchy we have now is: system -> per-user
-> repo. This could change this to: system -> per-user ->
per-directory -> repo. You could create a different hierarchy, but
with git-config now showing where a config var comes from, it's not
hard to troubleshoot.

>> +     ; include if $GIT_DIR is /path/to/foo/.git
>> +     [include "gitdir:/path/to/foo/.git"]
>> +             path = /path/to/foo.inc
>
> I find this way to specify a conditional unintuitive. Reading
> "gitdir:/path/to/foo/.git" suggests to me that the Git dir is *re-set*,
> not that this is a condition.

Well.. to me it's no different than [remote "foo"] to apply stuff to "foo".

> I would have expected something like
>
>         [include "if-gitdir-is:/path/to/foo/.git"]
>
> instead.

If we do this, should we change gitdir/i to
if-git-dir-case-insensitively-is ? I think we are supposed to read
documents before using any feature, not just guessing then trial and
error.
-- 
Duy

Re: [PATCH v4] config: add conditional include

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2926 bytes --]

Hi Duy,

On Thu, 14 Jul 2016, Duy Nguyen wrote:

> On Thu, Jul 14, 2016 at 5:53 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Hi Duy,
> >
> > On Thu, 14 Jul 2016, Nguyễn Thái Ngọc Duy wrote:
> >
> >> Helped-by: Jeff King <peff@peff.com>
> >> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> >
> > This commit message is quite a bit short on details.
> 
> Because it's fully documented in config.txt.

Surely there is more information left for the commit message, such as:
what motivated the patch, what alternative solutions were considered, was
any thought given to how this might break down, etc

> > I still fail to see what use case this would benefit,
> 
> It allows you to group repos together. The first mail that started all
> this [1] is one of the use cases: you may want to use one identity in
> a group of repos, and another identity in some other. I want some
> more, to use a private key one some repos and another private key on
> some other. Some more settings may be shareable this way, like coding
> style-related (trailing spaces or not...)
> 
> [1] http://thread.gmane.org/gmane.comp.version-control.git/273811
> 
> > and I already start to suspect that the change would open a can of worms that might not be desired.
> 
> You can choose not to use it, I guess.

Sadly, as the maintainer I am unable to share in that luxury of yours.

> From what I see it's nothing super tricky. The config hierarchy we have
> now is: system -> per-user -> repo. This could change this to: system ->
> per-user -> per-directory -> repo. You could create a different
> hierarchy, but with git-config now showing where a config var comes
> from, it's not hard to troubleshoot.

The more indirection you allow, the harder it gets. And that problem is
exacerbated when allowing conditional includes.

> >> +     ; include if $GIT_DIR is /path/to/foo/.git
> >> +     [include "gitdir:/path/to/foo/.git"]
> >> +             path = /path/to/foo.inc
> >
> > I find this way to specify a conditional unintuitive. Reading
> > "gitdir:/path/to/foo/.git" suggests to me that the Git dir is *re-set*,
> > not that this is a condition.
> 
> Well.. to me it's no different than [remote "foo"] to apply stuff to "foo".

Except that "include" is an imperative and "remote" is not.

Quite frankly, this conditional business scares me. If you introduce it
for [include], users will want it for every config setting. And the
current syntax is just not up to, say, making user.name conditional on
anything.

As an alternative solution to your problem, you could of course avoid all
conditional includes. Simply by adding the include.path settings
explicitly to the configs that require them. Now, that would make reasoning
and trouble-shooting simple, wouldn't it?

And the most beautiful aspect of it: no patch needed.

Ciao,
Dscho

Re: [PATCH v4] config: add conditional include

[Duy Nguyen]

On Sat, Jul 16, 2016 at 3:30 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> As an alternative solution to your problem, you could of course avoid all
> conditional includes. Simply by adding the include.path settings
> explicitly to the configs that require them. Now, that would make reasoning
> and trouble-shooting simple, wouldn't it?

I can't. Repos can be created and destroyed often (it's in the
process), and there are many of them. Using a wrong identity (among
other incorrect config settings) is a serious problem and I cannot
guarantee myself that I will never make a mistake, forgetting to
include stuff on new clones.

>> > I still fail to see what use case this would benefit,
>>
>> It allows you to group repos together. The first mail that started all
>> this [1] is one of the use cases: you may want to use one identity in
>> a group of repos, and another identity in some other. I want some
>> more, to use a private key one some repos and another private key on
>> some other. Some more settings may be shareable this way, like coding
>> style-related (trailing spaces or not...)
>>
>> [1] http://thread.gmane.org/gmane.comp.version-control.git/273811
>>
>> > and I already start to suspect that the change would open a can of worms that might not be desired.
>>
>> You can choose not to use it, I guess.
>
> Sadly, as the maintainer I am unable to share in that luxury of yours.

I need this. And I post it because people may need it too. But if it's
a bad thing, I guess I'll keep this patch on my tree then.
-- 
Duy

Re: [PATCH v4] config: add conditional include

[Jeff King]

On Sat, Jul 16, 2016 at 03:30:45PM +0200, Johannes Schindelin wrote:

> > >> +     ; include if $GIT_DIR is /path/to/foo/.git
> > >> +     [include "gitdir:/path/to/foo/.git"]
> > >> +             path = /path/to/foo.inc
> > >
> > > I find this way to specify a conditional unintuitive. Reading
> > > "gitdir:/path/to/foo/.git" suggests to me that the Git dir is *re-set*,
> > > not that this is a condition.
> > 
> > Well.. to me it's no different than [remote "foo"] to apply stuff to "foo".
> 
> Except that "include" is an imperative and "remote" is not.

In the very original version of config includes, I had planned out:

  [include-if "...some condition..."]
  path = ...

Later, since "[include ...]" had no other meaning, I think it got
shortened in discussion. But it would be easy to accept include-if (or
even accept either, for maximum confusion :) ).

> Quite frankly, this conditional business scares me. If you introduce it
> for [include], users will want it for every config setting. And the
> current syntax is just not up to, say, making user.name conditional on
> anything.

They already have it for every config setting with this. The reason to
add it to [include] and not as a general syntax is that you can put
user.name into your included file, and then conditionally include it.

That is not as nice as "if this then that" in a single file, but it is
backwards compatible with the existing syntax, and is probably fine in
practice. Each included file becomes a "profile" of multiple settings
that you apply.

> As an alternative solution to your problem, you could of course avoid all
> conditional includes. Simply by adding the include.path settings
> explicitly to the configs that require them. Now, that would make reasoning
> and trouble-shooting simple, wouldn't it?
> 
> And the most beautiful aspect of it: no patch needed.

And you can just "cat" the included files directly into your
.git/config. We don't even need include.path. Or ~/.gitconfig, for that
matter. But sometimes dynamic things are convenient.

-Peff

Re: [PATCH v4] config: add conditional include

[Johannes Schindelin]

Hi Peff,

On Sat, 16 Jul 2016, Jeff King wrote:

> On Sat, Jul 16, 2016 at 03:30:45PM +0200, Johannes Schindelin wrote:
> 
> > As an alternative solution to your problem, you could of course avoid all
> > conditional includes. Simply by adding the include.path settings
> > explicitly to the configs that require them. Now, that would make reasoning
> > and trouble-shooting simple, wouldn't it?
> > 
> > And the most beautiful aspect of it: no patch needed.
> 
> And you can just "cat" the included files directly into your
> .git/config. We don't even need include.path. Or ~/.gitconfig, for that
> matter. But sometimes dynamic things are convenient.

Well, apparently you are not convinced by my argument. I thought it was
pretty sound, but if you disagree, it cannot have been...

So I'll shut up ;-)

Ciao,
Dscho

Re: [PATCH v4] config: add conditional include

[Jeff King]

On Sat, Jul 16, 2016 at 06:36:03PM +0200, Johannes Schindelin wrote:

> > On Sat, Jul 16, 2016 at 03:30:45PM +0200, Johannes Schindelin wrote:
> > 
> > > As an alternative solution to your problem, you could of course avoid all
> > > conditional includes. Simply by adding the include.path settings
> > > explicitly to the configs that require them. Now, that would make reasoning
> > > and trouble-shooting simple, wouldn't it?
> > > 
> > > And the most beautiful aspect of it: no patch needed.
> > 
> > And you can just "cat" the included files directly into your
> > .git/config. We don't even need include.path. Or ~/.gitconfig, for that
> > matter. But sometimes dynamic things are convenient.
> 
> Well, apparently you are not convinced by my argument. I thought it was
> pretty sound, but if you disagree, it cannot have been...

Heh. Don't get me wrong, I do think there's room for digging ourselves a
deep hole with conditional includes, because anything dynamic opens up a
question of _when_ it is evaluated, and in what context. So any
condition should be something that we would consider static through the
whole run of the program. Looking at the "gitdir" is right on the
borderline of that, but I think it's OK, because we already have to
invalidate any cached config when we setup the gitdir, because
"$GIT_DIR/config" will have become a new source.

So I agree it's something we need to be thoughtful about, but I think
this particular instance is useful and probably not going to bite us.

-Peff

Re: [PATCH v4] config: add conditional include

[Johannes Schindelin]

Hi Peff,

On Sat, 16 Jul 2016, Jeff King wrote:

> On Sat, Jul 16, 2016 at 06:36:03PM +0200, Johannes Schindelin wrote:
> 
> > > On Sat, Jul 16, 2016 at 03:30:45PM +0200, Johannes Schindelin wrote:
> > > 
> > > > As an alternative solution to your problem, you could of course avoid all
> > > > conditional includes. Simply by adding the include.path settings
> > > > explicitly to the configs that require them. Now, that would make reasoning
> > > > and trouble-shooting simple, wouldn't it?
> > > > 
> > > > And the most beautiful aspect of it: no patch needed.
> > > 
> > > And you can just "cat" the included files directly into your
> > > .git/config. We don't even need include.path. Or ~/.gitconfig, for that
> > > matter. But sometimes dynamic things are convenient.
> > 
> > Well, apparently you are not convinced by my argument. I thought it was
> > pretty sound, but if you disagree, it cannot have been...
> 
> Heh. Don't get me wrong, I do think there's room for digging ourselves a
> deep hole with conditional includes, because anything dynamic opens up a
> question of _when_ it is evaluated, and in what context. So any
> condition should be something that we would consider static through the
> whole run of the program. Looking at the "gitdir" is right on the
> borderline of that, but I think it's OK, because we already have to
> invalidate any cached config when we setup the gitdir, because
> "$GIT_DIR/config" will have become a new source.
> 
> So I agree it's something we need to be thoughtful about, but I think
> this particular instance is useful and probably not going to bite us.

FWIW I am slightly less worried about the conditional includes (it is
already a horrible mess to figure out too-long include chains now, before
having conditional includes, for example). I am slightly more worried
about eventually needing to introduce support for something like

	[if-gitdir(...):section]
		thisSettingIsConditional = ...

or even

	[if (worktree==...):section]
		anotherConditional = ...

and then having two incompatible conditional constructs, one generic, the
other one specific to [include].

In other words, if we already introduce a conditional construct, I'd
rather have one that could easily be used for other conditions/sections
when (and if) needed.

I, for one, would rather have my repository-specific overrides in one single
file than having a bunch of files that are included conditionally and may
need to override one another: I can see the entries much easier in the
single file (and group them by section) than in the multiple files. My
working memory is just too filled up with other stuff to remember the
contents of the other file(s).

But I guess that boils down to preference.

Ciao,
Dscho

Re: [PATCH v4] config: add conditional include

[Jeff King]

On Sun, Jul 17, 2016 at 10:15:55AM +0200, Johannes Schindelin wrote:

> FWIW I am slightly less worried about the conditional includes (it is
> already a horrible mess to figure out too-long include chains now, before
> having conditional includes, for example). I am slightly more worried
> about eventually needing to introduce support for something like
> 
> 	[if-gitdir(...):section]
> 		thisSettingIsConditional = ...
> 
> or even
> 
> 	[if (worktree==...):section]
> 		anotherConditional = ...
> 
> and then having two incompatible conditional constructs, one generic, the
> other one specific to [include].
> 
> In other words, if we already introduce a conditional construct, I'd
> rather have one that could easily be used for other conditions/sections
> when (and if) needed.

I had assumed we would resist introducing anything like that, simply
because of backwards compatibility issues with the syntax. But I admit
that was just an assumption in my head; future compatibility with
reality is not guaranteed. :)

-Peff

Re: [PATCH v4] config: add conditional include

[Jakub Narębski]

W dniu 2016-07-16 o 18:47, Jeff King pisze:

> Heh. Don't get me wrong, I do think there's room for digging ourselves a
> deep hole with conditional includes, because anything dynamic opens up a
> question of _when_ it is evaluated, and in what context. So any
> condition should be something that we would consider static through the
> whole run of the program. Looking at the "gitdir" is right on the
> borderline of that, but I think it's OK, because we already have to
> invalidate any cached config when we setup the gitdir, because
> "$GIT_DIR/config" will have become a new source.
> 
> So I agree it's something we need to be thoughtful about, but I think
> this particular instance is useful and probably not going to bite us.

A question: we have a way to track where the value came from (tracking
includes). Do we have a way to check where the value didn't came from,
because for example error in the include condition?

-- 
Jakub Narębski

Re: [PATCH v4] config: add conditional include

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Jul 17, 2016 at 10:15:55AM +0200, Johannes Schindelin wrote:
>
>> FWIW I am slightly less worried about the conditional includes (it is
>> already a horrible mess to figure out too-long include chains now, before
>> having conditional includes, for example). I am slightly more worried
>> about eventually needing to introduce support for something like
>> 
>> 	[if-gitdir(...):section]
>> 		thisSettingIsConditional = ...
>> 
>> or even
>> 
>> 	[if (worktree==...):section]
>> 		anotherConditional = ...
>> 
>> and then having two incompatible conditional constructs, one generic, the
>> other one specific to [include].
>> 
>> In other words, if we already introduce a conditional construct, I'd
>> rather have one that could easily be used for other conditions/sections
>> when (and if) needed.
>
> I had assumed we would resist introducing anything like that, simply
> because of backwards compatibility issues with the syntax. But I admit
> that was just an assumption in my head; future compatibility with
> reality is not guaranteed. :)

I actually read that assumption between lines and almost wrote the
same response that begins with "I think the untold assumption ever
since the inclusion mechanism was introduced is..." ;-)

A config file with "[include] path=..." in it would not include from
the named path by ancient verison of Git, but at least it won't
cause its parser to barf, and the assumption has been that it is a
good property to keep when we introduce new and incompatible
features.

I can however understand it if somebody thinks it actually is better
to actively break older Git implementations by forcing them to stop
parsing when we introduce constructs that will lead them to do wrong
things (e.g. missing some configuration defintions by not reading
from the file that the user wanted to be read from), rather than
making them silently ignore.

Re: [PATCH v4] config: add conditional include

[Duy Nguyen]

Ping..

On Thu, Jul 14, 2016 at 10:33 PM, Nguyễn Thái Ngọc Duy
<pclouds@gmail.com> wrote:
> Helped-by: Jeff King <peff@peff.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  The diff from v3 is mostly clarification in code and document.
>
>     diff --git a/Documentation/config.txt b/Documentation/config.txt
>     index 18623ee..d971334 100644
>     --- a/Documentation/config.txt
>     +++ b/Documentation/config.txt
>     @@ -99,10 +99,9 @@ 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:
>     +   pattern for files to be included. The pattern shares the same
>     +   syntax as patterns in link:gitignore[5] with a few exceptions
>     +   below:
>
>       * If the pattern starts with `~/`, `~` will be substituted with the
>         content of the environment variable `HOME`.
>     diff --git a/config.c b/config.c
>     index ff44e00..690f3d5 100644
>     --- a/config.c
>     +++ b/config.c
>     @@ -183,6 +183,11 @@ static int prepare_include_condition_pattern(struct strbuf *pat)
>
>                 strbuf_add_absolute_path(&path, home);
>                 strbuf_splice(pat, 0, 1, path.buf, path.len);
>     +           /*
>     +            * This part, path.buf[0..len], should be considered
>     +            * a literal string even if it has wildcards in it,
>     +            * because those wildcards are not wanted by the user.
>     +            */
>                 prefix = path.len + 1 /*slash*/;
>                 strbuf_release(&path);
>         } else if (pat->buf[0] == '.' && is_dir_sep(pat->buf[1])) {
>     @@ -198,6 +203,11 @@ static int prepare_include_condition_pattern(struct strbuf *pat)
>                 if (!slash)
>                         die("BUG: how is this possible?");
>                 strbuf_splice(pat, 0, 1, path.buf, slash - path.buf);
>     +           /*
>     +            * This part, path.buf[0..slash], should be consider
>     +            * a literal string even if it has wildcards in it,
>     +            * because those wildcards are not wanted by the user.
>     +            */
>                 prefix = slash - path.buf + 1 /* slash */;
>                 strbuf_release(&path);
>         } else if (!is_absolute_path(pat->buf))
>     @@ -224,8 +234,9 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase)
>
>         if (prefix > 0) {
>                 /*
>     -            * perform literal matching on the prefix part so that
>     -            * any wildcard character in it can't create side effects.
>     +            * perform literal matching on the expanded prefix
>     +            * part so that any wildcard character in it (e.g in
>     +            * the expansion of ~) can't create side effects.
>                  */
>                 if (text.len < prefix)
>                         goto done;
>  Documentation/config.txt  |  47 +++++++++++++++++++
>  config.c                  | 113 +++++++++++++++++++++++++++++++++++++++++++++-
>  t/t1305-config-include.sh |  56 +++++++++++++++++++++++
>  3 files changed, 214 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index db05dec..d971334 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -91,6 +91,42 @@ found at the location of the include directive. If the value of the
>  relative to the configuration file in which the include directive was
>  found.  See below for examples.
>
> +Included files can be grouped into subsections where the subsection
> +name is the condition that must be met for the files to be included.
> +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 shares the same
> +       syntax as patterns in link:gitignore[5] with a few exceptions
> +       below:
> +
> + * 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 +155,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 "gitdir:/path/to/foo/.git"]
> +               path = /path/to/foo.inc
> +
> +       ; include for all repositories inside /path/to/group
> +       [include "gitdir:/path/to/group/"]
> +               path = /path/to/foo.inc
> +
> +       ; include for all repositories inside $HOME/to/group
> +       [include "gitdir:~/to/group/"]
> +               path = /path/to/foo.inc
>
>  Values
>  ~~~~~~
> diff --git a/config.c b/config.c
> index bea937e..690f3d5 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;
> @@ -168,9 +169,113 @@ static int handle_path_include(const char *path, struct config_include_data *inc
>         return ret;
>  }
>
> +static int prepare_include_condition_pattern(struct strbuf *pat)
> +{
> +       int prefix = 0;
> +
> +       /* TODO: maybe support ~user/ too */
> +       if (pat->buf[0] == '~' && is_dir_sep(pat->buf[1])) {
> +               struct strbuf path = STRBUF_INIT;
> +               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);
> +               /*
> +                * This part, path.buf[0..len], should be considered
> +                * a literal string even if it has wildcards in it,
> +                * because those wildcards are not wanted by the user.
> +                */
> +               prefix = path.len + 1 /*slash*/;
> +               strbuf_release(&path);
> +       } else if (pat->buf[0] == '.' && is_dir_sep(pat->buf[1])) {
> +               struct strbuf path = STRBUF_INIT;
> +               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);
> +               /*
> +                * This part, path.buf[0..slash], should be consider
> +                * a literal string even if it has wildcards in it,
> +                * because those wildcards are not wanted by the user.
> +                */
> +               prefix = slash - path.buf + 1 /* slash */;
> +               strbuf_release(&path);
> +       } 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, "**");
> +
> +       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 expanded prefix
> +                * part so that any wildcard character in it (e.g in
> +                * the expansion of ~) 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;
>
>         /*
> @@ -181,8 +286,12 @@ int git_config_include(const char *var, const char *value, void *data)
>         if (ret < 0)
>                 return ret;
>
> -       if (!strcmp(var, "include.path"))
> -               ret = handle_path_include(value, inc);
> +       if (!parse_config_key(var, "include", &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 9ba2ba1..83501ec 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 \"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 \"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 \"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 \"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 \"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.9.1.566.gbd532d4
>



-- 
Duy

Re: [PATCH v4] config: add conditional include

[Jeff King]

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

  - the syntax might be more clear as:

       [include-if "gitdir:..."]

    or

       [include "gitdir-is:..."]

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

-Peff

Re: [PATCH v4] config: add conditional include

[Jakub Narębski]

W dniu 19.08.2016 o 15:54, Jeff King pisze:
> 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")
> 
>   - the syntax might be more clear as:
> 
>        [include-if "gitdir:..."]
> 
>     or
> 
>        [include "gitdir-is:..."]

Or

         [include "if-gitdir:..."]

to continue bikeshedding.

-- 
Jakub Narębski

Re: [PATCH v4] config: add conditional include

[Duy Nguyen]

On Sun, Aug 21, 2016 at 4:08 AM, Jakub Narębski <jnareb@gmail.com> wrote:
> W dniu 19.08.2016 o 15:54, Jeff King pisze:
>> 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")
>>
>>   - the syntax might be more clear as:
>>
>>        [include-if "gitdir:..."]
>>
>>     or
>>
>>        [include "gitdir-is:..."]
>
> Or
>
>          [include "if-gitdir:..."]

I like this one. I can re-roll to address the first two bullet point,
if the last one, the open question, will not become a blocker later
on.
-- 
Duy

Re: [PATCH v4] config: add conditional include

[Matthieu Moy]

Duy Nguyen <pclouds@gmail.com> writes:

> On Sun, Aug 21, 2016 at 4:08 AM, Jakub Narębski <jnareb@gmail.com> wrote:
>> W dniu 19.08.2016 o 15:54, Jeff King pisze:
>>> 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")
>>>
>>>   - the syntax might be more clear as:
>>>
>>>        [include-if "gitdir:..."]
>>>
>>>     or
>>>
>>>        [include "gitdir-is:..."]
>>
>> Or
>>
>>          [include "if-gitdir:..."]
>
> I like this one. I can re-roll to address the first two bullet point,
> if the last one, the open question, will not become a blocker later
> on.

I think the syntax should be design to allow arbitrary boolean
expression later if needed. Then, I prefer one of

  [include-if "gitdir-is:..."]
  [include    "gitdir-is:..."]

because it may later be extended to e.g.

  [include-if "not(gitdir-is:...)"]
  [include-if "gitdir-matches:regex"]
  [include-if "gitdir-is:... and git-version-greater:2.9"]
  ...

I actually already use "conditional include on version number" because I
use push.default=upstream which makes older versions of Git crash, but
fortunately these versions of Git also ignore the "include" directive so
having this push.default=upstream in an included file works. It's a
hack, it worked once but it won't work again.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v4] config: add conditional include

[Duy Nguyen]

On Mon, Aug 22, 2016 at 7:59 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Sun, Aug 21, 2016 at 4:08 AM, Jakub Narębski <jnareb@gmail.com> wrote:
>>> W dniu 19.08.2016 o 15:54, Jeff King pisze:
>>>> 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")
>>>>
>>>>   - the syntax might be more clear as:
>>>>
>>>>        [include-if "gitdir:..."]
>>>>
>>>>     or
>>>>
>>>>        [include "gitdir-is:..."]
>>>
>>> Or
>>>
>>>          [include "if-gitdir:..."]
>>
>> I like this one. I can re-roll to address the first two bullet point,
>> if the last one, the open question, will not become a blocker later
>> on.
>
> I think the syntax should be design to allow arbitrary boolean
> expression later if needed.

I would be against that. We may extend it more in future, but it
should be under control, not full boolean expressions.

> Then, I prefer one of
>
>   [include-if "gitdir-is:..."]
>   [include    "gitdir-is:..."]
>
> because it may later be extended to e.g.
>
>   [include-if "not(gitdir-is:...)"]
>   [include-if "gitdir-matches:regex"]
>   [include-if "gitdir-is:... and git-version-greater:2.9"]
>   ...
>
> I actually already use "conditional include on version number" because I
> use push.default=upstream which makes older versions of Git crash, but
> fortunately these versions of Git also ignore the "include" directive so
> having this push.default=upstream in an included file works. It's a
> hack, it worked once but it won't work again.

We probably have a way to stop old git from reading new git's
features, including ones in config files: the config group
extensions.*. Assuming that "[include "blah"]" is new stuff, git can
be taught to accept that only when extensions.blah is present (which
old git would bail). It discourages adding too many fancy features
(because extensions.* in your config file would be a mess), which is
IMO a good point.
-- 
Duy

Re: [PATCH v4] config: add conditional include

[Matthieu Moy]

Duy Nguyen <pclouds@gmail.com> writes:

> On Mon, Aug 22, 2016 at 7:59 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Duy Nguyen <pclouds@gmail.com> writes:
>>
>>> On Sun, Aug 21, 2016 at 4:08 AM, Jakub Narębski <jnareb@gmail.com> wrote:
>>>> W dniu 19.08.2016 o 15:54, Jeff King pisze:
>>>>> 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")
>>>>>
>>>>>   - the syntax might be more clear as:
>>>>>
>>>>>        [include-if "gitdir:..."]
>>>>>
>>>>>     or
>>>>>
>>>>>        [include "gitdir-is:..."]
>>>>
>>>> Or
>>>>
>>>>          [include "if-gitdir:..."]
>>>
>>> I like this one. I can re-roll to address the first two bullet point,
>>> if the last one, the open question, will not become a blocker later
>>> on.
>>
>> I think the syntax should be design to allow arbitrary boolean
>> expression later if needed.
>
> I would be against that. We may extend it more in future, but it
> should be under control, not full boolean expressions.

Why?

I'm not saying we absolutely need it, but if we allow several kinds of
conditions (gitdir-is:... and others in the future), then it's natural
to allow combining them, and arbitrary boolean expression is both simple
and powerful (operators and/or/not and parenthesis and you have
everything you'll ever need).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v4] config: add conditional include

[Duy Nguyen]

On Mon, Aug 22, 2016 at 8:22 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
>>> I think the syntax should be design to allow arbitrary boolean
>>> expression later if needed.
>>
>> I would be against that. We may extend it more in future, but it
>> should be under control, not full boolean expressions.
>
> Why?
>
> I'm not saying we absolutely need it, but if we allow several kinds of
> conditions (gitdir-is:... and others in the future), then it's natural
> to allow combining them, and arbitrary boolean expression is both simple
> and powerful (operators and/or/not and parenthesis and you have
> everything you'll ever need).

For starter, we don't want another debate "python vs ruby vs lua vs
..." as the scripting language :) (for the record I vote Scheme! maybe
with infix syntax)

Seriously though, for things that are evaluated in the background
every single time a command is executed and things that come from many
different places (/etc, $HOME, $XDG, $GIT_DIR) I think absolute
flexibility just makes it harder to pinpoint when things go wrong.
Combination in this case would be a bad thing, not a good one. By
judging case by case before introducing a new condition type, we
probably can see what sort of combination would be and whether we
could accept that.
-- 
Duy

Re: [PATCH v4] config: add conditional include

[Johannes Schindelin]

Hi Duy,

On Mon, 22 Aug 2016, Duy Nguyen wrote:

> On Mon, Aug 22, 2016 at 8:22 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
> >>> I think the syntax should be design to allow arbitrary boolean
> >>> expression later if needed.
> >>
> >> I would be against that. We may extend it more in future, but it
> >> should be under control, not full boolean expressions.
> >
> > Why?
> >
> > I'm not saying we absolutely need it, but if we allow several kinds of
> > conditions (gitdir-is:... and others in the future), then it's natural
> > to allow combining them, and arbitrary boolean expression is both simple
> > and powerful (operators and/or/not and parenthesis and you have
> > everything you'll ever need).
> 
> For starter, we don't want another debate "python vs ruby vs lua vs
> ..." as the scripting language :) (for the record I vote Scheme! maybe
> with infix syntax)

FWIW I do not think that Matthieu implied that this has to be implemented.
But it does not make sense to slam the door shut prematurely, either.

Meaning: the more doors you can keep open with the new syntax, the better.

Ciao,
Dscho

Re: [PATCH v4] config: add conditional include

[Duy Nguyen]

On Tue, Aug 23, 2016 at 8:42 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Duy,
>
> On Mon, 22 Aug 2016, Duy Nguyen wrote:
>
>> On Mon, Aug 22, 2016 at 8:22 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> >>> I think the syntax should be design to allow arbitrary boolean
>> >>> expression later if needed.
>> >>
>> >> I would be against that. We may extend it more in future, but it
>> >> should be under control, not full boolean expressions.
>> >
>> > Why?
>> >
>> > I'm not saying we absolutely need it, but if we allow several kinds of
>> > conditions (gitdir-is:... and others in the future), then it's natural
>> > to allow combining them, and arbitrary boolean expression is both simple
>> > and powerful (operators and/or/not and parenthesis and you have
>> > everything you'll ever need).
>>
>> For starter, we don't want another debate "python vs ruby vs lua vs
>> ..." as the scripting language :) (for the record I vote Scheme! maybe
>> with infix syntax)
>
> FWIW I do not think that Matthieu implied that this has to be implemented.
> But it does not make sense to slam the door shut prematurely, either.
>
> Meaning: the more doors you can keep open with the new syntax, the better.

It works for me either way. So I'm going to assume we want
"[include-if "gitdir-is:..."]", unless people think it needs more
discussion (then just write something, anything, so I can put it back
in my backlog)
-- 
Duy

Re: [PATCH v4] config: add conditional include

[Jakub Narębski]

W dniu 24.08.2016 o 11:37, Duy Nguyen pisze:

> It works for me either way. So I'm going to assume we want
> "[include-if "gitdir-is:..."]", unless people think it needs more
> discussion (then just write something, anything, so I can put it back
> in my backlog)

A final note: [include "if-gitdir:..."] is inclusive by default
(I think), that is old Git would include it unconditionally,
while [include-if "gitdir-is:..."] is exclusive by default, that
is old Git would ignore it.

But I might be mistaken.

P.S. I personally prefer [include-if ...]
-- 
Jakub Narębski

Re: [PATCH v4] config: add conditional include

[Jeff King]

On Wed, Aug 24, 2016 at 02:44:29PM +0200, Jakub Narębski wrote:

> W dniu 24.08.2016 o 11:37, Duy Nguyen pisze:
> 
> > It works for me either way. So I'm going to assume we want
> > "[include-if "gitdir-is:..."]", unless people think it needs more
> > discussion (then just write something, anything, so I can put it back
> > in my backlog)
> 
> A final note: [include "if-gitdir:..."] is inclusive by default
> (I think), that is old Git would include it unconditionally,
> while [include-if "gitdir-is:..."] is exclusive by default, that
> is old Git would ignore it.
> 
> But I might be mistaken.

I don't think this is the case. Conditionals were considered when
the include feature was added, and anything except "include.path" is
explicitly ignored. So either syntax will behave the same.

-Peff