| Message ID | 39718048cd8f5be053a13ff73f531e3400b80b25.1606147507.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | config: add --fixed-value option | expand |
On Mon, Nov 23, 2020 at 04:05:05PM +0000, Derrick Stolee via GitGitGadget wrote: > > > 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] > } > 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] > @@ -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); > --- 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. > @@ -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. > 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) > + > 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. > + 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. > + 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. > + > + 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... > + > + 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 ;) ) > + > + 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
On Mon, Nov 23, 2020 at 5:22 PM Emily Shaffer <emilyshaffer@google.com> wrote: > On Mon, Nov 23, 2020 at 04:05:05PM +0000, Derrick Stolee via GitGitGadget wrote: > > + 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. For creating a one-line file, the verbosity of a here-doc may be overkill. At any rate, `echo` would be more idiomatic than `printf`: echo "fixed.test=bogus" >expect && (Though, this alone is likely not worth a re-roll.)
On 11/23/2020 5:21 PM, Emily Shaffer wrote: > On Mon, Nov 23, 2020 at 04:05:05PM +0000, Derrick Stolee via GitGitGadget wrote: >> + >> + 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. Good catch. >> @@ -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. Reminds me to call this 'fixed_value'. >> +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. Good idea. Will do. >> + 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. I can use a heredoc just to be consistent. (To also respond to Eric's message, I tend to use printf instead of echo because echo starts a process while printf does not.) >> + 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. If no value is provided, then 'git config <name>' _is_ a query. It's not a failed set. 'git config <name> ""' would be the way to try and set the value to an empty string. >> + >> + 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... This is specifically demonstrating the difference that --fixed-value provides. The tests here show "the match doesn't work by default, but then works with --fixed-value". I'll make this clearer in the commit message. >> + >> + 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 ;) ) Yes, accidentally over-zealous copy/paste. I'm less in favor of splitting the tests, since they rely on a shared initial config. Any failure in one part of this test is likely to also fail the rest of the commands, so grouping them by this toggle makes sense to me. Thanks, -Stolee
On Wed, Nov 25, 2020 at 10:49 AM Derrick Stolee <stolee@gmail.com> wrote: > On 11/23/2020 5:21 PM, Emily Shaffer wrote: > > On Mon, Nov 23, 2020 at 04:05:05PM +0000, Derrick Stolee via GitGitGadget wrote: > >> + 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. > > I can use a heredoc just to be consistent. > > (To also respond to Eric's message, I tend to use printf instead > of echo because echo starts a process while printf does not.) `echo` is a builtin[1] in many or most shells (just as `printf` also often is), thus is unlikely to start a process. [1] Aside: You can use `type <cmd>` or `command -V <cmd>` to see if <cmd> is a builtin in your shell.
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; } if (actions & PAGING_ACTIONS) @@ -832,7 +835,8 @@ int cmd_config(int argc, const char **argv, const char *prefix) value = normalize_value(argv[0], argv[1]); UNLEAK(value); return git_config_set_multivar_in_file_gently(given_config_source.file, - argv[0], value, argv[2], 0); + argv[0], value, argv[2], + flags); } else if (actions == ACTION_ADD) { check_write(); @@ -841,7 +845,8 @@ 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, - CONFIG_REGEX_NONE, 0); + CONFIG_REGEX_NONE, + flags); } 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); } else if (actions == ACTION_GET) { check_argc(argc, 1, 2); @@ -877,7 +882,8 @@ int cmd_config(int argc, const char **argv, const char *prefix) check_argc(argc, 1, 2); if (argc == 2) return git_config_set_multivar_in_file_gently(given_config_source.file, - argv[0], NULL, argv[1], 0); + argv[0], NULL, argv[1], + flags); else return git_config_set_in_file_gently(given_config_source.file, argv[0], NULL); @@ -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); } else if (actions == ACTION_RENAME_SECTION) { int ret; diff --git a/config.c b/config.c index 096f2eae0d..64bab2b08e 100644 --- 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); if (!store->value_regex) return 1; /* always matches */ if (store->value_regex == CONFIG_REGEX_NONE) @@ -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; 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 + * parameter. + */ +#define CONFIG_FLAGS_FIXED_VALUE (1 << 1) + int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned); void git_config_set_multivar(const char *, const char *, const char *, unsigned); int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned); 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 @@ -1983,4 +1983,61 @@ test_expect_success 'refuse --fixed-value for incompatible actions' ' test_must_fail git config --file=config --fixed-value --unset-all dev.null ' +test_expect_success '--fixed-value uses exact string matching' ' + GLOB="a+b*c?d[e]f.g" && + rm -f initial && + 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 && + 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 && + + 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 && + + 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 && + + 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 + EOF + test_cmp expect actual +' + test_done