From 504ee1290e38fb1ff0d76f940b124e21ab57a99f Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 25 Nov 2020 22:12:49 +0000 Subject: [PATCH 1/8] config: convert multi_replace to flags We will extend the flexibility of the config API. Before doing so, let's take an existing 'int multi_replace' parameter and replace it with a new 'unsigned flags' parameter that can take multiple options as a bit field. Update all callers that specified multi_replace to now specify the CONFIG_FLAGS_MULTI_REPLACE flag. To add more clarity, extend the documentation of git_config_set_multivar_in_file() including a clear labeling of its arguments. Other config API methods in config.h require only a change of the final parameter from 'int' to 'unsigned'. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- builtin/branch.c | 4 ++-- builtin/config.c | 6 ++++-- builtin/remote.c | 8 +++++--- config.c | 24 ++++++++++++------------ config.h | 29 ++++++++++++++++++++++------- 5 files changed, 45 insertions(+), 26 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index efb30b8820..173b736dff 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -829,10 +829,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix) die(_("Branch '%s' has no upstream information"), branch->name); strbuf_addf(&buf, "branch.%s.remote", branch->name); - git_config_set_multivar(buf.buf, NULL, NULL, 1); + git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE); strbuf_reset(&buf); strbuf_addf(&buf, "branch.%s.merge", branch->name); - git_config_set_multivar(buf.buf, NULL, NULL, 1); + git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE); strbuf_release(&buf); } else if (argc > 0 && argc <= 2) { if (filter.kind != FILTER_REFS_BRANCHES) diff --git a/builtin/config.c b/builtin/config.c index 963d65fd3f..7c15c5d743 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -844,7 +844,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], 1); + argv[0], value, argv[2], + CONFIG_FLAGS_MULTI_REPLACE); } else if (actions == ACTION_GET) { check_argc(argc, 1, 2); @@ -880,7 +881,8 @@ int cmd_config(int argc, const char **argv, const char *prefix) check_write(); check_argc(argc, 1, 2); return git_config_set_multivar_in_file_gently(given_config_source.file, - argv[0], NULL, argv[1], 1); + argv[0], NULL, argv[1], + CONFIG_FLAGS_MULTI_REPLACE); } else if (actions == ACTION_RENAME_SECTION) { int ret; diff --git a/builtin/remote.c b/builtin/remote.c index c1b211b272..d11a5589e4 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -712,7 +712,7 @@ static int mv(int argc, const char **argv) strbuf_reset(&buf); strbuf_addf(&buf, "remote.%s.fetch", rename.new_name); - git_config_set_multivar(buf.buf, NULL, NULL, 1); + git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE); strbuf_addf(&old_remote_context, ":refs/remotes/%s/", rename.old_name); for (i = 0; i < oldremote->fetch.raw_nr; i++) { char *ptr; @@ -1491,7 +1491,8 @@ static int update(int argc, const char **argv) static int remove_all_fetch_refspecs(const char *key) { - return git_config_set_multivar_gently(key, NULL, NULL, 1); + return git_config_set_multivar_gently(key, NULL, NULL, + CONFIG_FLAGS_MULTI_REPLACE); } static void add_branches(struct remote *remote, const char **branches, @@ -1686,7 +1687,8 @@ static int set_url(int argc, const char **argv) if (!delete_mode) git_config_set_multivar(name_buf.buf, newurl, oldurl, 0); else - git_config_set_multivar(name_buf.buf, NULL, oldurl, 1); + git_config_set_multivar(name_buf.buf, NULL, oldurl, + CONFIG_FLAGS_MULTI_REPLACE); out: strbuf_release(&name_buf); return 0; diff --git a/config.c b/config.c index 2bdff4457b..b5e8ab13b3 100644 --- a/config.c +++ b/config.c @@ -2729,9 +2729,9 @@ void git_config_set(const char *key, const char *value) * if value_regex!=NULL, disregard key/value pairs where value does not match. * if value_regex==CONFIG_REGEX_NONE, do not match any existing values * (only add a new one) - * if multi_replace==0, nothing, or only one matching key/value is replaced, - * else all matching key/values (regardless how many) are removed, - * before the new pair is written. + * if flags contains the CONFIG_FLAGS_MULTI_REPLACE flag, all matching + * key/values are removed before a single new pair is written. If the + * flag is not present, then replace only the first match. * * Returns 0 on success. * @@ -2752,7 +2752,7 @@ void git_config_set(const char *key, const char *value) int git_config_set_multivar_in_file_gently(const char *config_filename, const char *key, const char *value, const char *value_regex, - int multi_replace) + unsigned flags) { int fd = -1, in_fd = -1; int ret; @@ -2769,7 +2769,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, if (ret) goto out_free; - store.multi_replace = multi_replace; + store.multi_replace = (flags & CONFIG_FLAGS_MULTI_REPLACE) != 0; if (!config_filename) config_filename = filename_buf = git_pathdup("config"); @@ -2858,7 +2858,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, /* if nothing to unset, or too many matches, error out */ if ((store.seen_nr == 0 && value == NULL) || - (store.seen_nr > 1 && multi_replace == 0)) { + (store.seen_nr > 1 && !store.multi_replace)) { ret = CONFIG_NOTHING_SET; goto out_free; } @@ -2997,10 +2997,10 @@ write_err_out: void git_config_set_multivar_in_file(const char *config_filename, const char *key, const char *value, - const char *value_regex, int multi_replace) + const char *value_regex, unsigned flags) { if (!git_config_set_multivar_in_file_gently(config_filename, key, value, - value_regex, multi_replace)) + value_regex, flags)) return; if (value) die(_("could not set '%s' to '%s'"), key, value); @@ -3009,17 +3009,17 @@ void git_config_set_multivar_in_file(const char *config_filename, } int git_config_set_multivar_gently(const char *key, const char *value, - const char *value_regex, int multi_replace) + const char *value_regex, unsigned flags) { return git_config_set_multivar_in_file_gently(NULL, key, value, value_regex, - multi_replace); + flags); } void git_config_set_multivar(const char *key, const char *value, - const char *value_regex, int multi_replace) + const char *value_regex, unsigned flags) { git_config_set_multivar_in_file(NULL, key, value, value_regex, - multi_replace); + flags); } static int section_name_match (const char *buf, const char *name) diff --git a/config.h b/config.h index 91cdfbfb41..84fdf223c8 100644 --- a/config.h +++ b/config.h @@ -256,9 +256,22 @@ void git_config_set(const char *, const char *); int git_config_parse_key(const char *, char **, size_t *); int git_config_key_is_valid(const char *key); -int git_config_set_multivar_gently(const char *, const char *, const char *, int); -void git_config_set_multivar(const char *, const char *, const char *, int); -int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, int); + +/* + * The following macros specify flag bits that alter the behavior + * of the git_config_set_multivar*() methods. + */ + +/* + * When CONFIG_FLAGS_MULTI_REPLACE is specified, all matching key/values + * are removed before a single new pair is written. If the flag is not + * present, then set operations replace only the first match. + */ +#define CONFIG_FLAGS_MULTI_REPLACE (1 << 0) + +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); /** * takes four parameters: @@ -276,13 +289,15 @@ int git_config_set_multivar_in_file_gently(const char *, const char *, const cha * - the value regex, as a string. It will disregard key/value pairs where value * does not match. * - * - a multi_replace value, as an int. If value is equal to zero, nothing or only - * one matching key/value is replaced, else all matching key/values (regardless - * how many) are removed, before the new pair is written. + * - a flags value with bits corresponding to the CONFIG_FLAG_* macros. * * It returns 0 on success. */ -void git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int); +void git_config_set_multivar_in_file(const char *config_filename, + const char *key, + const char *value, + const char *value_regex, + unsigned flags); /** * rename or remove sections in the config file From 247e2f822e6ccdccaa19ca1a54d4082ce5d819e7 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 25 Nov 2020 22:12:50 +0000 Subject: [PATCH 2/8] config: replace 'value_regex' with 'value_pattern' The 'value_regex' argument in the 'git config' builtin is poorly named, especially related to an upcoming change that allows exact string matches instead of ERE pattern matches. Perform a mostly mechanical change of every instance of 'value_regex' to 'value_pattern' in the codebase. This is only critical for documentation and error messages, but it is best to be consistent inside the codebase, too. For documentation, use 'value-pattern' which is better punctuation. This affects Documentation/git-config.txt and the usage in builtin/config.c, which was already mixed between 'value_regex' and 'value-regex'. I gave some thought to leaving the value_regex variables inside config.c that are regex_t pointers. However, it is probably best to keep the name consistent with the rest of the variables. This does not update the translations inside the po/ directory, as that creates conflicts with ongoing work. The input strings should automatically update through automation, and a few of the output strings currently use "[value_regex]" directly. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- Documentation/git-config.txt | 20 ++++++------- builtin/config.c | 12 ++++---- config.c | 54 ++++++++++++++++++------------------ config.h | 2 +- 4 files changed, 44 insertions(+), 44 deletions(-) diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index 7573160f21..0be5499952 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' [] [--type=] [--show-origin] [--show-scope] [-z|--null] name [value [value_regex]] +'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] name [value [value-pattern]] 'git config' [] [--type=] --add name value -'git config' [] [--type=] --replace-all name value [value_regex] -'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] --get name [value_regex] -'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] --get-all name [value_regex] -'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--name-only] --get-regexp name_regex [value_regex] +'git config' [] [--type=] --replace-all name value [value-pattern] +'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] --get name [value-pattern] +'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] --get-all name [value-pattern] +'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--name-only] --get-regexp name_regex [value-pattern] 'git config' [] [--type=] [-z|--null] --get-urlmatch name URL -'git config' [] --unset name [value_regex] -'git config' [] --unset-all name [value_regex] +'git config' [] --unset name [value-pattern] +'git config' [] --unset-all name [value-pattern] 'git config' [] --rename-section old_name new_name 'git config' [] --remove-section name 'git config' [] [--show-origin] [--show-scope] [-z|--null] [--name-only] -l | --list @@ -33,7 +33,7 @@ escaped. Multiple lines can be added to an option by using the `--add` option. If you want to update or unset an option which can occur on multiple -lines, a POSIX regexp `value_regex` needs to be given. Only the +lines, a POSIX regexp `value-pattern` needs to be given. Only the existing values that match the regexp are updated or unset. If you want to handle the lines that do *not* match the regex, just prepend a single exclamation mark in front (see also <>). @@ -73,11 +73,11 @@ OPTIONS --replace-all:: Default behavior is to replace at most one line. This replaces - all lines matching the key (and optionally the value_regex). + all lines matching the key (and optionally the `value-pattern`). --add:: Adds a new line to the option without altering any existing - values. This is the same as providing '^$' as the value_regex + values. This is the same as providing '^$' as the `value-pattern` in `--replace-all`. --get:: diff --git a/builtin/config.c b/builtin/config.c index 7c15c5d743..590392c811 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -133,14 +133,14 @@ static struct option builtin_config_options[] = { OPT_STRING('f', "file", &given_config_source.file, N_("file"), N_("use given config file")), OPT_STRING(0, "blob", &given_config_source.blob, N_("blob-id"), N_("read config from given blob object")), OPT_GROUP(N_("Action")), - OPT_BIT(0, "get", &actions, N_("get value: name [value-regex]"), ACTION_GET), - OPT_BIT(0, "get-all", &actions, N_("get all values: key [value-regex]"), ACTION_GET_ALL), - OPT_BIT(0, "get-regexp", &actions, N_("get values for regexp: name-regex [value-regex]"), ACTION_GET_REGEXP), + OPT_BIT(0, "get", &actions, N_("get value: name [value-pattern]"), ACTION_GET), + OPT_BIT(0, "get-all", &actions, N_("get all values: key [value-pattern]"), ACTION_GET_ALL), + OPT_BIT(0, "get-regexp", &actions, N_("get values for regexp: name-regex [value-pattern]"), ACTION_GET_REGEXP), OPT_BIT(0, "get-urlmatch", &actions, N_("get value specific for the URL: section[.var] URL"), ACTION_GET_URLMATCH), - OPT_BIT(0, "replace-all", &actions, N_("replace all matching variables: name value [value_regex]"), ACTION_REPLACE_ALL), + OPT_BIT(0, "replace-all", &actions, N_("replace all matching variables: name value [value-pattern]"), ACTION_REPLACE_ALL), OPT_BIT(0, "add", &actions, N_("add a new variable: name value"), ACTION_ADD), - OPT_BIT(0, "unset", &actions, N_("remove a variable: name [value-regex]"), ACTION_UNSET), - OPT_BIT(0, "unset-all", &actions, N_("remove all matches: name [value-regex]"), ACTION_UNSET_ALL), + OPT_BIT(0, "unset", &actions, N_("remove a variable: name [value-pattern]"), ACTION_UNSET), + OPT_BIT(0, "unset-all", &actions, N_("remove all matches: name [value-pattern]"), ACTION_UNSET_ALL), 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), diff --git a/config.c b/config.c index b5e8ab13b3..5b2f00f073 100644 --- a/config.c +++ b/config.c @@ -2415,7 +2415,7 @@ struct config_store_data { size_t baselen; char *key; int do_not_match; - regex_t *value_regex; + regex_t *value_pattern; int multi_replace; struct { size_t begin, end; @@ -2429,10 +2429,10 @@ struct config_store_data { static void config_store_data_clear(struct config_store_data *store) { free(store->key); - if (store->value_regex != NULL && - store->value_regex != CONFIG_REGEX_NONE) { - regfree(store->value_regex); - free(store->value_regex); + if (store->value_pattern != NULL && + store->value_pattern != CONFIG_REGEX_NONE) { + regfree(store->value_pattern); + free(store->value_pattern); } free(store->parsed); free(store->seen); @@ -2444,13 +2444,13 @@ static int matches(const char *key, const char *value, { if (strcmp(key, store->key)) return 0; /* not ours */ - if (!store->value_regex) + if (!store->value_pattern) return 1; /* always matches */ - if (store->value_regex == CONFIG_REGEX_NONE) + if (store->value_pattern == CONFIG_REGEX_NONE) return 0; /* never matches */ return store->do_not_match ^ - (value && !regexec(store->value_regex, value, 0, NULL, 0)); + (value && !regexec(store->value_pattern, value, 0, NULL, 0)); } static int store_aux_event(enum config_event_t type, @@ -2726,8 +2726,8 @@ void git_config_set(const char *key, const char *value) /* * If value==NULL, unset in (remove from) config, - * if value_regex!=NULL, disregard key/value pairs where value does not match. - * if value_regex==CONFIG_REGEX_NONE, do not match any existing values + * if value_pattern!=NULL, disregard key/value pairs where value does not match. + * if value_pattern==CONFIG_REGEX_NONE, do not match any existing values * (only add a new one) * if flags contains the CONFIG_FLAGS_MULTI_REPLACE flag, all matching * key/values are removed before a single new pair is written. If the @@ -2751,7 +2751,7 @@ void git_config_set(const char *key, const char *value) */ int git_config_set_multivar_in_file_gently(const char *config_filename, const char *key, const char *value, - const char *value_regex, + const char *value_pattern, unsigned flags) { int fd = -1, in_fd = -1; @@ -2812,22 +2812,22 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, int i, new_line = 0; struct config_options opts; - if (value_regex == NULL) - store.value_regex = NULL; - else if (value_regex == CONFIG_REGEX_NONE) - store.value_regex = CONFIG_REGEX_NONE; + if (value_pattern == NULL) + store.value_pattern = NULL; + else if (value_pattern == CONFIG_REGEX_NONE) + store.value_pattern = CONFIG_REGEX_NONE; else { - if (value_regex[0] == '!') { + if (value_pattern[0] == '!') { store.do_not_match = 1; - value_regex++; + value_pattern++; } else store.do_not_match = 0; - store.value_regex = (regex_t*)xmalloc(sizeof(regex_t)); - if (regcomp(store.value_regex, value_regex, + store.value_pattern = (regex_t*)xmalloc(sizeof(regex_t)); + if (regcomp(store.value_pattern, value_pattern, REG_EXTENDED)) { - error(_("invalid pattern: %s"), value_regex); - FREE_AND_NULL(store.value_regex); + error(_("invalid pattern: %s"), value_pattern); + FREE_AND_NULL(store.value_pattern); ret = CONFIG_INVALID_PATTERN; goto out_free; } @@ -2997,10 +2997,10 @@ write_err_out: void git_config_set_multivar_in_file(const char *config_filename, const char *key, const char *value, - const char *value_regex, unsigned flags) + const char *value_pattern, unsigned flags) { if (!git_config_set_multivar_in_file_gently(config_filename, key, value, - value_regex, flags)) + value_pattern, flags)) return; if (value) die(_("could not set '%s' to '%s'"), key, value); @@ -3009,16 +3009,16 @@ void git_config_set_multivar_in_file(const char *config_filename, } int git_config_set_multivar_gently(const char *key, const char *value, - const char *value_regex, unsigned flags) + const char *value_pattern, unsigned flags) { - return git_config_set_multivar_in_file_gently(NULL, key, value, value_regex, + return git_config_set_multivar_in_file_gently(NULL, key, value, value_pattern, flags); } void git_config_set_multivar(const char *key, const char *value, - const char *value_regex, unsigned flags) + const char *value_pattern, unsigned flags) { - git_config_set_multivar_in_file(NULL, key, value, value_regex, + git_config_set_multivar_in_file(NULL, key, value, value_pattern, flags); } diff --git a/config.h b/config.h index 84fdf223c8..7535b1f856 100644 --- a/config.h +++ b/config.h @@ -296,7 +296,7 @@ int git_config_set_multivar_in_file_gently(const char *, const char *, const cha void git_config_set_multivar_in_file(const char *config_filename, const char *key, const char *value, - const char *value_regex, + const char *value_pattern, unsigned flags); /** From 2076dba281a0e9b16dc670d59a87c62c97b90a74 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 25 Nov 2020 22:12:51 +0000 Subject: [PATCH 3/8] t1300: test "set all" mode with value-pattern Without additional modifiers, 'git config ' attempts to set a single value in the .git/config file. When the value-pattern parameter is supplied, this command behaves in a non-trivial manner. Consider 'git config '. The expected behavior is as follows: 1. If there are multiple existing values that match 'value-pattern', then the command fails. Users should use --replace-all instead. 2. If there is no existing values match 'value-pattern', then the 'key=value' pair is appended, making this 'key' a multi-valued config setting. 3. If there is one existing value that matches 'value-pattern', then the new config has one entry where 'key=value'. Add a test that demonstrates these options. Break from the existing pattern in t1300-config.sh to use 'git config --file=' instead of modifying .git/config directly to prevent possibly incompatible repo states. Also use 'git config --file= --list' for config state comparison instead of the config file format. This makes the tests more readable. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- t/t1300-config.sh | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 825d9a184f..a74bcb4c85 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1917,4 +1917,43 @@ test_expect_success '--replace-all does not invent newlines' ' test_cmp expect .git/config ' +test_expect_success 'set all config with value-pattern' ' + test_when_finished rm -f config initial && + git config --file=initial abc.key one && + + # no match => add new entry + cp initial config && + git config --file=config abc.key two a+ && + git config --file=config --list >actual && + cat >expect <<-\EOF && + abc.key=one + abc.key=two + EOF + test_cmp expect actual && + + # multiple matches => failure + test_must_fail git config --file=config abc.key three o+ 2>err && + test_i18ngrep "has multiple values" err && + + # multiple values, no match => add + git config --file=config abc.key three a+ && + git config --file=config --list >actual && + cat >expect <<-\EOF && + abc.key=one + abc.key=two + abc.key=three + EOF + test_cmp expect actual && + + # single match => replace + git config --file=config abc.key four h+ && + git config --file=config --list >actual && + cat >expect <<-\EOF && + abc.key=one + abc.key=two + abc.key=four + EOF + test_cmp expect actual +' + test_done From d15671943e763902c7a9c070a988f3b0489f11f7 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 25 Nov 2020 22:12:52 +0000 Subject: [PATCH 4/8] t1300: add test for --replace-all with value-pattern The --replace-all option was added in 4ddba79d (git-config-set: add more options) but was not tested along with the 'value-pattern' parameter. Since we will be updating this option to optionally treat 'value-pattern' as a fixed string, let's add a test here that documents the current behavior. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- t/t1300-config.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/t/t1300-config.sh b/t/t1300-config.sh index a74bcb4c85..bddf5250dc 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1956,4 +1956,18 @@ test_expect_success 'set all config with value-pattern' ' test_cmp expect actual ' +test_expect_success '--replace-all and value-pattern' ' + test_when_finished rm -f config && + git config --file=config --add abc.key one && + git config --file=config --add abc.key two && + git config --file=config --add abc.key three && + git config --file=config --replace-all abc.key four "o+" && + git config --file=config --list >actual && + cat >expect <<-\EOF && + abc.key=four + abc.key=three + EOF + test_cmp expect actual +' + test_done From fda43942d7b78d2c529a40e5e38fc34034d929cb Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 25 Nov 2020 22:12:53 +0000 Subject: [PATCH 5/8] config: add --fixed-value option, un-implemented The 'git config' builtin takes a 'value-pattern' parameter for several actions. This can cause confusion when expecting exact value matches instead of regex matches, especially when the input string contains metacharacters. 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 be filled in by later changes 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-pattern' 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 Signed-off-by: Junio C Hamano --- Documentation/git-config.txt | 20 +++++++++++++------- builtin/config.c | 36 ++++++++++++++++++++++++++++++++++++ t/t1300-config.sh | 24 ++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 7 deletions(-) diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index 0be5499952..09a1d273a9 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' [] [--type=] [--show-origin] [--show-scope] [-z|--null] name [value [value-pattern]] +'git config' [] [--type=] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] name [value [value-pattern]] 'git config' [] [--type=] --add name value -'git config' [] [--type=] --replace-all name value [value-pattern] -'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] --get name [value-pattern] -'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] --get-all name [value-pattern] -'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--name-only] --get-regexp name_regex [value-pattern] +'git config' [] [--type=] [--fixed-value] --replace-all name value [value-pattern] +'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get name [value-pattern] +'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all name [value-pattern] +'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp name_regex [value-pattern] 'git config' [] [--type=] [-z|--null] --get-urlmatch name URL -'git config' [] --unset name [value-pattern] -'git config' [] --unset-all name [value-pattern] +'git config' [] [--fixed-value] --unset name [value-pattern] +'git config' [] [--fixed-value] --unset-all name [value-pattern] 'git config' [] --rename-section old_name new_name 'git config' [] --remove-section name 'git config' [] [--show-origin] [--show-scope] [-z|--null] [--name-only] -l | --list @@ -165,6 +165,12 @@ See also <>. --list:: List all variables set in config file, along with their values. +--fixed-value:: + When used with the `value-pattern` argument, treat `value-pattern` 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-pattern`. + --type :: 'git config' will ensure that any input or output is valid under the given type constraint(s), and will canonicalize outgoing values in ``'s diff --git a/builtin/config.c b/builtin/config.c index 590392c811..f1b73ac8b5 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) @@ -144,6 +145,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 comparing values to 'value-pattern'")), 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), @@ -766,6 +768,40 @@ int cmd_config(int argc, const char **argv, const char *prefix) usage_builtin_config(); } + /* check usage of --fixed-value */ + if (fixed_value) { + int allowed_usage = 0; + + switch (actions) { + /* git config --get */ + case ACTION_GET: + /* git config --get-all */ + case ACTION_GET_ALL: + /* git config --get-regexp */ + case ACTION_GET_REGEXP: + /* git config --unset */ + case ACTION_UNSET: + /* git config --unset-all */ + case ACTION_UNSET_ALL: + allowed_usage = argc > 1 && !!argv[1]; + break; + + /* git config */ + case ACTION_SET_ALL: + /* git config --replace-all */ + case ACTION_REPLACE_ALL: + allowed_usage = argc > 2 && !!argv[2]; + break; + + /* other options don't allow --fixed-value */ + } + + if (!allowed_usage) { + error(_("--fixed-value only applies with 'value-pattern'")); + 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 bddf5250dc..841ed204d6 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1970,4 +1970,28 @@ test_expect_success '--replace-all and value-pattern' ' test_cmp expect actual ' +test_expect_success 'refuse --fixed-value for incompatible actions' ' + test_when_finished rm -f config && + 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-pattern + 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 From c90702a1f6f7473a959994a2dff0f0dd450b8029 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 25 Nov 2020 22:12:54 +0000 Subject: [PATCH 6/8] config: plumb --fixed-value into config API 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_pattern 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 Signed-off-by: Junio C Hamano --- builtin/config.c | 16 ++++++++++----- config.c | 5 +++++ config.h | 7 +++++++ t/t1300-config.sh | 50 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 5 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index f1b73ac8b5..21892a784c 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -633,6 +633,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)); @@ -800,6 +801,8 @@ int cmd_config(int argc, const char **argv, const char *prefix) error(_("--fixed-value only applies with 'value-pattern'")); usage_builtin_config(); } + + flags |= CONFIG_FLAGS_FIXED_VALUE; } if (actions & PAGING_ACTIONS) @@ -863,7 +866,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(); @@ -872,7 +876,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(); @@ -881,7 +886,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); @@ -908,7 +913,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); @@ -918,7 +924,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 5b2f00f073..3b2edc0faf 100644 --- a/config.c +++ b/config.c @@ -2415,6 +2415,7 @@ struct config_store_data { size_t baselen; char *key; int do_not_match; + const char *fixed_value; regex_t *value_pattern; int multi_replace; struct { @@ -2444,6 +2445,8 @@ static int matches(const char *key, const char *value, { if (strcmp(key, store->key)) return 0; /* not ours */ + if (store->fixed_value) + return !strcmp(store->fixed_value, value); if (!store->value_pattern) return 1; /* always matches */ if (store->value_pattern == CONFIG_REGEX_NONE) @@ -2816,6 +2819,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, store.value_pattern = NULL; else if (value_pattern == CONFIG_REGEX_NONE) store.value_pattern = CONFIG_REGEX_NONE; + else if (flags & CONFIG_FLAGS_FIXED_VALUE) + store.fixed_value = value_pattern; else { if (value_pattern[0] == '!') { store.do_not_match = 1; diff --git a/config.h b/config.h index 7535b1f856..c1449bb790 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_pattern + * 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 841ed204d6..4293ba22af 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1994,4 +1994,54 @@ 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' ' + test_when_finished rm -f config initial && + META="a+b*c?d[e]f.g" && + git config --file=initial fixed.test "$META" && + + cp initial config && + git config --file=config fixed.test bogus "$META" && + git config --file=config --list >actual && + cat >expect <<-EOF && + fixed.test=$META + fixed.test=bogus + EOF + test_cmp expect actual && + + cp initial config && + git config --file=config --fixed-value fixed.test bogus "$META" && + git config --file=config --list >actual && + cat >expect <<-\EOF && + fixed.test=bogus + EOF + test_cmp expect actual && + + cp initial config && + test_must_fail git config --file=config --unset fixed.test "$META" && + git config --file=config --fixed-value --unset fixed.test "$META" && + test_must_fail git config --file=config fixed.test && + + cp initial config && + test_must_fail git config --file=config --unset-all fixed.test "$META" && + git config --file=config --fixed-value --unset-all fixed.test "$META" && + test_must_fail git config --file=config fixed.test && + + cp initial config && + git config --file=config --replace-all fixed.test bogus "$META" && + git config --file=config --list >actual && + cat >expect <<-EOF && + fixed.test=$META + fixed.test=bogus + EOF + test_cmp expect actual && + + git config --file=config --fixed-value --replace-all fixed.test bogus "$META" && + git config --file=config --list >actual && + cat >expect <<-EOF && + fixed.test=bogus + fixed.test=bogus + EOF + test_cmp expect actual +' + test_done From 3f1bae1dc3432acf7c586cd938e524f8d30eed31 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 25 Nov 2020 22:12:55 +0000 Subject: [PATCH 7/8] config: implement --fixed-value with --get* The config builtin does its own regex matching of values for the --get, --get-all, and --get-regexp modes. Plumb the existing 'flags' parameter to the get_value() method so we can initialize the value-pattern argument as a fixed string instead of a regex pattern. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- builtin/config.c | 15 ++++++++++----- t/t1300-config.sh | 22 ++++++++++++++++++++++ 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index 21892a784c..f71fa39b38 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -14,6 +14,7 @@ static const char *const builtin_config_usage[] = { static char *key; static regex_t *key_regexp; +static const char *value_pattern; static regex_t *regexp; static int show_keys; static int omit_values; @@ -298,6 +299,8 @@ static int collect_config(const char *key_, const char *value_, void *cb) return 0; if (use_key_regexp && regexec(key_regexp, key_, 0, NULL, 0)) return 0; + if (fixed_value && strcmp(value_pattern, (value_?value_:""))) + return 0; if (regexp != NULL && (do_not_match ^ !!regexec(regexp, (value_?value_:""), 0, NULL, 0))) return 0; @@ -308,7 +311,7 @@ static int collect_config(const char *key_, const char *value_, void *cb) return format_config(&values->items[values->nr++], key_, value_); } -static int get_value(const char *key_, const char *regex_) +static int get_value(const char *key_, const char *regex_, unsigned flags) { int ret = CONFIG_GENERIC_ERROR; struct strbuf_list values = {NULL}; @@ -345,7 +348,9 @@ static int get_value(const char *key_, const char *regex_) } } - if (regex_) { + if (regex_ && (flags & CONFIG_FLAGS_FIXED_VALUE)) + value_pattern = regex_; + else if (regex_) { if (regex_[0] == '!') { do_not_match = 1; regex_++; @@ -890,19 +895,19 @@ int cmd_config(int argc, const char **argv, const char *prefix) } else if (actions == ACTION_GET) { check_argc(argc, 1, 2); - return get_value(argv[0], argv[1]); + return get_value(argv[0], argv[1], flags); } else if (actions == ACTION_GET_ALL) { do_all = 1; check_argc(argc, 1, 2); - return get_value(argv[0], argv[1]); + return get_value(argv[0], argv[1], flags); } else if (actions == ACTION_GET_REGEXP) { show_keys = 1; use_key_regexp = 1; do_all = 1; check_argc(argc, 1, 2); - return get_value(argv[0], argv[1]); + return get_value(argv[0], argv[1], flags); } else if (actions == ACTION_GET_URLMATCH) { check_argc(argc, 2, 2); diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 4293ba22af..97a04c6cc2 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -2044,4 +2044,26 @@ test_expect_success '--fixed-value uses exact string matching' ' test_cmp expect actual ' +test_expect_success '--get and --get-all with --fixed-value' ' + test_when_finished rm -f config && + META="a+b*c?d[e]f.g" && + git config --file=config fixed.test bogus && + git config --file=config --add fixed.test "$META" && + + git config --file=config --get fixed.test bogus && + test_must_fail git config --file=config --get fixed.test "$META" && + git config --file=config --get --fixed-value fixed.test "$META" && + test_must_fail git config --file=config --get --fixed-value fixed.test non-existent && + + git config --file=config --get-all fixed.test bogus && + test_must_fail git config --file=config --get-all fixed.test "$META" && + git config --file=config --get-all --fixed-value fixed.test "$META" && + test_must_fail git config --file=config --get-all --fixed-value fixed.test non-existent && + + git config --file=config --get-regexp fixed+ bogus && + test_must_fail git config --file=config --get-regexp fixed+ "$META" && + git config --file=config --get-regexp --fixed-value fixed+ "$META" && + test_must_fail git config --file=config --get-regexp --fixed-value fixed+ non-existent +' + test_done From c902618795eee5dbc88ded238d0a359091ab0dee Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Wed, 25 Nov 2020 15:01:31 -0800 Subject: [PATCH 8/8] config doc: value-pattern is not necessarily a regexp The introductory part of the "git config --help" mentions the optional value-pattern argument, but give no hint that it can be something other than a regular expression (worse, it just says "POSIX regexp", which usually means BRE but the regexp the command takes is ERE). Also, it needs to be documented that the '!' prefix to negate the match, which is only mentioned in this part of the document, works only with regexp and not with the --fixed-value. Signed-off-by: Junio C Hamano --- Documentation/git-config.txt | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index 09a1d273a9..0e9351d3cb 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -33,10 +33,13 @@ escaped. Multiple lines can be added to an option by using the `--add` option. If you want to update or unset an option which can occur on multiple -lines, a POSIX regexp `value-pattern` needs to be given. Only the -existing values that match the regexp are updated or unset. If -you want to handle the lines that do *not* match the regex, just -prepend a single exclamation mark in front (see also <>). +lines, a `value-pattern` (which is an extended regular expression, +unless the `--fixed-value` option is given) needs to be given. Only the +existing values that match the pattern are updated or unset. If +you want to handle the lines that do *not* match the pattern, just +prepend a single exclamation mark in front (see also <>), +but note that this only works when the `--fixed-value` option is not +in use. The `--type=` option instructs 'git config' to ensure that incoming and outgoing values are canonicalize-able under the given . If no