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

[Jonathan Tan <jonathantanmy@google.com>]

Junio C Hamano <gitster@pobox.com> writes:
> > @@ -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.