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

From: Glen Choo <chooglen@google.com>
Date: Dec 8, 2021, 19:19 UTC
Message-ID: <kl6l35n2c3ay.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <de2be06818781bbef29926c0e8e8981746242c60.1638919346.git.jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 19 quoted lines

> @@ -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 :)