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

Jonathan Tan [off-list ref] writes:

+static int add_remote_url(const char *var, const char *value, void *data)
+{
+...
+}
+
+static void populate_remote_urls(struct config_include_data *inc)
+{
+...
+}
+
+static int forbid_remote_url(const char *var, const char *value, void *data)
+{
+...
+}
+
+static int at_least_one_url_matches_glob(const char *glob, int glob_len,
+					 struct string_list *remote_urls)
+{
+...
+}

All of the above makes sense; you prepare the remote URLs defined in
a string list, and have these helper functions that can determine if
the value given to hasremoteurl:* condition satisfies the condition.

This looks iffy, especialy in a patch that is not marked as [RFC].

I can see that include_condition_is_true() only passes inc->opts and
you need some other parts of inc for your purpose, and it may be the
primary reason why you munge this caller instead of adding function
include_by_remoteurl() and making include_condition_is_true() call
it.  But wouldn't it be sufficient to pass inc (not inc->opts) to
include_condition_is_true(), and have it dereference inc->opts when
calling include_by_gitdir() and friends that want opts, while
passing inc to include_by_remoteurl()?

Junio C Hamano [off-list ref] writes:

quoted

@@ -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);
+		}

This looks iffy, especialy in a patch that is not marked as [RFC].

I can see that include_condition_is_true() only passes inc->opts and
you need some other parts of inc for your purpose, and it may be the
primary reason why you munge this caller instead of adding function
include_by_remoteurl() and making include_condition_is_true() call
it.  But wouldn't it be sufficient to pass inc (not inc->opts) to
include_condition_is_true(), and have it dereference inc->opts when
calling include_by_gitdir() and friends that want opts, while
passing inc to include_by_remoteurl()?

Thanks for taking a look. I think the primary reason why I wrote it like
this was because originally (in v1) I included at the end, not inline,
but of course that is no longer the case so I should revisit it. I'll
have to pay attention to how inc->fn is swapped for forbid_remote_url
(to prevent remote URLs from being configured in included files), but
perhaps handle_path_include() can do that. I'll take a look.