Thread (73 messages) 73 messages, 8 authors, 2022-01-25

Re: [PATCH v5 2/2] config: include file if remote URL matches a glob

flat view

From: Glen Choo <hidden>
Date: 2021-12-06 22:33:02

Jonathan Tan [off-list ref] writes:
quoted hunk ↗ jump to hunk
 Documentation/config.txt |  16 +++++
 config.c                 | 122 +++++++++++++++++++++++++++++++++++----
 config.h                 |   9 +++
 t/t1300-config.sh        | 118 +++++++++++++++++++++++++++++++++++++
 4 files changed, 255 insertions(+), 10 deletions(-)
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 reader of this description doesn't have any reason to think that
`hasconfig:remote.*.url` wouldn't respect in-place semantics, so my
concern in [1] is addressed.

`hasconfig:foo.*.bar` seems reasonable from a forwards-compatibility
perspective. Ideally, it would be nice to see a generic implementation
that actually handles config values beyond `remote.*.url`, but unless we
take a closer look at all config values and the conditions we would like
to support, a generic implementation seems like a premature
optimization that won't age well.

So OK to having a forward-compatible name without a forward compatible
implementation.
quoted hunk ↗ jump to hunk
+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))
+	if (skip_prefix_mem(cond, cond_len, "gitdir:", &cond, &cond_len)) {
 		return include_by_gitdir(opts, cond, cond_len, 0);
-	else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len))
+	} else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len)) {
 		return include_by_gitdir(opts, cond, cond_len, 1);
-	else if (skip_prefix_mem(cond, cond_len, "onbranch:", &cond, &cond_len))
+	} 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)) {
+		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);
+	}
 
 	/* unknown conditionals are always false */
 	return 0;
Nit: I have a preference for Ævar's version [2], which looks more
consistent with the rest of the function i.e. handling the match using a
helper function.
+test_expect_success 'includeIf.hasconfig:remote.*.url forbids remote url in such included files' '
+	git init hasremoteurlTest &&
+	test_when_finished "rm -rf hasremoteurlTest" &&
+
+	cat >"$(pwd)"/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
-- 
2.34.1.400.ga245620fadb-goog
This addresses the test coverage comment in [1]. Great!

[1] https://lore.kernel.org/git/kl6lilwjre3m.fsf@chooglen-macbookpro.roam.corp.google.com (local)
[2] https://lore.kernel.org/git/211206.86zgpdpmyy.gmgdl@evledraar.gmail.com (local)
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help