| Message ID | 0e6a7371ed4696f6cc85df07466fb6c20d58d62e.1606147507.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | config: add --fixed-value option | expand |
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > + if (fixed_value) { > + int allowed_usage = 0; > + > + switch (actions) { > + case ACTION_GET: > + case ACTION_GET_ALL: > + case ACTION_GET_REGEXP: > + case ACTION_UNSET: > + case ACTION_UNSET_ALL: > + allowed_usage = argc > 1 && !!argv[1]; > + break; > + > + case ACTION_SET_ALL: > + case ACTION_REPLACE_ALL: > + allowed_usage = argc > 2 && !!argv[2]; > + break; Implicitly all other ops do not want to work with the --fixed-value option, which makes sense. > + } > + > + if (!allowed_usage) { > + error(_("--fixed-value only applies with 'value_regex'")); > + usage_builtin_config(); > + } > + } I was afraid that abandoning the bitmask based approach of the previous round may bloat the code but seeing the result here makes me quite happy ;-) It is quite clear what is going on. Nice.
On Mon, Nov 23, 2020 at 04:05:04PM +0000, Derrick Stolee via GitGitGadget wrote: > > > The 'git config' builtin takes a 'value_regex' parameter for several > actions. This can cause confusion when expecting exact value matches > instead of regex matches, especially when the input string contains glob > characters. While callers can escape the patterns themselves, it would > be more friendly to allow an argument to disable the pattern matching in > favor of an exact string match. > > Add a new '--fixed-value' option that does not currently change the > behavior. The implementation will follow for each appropriate action. > For now, check and test that --fixed-value will abort the command when > included with an incompatible action or without a 'value_regex' > argument. > > The name '--fixed-value' was chosen over something simpler like > '--fixed' because some commands allow regular expressions on the > key in addition to the value. > > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > Documentation/git-config.txt | 20 +++++++++++++------- > builtin/config.c | 26 ++++++++++++++++++++++++++ > t/t1300-config.sh | 23 +++++++++++++++++++++++ > 3 files changed, 62 insertions(+), 7 deletions(-) > > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt > index 7573160f21..d4bb928ea7 100644 > --- a/builtin/config.c > +++ b/builtin/config.c > @@ -34,6 +34,7 @@ static int respect_includes_opt = -1; > static struct config_options config_options; > static int show_origin; > static int show_scope; > +static int fixed_value; > > #define ACTION_GET (1<<0) > #define ACTION_GET_ALL (1<<1) > @@ -141,6 +142,7 @@ static struct option builtin_config_options[] = { > OPT_BIT(0, "rename-section", &actions, N_("rename section: old-name new-name"), ACTION_RENAME_SECTION), > OPT_BIT(0, "remove-section", &actions, N_("remove a section: name"), ACTION_REMOVE_SECTION), > OPT_BIT('l', "list", &actions, N_("list all"), ACTION_LIST), > + OPT_BOOL(0, "fixed-value", &fixed_value, N_("use string equality when matching values")), I'm not sure how to feel about this phrasing. I wonder if it would be clearer to say something like 'treat 'value_regex' as a literal string instead'? Hmmm. > + if (fixed_value) { > + int allowed_usage = 0; > + > + switch (actions) { > + case ACTION_GET: > + case ACTION_GET_ALL: > + case ACTION_GET_REGEXP: > + case ACTION_UNSET: > + case ACTION_UNSET_ALL: > + allowed_usage = argc > 1 && !!argv[1]; > + break; > + > + case ACTION_SET_ALL: > + case ACTION_REPLACE_ALL: > + allowed_usage = argc > 2 && !!argv[2]; > + break; > + } > + > + if (!allowed_usage) { > + error(_("--fixed-value only applies with 'value_regex'")); > + usage_builtin_config(); > + } > + } > + To me this really needs a comment. I think you are checking whether the value_regex is actually present, and the position of that regex changes depending on the rest of the arg list... What about something like: /* If set, ensure 'value_regex' was provided also */ if (fixed_value) { ... /* 'git config --unset-all <key> <value_regex>' */ case ACTION_UNSET_ALL: ... /* 'git config --replace-all <key> <new value> <value_regex>' */ case ACTION_REPLACE_ALL: ... > +test_expect_success 'refuse --fixed-value for incompatible actions' ' > + git config --file=config dev.null bogus && > + > + # These modes do not allow --fixed-value at all > + test_must_fail git config --file=config --fixed-value --add dev.null bogus && > + test_must_fail git config --file=config --fixed-value --get-urlmatch dev.null bogus && > + test_must_fail git config --file=config --fixed-value --get-urlmatch dev.null bogus && > + test_must_fail git config --file=config --fixed-value --rename-section dev null && > + test_must_fail git config --file=config --fixed-value --remove-section dev && > + test_must_fail git config --file=config --fixed-value --list && > + test_must_fail git config --file=config --fixed-value --get-color dev.null && > + test_must_fail git config --file=config --fixed-value --get-colorbool dev.null && > + > + # These modes complain when --fixed-value has no value_regex > + test_must_fail git config --file=config --fixed-value dev.null bogus && > + test_must_fail git config --file=config --fixed-value --replace-all dev.null bogus && > + test_must_fail git config --file=config --fixed-value --get dev.null && > + test_must_fail git config --file=config --fixed-value --get-all dev.null && > + test_must_fail git config --file=config --fixed-value --get-regexp "dev.*" && > + test_must_fail git config --file=config --fixed-value --unset dev.null && > + test_must_fail git config --file=config --fixed-value --unset-all dev.null > +' > + I'd expect to see a positive test as well, which you could later evolve into a test to ensure behavior. - Emily
Emily Shaffer <emilyshaffer@google.com> writes: >> OPT_BIT('l', "list", &actions, N_("list all"), ACTION_LIST), >> + OPT_BOOL(0, "fixed-value", &fixed_value, N_("use string equality when matching values")), > I'm not sure how to feel about this phrasing. I wonder if it would be > clearer to say something like 'treat 'value_regex' as a literal string > instead'? Hmmm. Update the document and help text with s/value_regex/value_pattern/ and say "use value_pattern as a fixed string, not an extended regexp", perhaps? > /* If set, ensure 'value_regex' was provided also */ > if (fixed_value) { > ... > /* 'git config --unset-all <key> <value_regex>' */ > case ACTION_UNSET_ALL: Nice.
On 11/23/2020 5:41 PM, Junio C Hamano wrote: > Emily Shaffer <emilyshaffer@google.com> writes: > >>> OPT_BIT('l', "list", &actions, N_("list all"), ACTION_LIST), >>> + OPT_BOOL(0, "fixed-value", &fixed_value, N_("use string equality when matching values")), >> I'm not sure how to feel about this phrasing. I wonder if it would be >> clearer to say something like 'treat 'value_regex' as a literal string >> instead'? Hmmm. > > Update the document and help text with s/value_regex/value_pattern/ > and say "use value_pattern as a fixed string, not an extended regexp", > perhaps? If I go about changing all documentation and error messages to say "value_pattern" instead of "value_regex", should I also update the uses in the *.po translation files? Or, should I leave them unmodified to trigger manual intervention by the translators? Thanks, -Stolee
On 11/25/2020 9:08 AM, Derrick Stolee wrote: > On 11/23/2020 5:41 PM, Junio C Hamano wrote: >> Emily Shaffer <emilyshaffer@google.com> writes: >> >>>> OPT_BIT('l', "list", &actions, N_("list all"), ACTION_LIST), >>>> + OPT_BOOL(0, "fixed-value", &fixed_value, N_("use string equality when matching values")), >>> I'm not sure how to feel about this phrasing. I wonder if it would be >>> clearer to say something like 'treat 'value_regex' as a literal string >>> instead'? Hmmm. >> >> Update the document and help text with s/value_regex/value_pattern/ >> and say "use value_pattern as a fixed string, not an extended regexp", >> perhaps? > > If I go about changing all documentation and error messages to say > "value_pattern" instead of "value_regex", should I also update the uses > in the *.po translation files? Or, should I leave them unmodified to > trigger manual intervention by the translators? I have discovered that changing these causes conflicts with automated line number updates. I'll leave these unchanged with a note for translators. Thanks, -Stolee
On Wed, Nov 25, 2020 at 9:09 AM Derrick Stolee <stolee@gmail.com> wrote: > On 11/23/2020 5:41 PM, Junio C Hamano wrote: > > Update the document and help text with s/value_regex/value_pattern/ > > and say "use value_pattern as a fixed string, not an extended regexp", > > perhaps? > > If I go about changing all documentation and error messages to say > "value_pattern" instead of "value_regex", should I also update the uses > in the *.po translation files? Or, should I leave them unmodified to > trigger manual intervention by the translators? A minor request: If you are going to put in the work to make that substitution, perhaps change it to "value-pattern" rather than "value_pattern" since a hyphen is more common in documentation for this sort of thing than underscore.
Derrick Stolee <stolee@gmail.com> writes: > On 11/23/2020 5:41 PM, Junio C Hamano wrote: >> Emily Shaffer <emilyshaffer@google.com> writes: >> >>>> OPT_BIT('l', "list", &actions, N_("list all"), ACTION_LIST), >>>> + OPT_BOOL(0, "fixed-value", &fixed_value, N_("use string equality when matching values")), >>> I'm not sure how to feel about this phrasing. I wonder if it would be >>> clearer to say something like 'treat 'value_regex' as a literal string >>> instead'? Hmmm. >> >> Update the document and help text with s/value_regex/value_pattern/ >> and say "use value_pattern as a fixed string, not an extended regexp", >> perhaps? > > If I go about changing all documentation and error messages to say > "value_pattern" instead of "value_regex", should I also update the uses > in the *.po translation files? Or, should I leave them unmodified to > trigger manual intervention by the translators? If you do, you do not have to worry. The i18n/l10n coordinator will update the po/git.pot file when we near the code freeze using an automated tool that extracts strings from the sources, and the po/xx.po files for languages are updated from the updated po/git.pot mechanically with another tool, reusing unmodified entries, adding new ones, and marking near-hit ones to help avoid unnecessary work by translators. I earlier thought that the "when matching values" phrase you chose in this round, without such a clean-up, would be OK in the context of this topic (which is depended on by a bugfix topic), but after seeing how we need to clarify the way '!' negation prefix works in the documentation that has deeply ingrained assumption that value is matched using ERE, it may be necessary to bite the bullet and do the regex->pattern now. Thanks.
Eric Sunshine <sunshine@sunshineco.com> writes: > On Wed, Nov 25, 2020 at 9:09 AM Derrick Stolee <stolee@gmail.com> wrote: >> On 11/23/2020 5:41 PM, Junio C Hamano wrote: >> > Update the document and help text with s/value_regex/value_pattern/ >> > and say "use value_pattern as a fixed string, not an extended regexp", >> > perhaps? >> >> If I go about changing all documentation and error messages to say >> "value_pattern" instead of "value_regex", should I also update the uses >> in the *.po translation files? Or, should I leave them unmodified to >> trigger manual intervention by the translators? > > A minor request: If you are going to put in the work to make that > substitution, perhaps change it to "value-pattern" rather than > "value_pattern" since a hyphen is more common in documentation for > this sort of thing than underscore. Yup, I misspoke. s/value_regex/value-regex/ unless it is a variable name where '-' cannot appear.
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index 7573160f21..d4bb928ea7 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -9,15 +9,15 @@ git-config - Get and set repository or global options SYNOPSIS -------- [verse] -'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] name [value [value_regex]] +'git config' [<file-option>] [--type=<type>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] name [value [value_regex]] 'git config' [<file-option>] [--type=<type>] --add name value -'git config' [<file-option>] [--type=<type>] --replace-all name value [value_regex] -'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] --get name [value_regex] -'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] --get-all name [value_regex] -'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--name-only] --get-regexp name_regex [value_regex] +'git config' [<file-option>] [--type=<type>] [--fixed-value] --replace-all name value [value_regex] +'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get name [value_regex] +'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all name [value_regex] +'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp name_regex [value_regex] 'git config' [<file-option>] [--type=<type>] [-z|--null] --get-urlmatch name URL -'git config' [<file-option>] --unset name [value_regex] -'git config' [<file-option>] --unset-all name [value_regex] +'git config' [<file-option>] [--fixed-value] --unset name [value_regex] +'git config' [<file-option>] [--fixed-value] --unset-all name [value_regex] 'git config' [<file-option>] --rename-section old_name new_name 'git config' [<file-option>] --remove-section name 'git config' [<file-option>] [--show-origin] [--show-scope] [-z|--null] [--name-only] -l | --list @@ -165,6 +165,12 @@ See also <<FILES>>. --list:: List all variables set in config file, along with their values. +--fixed-value:: + When used with the `value_regex` argument, treat `value_regex` as + an exact string instead of a regular expression. This will restrict + the name/value pairs that are matched to only those where the value + is exactly equal to the `value_regex`. + --type <type>:: 'git config' will ensure that any input or output is valid under the given type constraint(s), and will canonicalize outgoing values in `<type>`'s diff --git a/builtin/config.c b/builtin/config.c index e7c7f3d455..bfb55a96df 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -34,6 +34,7 @@ static int respect_includes_opt = -1; static struct config_options config_options; static int show_origin; static int show_scope; +static int fixed_value; #define ACTION_GET (1<<0) #define ACTION_GET_ALL (1<<1) @@ -141,6 +142,7 @@ static struct option builtin_config_options[] = { OPT_BIT(0, "rename-section", &actions, N_("rename section: old-name new-name"), ACTION_RENAME_SECTION), OPT_BIT(0, "remove-section", &actions, N_("remove a section: name"), ACTION_REMOVE_SECTION), OPT_BIT('l', "list", &actions, N_("list all"), ACTION_LIST), + OPT_BOOL(0, "fixed-value", &fixed_value, N_("use string equality when matching values")), OPT_BIT('e', "edit", &actions, N_("open an editor"), ACTION_EDIT), OPT_BIT(0, "get-color", &actions, N_("find the color configured: slot [default]"), ACTION_GET_COLOR), OPT_BIT(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL), @@ -745,6 +747,30 @@ int cmd_config(int argc, const char **argv, const char *prefix) usage_builtin_config(); } + if (fixed_value) { + int allowed_usage = 0; + + switch (actions) { + case ACTION_GET: + case ACTION_GET_ALL: + case ACTION_GET_REGEXP: + case ACTION_UNSET: + case ACTION_UNSET_ALL: + allowed_usage = argc > 1 && !!argv[1]; + break; + + case ACTION_SET_ALL: + case ACTION_REPLACE_ALL: + allowed_usage = argc > 2 && !!argv[2]; + break; + } + + if (!allowed_usage) { + error(_("--fixed-value only applies with 'value_regex'")); + usage_builtin_config(); + } + } + if (actions & PAGING_ACTIONS) setup_auto_pager("config", 1); diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 8783767d4f..6dc8117241 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1960,4 +1960,27 @@ test_expect_success '--replace-all and value_regex' ' test_cmp expect actual ' +test_expect_success 'refuse --fixed-value for incompatible actions' ' + git config --file=config dev.null bogus && + + # These modes do not allow --fixed-value at all + test_must_fail git config --file=config --fixed-value --add dev.null bogus && + test_must_fail git config --file=config --fixed-value --get-urlmatch dev.null bogus && + test_must_fail git config --file=config --fixed-value --get-urlmatch dev.null bogus && + test_must_fail git config --file=config --fixed-value --rename-section dev null && + test_must_fail git config --file=config --fixed-value --remove-section dev && + test_must_fail git config --file=config --fixed-value --list && + test_must_fail git config --file=config --fixed-value --get-color dev.null && + test_must_fail git config --file=config --fixed-value --get-colorbool dev.null && + + # These modes complain when --fixed-value has no value_regex + test_must_fail git config --file=config --fixed-value dev.null bogus && + test_must_fail git config --file=config --fixed-value --replace-all dev.null bogus && + test_must_fail git config --file=config --fixed-value --get dev.null && + test_must_fail git config --file=config --fixed-value --get-all dev.null && + test_must_fail git config --file=config --fixed-value --get-regexp "dev.*" && + test_must_fail git config --file=config --fixed-value --unset dev.null && + test_must_fail git config --file=config --fixed-value --unset-all dev.null +' + test_done