| Message ID | 967680d27aa7a2709e528ff989a9dd534886efba.1634077795.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Conditional config includes based on remote URL | expand |
On Tue, Oct 12, 2021 at 03:57:23PM -0700, Jonathan Tan wrote: > This is a feature that supports config file inclusion conditional on > whether the repo has a remote with a URL that matches a glob. > > Similar to my previous work on remote-suggested hooks [1], the main > motivation is to allow remote repo administrators to provide recommended > configs in a way that can be consumed more easily (e.g. through a > package installable by a package manager). But this can also be used by, > say, an individual that wants certain configs to apply to a certain set > of local repos but not others. OK. I was a little wary after reading the subject that this would be "when we are using such a URL", which is full of all kinds of odd corner cases. But if it is "a remote is defined with a matching URL" that makes it a property of the repository, not the operation. I think in general this kind of feature is currently served by just correlating filesystem paths with their function. So with your patch I could do: [includeIf "hasremoteurl:https://myjob.example.com"] path = foo.conf But in general, I'd imagine most people put their repository in ~/work or similar, and just do: [includeIf "gitdir:~/work"] path = foo.conf (and of course you can imagine more subdivisions as necessary). So I find the use-case only sort-of compelling. In general, I'm in favor of adding new includeIf directions even if they're only moderately convenient. But this one is rather sticky, because it is dependent on other config keys being defined. So it introduces a new and complicated ordering issue. Is it worth it? Maybe I'm not being imaginative enough in seeing the use cases. > I marked this as RFC because there are some design points that need to > be resolved: > > - The existing "include" and "includeIf" instructions are executed > immediately, whereas in order to be useful, the execution of > "includeIf hasremoteurl" needs to be delayed until all config files > are read. Are there better ways to do this? Note that this violates the "as if they had been found at the location of the include directive" rule which we advertise to users. I'd imagine that most of the time it doesn't matter, but this is a pretty big exception we'll need to document. Just brainstorming some alternatives: - We could stop the world while we are parsing and do a _new_ parse that just looks at the remote config (in fact, this is the natural thing if you were consulting the regular remote.c code for the list of remotes, because it does its own config parse). That does mean that the remote-conditional includes cannot themselves define new remotes. But I think that is already the case with your patch (and violating that gets you into weird circular problems). - We could simply document that if you want to depend on conditional includes based on a particular remote.*.url existing, then that remote config must appear earlier in the sequence. This is a bit ugly, because I'm sure it will bite somebody eventually. But at the same time, it resolves all of the weird timing issues, and does so in a way that will be easy to match if we have any other config dependencies. > - Is the conditionally-included file allowed to have its own > "include{,If}" instructions? I'm thinking that we should forbid it > because, for example, if we had 4 files as follows: A includes B and > C includes D, and we include A and C in our main config (in that > order), it wouldn't be clear whether B (because A was first included) > or C (because we should execute everything at the same depth first) > should be executed first. (In this patch, I didn't do anything about > includes.) I'd say that A would expand B at the moment it is parsed, by the usual as-if rule. If it has a recursive includeIf on remotes, then my head may explode. I'd argue that we should refuse to do recursive remote-ifs in that case (though all of this is a consequence of the after-the-fact parsing; I'd much prefer one of the alternatives I gave earlier). > - A small one: the exact format of the glob. I probably will treat the > URL like a path. You might want to use the matcher from urlmatch.[ch], which understands things like wildcards. Of course remote "URLs" are not always real syntactically valid URLs, which may make that awkward. Barring that the usual fnmatch glob is probably our best bet. > @@ -319,10 +331,18 @@ static int include_condition_is_true(const struct config_options *opts, > static int git_config_include(const char *var, const char *value, void *data) > { > struct config_include_data *inc = data; > + const char *remote_name; > + size_t remote_name_len; > const char *cond, *key; > size_t cond_len; > int ret; > > + if (!parse_config_key(var, "remote", &remote_name, &remote_name_len, > + &key) && > + remote_name && > + !strcmp(key, "url")) > + string_list_append(&inc->remote_urls, value); So we make a copy of every remote name on the off chance that somebody has an includeIf which looks at it. That feels wasteful, though in practice it's probably not that big a deal. By doing the config parsing ourselves here we're missing out on any other forms of remote, like .git/remotes. Those are old and not widely used, and I'd be OK with skipping them. But we should clearly document that this is matching remote.*.url, not any of the other mechanisms. > [...] I only lightly read the rest of the patch. I didn't see anything obviously wrong, but I think the goal at this point is figuring out the design. -Peff
Jonathan Tan <jonathantanmy@google.com> writes: > I marked this as RFC because there are some design points that need to > be resolved: > > - The existing "include" and "includeIf" instructions are executed > immediately, whereas in order to be useful, the execution of > "includeIf hasremoteurl" needs to be delayed until all config files > are read. Are there better ways to do this? An interesting chicken-and-egg problem. Even if an included configuration file does not have further "include", you may discover there are more remotes, which may add new includes to fire from the top-level configuration file. What if we have multiple remotes? Is it a sufficient match for only one of them match what is mentioned in the includeIf condition? Should all of them must match the pattern instead? Majority, perhaps? Something else? > - Is the conditionally-included file allowed to have its own > "include{,If}" instructions? I'm thinking that we should forbid it > because, for example, if we had 4 files as follows: A includes B and > C includes D, and we include A and C in our main config (in that > order), it wouldn't be clear whether B (because A was first included) > or C (because we should execute everything at the same depth first) > should be executed first. (In this patch, I didn't do anything about > includes.) Interesting. The order of real inclusion obviously would affect the outcome of the "last one wins" rule. And this does not have to be limited to this "hasremote" condition, so we need to design it with a bit of care. Would it be possible for a newly included file to invalidate an earlier condition that was used to decide whether to include another file or not? If not, then you can take a two-pass approach where the first pass is used to decide solely to discover which conditionally included files are taken, clear the slate and the parse these files in the textual order. In the case of your example above, the early part of the primary config would be the first to be read, then comes A's early part, then comes B in its entirety, then the rest of A, and then the middle part of the primary config, then C's early part, then D, and then the rest of C,... you got the idea. If it is possible for an included file to invalidate a condition we have already evaluated to make a decision, it would become messy. For example, we might decide to include another file based on the value we discovered for a config variable: === .git/config === [my] variable [your] variable = false [includeIf "configEq:my.variable==true"] file = fileA but the included file may override the condition, e.g. === fileA === [my] variable = false [your] variable = true and applying the "last one wins" rule becomes messy. I do not offhand know what these $ git config --bool my.variable $ git config --bool your.variable should say, and do not have a good explanation for possible outcomes. Maybe the above example can serve as a way to guide us when we design the types of conditionals we allow in includeIf. This example tells us that it is probably a terrible idea to allow using values of configuration variables as part of "includeIf" condition. There may lead to similar "'hasremoteurl' makes a well-behaved condition, because [remote] are additive and not 'last-one-wins', but we cannot add 'lacksremoteurl' as a condition, because a file we decide to include based on a 'lacks' predicate may invalidate the 'lacks' condition by defining such a remote" design decisions you'd need to make around the URLs of the remotes defined for the repository.
> OK. I was a little wary after reading the subject that this would be > "when we are using such a URL", which is full of all kinds of odd corner > cases. But if it is "a remote is defined with a matching URL" that makes > it a property of the repository, not the operation. > > I think in general this kind of feature is currently served by just > correlating filesystem paths with their function. So with your patch I > could do: > > [includeIf "hasremoteurl:https://myjob.example.com"] > path = foo.conf > > But in general, I'd imagine most people put their repository in ~/work > or similar, and just do: > > [includeIf "gitdir:~/work"] > path = foo.conf > > (and of course you can imagine more subdivisions as necessary). So I > find the use-case only sort-of compelling. In general, I'm in favor of > adding new includeIf directions even if they're only moderately > convenient. But this one is rather sticky, because it is dependent on > other config keys being defined. So it introduces a new and complicated > ordering issue. Is it worth it? Maybe I'm not being imaginative enough > in seeing the use cases. My main use case is for a remote repo administrator to offer a recommended config to anyone who clones that repo. For this, I don't think we can prescribe a local directory structure (e.g. "~/work") without being too restrictive or broad (that is, if the user ends up creating a repo that so happens to match our glob but did not intend the config to apply to it). I did bring up the idea that an individual could use this to have config in one place that affects a subset of remotes, but you're right that they could just do this by putting repositories at different places in the filesystem. > > I marked this as RFC because there are some design points that need to > > be resolved: > > > > - The existing "include" and "includeIf" instructions are executed > > immediately, whereas in order to be useful, the execution of > > "includeIf hasremoteurl" needs to be delayed until all config files > > are read. Are there better ways to do this? > > Note that this violates the "as if they had been found at the location > of the include directive" rule which we advertise to users. I'd imagine > that most of the time it doesn't matter, but this is a pretty big > exception we'll need to document. Yes, that's true. Another thing I just thought of is to add a new "deferIncludeIf" which makes clear the different semantics (deferred include, and perhaps not allow recursive includes). > Just brainstorming some alternatives: > > - We could stop the world while we are parsing and do a _new_ parse > that just looks at the remote config (in fact, this is the natural > thing if you were consulting the regular remote.c code for the list > of remotes, because it does its own config parse). > > That does mean that the remote-conditional includes cannot > themselves define new remotes. But I think that is already the case > with your patch (and violating that gets you into weird circular > problems). Hmm...yes, having a special-case rule that such an included file cannot define new remotes would be complex. > - We could simply document that if you want to depend on conditional > includes based on a particular remote.*.url existing, then that > remote config must appear earlier in the sequence. > > This is a bit ugly, because I'm sure it will bite somebody > eventually. But at the same time, it resolves all of the weird > timing issues, and does so in a way that will be easy to match if we > have any other config dependencies. My main issue with this is that different config files are read at different times, and the repo config (that usually contains the remote) is read last. > > - Is the conditionally-included file allowed to have its own > > "include{,If}" instructions? I'm thinking that we should forbid it > > because, for example, if we had 4 files as follows: A includes B and > > C includes D, and we include A and C in our main config (in that > > order), it wouldn't be clear whether B (because A was first included) > > or C (because we should execute everything at the same depth first) > > should be executed first. (In this patch, I didn't do anything about > > includes.) > > I'd say that A would expand B at the moment it is parsed, by the usual > as-if rule. If it has a recursive includeIf on remotes, then my head may > explode. I'd argue that we should refuse to do recursive remote-ifs in > that case (though all of this is a consequence of the after-the-fact > parsing; I'd much prefer one of the alternatives I gave earlier). If we can't expand in place, I would say that any recursive includes should be refused. But as you said, we could still think about whether in-place expansion can be done before addressing this question. > > - A small one: the exact format of the glob. I probably will treat the > > URL like a path. > > You might want to use the matcher from urlmatch.[ch], which understands > things like wildcards. Of course remote "URLs" are not always real > syntactically valid URLs, which may make that awkward. > > Barring that the usual fnmatch glob is probably our best bet. OK. > So we make a copy of every remote name on the off chance that somebody > has an includeIf which looks at it. That feels wasteful, though in > practice it's probably not that big a deal. > > By doing the config parsing ourselves here we're missing out on any > other forms of remote, like .git/remotes. Those are old and not widely > used, and I'd be OK with skipping them. But we should clearly document > that this is matching remote.*.url, not any of the other mechanisms. Sounds good. > I only lightly read the rest of the patch. I didn't see anything > obviously wrong, but I think the goal at this point is figuring out the > design. Yes, that's right.
> Jonathan Tan <jonathantanmy@google.com> writes: > > > I marked this as RFC because there are some design points that need to > > be resolved: > > > > - The existing "include" and "includeIf" instructions are executed > > immediately, whereas in order to be useful, the execution of > > "includeIf hasremoteurl" needs to be delayed until all config files > > are read. Are there better ways to do this? > > An interesting chicken-and-egg problem. Even if an included > configuration file does not have further "include", you may discover > there are more remotes, which may add new includes to fire from the > top-level configuration file. That's true. We might need to say that such conditional includes are based only on what happened during the main config parsing. > What if we have multiple remotes? Is it a sufficient match for only > one of them match what is mentioned in the includeIf condition? > Should all of them must match the pattern instead? Majority, > perhaps? Something else? I think at least one remote should match. > > - Is the conditionally-included file allowed to have its own > > "include{,If}" instructions? I'm thinking that we should forbid it > > because, for example, if we had 4 files as follows: A includes B and > > C includes D, and we include A and C in our main config (in that > > order), it wouldn't be clear whether B (because A was first included) > > or C (because we should execute everything at the same depth first) > > should be executed first. (In this patch, I didn't do anything about > > includes.) > > Interesting. The order of real inclusion obviously would affect the > outcome of the "last one wins" rule. And this does not have to be > limited to this "hasremote" condition, so we need to design it with > a bit of care. > > Would it be possible for a newly included file to invalidate an > earlier condition that was used to decide whether to include another > file or not? If not, then you can take a two-pass approach where > the first pass is used to decide solely to discover which > conditionally included files are taken, clear the slate and the > parse these files in the textual order. In the case of your example > above, the early part of the primary config would be the first to be > read, then comes A's early part, then comes B in its entirety, then > the rest of A, and then the middle part of the primary config, then > C's early part, then D, and then the rest of C,... you got the idea. > > If it is possible for an included file to invalidate a condition we > have already evaluated to make a decision, it would become messy. > For example, we might decide to include another file based on the > value we discovered for a config variable: > > === .git/config === > [my] variable > [your] variable = false > > [includeIf "configEq:my.variable==true"] > file = fileA > > but the included file may override the condition, e.g. > > === fileA === > [my] variable = false > [your] variable = true > > and applying the "last one wins" rule becomes messy. I do not > offhand know what these > > $ git config --bool my.variable > $ git config --bool your.variable > > should say, and do not have a good explanation for possible > outcomes. In this case, it makes sense to me to think that files are included entirely or not at all, so my.variable would be false and your.variable would be true. I guess the tricky part is something like: === .git/config === [my] variable = true [your] variable = false [includeIf "configEq:my.variable==true"] file = fileA [includeIf "configEq:my.variable==false"] file = fileB === fileA === my.variable = false === fileB === your.variable = true and what my.variable and your.variable would end up being. > Maybe the above example can serve as a way to guide us when we > design the types of conditionals we allow in includeIf. This > example tells us that it is probably a terrible idea to allow using > values of configuration variables as part of "includeIf" condition. Hmm...well, remote.foo.url is a configuration variable. I think that the two-pass approach you describe would work if we prohibit subsequent inclusions. > There may lead to similar "'hasremoteurl' makes a well-behaved > condition, because [remote] are additive and not 'last-one-wins', > but we cannot add 'lacksremoteurl' as a condition, because a file we > decide to include based on a 'lacks' predicate may invalidate the > 'lacks' condition by defining such a remote" design decisions you'd > need to make around the URLs of the remotes defined for the > repository. And if we implement two-pass with no subsequent inclusions, "lacks" would work the same way.
On Wed, Oct 13, 2021 at 11:33:41AM -0700, Jonathan Tan wrote: > > But in general, I'd imagine most people put their repository in ~/work > > or similar, and just do: > > > > [includeIf "gitdir:~/work"] > > path = foo.conf > > > > (and of course you can imagine more subdivisions as necessary). So I > > find the use-case only sort-of compelling. In general, I'm in favor of > > adding new includeIf directions even if they're only moderately > > convenient. But this one is rather sticky, because it is dependent on > > other config keys being defined. So it introduces a new and complicated > > ordering issue. Is it worth it? Maybe I'm not being imaginative enough > > in seeing the use cases. > > My main use case is for a remote repo administrator to offer a > recommended config to anyone who clones that repo. For this, I don't > think we can prescribe a local directory structure (e.g. "~/work") > without being too restrictive or broad (that is, if the user ends up > creating a repo that so happens to match our glob but did not intend the > config to apply to it). Yeah, I agree that it's not quite as turnkey if you have to assume something about the user's directory structure. On the other hand, they have to decide to put the included config file somewhere, too, so it seems like you need to give the user "do something like this" instructions rather than purely something they can copy and paste. I dunno. I guess you can assume they'll put it in ~/.gitconfig-foo or similar, and come up with copy-and-pastable directions from that. I agree that the "match the remote" rule makes things _more_ convenient. Mostly I was just wondering if it changed things enough to merit the complications it introduces. I'm not sure I have an answer, and clearly it's pretty subjective. > > Just brainstorming some alternatives: > > > > - We could stop the world while we are parsing and do a _new_ parse > > that just looks at the remote config (in fact, this is the natural > > thing if you were consulting the regular remote.c code for the list > > of remotes, because it does its own config parse). > > > > That does mean that the remote-conditional includes cannot > > themselves define new remotes. But I think that is already the case > > with your patch (and violating that gets you into weird circular > > problems). > > Hmm...yes, having a special-case rule that such an included file cannot > define new remotes would be complex. I think that's mostly true of your "defer" system, too, unless you keep applying it recursively. The rule is slightly different there: it's not "you can't define new remotes", but rather "you can't do a remote-conditional include based on a remote included by remote-conditional". > > - We could simply document that if you want to depend on conditional > > includes based on a particular remote.*.url existing, then that > > remote config must appear earlier in the sequence. > > > > This is a bit ugly, because I'm sure it will bite somebody > > eventually. But at the same time, it resolves all of the weird > > timing issues, and does so in a way that will be easy to match if we > > have any other config dependencies. > > My main issue with this is that different config files are read at > different times, and the repo config (that usually contains the remote) > is read last. Ah, right. I was thinking of the definitions within a single file, but you're right that the common case would be having the include in ~/.gitconfig, and the remotes defined in $GIT_DIR/config. So yeah, any ordering constraint like that is a non-starter, I'd think. -Peff
> Yeah, I agree that it's not quite as turnkey if you have to assume > something about the user's directory structure. On the other hand, they > have to decide to put the included config file somewhere, too, so it > seems like you need to give the user "do something like this" > instructions rather than purely something they can copy and paste. They can copy and paste instructions to add a package repository (e.g. by editing /etc/apt/sources.list) and then install a package. > I dunno. I guess you can assume they'll put it in ~/.gitconfig-foo or > similar, and come up with copy-and-pastable directions from that. > > I agree that the "match the remote" rule makes things _more_ convenient. > Mostly I was just wondering if it changed things enough to merit the > complications it introduces. I'm not sure I have an answer, and clearly > it's pretty subjective. I am almost done with the implementation, so maybe the community could look at it and concretely see the extent of the complication. > > > Just brainstorming some alternatives: > > > > > > - We could stop the world while we are parsing and do a _new_ parse > > > that just looks at the remote config (in fact, this is the natural > > > thing if you were consulting the regular remote.c code for the list > > > of remotes, because it does its own config parse). > > > > > > That does mean that the remote-conditional includes cannot > > > themselves define new remotes. But I think that is already the case > > > with your patch (and violating that gets you into weird circular > > > problems). > > > > Hmm...yes, having a special-case rule that such an included file cannot > > define new remotes would be complex. > > I think that's mostly true of your "defer" system, too, unless you keep > applying it recursively. The rule is slightly different there: it's not > "you can't define new remotes", but rather "you can't do a > remote-conditional include based on a remote included by > remote-conditional". I was thinking that deferred includes cannot themselves have other deferred includes and that which deferred includes are included would be computed only once, and those would be the only rules. (But I guess this is moot now - we're not doing this approach.) > > > - We could simply document that if you want to depend on conditional > > > includes based on a particular remote.*.url existing, then that > > > remote config must appear earlier in the sequence. > > > > > > This is a bit ugly, because I'm sure it will bite somebody > > > eventually. But at the same time, it resolves all of the weird > > > timing issues, and does so in a way that will be easy to match if we > > > have any other config dependencies. > > > > My main issue with this is that different config files are read at > > different times, and the repo config (that usually contains the remote) > > is read last. > > Ah, right. I was thinking of the definitions within a single file, but > you're right that the common case would be having the include in > ~/.gitconfig, and the remotes defined in $GIT_DIR/config. So yeah, any > ordering constraint like that is a non-starter, I'd think. > > -Peff Yeah. Thanks for continuing to take a look at this.
diff --git a/config.c b/config.c index 365d57833b..448509d549 100644 --- a/config.c +++ b/config.c @@ -125,8 +125,20 @@ struct config_include_data { config_fn_t fn; void *data; const struct config_options *opts; + + /* + * All remote URLs discovered when reading all config files. + */ + struct string_list remote_urls; + + /* + * All "includeif.hasremoteurl:" entries. The item is the URL glob and the + * util is the path (must be freed). + */ + struct string_list include_url_glob_to_path; }; -#define CONFIG_INCLUDE_INIT { 0 } +#define CONFIG_INCLUDE_INIT { .remote_urls = STRING_LIST_INIT_DUP, \ + .include_url_glob_to_path = STRING_LIST_INIT_DUP } static int git_config_include(const char *var, const char *value, void *data); @@ -319,10 +331,18 @@ static int include_condition_is_true(const struct config_options *opts, static int git_config_include(const char *var, const char *value, void *data) { struct config_include_data *inc = data; + const char *remote_name; + size_t remote_name_len; const char *cond, *key; size_t cond_len; int ret; + if (!parse_config_key(var, "remote", &remote_name, &remote_name_len, + &key) && + remote_name && + !strcmp(key, "url")) + string_list_append(&inc->remote_urls, value); + /* * Pass along all values, including "include" directives; this makes it * possible to query information on the includes themselves. @@ -335,9 +355,18 @@ static int git_config_include(const char *var, const char *value, void *data) ret = handle_path_include(value, inc); if (!parse_config_key(var, "includeif", &cond, &cond_len, &key) && - (cond && include_condition_is_true(inc->opts, cond, cond_len)) && - !strcmp(key, "path")) - ret = handle_path_include(value, inc); + cond && !strcmp(key, "path")) { + if (skip_prefix_mem(cond, cond_len, "hasremoteurl:", &cond, + &cond_len)) { + struct string_list_item *item = string_list_append_nodup( + &inc->include_url_glob_to_path, + xmemdupz(cond, cond_len)); + item->util = xstrdup(value); + ret = 0; + } else if (include_condition_is_true(inc->opts, cond, cond_len)) { + ret = handle_path_include(value, inc); + } + } return ret; } @@ -1951,6 +1980,8 @@ int config_with_options(config_fn_t fn, void *data, const struct config_options *opts) { struct config_include_data inc = CONFIG_INCLUDE_INIT; + int ret; + struct string_list_item *glob_item; if (opts->respect_includes) { inc.fn = fn; @@ -1968,17 +1999,40 @@ int config_with_options(config_fn_t fn, void *data, * regular lookup sequence. */ if (config_source && config_source->use_stdin) { - return git_config_from_stdin(fn, data); + ret = git_config_from_stdin(fn, data); } else if (config_source && config_source->file) { - return git_config_from_file(fn, config_source->file, data); + ret = git_config_from_file(fn, config_source->file, data); } else if (config_source && config_source->blob) { struct repository *repo = config_source->repo ? config_source->repo : the_repository; - return git_config_from_blob_ref(fn, repo, config_source->blob, + ret = git_config_from_blob_ref(fn, repo, config_source->blob, data); + } else { + ret = do_git_config_sequence(opts, fn, data); + } + + for_each_string_list_item(glob_item, &inc.include_url_glob_to_path) { + struct strbuf pattern = STRBUF_INIT; + struct string_list_item *url_item; + int found = 0; + + strbuf_addstr(&pattern, glob_item->string); + add_trailing_starstar_for_dir(&pattern); + for_each_string_list_item(url_item, &inc.remote_urls) { + if (!wildmatch(pattern.buf, url_item->string, WM_PATHNAME)) { + found = 1; + break; + } + } + strbuf_release(&pattern); + if (found) { + handle_path_include(glob_item->util, &inc); + } } - return do_git_config_sequence(opts, fn, data); + string_list_clear(&inc.remote_urls, 0); + string_list_clear(&inc.include_url_glob_to_path, 1); + return ret; } static void configset_iter(struct config_set *cs, config_fn_t fn, void *data) diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 9ff46f3b04..4803155f89 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -2387,4 +2387,31 @@ test_expect_success '--get and --get-all with --fixed-value' ' test_must_fail git config --file=config --get-regexp --fixed-value fixed+ non-existent ' +test_expect_success 'includeIf.hasremoteurl' ' + test_create_repo hasremoteurlTest && + + cat >"$(pwd)"/include-this <<-\EOF && + [user] + this = this-is-included + EOF + cat >"$(pwd)"/dont-include-that <<-\EOF && + [user] + that = that-is-not-included + EOF + cat >>hasremoteurlTest/.git/config <<-EOF && + [includeIf "hasremoteurl:foo"] + path = "$(pwd)/include-this" + [includeIf "hasremoteurl:bar"] + path = "$(pwd)/dont-include-that" + [remote "foo"] + url = foo + EOF + + echo this-is-included >expect-this && + git -C hasremoteurlTest config --get user.this >actual-this && + test_cmp expect-this actual-this && + + test_must_fail git -C hasremoteurlTest config --get user.that +' + test_done
This is a feature that supports config file inclusion conditional on whether the repo has a remote with a URL that matches a glob. Similar to my previous work on remote-suggested hooks [1], the main motivation is to allow remote repo administrators to provide recommended configs in a way that can be consumed more easily (e.g. through a package installable by a package manager). But this can also be used by, say, an individual that wants certain configs to apply to a certain set of local repos but not others. I marked this as RFC because there are some design points that need to be resolved: - The existing "include" and "includeIf" instructions are executed immediately, whereas in order to be useful, the execution of "includeIf hasremoteurl" needs to be delayed until all config files are read. Are there better ways to do this? - Is the conditionally-included file allowed to have its own "include{,If}" instructions? I'm thinking that we should forbid it because, for example, if we had 4 files as follows: A includes B and C includes D, and we include A and C in our main config (in that order), it wouldn't be clear whether B (because A was first included) or C (because we should execute everything at the same depth first) should be executed first. (In this patch, I didn't do anything about includes.) - A small one: the exact format of the glob. I probably will treat the URL like a path. [1] https://lore.kernel.org/git/cover.1623881977.git.jonathantanmy@google.com/ Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- config.c | 70 +++++++++++++++++++++++++++++++++++++++++------ t/t1300-config.sh | 27 ++++++++++++++++++ 2 files changed, 89 insertions(+), 8 deletions(-)