git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Glen Choo <chooglen@google.com>
Date
Nov 22, 2021, 22:59 UTC
Message-ID
<kl6lilwjre3m.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To
<1c1a07a0b65d4bbbb0f2628a3ddf1980e37d5065.1637020610.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
Show 6 quoted lines
> +`hasremoteurl`::
> +	The data that follows the keyword `hasremoteurl:` is taken to
> +	be a pattern with standard globbing wildcards and two
> +	additional ones, `**/` and `/**`, that can match multiple
> +	components. The rest of the config files will be scanned for
> +	remote URLs, and then if there at least one remote URL that
  if there {is,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.

As Jeff mentioned earlier in this thread, this "last-config-wins" is a pretty big exception to the existing semantics, as Documentation/config.txt reads:

  The contents of the included file are inserted immediately, as if they
  had been found at the location of the include directive.

At minimum, I think we should call out this exception in Documentation/config.txt and the commit message, but calling out *just* hasremoteurl makes this exception seem like a strange anomaly at first glance, even though we actually have a good idea of when and why we are doing this (which is that it simplifies includes that rely on config values).

I was a big fan of your includeIfDeferred proposal, and I still think that it's easier for users to understand if we explicitly require "includeIfDeferred" instead of counting on them to remember when "includeIf" behaves as it always did vs this new 'deferred' behavior. That said, I doubt most users actually rely on the inclusion order, and I am ok with this approach as long as we document the different inclusion order.

Show 23 quoted lines
> +static void populate_remote_urls(struct config_include_data *inc)
> +{
> +	struct config_options opts;
> +
> +	struct config_source *store_cf = cf;
> +	struct key_value_info *store_kvi = current_config_kvi;
> +	enum config_scope store_scope = current_parsing_scope;
> +
> +	opts = *inc->opts;
> +	opts.unconditional_remote_url = 1;
> +
> +	cf = NULL;
> +	current_config_kvi = NULL;
> +	current_parsing_scope = 0;
> +
> +	inc->remote_urls = xmalloc(sizeof(*inc->remote_urls));
> +	string_list_init_dup(inc->remote_urls);
> +	config_with_options(add_remote_url, inc->remote_urls, inc->config_source, &opts);
> +
> +	cf = store_cf;
> +	current_config_kvi = store_kvi;
> +	current_parsing_scope = store_scope;
> +}

The algorithm is easy to understand and reuses config_with_options(), which is great.

Show 59 quoted lines
> +static int forbid_remote_url(const char *var, const char *value, void *data)
> +{
> +	const char *remote_name;
> +	size_t remote_name_len;
> +	const char *key;
> +
> +	if (!parse_config_key(var, "remote", &remote_name, &remote_name_len,
> +			      &key) &&
> +	    remote_name &&
> +	    !strcmp(key, "url"))
> +		die(_("remote URLs cannot be configured in file directly or indirectly included by includeIf.hasremoteurl"));
> +	return 0;
> +}
> +
> +static int at_least_one_url_matches_glob(const char *glob, int glob_len,
> +					 struct string_list *remote_urls)
> +{
> +	struct strbuf pattern = STRBUF_INIT;
> +	struct string_list_item *url_item;
> +	int found = 0;
> +
> +	strbuf_add(&pattern, glob, glob_len);
> +	for_each_string_list_item(url_item, remote_urls) {
> +		if (!wildmatch(pattern.buf, url_item->string, WM_PATHNAME)) {
> +			found = 1;
> +			break;
> +		}
> +	}
> +	strbuf_release(&pattern);
> +	return found;
> +}
> +
>  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;

When unconditional_remote_url is true, we forbid remote urls in the included files as expected, but...

Show 16 quoted lines
> +				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);
> +		}
> +	}
>  
>  	return ret;
>  }

It's not clear to me whether we are forbidding the remote urls correctly when uncondition_remote_url is false. I would be convinced if we had tests that convered this behavior, but I did not find any such test cases.

Show 91 quoted lines
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 9ff46f3b04..9daab4c6da 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -2387,4 +2387,104 @@ test_expect_success '--get and --get-all with --fixed-value' '
>  	test_must_fail git config --file=config --get-regexp --fixed-value fixed+ non-existent
>  '
>  
> +test_expect_success 'includeIf.hasremoteurl' '
> +	git init hasremoteurlTest &&
> +	test_when_finished "rm -rf hasremoteurlTest" &&
> +
> +	cat >"$(pwd)"/include-this <<-\EOF &&
> +	[user]
> +		this = this-is-included
> +	EOF
> +	cat >"$(pwd)"/dont-include-that <<-\EOF &&
> +	[user]
> +		that = that-is-not-included
> +	EOF
> +	cat >>hasremoteurlTest/.git/config <<-EOF &&
> +	[includeIf "hasremoteurl:foo"]
> +		path = "$(pwd)/include-this"
> +	[includeIf "hasremoteurl:bar"]
> +		path = "$(pwd)/dont-include-that"
> +	[remote "foo"]
> +		url = foo
> +	EOF
> +
> +	echo this-is-included >expect-this &&
> +	git -C hasremoteurlTest config --get user.this >actual-this &&
> +	test_cmp expect-this actual-this &&
> +
> +	test_must_fail git -C hasremoteurlTest config --get user.that
> +'
> +
> +test_expect_success 'includeIf.hasremoteurl respects last-config-wins' '
> +	git init hasremoteurlTest &&
> +	test_when_finished "rm -rf hasremoteurlTest" &&
> +
> +	cat >"$(pwd)"/include-two-three <<-\EOF &&
> +	[user]
> +		two = included-config
> +		three = included-config
> +	EOF
> +	cat >>hasremoteurlTest/.git/config <<-EOF &&
> +	[remote "foo"]
> +		url = foo
> +	[user]
> +		one = main-config
> +		two = main-config
> +	[includeIf "hasremoteurl:foo"]
> +		path = "$(pwd)/include-two-three"
> +	[user]
> +		three = main-config
> +	EOF
> +
> +	echo main-config >expect-main-config &&
> +	echo included-config >expect-included-config &&
> +
> +	git -C hasremoteurlTest config --get user.one >actual &&
> +	test_cmp expect-main-config actual &&
> +
> +	git -C hasremoteurlTest config --get user.two >actual &&
> +	test_cmp expect-included-config actual &&
> +
> +	git -C hasremoteurlTest config --get user.three >actual &&
> +	test_cmp expect-main-config actual
> +'
> +
> +test_expect_success 'includeIf.hasremoteurl globs' '
> +	git init hasremoteurlTest &&
> +	test_when_finished "rm -rf hasremoteurlTest" &&
> +
> +	printf "[user]\ndss = yes\n" >"$(pwd)/double-star-start" &&
> +	printf "[user]\ndse = yes\n" >"$(pwd)/double-star-end" &&
> +	printf "[user]\ndsm = yes\n" >"$(pwd)/double-star-middle" &&
> +	printf "[user]\nssm = yes\n" >"$(pwd)/single-star-middle" &&
> +	printf "[user]\nno = no\n" >"$(pwd)/no" &&
> +
> +	cat >>hasremoteurlTest/.git/config <<-EOF &&
> +	[remote "foo"]
> +		url = https://foo/bar/baz
> +	[includeIf "hasremoteurl:**/baz"]
> +		path = "$(pwd)/double-star-start"
> +	[includeIf "hasremoteurl:**/nomatch"]
> +		path = "$(pwd)/no"
> +	[includeIf "hasremoteurl:https:/**"]
> +		path = "$(pwd)/double-star-end"
> +	[includeIf "hasremoteurl:nomatch:/**"]
> +		path = "$(pwd)/no"

As mentioned above, I would have expected to find test cases that test whether or not we forbid the remote urls correctly, but the tests are pretty clear.

Previous: Jonathan TanNext: Jonathan Tan
Message 36 of 87 in “Conditional config includes based on remote URL”
  1. 0/2 Conditional config includes based on remote URLJonathan Tan, Oct 12, 2021
  2. 1/2 config: make git_config_include() staticJonathan Tan, Oct 12, 2021
  3. Jeff KingOct 12, 2021
  4. Junio C HamanoOct 12, 2021
  5. Ævar Arnfjörð BjarmasonOct 13, 2021
  6. Junio C HamanoOct 13, 2021
  7. Jonathan TanOct 13, 2021
  8. 2/2 config: include file if remote URL matches a globJonathan Tan, Oct 12, 2021
  9. Jeff KingOct 12, 2021
  10. Jonathan TanOct 13, 2021
  11. Jeff KingOct 27, 2021
  12. Jonathan TanOct 27, 2021
  13. Junio C HamanoOct 12, 2021
  14. Jonathan TanOct 13, 2021
  15. brian m. carlsonOct 13, 2021
  16. Jonathan TanOct 13, 2021
  17. Jonathan TanOct 18, 2021
  18. Emily ShafferOct 22, 2021
  19. Jeff KingOct 27, 2021
  20. Jonathan TanOct 27, 2021
  21. Jeff KingOct 27, 2021
  22. Ævar Arnfjörð BjarmasonOct 25, 2021
  23. Jonathan TanOct 25, 2021
  24. Ævar Arnfjörð BjarmasonOct 26, 2021
  25. 0/2 Conditional config includes based on remote URLJonathan Tan, Oct 29, 2021
  26. 1/2 config: make git_config_include() staticJonathan Tan, Oct 29, 2021
  27. Emily ShafferNov 5, 2021
  28. 2/2 config: include file if remote URL matches a globJonathan Tan, Oct 29, 2021
  29. Emily ShafferNov 5, 2021
  30. Ævar Arnfjörð BjarmasonNov 6, 2021
  31. Jonathan TanNov 9, 2021
  32. Jonathan TanNov 9, 2021
  33. 0/2 Conditional config includes based on remote URLJonathan Tan, Nov 16, 2021
  34. 1/2 config: make git_config_include() staticJonathan Tan, Nov 16, 2021
  35. 2/2 config: include file if remote URL matches a globJonathan Tan, Nov 16, 2021
  36. Glen ChooNov 22, 2021
  37. Jonathan TanNov 29, 2021
  38. Junio C HamanoNov 23, 2021
  39. Jonathan TanNov 29, 2021
  40. Junio C HamanoDec 1, 2021
  41. Jonathan TanDec 2, 2021
  42. Ævar Arnfjörð BjarmasonNov 23, 2021
  43. Jonathan TanNov 29, 2021
  44. Ævar Arnfjörð BjarmasonNov 29, 2021
  45. 0/2 Conditional config includes based on remote URLJonathan Tan, Nov 29, 2021
  46. 1/2 config: make git_config_include() staticJonathan Tan, Nov 29, 2021
  47. Ævar Arnfjörð BjarmasonNov 29, 2021
  48. Junio C HamanoNov 30, 2021
  49. 2/2 config: include file if remote URL matches a globJonathan Tan, Nov 29, 2021
  50. Junio C HamanoDec 2, 2021
  51. Jonathan TanDec 2, 2021
  52. 0/2 Conditional config includes based on remote URLJonathan Tan, Dec 2, 2021
  53. 1/2 config: make git_config_include() staticJonathan Tan, Dec 2, 2021
  54. 2/2 config: include file if remote URL matches a globJonathan Tan, Dec 2, 2021
  55. Glen ChooDec 6, 2021
  56. Jonathan TanDec 7, 2021
  57. Ævar Arnfjörð BjarmasonDec 6, 2021
  58. Jonathan TanDec 7, 2021
  59. Ævar Arnfjörð BjarmasonDec 7, 2021
  60. Jonathan TanDec 7, 2021
  61. 0/2 Conditional config includes based on remote URLJonathan Tan, Dec 7, 2021
  62. 1/2 config: make git_config_include() staticJonathan Tan, Dec 7, 2021
  63. 2/2 config: include file if remote URL matches a globJonathan Tan, Dec 7, 2021
  64. Glen ChooDec 8, 2021
  65. Jonathan TanDec 9, 2021
  66. Glen ChooDec 8, 2021
  67. Jonathan TanDec 9, 2021
  68. Glen ChooDec 9, 2021
  69. Jonathan TanDec 13, 2021
  70. Junio C HamanoDec 10, 2021
  71. Jonathan TanDec 13, 2021
  72. 0/2 Conditional config includes based on remote URLJonathan Tan, Dec 14, 2021
  73. 1/2 config: make git_config_include() staticJonathan Tan, Dec 14, 2021
  74. 2/2 config: include file if remote URL matches a globJonathan Tan, Dec 14, 2021
  75. Glen ChooDec 16, 2021
  76. Elijah NewrenDec 28, 2021
  77. Jonathan TanJan 10, 2022
  78. Glen ChooDec 16, 2021
  79. Elijah NewrenDec 28, 2021
  80. Glen ChooDec 28, 2021
  81. Jonathan TanJan 10, 2022
  82. Elijah NewrenJan 10, 2022
  83. Scalar vs JGit, was Re: [PATCH v7 0/2] Conditional config includes based on remote URLJohannes Schindelin, Jan 25, 2022
  84. 0/2 Conditional config includes based on remote URLJonathan Tan, Jan 18, 2022
  85. 1/2 config: make git_config_include() staticJonathan Tan, Jan 18, 2022
  86. 2/2 config: include file if remote URL matches a globJonathan Tan, Jan 18, 2022
  87. Elijah NewrenJan 18, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.