Jonathan Tan <jonathantanmy@xxxxxxxxxx> 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.
> 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);
> + }
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()?
