Re: [PATCH] make config --add behave correctly for empty and NULL values
From: Jeff King <hidden>
Date: 2016-06-15 23:02:17
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2016-06-15 · Re: [PATCH] make config --add behave correctly for empty and NULL values · Junio C Hamano <hidden>
- 2016-06-15 · Re: [PATCH] make config --add behave correctly for empty and NULL values · Jeff King <hidden>
- 2016-06-15 · [PATCH] make config --add behave correctly for empty and NULL values · Tanay Abhra <hidden>
- 2016-06-15 · Re: [PATCH] make config --add behave correctly for empty and NULL values · Matthieu Moy <hidden>
- 2016-06-15 · Re: [PATCH] make config --add behave correctly for empty and NULL values · Junio C Hamano <hidden>
On Mon, Aug 18, 2014 at 11:03:51PM -0700, Junio C Hamano wrote:
We already have some code paths that use ((void *)1) as a special pointer value, so in that sense I would say it is not the end of the world if you added a new one.
No, but if you use it to replace the regexp, you end up having to check for it in the code paths that regfree() the result. I think a separate bit is nicer for that reason.
At the end-user level (i.e. people who write callers to set-multivar-in-file function), I actually like your idea of inventing our own string syntax and parse it at the place where we strip '!' out and remember that the pattern's match status needs to be negated.
I thought at first that "!" by itself might be fine, but that is actually a valid regex. And we may feed user input directly to the function, so we have to be careful not to get too clever.
For example, instead of "a^" (to which I cannot say with confidence that no implementation would match the not-at-the-beginning caret literally), I would not mind if we taught set-multivar-in-file that we use "!*" as a mark to tell "this pattern never matches",
That would work, but I think CONFIG_REGEX_NONE or similar is a bit less cryptic, and not any harder to implement. Here is the patch I wrote, for reference (I also think breaking the "matches" function into a series of conditionals, as you showed, is way more readable):
diff --git a/builtin/config.c b/builtin/config.c
index b9e7dce..7bba516 100644
--- a/builtin/config.c
+++ b/builtin/config.c@@ -586,7 +586,8 @@ int cmd_config(int argc, const char **argv, const char *prefix) check_argc(argc, 2, 2); value = normalize_value(argv[0], argv[1]); return git_config_set_multivar_in_file(given_config_source.file, - argv[0], value, "a^", 0); + argv[0], value, + CONFIG_REGEX_NONE, 0); } else if (actions == ACTION_REPLACE_ALL) { check_write();
diff --git a/cache.h b/cache.h
index fcb511d..dcf3a2a 100644
--- a/cache.h
+++ b/cache.h@@ -1281,6 +1281,8 @@ extern int update_server_info(int); #define CONFIG_INVALID_PATTERN 6 #define CONFIG_GENERIC_ERROR 7 +#define CONFIG_REGEX_NONE ((void *)1) + struct git_config_source { unsigned int use_stdin:1; const char *file;
diff --git a/config.c b/config.c
index 67a7729..1199faf 100644
--- a/config.c
+++ b/config.c@@ -1229,6 +1229,7 @@ static struct { static int matches(const char *key, const char *value) { return !strcmp(key, store.key) && + store.value_regex != CONFIG_REGEX_NONE && (store.value_regex == NULL || (store.do_not_match ^ (value && !regexec(store.value_regex, value, 0, NULL, 0))));
@@ -1493,6 +1494,8 @@ out_free_ret_1: /* * 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 + * (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.
@@ -1576,6 +1579,8 @@ int git_config_set_multivar_in_file(const char *config_filename, if (value_regex == NULL) store.value_regex = NULL; + else if (value_regex == CONFIG_REGEX_NONE) + store.value_regex = CONFIG_REGEX_NONE; else { if (value_regex[0] == '!') { store.do_not_match = 1;
@@ -1607,7 +1612,8 @@ int git_config_set_multivar_in_file(const char *config_filename, if (git_config_from_file(store_aux, config_filename, NULL)) { error("invalid config file %s", config_filename); free(store.key); - if (store.value_regex != NULL) { + if (store.value_regex != NULL && + store.value_regex != CONFIG_REGEX_NONE) { regfree(store.value_regex); free(store.value_regex); }
@@ -1616,7 +1622,8 @@ int git_config_set_multivar_in_file(const char *config_filename, } free(store.key); - if (store.value_regex != NULL) { + if (store.value_regex != NULL && + store.value_regex != CONFIG_REGEX_NONE) { regfree(store.value_regex); free(store.value_regex); }