diff mbox series

[v6,2/2] config: include file if remote URL matches a glob

Message ID de2be06818781bbef29926c0e8e8981746242c60.1638919346.git.jonathantanmy@google.com (mailing list archive)
State Superseded
Headers show
Series Conditional config includes based on remote URL | expand

Commit Message

Jonathan Tan Dec. 7, 2021, 11:23 p.m. UTC
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 |  16 ++++++
 config.c                 | 121 ++++++++++++++++++++++++++++++++++++---
 config.h                 |   9 +++
 t/t1300-config.sh        | 118 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 257 insertions(+), 7 deletions(-)

Comments

Glen Choo Dec. 8, 2021, 7:19 p.m. UTC | #1
Jonathan Tan <jonathantanmy@google.com> writes:

> @@ -335,9 +427,16 @@ 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"))
> +	    cond && include_condition_is_true(inc, cond, cond_len) &&
> +	    !strcmp(key, "path")) {
> +		config_fn_t old_fn = inc->fn;
> +
> +		if (inc->opts->unconditional_remote_url)
> +			inc->fn = forbid_remote_url;
>  		ret = handle_path_include(value, inc);
> +		if (inc->opts->unconditional_remote_url)
> +			inc->fn = old_fn;
> +	}
>  
>  	return ret;
>  }

Minor nit: it looks like we don't need to restore inc->fn conditionally,
so instead of:

	if (inc->opts->unconditional_remote_url)
			inc->fn = old_fn;

we could just have:

  inc->fn = old_fn;

which (purely as a matter of personal taste) looks a bit more consistent
with the unconditional assignment of:

  config_fn_t old_fn = inc->fn;



No comments on the rest of the patch; it looks clean and
easy-to-understand :)
Glen Choo Dec. 8, 2021, 7:55 p.m. UTC | #2
Jonathan Tan <jonathantanmy@google.com> writes:

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 0c0e6b859f..e0e5ca558e 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -159,6 +159,22 @@ 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.
>  
> +`hasconfig:remote.*.url:`::
> +	The data that follows this keyword is taken to
> +	be a pattern with standard globbing wildcards and two
> +	additional ones, `**/` and `/**`, that can match multiple
> +	components. The first time this keyword is seen, the rest of
> +	the config files will be scanned for remote URLs (without
> +	applying any values). If there 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.

Wondering out loud.. Reading this and Ævar's comment [1], I wonder if we
should make it clear to users *why* we choose to forbid remote URLs.

Since this series is setting a precedent for future "hasconfig:"
conditions (files included by "hasconfig:foo.*.bar" cannot contain any
"foo.*.bar" values), it would be useful to git developers to explain
*why* we chose to do this. And if we're documenting it for ourselves,
we might as well write it in the public docs. That way, users would know
that this is more of a guardrail (because it's simpler to understand
this way) than a hard limitation.

[1] https://lore.kernel.org/git/211207.86k0ggnvfo.gmgdl@evledraar.gmail.com
Jonathan Tan Dec. 9, 2021, 10:16 p.m. UTC | #3
Glen Choo <chooglen@google.com> writes:
> Minor nit: it looks like we don't need to restore inc->fn conditionally,
> so instead of:
> 
> 	if (inc->opts->unconditional_remote_url)
> 			inc->fn = old_fn;
> 
> we could just have:
> 
>   inc->fn = old_fn;
> 
> which (purely as a matter of personal taste) looks a bit more consistent
> with the unconditional assignment of:
> 
>   config_fn_t old_fn = inc->fn;
> 
> 
> 
> No comments on the rest of the patch; it looks clean and
> easy-to-understand :)

Thanks for taking a look. This is a good suggestion - I'll use it.
Jonathan Tan Dec. 9, 2021, 10:39 p.m. UTC | #4
Glen Choo <chooglen@google.com> writes:
> > +`hasconfig:remote.*.url:`::
> > +	The data that follows this keyword is taken to
> > +	be a pattern with standard globbing wildcards and two
> > +	additional ones, `**/` and `/**`, that can match multiple
> > +	components. The first time this keyword is seen, the rest of
> > +	the config files will be scanned for remote URLs (without
> > +	applying any values). If there 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.
> 
> Wondering out loud.. Reading this and Ævar's comment [1], I wonder if we
> should make it clear to users *why* we choose to forbid remote URLs.
> 
> Since this series is setting a precedent for future "hasconfig:"
> conditions (files included by "hasconfig:foo.*.bar" cannot contain any
> "foo.*.bar" values), it would be useful to git developers to explain
> *why* we chose to do this. And if we're documenting it for ourselves,
> we might as well write it in the public docs. That way, users would know
> that this is more of a guardrail (because it's simpler to understand
> this way) than a hard limitation.
> 
> [1] https://lore.kernel.org/git/211207.86k0ggnvfo.gmgdl@evledraar.gmail.com

The explanation is rather long, though. It goes something like this:

  If the main config is:

  [remote a]
    url = bar
  [includeif hasconfig:remote.*.url:foo]
    path = foo
  [includeif hasconfig:remote.*.url:bar]
    path = bar

  and "bar" contains:

  [remote b]
    url = foo

  Should "foo" be included? For now, we avoid these situations
  completely by prohibiting URLs from being configured in "includeif
  hasconfig".

If you can think of a concise explanation, maybe we can include it.
Glen Choo Dec. 9, 2021, 11:33 p.m. UTC | #5
Jonathan Tan <jonathantanmy@google.com> writes:

>> > +`hasconfig:remote.*.url:`::
>> > +	The data that follows this keyword is taken to
>> > +	be a pattern with standard globbing wildcards and two
>> > +	additional ones, `**/` and `/**`, that can match multiple
>> > +	components. The first time this keyword is seen, the rest of
>> > +	the config files will be scanned for remote URLs (without
>> > +	applying any values). If there 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.
>> 
>> Wondering out loud.. Reading this and Ævar's comment [1], I wonder if we
>> should make it clear to users *why* we choose to forbid remote URLs.
>> 
>> Since this series is setting a precedent for future "hasconfig:"
>> conditions (files included by "hasconfig:foo.*.bar" cannot contain any
>> "foo.*.bar" values), it would be useful to git developers to explain
>> *why* we chose to do this. And if we're documenting it for ourselves,
>> we might as well write it in the public docs. That way, users would know
>> that this is more of a guardrail (because it's simpler to understand
>> this way) than a hard limitation.
>> 
>> [1] https://lore.kernel.org/git/211207.86k0ggnvfo.gmgdl@evledraar.gmail.com
>
> The explanation is rather long, though. It goes something like this:
>
>   If the main config is:
>
>   [remote a]
>     url = bar
>   [includeif hasconfig:remote.*.url:foo]
>     path = foo
>   [includeif hasconfig:remote.*.url:bar]
>     path = bar
>
>   and "bar" contains:
>
>   [remote b]
>     url = foo
>
>   Should "foo" be included? For now, we avoid these situations
>   completely by prohibiting URLs from being configured in "includeif
>   hasconfig".
>
> If you can think of a concise explanation, maybe we can include it.

Yeah, I can't think of a concise-yet-clear way to convey this to users
(if I had thought of one, I wouldn't have prefaced my original comment
with "Wondering out loud").

Spitballing here...

  `hasconfig:remote.*.url:`::
    The data that follows this keyword is taken to
    be a pattern with standard globbing wildcards and two
    additional ones, `**/` and `/**`, that can match multiple
    components. The first time this keyword is seen, the rest of
    the config files will be scanned for remote URLs (without
    applying any values). If there 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.
  + Because new remote URLs might affect the correctness of the include
  + condition, files included by this option (directly or indirectly) are
  + not allowed to contain remote URLs.

Although, upon further reflection, I wonder if this approach of banning
config variables really gives us the safety we want after all. Reworking
your example, say we expand "hasconfig" to include
"hasconfig:branch.*.merge" then we can have this in the main config:

   [remote a]
     url = baz
   [branch c]
     merge = bar

   [includeif hasconfig:remote.*.url:foo]
     path = foo
   [includeif hasconfig:branch.*.merge:bar]
     path = bar

and "bar" contains:

   [remote b]
     url = foo

we end up with the exact same question of "Should "foo" be included?".
This shows that the rule isn't actually "files included by
hasconfig:remote.*.url cannot include remote.*.url", but the much more
restrictive "files included by hasconfig:<anything> cannot include any
config values that can appear in hasconfig". This sounds pretty unusable
to me..

But I think that with the semantics you've defined, we don't really need
to forbid config variables. This section describes:

  The first time this keyword is seen, the rest of the config files will
  be scanned for remote URLs (without applying any values). If there
  exists at least one remote URL that matches this pattern, the include
  condition is met.

which, to me, gives us a pass to say "the first time we see a hasconfig,
we will do an additional scan without applying values". That doesn't
sound _too_ confusing to me, but I don't know how it looks to someone
with fresh eyes.

Forgive me if this exact suggestion came up before on-list (I know we've
discussed this exact approach off-list).
Junio C Hamano Dec. 10, 2021, 9:45 p.m. UTC | #6
Jonathan Tan <jonathantanmy@google.com> writes:

> The explanation is rather long, though. It goes something like this:
>
>   If the main config is:
>
>   [remote a]
>     url = bar
>   [includeif hasconfig:remote.*.url:foo]
>     path = foo
>   [includeif hasconfig:remote.*.url:bar]
>     path = bar
>
>   and "bar" contains:
>
>   [remote b]
>     url = foo
>
>   Should "foo" be included? For now, we avoid these situations
>   completely by prohibiting URLs from being configured in "includeif
>   hasconfig".
>
> If you can think of a concise explanation, maybe we can include it.

Perhaps it is easier to approach it from the viewpoint of a new
user who is unfamiliar with what you designed.

I would imagine that most users would find it natural if a single
pass precedure read and processed lines as it sees them.

That is, when the first includeif is evaluated, we have seen only
'remote.a.url' whose value is 'bar', so the condition does not hold.
and then when the second includeif is evaluated, it gets included,
and we read 'bar'.  But that is wher configuration reading ends;
remote.b.url is not asked for after we process the second includeif
til the end.

If you explain

 (1) why such a simplest design would not work well; and

 (2) how the actual design is different from that simplest design to
     overcome it.

it would be easier to grok?

Thanks.
Jonathan Tan Dec. 13, 2021, 11:35 p.m. UTC | #7
Glen Choo <chooglen@google.com> writes:
> Yeah, I can't think of a concise-yet-clear way to convey this to users
> (if I had thought of one, I wouldn't have prefaced my original comment
> with "Wondering out loud").
> 
> Spitballing here...
> 
>   `hasconfig:remote.*.url:`::
>     The data that follows this keyword is taken to
>     be a pattern with standard globbing wildcards and two
>     additional ones, `**/` and `/**`, that can match multiple
>     components. The first time this keyword is seen, the rest of
>     the config files will be scanned for remote URLs (without
>     applying any values). If there 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.
>   + Because new remote URLs might affect the correctness of the include
>   + condition, files included by this option (directly or indirectly) are
>   + not allowed to contain remote URLs.

Junio suggested another approach [1] - I'll try that and see what I come
up with.

[1] https://lore.kernel.org/git/xmqqmtl8m8wj.fsf@gitster.g/

> Although, upon further reflection, I wonder if this approach of banning
> config variables really gives us the safety we want after all. Reworking
> your example, say we expand "hasconfig" to include
> "hasconfig:branch.*.merge" then we can have this in the main config:
> 
>    [remote a]
>      url = baz
>    [branch c]
>      merge = bar
> 
>    [includeif hasconfig:remote.*.url:foo]
>      path = foo
>    [includeif hasconfig:branch.*.merge:bar]
>      path = bar
> 
> and "bar" contains:
> 
>    [remote b]
>      url = foo
> 
> we end up with the exact same question of "Should "foo" be included?".
> This shows that the rule isn't actually "files included by
> hasconfig:remote.*.url cannot include remote.*.url", but the much more
> restrictive "files included by hasconfig:<anything> cannot include any
> config values that can appear in hasconfig". This sounds pretty unusable
> to me..

This was my original idea actually (using any config variable anywhere
bans you from that config variable in all "includeif hasconfig"). I
think it would still be usable - you just have to be careful in which
config variables you use. But we don't have plans to include other
variables now anyway.

> But I think that with the semantics you've defined, we don't really need
> to forbid config variables. This section describes:
> 
>   The first time this keyword is seen, the rest of the config files will
>   be scanned for remote URLs (without applying any values). If there
>   exists at least one remote URL that matches this pattern, the include
>   condition is met.
> 
> which, to me, gives us a pass to say "the first time we see a hasconfig,
> we will do an additional scan without applying values". That doesn't
> sound _too_ confusing to me, but I don't know how it looks to someone
> with fresh eyes.
> 
> Forgive me if this exact suggestion came up before on-list (I know we've
> discussed this exact approach off-list).

This "additional scan without applying values" is not very well-defined,
though. In the scenario I described in [2], should "foo" be included?
"Yes" because it is referenced (even though at that time, nobody has
ever head of the URL "foo") or "no" because at that point in time in the
scan, nobody has ever heard of the URL "foo"?

[2] https://lore.kernel.org/git/20211209223919.513113-1-jonathantanmy@google.com/
Jonathan Tan Dec. 13, 2021, 11:37 p.m. UTC | #8
Junio C Hamano <gitster@pobox.com> writes:
> Perhaps it is easier to approach it from the viewpoint of a new
> user who is unfamiliar with what you designed.
> 
> I would imagine that most users would find it natural if a single
> pass precedure read and processed lines as it sees them.
> 
> That is, when the first includeif is evaluated, we have seen only
> 'remote.a.url' whose value is 'bar', so the condition does not hold.
> and then when the second includeif is evaluated, it gets included,
> and we read 'bar'.  But that is wher configuration reading ends;
> remote.b.url is not asked for after we process the second includeif
> til the end.
> 
> If you explain
> 
>  (1) why such a simplest design would not work well; and
> 
>  (2) how the actual design is different from that simplest design to
>      overcome it.
> 
> it would be easier to grok?
> 
> Thanks.

Thanks - this sounds like a good approach. I'll try this.
diff mbox series

Patch

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0c0e6b859f..e0e5ca558e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -159,6 +159,22 @@  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.
 
+`hasconfig:remote.*.url:`::
+	The data that follows this keyword is taken to
+	be a pattern with standard globbing wildcards and two
+	additional ones, `**/` and `/**`, that can match multiple
+	components. The first time this keyword is seen, the rest of
+	the config files will be scanned for remote URLs (without
+	applying any values). If there 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.
++
+This keyword is designed to be forwards compatible with a naming
+scheme that supports more variable-based include conditions, but
+currently Git only supports the exact keyword described above.
+
 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..f17053d91b 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 }
 
@@ -301,9 +307,92 @@  static int include_by_branch(const char *cond, size_t cond_len)
 	return ret;
 }
 
-static int include_condition_is_true(const struct config_options *opts,
+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.hasconfig:remote.*.url"));
+	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 include_by_remote_url(struct config_include_data *inc,
+		const char *cond, size_t cond_len)
+{
+	if (inc->opts->unconditional_remote_url)
+		return 1;
+	if (!inc->remote_urls)
+		populate_remote_urls(inc);
+	return at_least_one_url_matches_glob(cond, cond_len,
+					     inc->remote_urls);
+}
+
+static int include_condition_is_true(struct config_include_data *inc,
 				     const char *cond, size_t cond_len)
 {
+	const struct config_options *opts = inc->opts;
 
 	if (skip_prefix_mem(cond, cond_len, "gitdir:", &cond, &cond_len))
 		return include_by_gitdir(opts, cond, cond_len, 0);
@@ -311,6 +400,9 @@  static int include_condition_is_true(const struct config_options *opts,
 		return include_by_gitdir(opts, cond, cond_len, 1);
 	else if (skip_prefix_mem(cond, cond_len, "onbranch:", &cond, &cond_len))
 		return include_by_branch(cond, cond_len);
+	else if (skip_prefix_mem(cond, cond_len, "hasconfig:remote.*.url:", &cond,
+				   &cond_len))
+		return include_by_remote_url(inc, cond, cond_len);
 
 	/* unknown conditionals are always false */
 	return 0;
@@ -335,9 +427,16 @@  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"))
+	    cond && include_condition_is_true(inc, cond, cond_len) &&
+	    !strcmp(key, "path")) {
+		config_fn_t old_fn = inc->fn;
+
+		if (inc->opts->unconditional_remote_url)
+			inc->fn = forbid_remote_url;
 		ret = handle_path_include(value, inc);
+		if (inc->opts->unconditional_remote_url)
+			inc->fn = old_fn;
+	}
 
 	return ret;
 }
@@ -1933,11 +2032,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 +2051,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..ab0106d287 100644
--- a/config.h
+++ b/config.h
@@ -89,6 +89,15 @@  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, and when doing so, verify
+	 * that files included in this way do not configure any remote URLs
+	 * themselves.
+	 */
+	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..8310562b84 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2387,4 +2387,122 @@  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.hasconfig:remote.*.url' '
+	git init hasremoteurlTest &&
+	test_when_finished "rm -rf hasremoteurlTest" &&
+
+	cat >include-this <<-\EOF &&
+	[user]
+		this = this-is-included
+	EOF
+	cat >dont-include-that <<-\EOF &&
+	[user]
+		that = that-is-not-included
+	EOF
+	cat >>hasremoteurlTest/.git/config <<-EOF &&
+	[includeIf "hasconfig:remote.*.url:foo"]
+		path = "$(pwd)/include-this"
+	[includeIf "hasconfig:remote.*.url: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.hasconfig:remote.*.url respects last-config-wins' '
+	git init hasremoteurlTest &&
+	test_when_finished "rm -rf hasremoteurlTest" &&
+
+	cat >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 "hasconfig:remote.*.url: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.hasconfig:remote.*.url globs' '
+	git init hasremoteurlTest &&
+	test_when_finished "rm -rf hasremoteurlTest" &&
+
+	printf "[user]\ndss = yes\n" >double-star-start &&
+	printf "[user]\ndse = yes\n" >double-star-end &&
+	printf "[user]\ndsm = yes\n" >double-star-middle &&
+	printf "[user]\nssm = yes\n" >single-star-middle &&
+	printf "[user]\nno = no\n" >no &&
+
+	cat >>hasremoteurlTest/.git/config <<-EOF &&
+	[remote "foo"]
+		url = https://foo/bar/baz
+	[includeIf "hasconfig:remote.*.url:**/baz"]
+		path = "$(pwd)/double-star-start"
+	[includeIf "hasconfig:remote.*.url:**/nomatch"]
+		path = "$(pwd)/no"
+	[includeIf "hasconfig:remote.*.url:https:/**"]
+		path = "$(pwd)/double-star-end"
+	[includeIf "hasconfig:remote.*.url:nomatch:/**"]
+		path = "$(pwd)/no"
+	[includeIf "hasconfig:remote.*.url:https:/**/baz"]
+		path = "$(pwd)/double-star-middle"
+	[includeIf "hasconfig:remote.*.url:https:/**/nomatch"]
+		path = "$(pwd)/no"
+	[includeIf "hasconfig:remote.*.url:https://*/bar/baz"]
+		path = "$(pwd)/single-star-middle"
+	[includeIf "hasconfig:remote.*.url: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_expect_success 'includeIf.hasconfig:remote.*.url forbids remote url in such included files' '
+	git init hasremoteurlTest &&
+	test_when_finished "rm -rf hasremoteurlTest" &&
+
+	cat >include-with-url <<-\EOF &&
+	[remote "bar"]
+		url = bar
+	EOF
+	cat >>hasremoteurlTest/.git/config <<-EOF &&
+	[includeIf "hasconfig:remote.*.url:foo"]
+		path = "$(pwd)/include-with-url"
+	EOF
+
+	# test with any Git command
+	test_must_fail git -C hasremoteurlTest status 2>err &&
+	grep "fatal: remote URLs cannot be configured in file directly or indirectly included by includeIf.hasconfig:remote.*.url" err
+'
+
 test_done