* [PATCH] config: add conditional include @ 2016-06-26 7:06 Nguyễn Thái Ngọc Duy 2016-06-26 18:27 ` Jeff King 2016-06-28 17:26 ` [PATCH v2 0/2] Config " Nguyễn Thái Ngọc Duy 0 siblings, 2 replies; 46+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2016-06-26 7:06 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, sschuberth, 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 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH] config: add conditional include 2016-06-26 7:06 [PATCH] config: add conditional include Nguyễn Thái Ngọc Duy @ 2016-06-26 18:27 ` Jeff King 2016-06-27 16:14 ` Duy Nguyen 2016-06-28 17:26 ` [PATCH v2 0/2] Config " Nguyễn Thái Ngọc Duy 1 sibling, 1 reply; 46+ messages in thread From: Jeff King @ 2016-06-26 18:27 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano, sschuberth 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH] config: add conditional include 2016-06-26 18:27 ` Jeff King @ 2016-06-27 16:14 ` Duy Nguyen 2016-06-27 16:20 ` Jeff King 0 siblings, 1 reply; 46+ messages in thread From: Duy Nguyen @ 2016-06-27 16:14 UTC (permalink / raw) To: Jeff King; +Cc: Git Mailing List, Junio C Hamano, Sebastian Schuberth 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH] config: add conditional include 2016-06-27 16:14 ` Duy Nguyen @ 2016-06-27 16:20 ` Jeff King 2016-06-27 16:32 ` Duy Nguyen 0 siblings, 1 reply; 46+ messages in thread From: Jeff King @ 2016-06-27 16:20 UTC (permalink / raw) To: Duy Nguyen; +Cc: Git Mailing List, Junio C Hamano, Sebastian Schuberth 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH] config: add conditional include 2016-06-27 16:20 ` Jeff King @ 2016-06-27 16:32 ` Duy Nguyen 2016-06-27 16:35 ` Jeff King 0 siblings, 1 reply; 46+ messages in thread From: Duy Nguyen @ 2016-06-27 16:32 UTC (permalink / raw) To: Jeff King; +Cc: Git Mailing List, Junio C Hamano, Sebastian Schuberth 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH] config: add conditional include 2016-06-27 16:32 ` Duy Nguyen @ 2016-06-27 16:35 ` Jeff King 0 siblings, 0 replies; 46+ messages in thread From: Jeff King @ 2016-06-27 16:35 UTC (permalink / raw) To: Duy Nguyen; +Cc: Git Mailing List, Junio C Hamano, Sebastian Schuberth 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v2 0/2] Config conditional include 2016-06-26 7:06 [PATCH] config: add conditional include Nguyễn Thái Ngọc Duy 2016-06-26 18:27 ` Jeff King @ 2016-06-28 17:26 ` Nguyễn Thái Ngọc Duy 2016-06-28 17:26 ` [PATCH v2 1/2] add skip_prefix_mem helper Nguyễn Thái Ngọc Duy ` (3 more replies) 1 sibling, 4 replies; 46+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2016-06-28 17:26 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, sschuberth, 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v2 1/2] add skip_prefix_mem helper 2016-06-28 17:26 ` [PATCH v2 0/2] Config " Nguyễn Thái Ngọc Duy @ 2016-06-28 17:26 ` Nguyễn Thái Ngọc Duy 2016-06-28 17:26 ` [PATCH v2 2/2] config: add conditional include Nguyễn Thái Ngọc Duy ` (2 subsequent siblings) 3 siblings, 0 replies; 46+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2016-06-28 17:26 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, sschuberth, 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 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH v2 2/2] config: add conditional include 2016-06-28 17:26 ` [PATCH v2 0/2] Config " Nguyễn Thái Ngọc Duy 2016-06-28 17:26 ` [PATCH v2 1/2] add skip_prefix_mem helper Nguyễn Thái Ngọc Duy @ 2016-06-28 17:26 ` Nguyễn Thái Ngọc Duy 2016-06-28 20:49 ` Jeff King ` (2 more replies) 2016-06-28 20:28 ` [PATCH v2 0/2] Config " Jeff King 2016-06-28 22:11 ` Junio C Hamano 3 siblings, 3 replies; 46+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2016-06-28 17:26 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, sschuberth, 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 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH v2 2/2] config: add conditional include 2016-06-28 17:26 ` [PATCH v2 2/2] config: add conditional include Nguyễn Thái Ngọc Duy @ 2016-06-28 20:49 ` Jeff King 2016-06-29 4:06 ` Duy Nguyen 2016-06-28 23:11 ` Eric Sunshine 2016-07-12 16:42 ` [PATCH v3] " Nguyễn Thái Ngọc Duy 2 siblings, 1 reply; 46+ messages in thread From: Jeff King @ 2016-06-28 20:49 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano, sschuberth 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v2 2/2] config: add conditional include 2016-06-28 20:49 ` Jeff King @ 2016-06-29 4:06 ` Duy Nguyen 0 siblings, 0 replies; 46+ messages in thread From: Duy Nguyen @ 2016-06-29 4:06 UTC (permalink / raw) To: Jeff King; +Cc: Git Mailing List, Junio C Hamano, Sebastian Schuberth 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v2 2/2] config: add conditional include 2016-06-28 17:26 ` [PATCH v2 2/2] config: add conditional include Nguyễn Thái Ngọc Duy 2016-06-28 20:49 ` Jeff King @ 2016-06-28 23:11 ` Eric Sunshine 2016-07-12 16:42 ` [PATCH v3] " Nguyễn Thái Ngọc Duy 2 siblings, 0 replies; 46+ messages in thread From: Eric Sunshine @ 2016-06-28 23:11 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy Cc: Git List, Junio C Hamano, Jeff King, Sebastian Schuberth 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; > +} ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v3] config: add conditional include 2016-06-28 17:26 ` [PATCH v2 2/2] config: add conditional include Nguyễn Thái Ngọc Duy 2016-06-28 20:49 ` Jeff King 2016-06-28 23:11 ` Eric Sunshine @ 2016-07-12 16:42 ` Nguyễn Thái Ngọc Duy 2016-07-13 7:21 ` Matthieu Moy 2016-07-14 15:33 ` [PATCH v4] " Nguyễn Thái Ngọc Duy 2 siblings, 2 replies; 46+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2016-07-12 16:42 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, sschuberth, 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 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH v3] config: add conditional include 2016-07-12 16:42 ` [PATCH v3] " Nguyễn Thái Ngọc Duy @ 2016-07-13 7:21 ` Matthieu Moy 2016-07-13 7:26 ` Jeff King 2016-07-13 15:57 ` Duy Nguyen 2016-07-14 15:33 ` [PATCH v4] " Nguyễn Thái Ngọc Duy 1 sibling, 2 replies; 46+ messages in thread From: Matthieu Moy @ 2016-07-13 7:21 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy Cc: git, Junio C Hamano, Jeff King, sschuberth 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/ ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3] config: add conditional include 2016-07-13 7:21 ` Matthieu Moy @ 2016-07-13 7:26 ` Jeff King 2016-07-13 12:48 ` Matthieu Moy 2016-07-13 15:57 ` Duy Nguyen 1 sibling, 1 reply; 46+ messages in thread From: Jeff King @ 2016-07-13 7:26 UTC (permalink / raw) To: Matthieu Moy Cc: Nguyễn Thái Ngọc Duy, git, Junio C Hamano, sschuberth 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3] config: add conditional include 2016-07-13 7:26 ` Jeff King @ 2016-07-13 12:48 ` Matthieu Moy 0 siblings, 0 replies; 46+ messages in thread From: Matthieu Moy @ 2016-07-13 12:48 UTC (permalink / raw) To: Jeff King Cc: Nguyễn Thái Ngọc Duy, git, Junio C Hamano, sschuberth 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/ ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3] config: add conditional include 2016-07-13 7:21 ` Matthieu Moy 2016-07-13 7:26 ` Jeff King @ 2016-07-13 15:57 ` Duy Nguyen 1 sibling, 0 replies; 46+ messages in thread From: Duy Nguyen @ 2016-07-13 15:57 UTC (permalink / raw) To: Matthieu Moy Cc: Git Mailing List, Junio C Hamano, Jeff King, Sebastian Schuberth 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v4] config: add conditional include 2016-07-12 16:42 ` [PATCH v3] " Nguyễn Thái Ngọc Duy 2016-07-13 7:21 ` Matthieu Moy @ 2016-07-14 15:33 ` Nguyễn Thái Ngọc Duy 2016-07-14 15:53 ` Johannes Schindelin 2016-08-13 8:40 ` Duy Nguyen 1 sibling, 2 replies; 46+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2016-07-14 15:33 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, sschuberth, Matthieu Moy, 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 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH v4] config: add conditional include 2016-07-14 15:33 ` [PATCH v4] " Nguyễn Thái Ngọc Duy @ 2016-07-14 15:53 ` Johannes Schindelin 2016-07-14 16:13 ` Duy Nguyen 2016-08-13 8:40 ` Duy Nguyen 1 sibling, 1 reply; 46+ messages in thread From: Johannes Schindelin @ 2016-07-14 15:53 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy Cc: git, Junio C Hamano, Jeff King, sschuberth, Matthieu Moy [-- 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] config: add conditional include 2016-07-14 15:53 ` Johannes Schindelin @ 2016-07-14 16:13 ` Duy Nguyen 2016-07-16 13:30 ` Johannes Schindelin 0 siblings, 1 reply; 46+ messages in thread From: Duy Nguyen @ 2016-07-14 16:13 UTC (permalink / raw) To: Johannes Schindelin Cc: Git Mailing List, Junio C Hamano, Jeff King, Sebastian Schuberth, Matthieu Moy 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] config: add conditional include 2016-07-14 16:13 ` Duy Nguyen @ 2016-07-16 13:30 ` Johannes Schindelin 2016-07-16 14:48 ` Duy Nguyen 2016-07-16 15:08 ` Jeff King 0 siblings, 2 replies; 46+ messages in thread From: Johannes Schindelin @ 2016-07-16 13:30 UTC (permalink / raw) To: Duy Nguyen Cc: Git Mailing List, Junio C Hamano, Jeff King, Sebastian Schuberth, Matthieu Moy [-- 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] config: add conditional include 2016-07-16 13:30 ` Johannes Schindelin @ 2016-07-16 14:48 ` Duy Nguyen 2016-07-16 15:08 ` Jeff King 1 sibling, 0 replies; 46+ messages in thread From: Duy Nguyen @ 2016-07-16 14:48 UTC (permalink / raw) To: Johannes Schindelin Cc: Git Mailing List, Junio C Hamano, Jeff King, Sebastian Schuberth, Matthieu Moy 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] config: add conditional include 2016-07-16 13:30 ` Johannes Schindelin 2016-07-16 14:48 ` Duy Nguyen @ 2016-07-16 15:08 ` Jeff King 2016-07-16 16:36 ` Johannes Schindelin 1 sibling, 1 reply; 46+ messages in thread From: Jeff King @ 2016-07-16 15:08 UTC (permalink / raw) To: Johannes Schindelin Cc: Duy Nguyen, Git Mailing List, Junio C Hamano, Sebastian Schuberth, Matthieu Moy 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] config: add conditional include 2016-07-16 15:08 ` Jeff King @ 2016-07-16 16:36 ` Johannes Schindelin 2016-07-16 16:47 ` Jeff King 0 siblings, 1 reply; 46+ messages in thread From: Johannes Schindelin @ 2016-07-16 16:36 UTC (permalink / raw) To: Jeff King Cc: Duy Nguyen, Git Mailing List, Junio C Hamano, Sebastian Schuberth, Matthieu Moy 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] config: add conditional include 2016-07-16 16:36 ` Johannes Schindelin @ 2016-07-16 16:47 ` Jeff King 2016-07-17 8:15 ` Johannes Schindelin 2016-07-20 16:39 ` Jakub Narębski 0 siblings, 2 replies; 46+ messages in thread From: Jeff King @ 2016-07-16 16:47 UTC (permalink / raw) To: Johannes Schindelin Cc: Duy Nguyen, Git Mailing List, Junio C Hamano, Sebastian Schuberth, Matthieu Moy 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] config: add conditional include 2016-07-16 16:47 ` Jeff King @ 2016-07-17 8:15 ` Johannes Schindelin 2016-07-20 13:31 ` Jeff King 2016-07-20 16:39 ` Jakub Narębski 1 sibling, 1 reply; 46+ messages in thread From: Johannes Schindelin @ 2016-07-17 8:15 UTC (permalink / raw) To: Jeff King Cc: Duy Nguyen, Git Mailing List, Junio C Hamano, Sebastian Schuberth, Matthieu Moy 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] config: add conditional include 2016-07-17 8:15 ` Johannes Schindelin @ 2016-07-20 13:31 ` Jeff King 2016-07-20 22:07 ` Junio C Hamano 0 siblings, 1 reply; 46+ messages in thread From: Jeff King @ 2016-07-20 13:31 UTC (permalink / raw) To: Johannes Schindelin Cc: Duy Nguyen, Git Mailing List, Junio C Hamano, Sebastian Schuberth, Matthieu Moy 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] config: add conditional include 2016-07-20 13:31 ` Jeff King @ 2016-07-20 22:07 ` Junio C Hamano 0 siblings, 0 replies; 46+ messages in thread From: Junio C Hamano @ 2016-07-20 22:07 UTC (permalink / raw) To: Jeff King Cc: Johannes Schindelin, Duy Nguyen, Git Mailing List, Sebastian Schuberth, Matthieu Moy 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. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] config: add conditional include 2016-07-16 16:47 ` Jeff King 2016-07-17 8:15 ` Johannes Schindelin @ 2016-07-20 16:39 ` Jakub Narębski 1 sibling, 0 replies; 46+ messages in thread From: Jakub Narębski @ 2016-07-20 16:39 UTC (permalink / raw) To: Jeff King, Johannes Schindelin Cc: Duy Nguyen, Git Mailing List, Junio C Hamano, Sebastian Schuberth, Matthieu Moy 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] config: add conditional include 2016-07-14 15:33 ` [PATCH v4] " Nguyễn Thái Ngọc Duy 2016-07-14 15:53 ` Johannes Schindelin @ 2016-08-13 8:40 ` Duy Nguyen 2016-08-19 13:54 ` Jeff King 1 sibling, 1 reply; 46+ messages in thread From: Duy Nguyen @ 2016-08-13 8:40 UTC (permalink / raw) To: Git Mailing List, Junio C Hamano Cc: Jeff King, Sebastian Schuberth, Matthieu Moy, Nguyễn Thái Ngọc Duy 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] config: add conditional include 2016-08-13 8:40 ` Duy Nguyen @ 2016-08-19 13:54 ` Jeff King 2016-08-20 21:08 ` Jakub Narębski 0 siblings, 1 reply; 46+ messages in thread From: Jeff King @ 2016-08-19 13:54 UTC (permalink / raw) To: Duy Nguyen Cc: Git Mailing List, Junio C Hamano, Sebastian Schuberth, Matthieu Moy 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] config: add conditional include 2016-08-19 13:54 ` Jeff King @ 2016-08-20 21:08 ` Jakub Narębski 2016-08-22 12:43 ` Duy Nguyen 0 siblings, 1 reply; 46+ messages in thread From: Jakub Narębski @ 2016-08-20 21:08 UTC (permalink / raw) To: Jeff King, Duy Nguyen Cc: Git Mailing List, Junio C Hamano, Sebastian Schuberth, Matthieu Moy 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] config: add conditional include 2016-08-20 21:08 ` Jakub Narębski @ 2016-08-22 12:43 ` Duy Nguyen 2016-08-22 12:59 ` Matthieu Moy 0 siblings, 1 reply; 46+ messages in thread From: Duy Nguyen @ 2016-08-22 12:43 UTC (permalink / raw) To: Jakub Narębski Cc: Jeff King, Git Mailing List, Junio C Hamano, Sebastian Schuberth, Matthieu Moy 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] config: add conditional include 2016-08-22 12:43 ` Duy Nguyen @ 2016-08-22 12:59 ` Matthieu Moy 2016-08-22 13:09 ` Duy Nguyen 0 siblings, 1 reply; 46+ messages in thread From: Matthieu Moy @ 2016-08-22 12:59 UTC (permalink / raw) To: Duy Nguyen Cc: Jakub Narębski, Jeff King, Git Mailing List, Junio C Hamano, Sebastian Schuberth 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/ ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] config: add conditional include 2016-08-22 12:59 ` Matthieu Moy @ 2016-08-22 13:09 ` Duy Nguyen 2016-08-22 13:22 ` Matthieu Moy 0 siblings, 1 reply; 46+ messages in thread From: Duy Nguyen @ 2016-08-22 13:09 UTC (permalink / raw) To: Matthieu Moy Cc: Jakub Narębski, Jeff King, Git Mailing List, Junio C Hamano, Sebastian Schuberth 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] config: add conditional include 2016-08-22 13:09 ` Duy Nguyen @ 2016-08-22 13:22 ` Matthieu Moy 2016-08-22 13:32 ` Duy Nguyen 0 siblings, 1 reply; 46+ messages in thread From: Matthieu Moy @ 2016-08-22 13:22 UTC (permalink / raw) To: Duy Nguyen Cc: Jakub Narębski, Jeff King, Git Mailing List, Junio C Hamano, Sebastian Schuberth 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/ ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] config: add conditional include 2016-08-22 13:22 ` Matthieu Moy @ 2016-08-22 13:32 ` Duy Nguyen 2016-08-23 13:42 ` Johannes Schindelin 0 siblings, 1 reply; 46+ messages in thread From: Duy Nguyen @ 2016-08-22 13:32 UTC (permalink / raw) To: Matthieu Moy Cc: Jakub Narębski, Jeff King, Git Mailing List, Junio C Hamano, Sebastian Schuberth 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] config: add conditional include 2016-08-22 13:32 ` Duy Nguyen @ 2016-08-23 13:42 ` Johannes Schindelin 2016-08-24 9:37 ` Duy Nguyen 0 siblings, 1 reply; 46+ messages in thread From: Johannes Schindelin @ 2016-08-23 13:42 UTC (permalink / raw) To: Duy Nguyen Cc: Matthieu Moy, Jakub Narębski, Jeff King, Git Mailing List, Junio C Hamano, Sebastian Schuberth 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] config: add conditional include 2016-08-23 13:42 ` Johannes Schindelin @ 2016-08-24 9:37 ` Duy Nguyen 2016-08-24 12:44 ` Jakub Narębski 0 siblings, 1 reply; 46+ messages in thread From: Duy Nguyen @ 2016-08-24 9:37 UTC (permalink / raw) To: Johannes Schindelin Cc: Matthieu Moy, Jakub Narębski, Jeff King, Git Mailing List, Junio C Hamano, Sebastian Schuberth 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] config: add conditional include 2016-08-24 9:37 ` Duy Nguyen @ 2016-08-24 12:44 ` Jakub Narębski 2016-08-24 14:17 ` Jeff King 0 siblings, 1 reply; 46+ messages in thread From: Jakub Narębski @ 2016-08-24 12:44 UTC (permalink / raw) To: Duy Nguyen, Johannes Schindelin Cc: Matthieu Moy, Jeff King, Git Mailing List, Junio C Hamano, Sebastian Schuberth 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] config: add conditional include 2016-08-24 12:44 ` Jakub Narębski @ 2016-08-24 14:17 ` Jeff King 0 siblings, 0 replies; 46+ messages in thread From: Jeff King @ 2016-08-24 14:17 UTC (permalink / raw) To: Jakub Narębski Cc: Duy Nguyen, Johannes Schindelin, Matthieu Moy, Git Mailing List, Junio C Hamano, Sebastian Schuberth 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v2 0/2] Config conditional include 2016-06-28 17:26 ` [PATCH v2 0/2] Config " Nguyễn Thái Ngọc Duy 2016-06-28 17:26 ` [PATCH v2 1/2] add skip_prefix_mem helper Nguyễn Thái Ngọc Duy 2016-06-28 17:26 ` [PATCH v2 2/2] config: add conditional include Nguyễn Thái Ngọc Duy @ 2016-06-28 20:28 ` Jeff King 2016-06-28 20:51 ` Matthieu Moy 2016-06-29 4:09 ` Duy Nguyen 2016-06-28 22:11 ` Junio C Hamano 3 siblings, 2 replies; 46+ messages in thread From: Jeff King @ 2016-06-28 20:28 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano, sschuberth 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v2 0/2] Config conditional include 2016-06-28 20:28 ` [PATCH v2 0/2] Config " Jeff King @ 2016-06-28 20:51 ` Matthieu Moy 2016-06-28 21:03 ` Jeff King 2016-06-29 4:09 ` Duy Nguyen 1 sibling, 1 reply; 46+ messages in thread From: Matthieu Moy @ 2016-06-28 20:51 UTC (permalink / raw) To: Jeff King Cc: Nguyễn Thái Ngọc Duy, git, Junio C Hamano, sschuberth 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/ ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v2 0/2] Config conditional include 2016-06-28 20:51 ` Matthieu Moy @ 2016-06-28 21:03 ` Jeff King 0 siblings, 0 replies; 46+ messages in thread From: Jeff King @ 2016-06-28 21:03 UTC (permalink / raw) To: Matthieu Moy Cc: Nguyễn Thái Ngọc Duy, git, Junio C Hamano, sschuberth 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v2 0/2] Config conditional include 2016-06-28 20:28 ` [PATCH v2 0/2] Config " Jeff King 2016-06-28 20:51 ` Matthieu Moy @ 2016-06-29 4:09 ` Duy Nguyen 1 sibling, 0 replies; 46+ messages in thread From: Duy Nguyen @ 2016-06-29 4:09 UTC (permalink / raw) To: Jeff King; +Cc: Git Mailing List, Junio C Hamano, Sebastian Schuberth 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v2 0/2] Config conditional include 2016-06-28 17:26 ` [PATCH v2 0/2] Config " Nguyễn Thái Ngọc Duy ` (2 preceding siblings ...) 2016-06-28 20:28 ` [PATCH v2 0/2] Config " Jeff King @ 2016-06-28 22:11 ` Junio C Hamano 3 siblings, 0 replies; 46+ messages in thread From: Junio C Hamano @ 2016-06-28 22:11 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, Jeff King, sschuberth 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. ^ permalink raw reply [flat|nested] 46+ messages in thread
end of thread, other threads:[~2016-08-24 14:17 UTC | newest] Thread overview: 46+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-26 7:06 [PATCH] config: add conditional include Nguyễn Thái Ngọc Duy 2016-06-26 18:27 ` Jeff King 2016-06-27 16:14 ` Duy Nguyen 2016-06-27 16:20 ` Jeff King 2016-06-27 16:32 ` Duy Nguyen 2016-06-27 16:35 ` Jeff King 2016-06-28 17:26 ` [PATCH v2 0/2] Config " Nguyễn Thái Ngọc Duy 2016-06-28 17:26 ` [PATCH v2 1/2] add skip_prefix_mem helper Nguyễn Thái Ngọc Duy 2016-06-28 17:26 ` [PATCH v2 2/2] config: add conditional include Nguyễn Thái Ngọc Duy 2016-06-28 20:49 ` Jeff King 2016-06-29 4:06 ` Duy Nguyen 2016-06-28 23:11 ` Eric Sunshine 2016-07-12 16:42 ` [PATCH v3] " Nguyễn Thái Ngọc Duy 2016-07-13 7:21 ` Matthieu Moy 2016-07-13 7:26 ` Jeff King 2016-07-13 12:48 ` Matthieu Moy 2016-07-13 15:57 ` Duy Nguyen 2016-07-14 15:33 ` [PATCH v4] " Nguyễn Thái Ngọc Duy 2016-07-14 15:53 ` Johannes Schindelin 2016-07-14 16:13 ` Duy Nguyen 2016-07-16 13:30 ` Johannes Schindelin 2016-07-16 14:48 ` Duy Nguyen 2016-07-16 15:08 ` Jeff King 2016-07-16 16:36 ` Johannes Schindelin 2016-07-16 16:47 ` Jeff King 2016-07-17 8:15 ` Johannes Schindelin 2016-07-20 13:31 ` Jeff King 2016-07-20 22:07 ` Junio C Hamano 2016-07-20 16:39 ` Jakub Narębski 2016-08-13 8:40 ` Duy Nguyen 2016-08-19 13:54 ` Jeff King 2016-08-20 21:08 ` Jakub Narębski 2016-08-22 12:43 ` Duy Nguyen 2016-08-22 12:59 ` Matthieu Moy 2016-08-22 13:09 ` Duy Nguyen 2016-08-22 13:22 ` Matthieu Moy 2016-08-22 13:32 ` Duy Nguyen 2016-08-23 13:42 ` Johannes Schindelin 2016-08-24 9:37 ` Duy Nguyen 2016-08-24 12:44 ` Jakub Narębski 2016-08-24 14:17 ` Jeff King 2016-06-28 20:28 ` [PATCH v2 0/2] Config " Jeff King 2016-06-28 20:51 ` Matthieu Moy 2016-06-28 21:03 ` Jeff King 2016-06-29 4:09 ` Duy Nguyen 2016-06-28 22:11 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).