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

Re: [PATCH v2 5/7] config: plumb --fixed-value into config API

From
Emily Shaffer <emilyshaffer@google.com>
Date
Nov 23, 2020, 22:21 UTC
Message-ID
<20201123222123.GE499823@google.com>
In-Reply-To
<39718048cd8f5be053a13ff73f531e3400b80b25.1606147507.git.gitgitgadget@gmail.com>
On Mon, Nov 23, 2020 at 04:05:05PM +0000, Derrick Stolee via GitGitGadget wrote:
Show 42 quoted lines
> 
> 
> The git_config_set_multivar_in_file_gently() and related methods now
> take a 'flags' bitfield, so add a new bit representing the --fixed-value
> option from 'git config'. This alters the purpose of the value_regex
> parameter to be an exact string match. This requires some initialization
> changes in git_config_set_multivar_in_file_gently() and a new strcmp()
> call in the matches() method.
> 
> The new CONFIG_FLAGS_FIXED_VALUE flag is initialized in builtin/config.c
> based on the --fixed-value option, and that needs to be updated in
> several callers.
> 
> This patch only affects some of the modes of 'git config', and the rest
> will be completed in the next change.
> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  builtin/config.c  | 16 ++++++++-----
>  config.c          |  5 +++++
>  config.h          |  7 ++++++
>  t/t1300-config.sh | 57 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 80 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin/config.c b/builtin/config.c
> index bfb55a96df..3e49e04411 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -616,6 +616,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  {
>  	int nongit = !startup_info->have_repository;
>  	char *value;
> +	int flags = 0;
>  
>  	given_config_source.file = xstrdup_or_null(getenv(CONFIG_ENVIRONMENT));
>  
> @@ -769,6 +770,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  			error(_("--fixed-value only applies with 'value_regex'"));
>  			usage_builtin_config();
>  		}
> +
> +		flags = CONFIG_FLAGS_FIXED_VALUE;

I wonder whether using |= here will save someone from a headache later, when they want to add another flag value or move the CONFIG_FLAGS_MULTI_REPLACE calculation out of the tail calls below.

[snip]
Show 9 quoted lines
>  	}
>  	else if (actions == ACTION_REPLACE_ALL) {
>  		check_write();
> @@ -850,7 +855,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  		UNLEAK(value);
>  		return git_config_set_multivar_in_file_gently(given_config_source.file,
>  							      argv[0], value, argv[2],
> -							      CONFIG_FLAGS_MULTI_REPLACE);
> +							      flags | CONFIG_FLAGS_MULTI_REPLACE);
[snip]
Show 6 quoted lines
> @@ -887,7 +893,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  		check_argc(argc, 1, 2);
>  		return git_config_set_multivar_in_file_gently(given_config_source.file,
>  							      argv[0], NULL, argv[1],
> -							      CONFIG_FLAGS_MULTI_REPLACE);
> +							      flags | CONFIG_FLAGS_MULTI_REPLACE);
Show 16 quoted lines
> --- a/config.c
> +++ b/config.c
> @@ -2402,6 +2402,7 @@ struct config_store_data {
>  	size_t baselen;
>  	char *key;
>  	int do_not_match;
> +	const char *literal_value;
>  	regex_t *value_regex;
>  	int multi_replace;
>  	struct {
> @@ -2431,6 +2432,8 @@ static int matches(const char *key, const char *value,
>  {
>  	if (strcmp(key, store->key))
>  		return 0; /* not ours */
> +	if (store->literal_value)
> +		return !strcmp(store->literal_value, value);
Nice. Short-circuiting the entire regex process if necessary.
Show 6 quoted lines
> @@ -2803,6 +2806,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
>  			store.value_regex = NULL;
>  		else if (value_regex == CONFIG_REGEX_NONE)
>  			store.value_regex = CONFIG_REGEX_NONE;
> +		else if (flags & CONFIG_FLAGS_FIXED_VALUE)
> +			store.literal_value = value_regex;

Ah, so we use .literal_value instead of pulling the string from value_regex because value_regex undergoes some special parsing and is packed into a regex_t instead.

Show 14 quoted lines
>  		else {
>  			if (value_regex[0] == '!') {
>  				store.do_not_match = 1;
> diff --git a/config.h b/config.h
> index 80844604ab..977e690be8 100644
> --- a/config.h
> +++ b/config.h
> @@ -269,6 +269,13 @@ int git_config_key_is_valid(const char *key);
>   */
>  #define CONFIG_FLAGS_MULTI_REPLACE (1 << 0)
>  
> +/*
> + * When CONFIG_FLAGS_FIXED_VALUE is specified, match key/value pairs
> + * by string comparison (not regex match) to the provided value_regex

In contrast to my comment elsewhere, I think here it makes sense to talk about strcmp like you did. But notice that here in the dev-facing documentation you said "not regex" - so I think that you thought it was a useful distinction, but forgot to say so to the user.

> + * parameter.
> + */
> +#define CONFIG_FLAGS_FIXED_VALUE (1 << 1)
> +
Show 7 quoted lines
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 6dc8117241..30e80ae9cb 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> +test_expect_success '--fixed-value uses exact string matching' '
> +	GLOB="a+b*c?d[e]f.g" &&
> +	rm -f initial &&

This tells me that when you used 'initial' earlier you probably should have done something like 'test_when_finished rm initial' in this test and your earlier ones. Whoops.

Show 15 quoted lines
> +	git config --file=initial fixed.test "$GLOB" &&
> +
> +	cp initial config &&
> +	git config --file=config fixed.test bogus "$GLOB" &&
> +	git config --file=config --list >actual &&
> +	cat >expect <<-EOF &&
> +	fixed.test=$GLOB
> +	fixed.test=bogus
> +	EOF
> +	test_cmp expect actual &&
> +
> +	cp initial config &&
> +	git config --file=config --fixed-value fixed.test bogus "$GLOB" &&
> +	git config --file=config --list >actual &&
> +	printf "fixed.test=bogus\n" >expect &&

It is jarring to me to see a printf here when everywhere else we use heredocs. 'git grep' tells me it's not unheard of, but it looks like those are cases where the whole file doesn't use heredocs.

Show 6 quoted lines
> +	test_cmp expect actual &&
> +
> +	cp initial config &&
> +	test_must_fail git config --file=config --unset fixed.test "$GLOB" &&
> +	git config --file=config --fixed-value --unset fixed.test "$GLOB" &&
> +	test_must_fail git config --file=config fixed.test &&

Is this one supposed to verify that there is a 'fixed.test' value already in 'config'? I'd prefer to see that explicitly checked with 'git config --get' rather than watching for a symptom, that is, fail to set. This comment applies to the next case too.

Show 14 quoted lines
> +
> +	cp initial config &&
> +	test_must_fail git config --file=config --unset-all fixed.test "$GLOB" &&
> +	git config --file=config --fixed-value --unset-all fixed.test "$GLOB" &&
> +	test_must_fail git config --file=config fixed.test &&
> +
> +	cp initial config &&
> +	git config --file=config --replace-all fixed.test bogus "$GLOB" &&
> +	git config --file=config --list >actual &&
> +	cat >expect <<-EOF &&
> +	fixed.test=$GLOB
> +	fixed.test=bogus
> +	EOF
> +	test_cmp expect actual &&

Hm, isn't this the same functionality as the tests you added at the beginning of this series? I guess you are setting up for the last case with --replace-all...

Show 9 quoted lines
> +
> +	cp initial config &&
> +	git config --file=config --replace-all fixed.test bogus "$GLOB" &&
> +	git config --file=config --list >actual &&
> +	cat >expect <<-EOF &&
> +	fixed.test=$GLOB
> +	fixed.test=bogus
> +	EOF
> +	test_cmp expect actual &&
Is this one identical to the previous one? I think it is, but if it
isn't and I can't tell, all the more reason that each case here should
either be labeled with a comment or separated into its own test. (Bonus
- you could extend the individual tests from patch 1 to make sure they
work correctly with --fixed-value too ;) )
Show 6 quoted lines
> +
> +	git config --file=config --fixed-value --replace-all fixed.test bogus "$GLOB" &&
> +	git config --file=config --list >actual &&
> +	cat >expect <<-EOF &&
> +	fixed.test=bogus
> +	fixed.test=bogus

Hum, it does what it says on the box, but is this a valid config, I wonder?

  $ git config --file=test foo.test aaa
  $ git config --file=test --add foo.test bbb
  $ git config --list --file=test
  foo.test=aaa
  foo.test=bbb
  $ git config --file=test --replace-all foo.test bbb a+
  $ git config --list --file=test
  foo.test=bbb
  foo.test=bbb
So I guess it's fine :)
 - Emily
Previous: Derrick Stolee via GitGitGadgetNext: Eric Sunshine
Message 46 of 72 in “config: add --literal-value option”
  1. 0/7 config: add --literal-value optionDerrick Stolee via GitGitGadget, Nov 19, 2020
  2. 1/7 t1300: test "set all" mode with value_regexDerrick Stolee via GitGitGadget, Nov 19, 2020
  3. Junio C HamanoNov 19, 2020
  4. brian m. carlsonNov 20, 2020
  5. Junio C HamanoNov 20, 2020
  6. Jeff KingNov 20, 2020
  7. Junio C HamanoNov 20, 2020
  8. brian m. carlsonNov 21, 2020
  9. Junio C HamanoNov 22, 2020
  10. Jeff KingNov 24, 2020
  11. Junio C HamanoNov 24, 2020
  12. 2/7 t1300: add test for --replace-all with value_regexDerrick Stolee via GitGitGadget, Nov 19, 2020
  13. 5/7 config: plumb --literal-value into config APIDerrick Stolee via GitGitGadget, Nov 19, 2020
  14. Junio C HamanoNov 19, 2020
  15. 4/7 config: add --literal-value option, un-implementedDerrick Stolee via GitGitGadget, Nov 19, 2020
  16. Junio C HamanoNov 19, 2020
  17. Martin ÅgrenNov 20, 2020
  18. 3/7 config: convert multi_replace to flagsDerrick Stolee via GitGitGadget, Nov 19, 2020
  19. Junio C HamanoNov 19, 2020
  20. 7/7 maintenance: use 'git config --literal-value'Derrick Stolee via GitGitGadget, Nov 19, 2020
  21. Junio C HamanoNov 19, 2020
  22. 6/7 config: implement --literal-value with --get*Derrick Stolee via GitGitGadget, Nov 19, 2020
  23. Ævar Arnfjörð BjarmasonNov 20, 2020
  24. Derrick StoleeNov 20, 2020
  25. Junio C HamanoNov 20, 2020
  26. Derrick StoleeNov 20, 2020
  27. Junio C HamanoNov 20, 2020
  28. Ævar Arnfjörð BjarmasonNov 24, 2020
  29. 0/7 config: add --fixed-value optionDerrick Stolee via GitGitGadget, Nov 23, 2020
  30. 1/7 t1300: test "set all" mode with value_regexDerrick Stolee via GitGitGadget, Nov 23, 2020
  31. Emily ShafferNov 23, 2020
  32. 2/7 t1300: add test for --replace-all with value_regexDerrick Stolee via GitGitGadget, Nov 23, 2020
  33. Emily ShafferNov 23, 2020
  34. 4/7 config: add --fixed-value option, un-implementedDerrick Stolee via GitGitGadget, Nov 23, 2020
  35. Junio C HamanoNov 23, 2020
  36. Emily ShafferNov 23, 2020
  37. Junio C HamanoNov 23, 2020
  38. Derrick StoleeNov 25, 2020
  39. Derrick StoleeNov 25, 2020
  40. Eric SunshineNov 25, 2020
  41. Junio C HamanoNov 25, 2020
  42. Junio C HamanoNov 25, 2020
  43. 3/7 config: convert multi_replace to flagsDerrick Stolee via GitGitGadget, Nov 23, 2020
  44. Emily ShafferNov 23, 2020
  45. 5/7 config: plumb --fixed-value into config APIDerrick Stolee via GitGitGadget, Nov 23, 2020
  46. Emily ShafferNov 23, 2020
  47. Eric SunshineNov 24, 2020
  48. Derrick StoleeNov 25, 2020
  49. Eric SunshineNov 25, 2020
  50. 6/7 config: implement --fixed-value with --get*Derrick Stolee via GitGitGadget, Nov 23, 2020
  51. Junio C HamanoNov 23, 2020
  52. Emily ShafferNov 23, 2020
  53. 7/7 maintenance: use 'git config --fixed-value'Derrick Stolee via GitGitGadget, Nov 23, 2020
  54. Junio C HamanoNov 23, 2020
  55. Emily ShafferNov 23, 2020
  56. Junio C HamanoNov 23, 2020
  57. Junio C HamanoNov 23, 2020
  58. 0/8 config: add --fixed-value optionDerrick Stolee via GitGitGadget, Nov 25, 2020
  59. 3/8 t1300: test "set all" mode with value-patternDerrick Stolee via GitGitGadget, Nov 25, 2020
  60. 4/8 t1300: add test for --replace-all with value-patternDerrick Stolee via GitGitGadget, Nov 25, 2020
  61. 6/8 config: plumb --fixed-value into config APIDerrick Stolee via GitGitGadget, Nov 25, 2020
  62. 8/8 maintenance: use 'git config --fixed-value'Derrick Stolee via GitGitGadget, Nov 25, 2020
  63. Junio C HamanoNov 25, 2020
  64. 5/8 config: add --fixed-value option, un-implementedDerrick Stolee via GitGitGadget, Nov 25, 2020
  65. Eric SunshineNov 25, 2020
  66. 2/8 config: replace 'value_regex' with 'value_pattern'Derrick Stolee via GitGitGadget, Nov 25, 2020
  67. Eric SunshineNov 25, 2020
  68. 1/8 config: convert multi_replace to flagsDerrick Stolee via GitGitGadget, Nov 25, 2020
  69. 7/8 config: implement --fixed-value with --get*Derrick Stolee via GitGitGadget, Nov 25, 2020
  70. Junio C HamanoNov 25, 2020
  71. Derrick StoleeNov 26, 2020
  72. Junio C HamanoDec 1, 2020

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.