| Message ID | 1c1a07a0b65d4bbbb0f2628a3ddf1980e37d5065.1637020610.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Conditional config includes based on remote URL | expand |
Jonathan Tan <jonathantanmy@google.com> writes: > +`hasremoteurl`:: > + The data that follows the keyword `hasremoteurl:` is taken to > + be a pattern with standard globbing wildcards and two > + additional ones, `**/` and `/**`, that can match multiple > + components. The rest of the config files will be scanned for > + remote URLs, and then if there at least one remote URL that if there {is,exists}* at least one remote URL that > + matches this pattern, the include condition is met. > ++ > +Files included by this option (directly or indirectly) are not allowed > +to contain remote URLs. As Jeff mentioned earlier in this thread, this "last-config-wins" is a pretty big exception to the existing semantics, as Documentation/config.txt reads: The contents of the included file are inserted immediately, as if they had been found at the location of the include directive. At minimum, I think we should call out this exception in Documentation/config.txt and the commit message, but calling out *just* hasremoteurl makes this exception seem like a strange anomaly at first glance, even though we actually have a good idea of when and why we are doing this (which is that it simplifies includes that rely on config values). I was a big fan of your includeIfDeferred proposal, and I still think that it's easier for users to understand if we explicitly require "includeIfDeferred" instead of counting on them to remember when "includeIf" behaves as it always did vs this new 'deferred' behavior. That said, I doubt most users actually rely on the inclusion order, and I am ok with this approach as long as we document the different inclusion order. > +static void populate_remote_urls(struct config_include_data *inc) > +{ > + struct config_options opts; > + > + struct config_source *store_cf = cf; > + struct key_value_info *store_kvi = current_config_kvi; > + enum config_scope store_scope = current_parsing_scope; > + > + opts = *inc->opts; > + opts.unconditional_remote_url = 1; > + > + cf = NULL; > + current_config_kvi = NULL; > + current_parsing_scope = 0; > + > + inc->remote_urls = xmalloc(sizeof(*inc->remote_urls)); > + string_list_init_dup(inc->remote_urls); > + config_with_options(add_remote_url, inc->remote_urls, inc->config_source, &opts); > + > + cf = store_cf; > + current_config_kvi = store_kvi; > + current_parsing_scope = store_scope; > +} The algorithm is easy to understand and reuses config_with_options(), which is great. > +static int forbid_remote_url(const char *var, const char *value, void *data) > +{ > + const char *remote_name; > + size_t remote_name_len; > + const char *key; > + > + if (!parse_config_key(var, "remote", &remote_name, &remote_name_len, > + &key) && > + remote_name && > + !strcmp(key, "url")) > + die(_("remote URLs cannot be configured in file directly or indirectly included by includeIf.hasremoteurl")); > + return 0; > +} > + > +static int at_least_one_url_matches_glob(const char *glob, int glob_len, > + struct string_list *remote_urls) > +{ > + struct strbuf pattern = STRBUF_INIT; > + struct string_list_item *url_item; > + int found = 0; > + > + strbuf_add(&pattern, glob, glob_len); > + for_each_string_list_item(url_item, remote_urls) { > + if (!wildmatch(pattern.buf, url_item->string, WM_PATHNAME)) { > + found = 1; > + break; > + } > + } > + strbuf_release(&pattern); > + return found; > +} > + > static int git_config_include(const char *var, const char *value, void *data) > { > struct config_include_data *inc = data; > const char *cond, *key; > size_t cond_len; > - int ret; > + int ret = 0; > > /* > * Pass along all values, including "include" directives; this makes it > @@ -335,9 +412,29 @@ 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")) { > + const char *url; > + size_t url_len; > + > + if (skip_prefix_mem(cond, cond_len, "hasremoteurl:", &url, > + &url_len)) { > + if (inc->opts->unconditional_remote_url) { > + config_fn_t old_fn = inc->fn; > + > + inc->fn = forbid_remote_url; When unconditional_remote_url is true, we forbid remote urls in the included files as expected, but... > + ret = handle_path_include(value, inc); > + inc->fn = old_fn; > + } else { > + if (!inc->remote_urls) > + populate_remote_urls(inc); > + if (at_least_one_url_matches_glob( > + url, url_len, inc->remote_urls)) > + ret = handle_path_include(value, inc); > + } > + } else if (include_condition_is_true(inc->opts, cond, cond_len)) { > + ret = handle_path_include(value, inc); > + } > + } > > return ret; > } It's not clear to me whether we are forbidding the remote urls correctly when uncondition_remote_url is false. I would be convinced if we had tests that convered this behavior, but I did not find any such test cases. > diff --git a/t/t1300-config.sh b/t/t1300-config.sh > index 9ff46f3b04..9daab4c6da 100755 > --- a/t/t1300-config.sh > +++ b/t/t1300-config.sh > @@ -2387,4 +2387,104 @@ 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' ' > + git init hasremoteurlTest && > + test_when_finished "rm -rf 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_expect_success 'includeIf.hasremoteurl respects last-config-wins' ' > + git init hasremoteurlTest && > + test_when_finished "rm -rf hasremoteurlTest" && > + > + cat >"$(pwd)"/include-two-three <<-\EOF && > + [user] > + two = included-config > + three = included-config > + EOF > + cat >>hasremoteurlTest/.git/config <<-EOF && > + [remote "foo"] > + url = foo > + [user] > + one = main-config > + two = main-config > + [includeIf "hasremoteurl:foo"] > + path = "$(pwd)/include-two-three" > + [user] > + three = main-config > + EOF > + > + echo main-config >expect-main-config && > + echo included-config >expect-included-config && > + > + git -C hasremoteurlTest config --get user.one >actual && > + test_cmp expect-main-config actual && > + > + git -C hasremoteurlTest config --get user.two >actual && > + test_cmp expect-included-config actual && > + > + git -C hasremoteurlTest config --get user.three >actual && > + test_cmp expect-main-config actual > +' > + > +test_expect_success 'includeIf.hasremoteurl globs' ' > + git init hasremoteurlTest && > + test_when_finished "rm -rf hasremoteurlTest" && > + > + printf "[user]\ndss = yes\n" >"$(pwd)/double-star-start" && > + printf "[user]\ndse = yes\n" >"$(pwd)/double-star-end" && > + printf "[user]\ndsm = yes\n" >"$(pwd)/double-star-middle" && > + printf "[user]\nssm = yes\n" >"$(pwd)/single-star-middle" && > + printf "[user]\nno = no\n" >"$(pwd)/no" && > + > + cat >>hasremoteurlTest/.git/config <<-EOF && > + [remote "foo"] > + url = https://foo/bar/baz > + [includeIf "hasremoteurl:**/baz"] > + path = "$(pwd)/double-star-start" > + [includeIf "hasremoteurl:**/nomatch"] > + path = "$(pwd)/no" > + [includeIf "hasremoteurl:https:/**"] > + path = "$(pwd)/double-star-end" > + [includeIf "hasremoteurl:nomatch:/**"] > + path = "$(pwd)/no" As mentioned above, I would have expected to find test cases that test whether or not we forbid the remote urls correctly, but the tests are pretty clear.
Jonathan Tan <jonathantanmy@google.com> writes: > 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 - it could, for example, > contain a file to be included conditionally and a post-install script > that adds the include directive to the system-wide config file). > > In order to do this, Git reruns the config parsing mechanism upon > noticing the first URL-conditional include in order to find all remote > URLs, and these remote URLs are then used to determine if that first and > all subsequent includes are executed. Remote URLs are not allowed to be > configued in any URL-conditionally-included file. > > [1] https://lore.kernel.org/git/cover.1623881977.git.jonathantanmy@google.com/ > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > Documentation/config.txt | 11 ++++ > config.c | 121 ++++++++++++++++++++++++++++++++++++--- > config.h | 7 +++ > t/t1300-config.sh | 100 ++++++++++++++++++++++++++++++++ > 4 files changed, 231 insertions(+), 8 deletions(-) Here is just a design level comment, without trying to outline the implementation in my head like I usually do before making any suggestion, but it strikes me somewhat sad that config.c needs to know specifically about "remote_url". I wonder if this can be a more generalized framework that allows us to say "we introduce a new [includeIf] variant to get another file included only if some condition is met for the configuration variables we read without the includeIf directive", with variations of "condition" including - a literal X is among the values of multi-valued variable Y. - a pattern X matches one of the values of multi-valued variable Y. - a literal Y is the name of an existing configuration variable. - a pattern Y matches the name of an existing configuration variable. If that is done, I would imagine that the feature can become a thin specialization e.g. "there is an existing configuration variable whose name is 'remotes.https://github.com/janathantanmy/git.url'" Perhaps I am dreaming? Thanks.
On Mon, Nov 15 2021, Jonathan Tan wrote: > +`hasremoteurl`:: > + The data that follows the keyword `hasremoteurl:` is taken to Both here.. > + die(_("remote URLs cannot be configured in file directly or indirectly included by includeIf.hasremoteurl")); ..and here... > + if (skip_prefix_mem(cond, cond_len, "hasremoteurl:", &url, ...but not here (C code).. > + * For internal use. Include all includeif.hasremoteurl paths without ..but here.. > +test_expect_success 'includeIf.hasremoteurl' ' ..and also here etc., let's consistently camelCase config keys whenever we're not using them for lookups in the C code. I.e. "includeIf.hasRemoteUrl" (possibly "includeIf.hasRemoteURL"?). It makes them a lot easier to read, and makes the end-user documentation & messaging more consistent.
Glen Choo <chooglen@google.com> writes: > Jonathan Tan <jonathantanmy@google.com> writes: > > > +`hasremoteurl`:: > > + The data that follows the keyword `hasremoteurl:` is taken to > > + be a pattern with standard globbing wildcards and two > > + additional ones, `**/` and `/**`, that can match multiple > > + components. The rest of the config files will be scanned for > > + remote URLs, and then if there at least one remote URL that > > if there {is,exists}* at least one remote URL that Ah, good catch. > > + matches this pattern, the include condition is met. > > ++ > > +Files included by this option (directly or indirectly) are not allowed > > +to contain remote URLs. > > As Jeff mentioned earlier in this thread, this "last-config-wins" is a > pretty big exception to the existing semantics, as > Documentation/config.txt reads: > > The contents of the included file are inserted immediately, as if they > had been found at the location of the include directive. > > At minimum, I think we should call out this exception in > Documentation/config.txt and the commit message, but calling out *just* > hasremoteurl makes this exception seem like a strange anomaly at first > glance, even though we actually have a good idea of when and why we are > doing this (which is that it simplifies includes that rely on config > values). I've switched it to expand-in-place semantics. The scanning for remote URLs does not mean that those configs are applied before the include. I'll add a note to the documentation about that, but if you can think of a better way to explain that, that would be great. The patch includes a test "includeIf.hasremoteurl respects last-config-wins". Take a look and see if it matches your expected behavior, and let me know if it could be clearer. > I was a big fan of your includeIfDeferred proposal, and I still think > that it's easier for users to understand if we explicitly require > "includeIfDeferred" instead of counting on them to remember when > "includeIf" behaves as it always did vs this new 'deferred' behavior. > That said, I doubt most users actually rely on the inclusion order, and > I am ok with this approach as long as we document the different > inclusion order. The user still needs to know that config variables in the future can affect the behavior of the include, but perhaps that will be easier than remembering that certain configs are deferred. > It's not clear to me whether we are forbidding the remote urls correctly > when uncondition_remote_url is false. I would be convinced if we had > tests that convered this behavior, but I did not find any such test > cases. [snip] > As mentioned above, I would have expected to find test cases that test > whether or not we forbid the remote urls correctly, but the tests are > pretty clear. Ah yes, I should include a test for this. I'll include it in the next reroll.
Junio C Hamano <gitster@pobox.com> writes: > Here is just a design level comment, without trying to outline the > implementation in my head like I usually do before making any > suggestion, but it strikes me somewhat sad that config.c needs to > know specifically about "remote_url". > > I wonder if this can be a more generalized framework that allows us > to say "we introduce a new [includeIf] variant to get another file > included only if some condition is met for the configuration > variables we read without the includeIf directive", with variations > of "condition" including > > - a literal X is among the values of multi-valued variable Y. > - a pattern X matches one of the values of multi-valued variable Y. > - a literal Y is the name of an existing configuration variable. > - a pattern Y matches the name of an existing configuration variable. > > If that is done, I would imagine that the feature can become a thin > specialization e.g. "there is an existing configuration variable > whose name is 'remotes.https://github.com/janathantanmy/git.url'" > > Perhaps I am dreaming? > > Thanks. I think that the hard part of this is how to present this to the end user - right now, we just have one pattern of variable (remote.*.url, where "*" is a wildcard) and one pattern of value with specific properties (e.g. this is a glob, not a regular expression, and the special value "**" is supported). Once we figure that out, I would think that we could implement it in a way similar to this patch. As for whether we should wait until the full feature before merging any code that does includeIf based on a variable (in order to avoid having code that would quickly be replaced by other code), in this case, unless there is another use case for this, I think we should proceed with the use case that we know about first (conditional include of a file supplied by a remote repo administrator).
> On Mon, Nov 15 2021, Jonathan Tan wrote: > > > +`hasremoteurl`:: > > + The data that follows the keyword `hasremoteurl:` is taken to > > Both here.. > > > + die(_("remote URLs cannot be configured in file directly or indirectly included by includeIf.hasremoteurl")); > > ..and here... > > > + if (skip_prefix_mem(cond, cond_len, "hasremoteurl:", &url, > > ...but not here (C code).. > > > + * For internal use. Include all includeif.hasremoteurl paths without > > ..but here.. > > > +test_expect_success 'includeIf.hasremoteurl' ' > > ..and also here etc., let's consistently camelCase config keys whenever > we're not using them for lookups in the C > code. > > I.e. "includeIf.hasRemoteUrl" (possibly "includeIf.hasRemoteURL"?). It > makes them a lot easier to read, and makes the end-user documentation & > messaging more consistent. The middle part is not case-insensitive, though - I tried changing it in the test and the test now fails. (Unless you mean that we should also change the code to make it case-insensitive - but I would think that it's better for the URL to be case-sensitive, and by extension, the "hasremoteurl:" part connected to it.)
On Mon, Nov 29 2021, Jonathan Tan wrote: >> On Mon, Nov 15 2021, Jonathan Tan wrote: >> >> > +`hasremoteurl`:: >> > + The data that follows the keyword `hasremoteurl:` is taken to >> >> Both here.. >> >> > + die(_("remote URLs cannot be configured in file directly or indirectly included by includeIf.hasremoteurl")); >> >> ..and here... >> >> > + if (skip_prefix_mem(cond, cond_len, "hasremoteurl:", &url, >> >> ...but not here (C code).. >> >> > + * For internal use. Include all includeif.hasremoteurl paths without >> >> ..but here.. >> >> > +test_expect_success 'includeIf.hasremoteurl' ' >> >> ..and also here etc., let's consistently camelCase config keys whenever >> we're not using them for lookups in the C >> code. >> >> I.e. "includeIf.hasRemoteUrl" (possibly "includeIf.hasRemoteURL"?). It >> makes them a lot easier to read, and makes the end-user documentation & >> messaging more consistent. > > The middle part is not case-insensitive, though - I tried changing it in > the test and the test now fails. (Unless you mean that we should also > change the code to make it case-insensitive - but I would think that > it's better for the URL to be case-sensitive, and by extension, the > "hasremoteurl:" part connected to it.) Ah, I forgot about that edge case. sorry. And sent [1] without having seen this as a reminder on v4. Makes sense. (I seem to be getting really slow delivery from kernel.org to GMail these days, sometimes I can see things on lore.kernel.org hours or half a day before it pops up in my mail...) 1. https://lore.kernel.org/git/211129.864k7ug02c.gmgdl@evledraar.gmail.com/
Jonathan Tan <jonathantanmy@google.com> writes: >> variables we read without the includeIf directive", with variations >> of "condition" including >> >> - a literal X is among the values of multi-valued variable Y. >> - a pattern X matches one of the values of multi-valued variable Y. >> - a literal Y is the name of an existing configuration variable. >> - a pattern Y matches the name of an existing configuration variable. > ... > code), in this case, unless there is another use case for this, I think > we should proceed with the use case that we know about first > (conditional include of a file supplied by a remote repo administrator). Doing it that way without thinking flexibility through will paint us into a corner, from which we cannot get out of, doesn't it? People will start asking "Why should we even have 'hasremoteurl:$URL' variant in 'includeIf' conditions, when one of the 'variableExists:Y' and friends can express the same thing", somebody new who is not yet in this community today will propose deprecating hasremoteurl in favor of more generalized approach and we have to give a sad answer "no, we earlier made a mistake of starting with a special case variant for expediency's sake, without thinking the general cases through. Because existing users depend on it, we have to support it til the end of time." We have the same regret with "why do we need grep.extendedRegexp when grep.patternType suffices?" I am reluctant to see us knowingly commit the same mistake here, unless there is a very good reason.
Junio C Hamano <gitster@pobox.com> writes: > Jonathan Tan <jonathantanmy@google.com> writes: > > >> variables we read without the includeIf directive", with variations > >> of "condition" including > >> > >> - a literal X is among the values of multi-valued variable Y. > >> - a pattern X matches one of the values of multi-valued variable Y. > >> - a literal Y is the name of an existing configuration variable. > >> - a pattern Y matches the name of an existing configuration variable. > > ... > > code), in this case, unless there is another use case for this, I think > > we should proceed with the use case that we know about first > > (conditional include of a file supplied by a remote repo administrator). > > Doing it that way without thinking flexibility through will paint us > into a corner, from which we cannot get out of, doesn't it? > > People will start asking "Why should we even have > 'hasremoteurl:$URL' variant in 'includeIf' conditions, when one of > the 'variableExists:Y' and friends can express the same thing", > somebody new who is not yet in this community today will propose > deprecating hasremoteurl in favor of more generalized approach and > we have to give a sad answer "no, we earlier made a mistake of > starting with a special case variant for expediency's sake, without > thinking the general cases through. Because existing users depend > on it, we have to support it til the end of time." > > We have the same regret with "why do we need grep.extendedRegexp > when grep.patternType suffices?" I am reluctant to see us knowingly > commit the same mistake here, unless there is a very good reason. Hmm...that's true. I was thinking that there wouldn't be a way to predict exactly what we'll need, but perhaps making the config variable name be of the form 'includeIf."hasconfig:remote.*.url".path' might give us enough flexibility for the future.. I'll take a look.
diff --git a/Documentation/config.txt b/Documentation/config.txt index 0c0e6b859f..93d18b2fe9 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -159,6 +159,17 @@ all branches that begin with `foo/`. This is useful if your branches are organized hierarchically and you would like to apply a configuration to all the branches in that hierarchy. +`hasremoteurl`:: + The data that follows the keyword `hasremoteurl:` is taken to + be a pattern with standard globbing wildcards and two + additional ones, `**/` and `/**`, that can match multiple + components. The rest of the config files will be scanned for + remote URLs, and then if there at least one remote URL that + matches this pattern, the include condition is met. ++ +Files included by this option (directly or indirectly) are not allowed +to contain remote URLs. + A few more notes on matching via `gitdir` and `gitdir/i`: * Symlinks in `$GIT_DIR` are not resolved before matching. diff --git a/config.c b/config.c index 94ad5ce913..4ffc1e87e9 100644 --- a/config.c +++ b/config.c @@ -125,6 +125,12 @@ struct config_include_data { config_fn_t fn; void *data; const struct config_options *opts; + struct git_config_source *config_source; + + /* + * All remote URLs discovered when reading all config files. + */ + struct string_list *remote_urls; }; #define CONFIG_INCLUDE_INIT { 0 } @@ -316,12 +322,83 @@ static int include_condition_is_true(const struct config_options *opts, return 0; } +static int add_remote_url(const char *var, const char *value, void *data) +{ + struct string_list *remote_urls = data; + const char *remote_name; + size_t remote_name_len; + const char *key; + + if (!parse_config_key(var, "remote", &remote_name, &remote_name_len, + &key) && + remote_name && + !strcmp(key, "url")) + string_list_append(remote_urls, value); + return 0; +} + +static void populate_remote_urls(struct config_include_data *inc) +{ + struct config_options opts; + + struct config_source *store_cf = cf; + struct key_value_info *store_kvi = current_config_kvi; + enum config_scope store_scope = current_parsing_scope; + + opts = *inc->opts; + opts.unconditional_remote_url = 1; + + cf = NULL; + current_config_kvi = NULL; + current_parsing_scope = 0; + + inc->remote_urls = xmalloc(sizeof(*inc->remote_urls)); + string_list_init_dup(inc->remote_urls); + config_with_options(add_remote_url, inc->remote_urls, inc->config_source, &opts); + + cf = store_cf; + current_config_kvi = store_kvi; + current_parsing_scope = store_scope; +} + +static int forbid_remote_url(const char *var, const char *value, void *data) +{ + const char *remote_name; + size_t remote_name_len; + const char *key; + + if (!parse_config_key(var, "remote", &remote_name, &remote_name_len, + &key) && + remote_name && + !strcmp(key, "url")) + die(_("remote URLs cannot be configured in file directly or indirectly included by includeIf.hasremoteurl")); + return 0; +} + +static int at_least_one_url_matches_glob(const char *glob, int glob_len, + struct string_list *remote_urls) +{ + struct strbuf pattern = STRBUF_INIT; + struct string_list_item *url_item; + int found = 0; + + strbuf_add(&pattern, glob, glob_len); + for_each_string_list_item(url_item, remote_urls) { + if (!wildmatch(pattern.buf, url_item->string, WM_PATHNAME)) { + found = 1; + break; + } + } + strbuf_release(&pattern); + return found; +} + static int git_config_include(const char *var, const char *value, void *data) { struct config_include_data *inc = data; const char *cond, *key; size_t cond_len; - int ret; + int ret = 0; /* * Pass along all values, including "include" directives; this makes it @@ -335,9 +412,29 @@ 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")) { + const char *url; + size_t url_len; + + if (skip_prefix_mem(cond, cond_len, "hasremoteurl:", &url, + &url_len)) { + if (inc->opts->unconditional_remote_url) { + config_fn_t old_fn = inc->fn; + + inc->fn = forbid_remote_url; + ret = handle_path_include(value, inc); + inc->fn = old_fn; + } else { + if (!inc->remote_urls) + populate_remote_urls(inc); + if (at_least_one_url_matches_glob( + url, url_len, inc->remote_urls)) + ret = handle_path_include(value, inc); + } + } else if (include_condition_is_true(inc->opts, cond, cond_len)) { + ret = handle_path_include(value, inc); + } + } return ret; } @@ -1933,11 +2030,13 @@ 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; if (opts->respect_includes) { inc.fn = fn; inc.data = data; inc.opts = opts; + inc.config_source = config_source; fn = git_config_include; data = &inc; } @@ -1950,17 +2049,23 @@ 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); } - return do_git_config_sequence(opts, fn, data); + if (inc.remote_urls) { + string_list_clear(inc.remote_urls, 0); + FREE_AND_NULL(inc.remote_urls); + } + return ret; } static void configset_iter(struct config_set *cs, config_fn_t fn, void *data) diff --git a/config.h b/config.h index 48a5e472ca..c24458b10a 100644 --- a/config.h +++ b/config.h @@ -89,6 +89,13 @@ struct config_options { unsigned int ignore_worktree : 1; unsigned int ignore_cmdline : 1; unsigned int system_gently : 1; + + /* + * For internal use. Include all includeif.hasremoteurl paths without + * checking if the repo has that remote URL. + */ + unsigned int unconditional_remote_url : 1; + const char *commondir; const char *git_dir; config_parser_event_fn_t event_fn; diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 9ff46f3b04..9daab4c6da 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -2387,4 +2387,104 @@ 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' ' + git init hasremoteurlTest && + test_when_finished "rm -rf 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_expect_success 'includeIf.hasremoteurl respects last-config-wins' ' + git init hasremoteurlTest && + test_when_finished "rm -rf hasremoteurlTest" && + + cat >"$(pwd)"/include-two-three <<-\EOF && + [user] + two = included-config + three = included-config + EOF + cat >>hasremoteurlTest/.git/config <<-EOF && + [remote "foo"] + url = foo + [user] + one = main-config + two = main-config + [includeIf "hasremoteurl:foo"] + path = "$(pwd)/include-two-three" + [user] + three = main-config + EOF + + echo main-config >expect-main-config && + echo included-config >expect-included-config && + + git -C hasremoteurlTest config --get user.one >actual && + test_cmp expect-main-config actual && + + git -C hasremoteurlTest config --get user.two >actual && + test_cmp expect-included-config actual && + + git -C hasremoteurlTest config --get user.three >actual && + test_cmp expect-main-config actual +' + +test_expect_success 'includeIf.hasremoteurl globs' ' + git init hasremoteurlTest && + test_when_finished "rm -rf hasremoteurlTest" && + + printf "[user]\ndss = yes\n" >"$(pwd)/double-star-start" && + printf "[user]\ndse = yes\n" >"$(pwd)/double-star-end" && + printf "[user]\ndsm = yes\n" >"$(pwd)/double-star-middle" && + printf "[user]\nssm = yes\n" >"$(pwd)/single-star-middle" && + printf "[user]\nno = no\n" >"$(pwd)/no" && + + cat >>hasremoteurlTest/.git/config <<-EOF && + [remote "foo"] + url = https://foo/bar/baz + [includeIf "hasremoteurl:**/baz"] + path = "$(pwd)/double-star-start" + [includeIf "hasremoteurl:**/nomatch"] + path = "$(pwd)/no" + [includeIf "hasremoteurl:https:/**"] + path = "$(pwd)/double-star-end" + [includeIf "hasremoteurl:nomatch:/**"] + path = "$(pwd)/no" + [includeIf "hasremoteurl:https:/**/baz"] + path = "$(pwd)/double-star-middle" + [includeIf "hasremoteurl:https:/**/nomatch"] + path = "$(pwd)/no" + [includeIf "hasremoteurl:https://*/bar/baz"] + path = "$(pwd)/single-star-middle" + [includeIf "hasremoteurl:https://*/baz"] + path = "$(pwd)/no" + EOF + + git -C hasremoteurlTest config --get user.dss && + git -C hasremoteurlTest config --get user.dse && + git -C hasremoteurlTest config --get user.dsm && + git -C hasremoteurlTest config --get user.ssm && + test_must_fail git -C hasremoteurlTest config --get user.no +' + 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 - it could, for example, contain a file to be included conditionally and a post-install script that adds the include directive to the system-wide config file). In order to do this, Git reruns the config parsing mechanism upon noticing the first URL-conditional include in order to find all remote URLs, and these remote URLs are then used to determine if that first and all subsequent includes are executed. Remote URLs are not allowed to be configued in any URL-conditionally-included file. [1] https://lore.kernel.org/git/cover.1623881977.git.jonathantanmy@google.com/ Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- Documentation/config.txt | 11 ++++ config.c | 121 ++++++++++++++++++++++++++++++++++++--- config.h | 7 +++ t/t1300-config.sh | 100 ++++++++++++++++++++++++++++++++ 4 files changed, 231 insertions(+), 8 deletions(-)