{"thread":{"id":"37369","subject":"[PATCH] make config --add behave correctly for empty and NULL values","startedAt":"2014-08-18T10:17:57Z","lastAt":"2014-09-12T17:29:25Z","messageCount":12,"participants":["Tanay Abhra","Matthieu Moy","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"247833","messageId":"1408357077-4745-1-git-send-email-tanayabh@gmail.com","threadId":"37369","inReplyTo":null,"subject":"[PATCH] make config --add behave correctly for empty and NULL values","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-18T10:17:57Z","receivedAt":"2014-08-18T10:17:57Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Currently if we have a config file like,\n[foo]\n        baz\n        bar =\n\nand we try something like, \"git config --add foo.baz roll\", Git will\nsegfault. Moreover, for \"git config --add foo.bar roll\", it will\noverwrite the original value instead of appending after the existing\nempty value.\n\nThe problem lies with the regexp used for simulating --add in\n`git_config_set_multivar_in_file()`, \"^$\", which in ideal case should\nnot match with any string but is true for empty strings. Instead use a\nregexp like \"a^\" which can not be true for any string, empty or not.\n\nFor removing the segfault add a check for NULL values in `matches()` in\nconfig.c.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n builtin/config.c        |  2 +-\n config.c                |  2 +-\n t/t1303-wacky-config.sh | 20 ++++++++++++++++++++\n 3 files changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex fcd8474..b9e7dce 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -586,7 +586,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_argc(argc, 2, 2);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n \t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t       argv[0], value, \"^$\", 0);\n+\t\t\t\t\t\t       argv[0], value, \"a^\", 0);\n \t}\n \telse if (actions == ACTION_REPLACE_ALL) {\n \t\tcheck_write();\ndiff --git a/config.c b/config.c\nindex 058505c..67a7729 100644\n--- a/config.c\n+++ b/config.c\n@@ -1231,7 +1231,7 @@ static int matches(const char *key, const char *value)\n \treturn !strcmp(key, store.key) &&\n \t\t(store.value_regex == NULL ||\n \t\t (store.do_not_match ^\n-\t\t  !regexec(store.value_regex, value, 0, NULL, 0)));\n+\t\t  (value && !regexec(store.value_regex, value, 0, NULL, 0))));\n }\n \n static int store_aux(const char *key, const char *value, void *cb)\ndiff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh\nindex 3a2c819..3b92083 100755\n--- a/t/t1303-wacky-config.sh\n+++ b/t/t1303-wacky-config.sh\n@@ -111,4 +111,24 @@ test_expect_success 'unset many entries' '\n \ttest_must_fail git config section.key\n '\n \n+test_expect_success '--add appends new value after existing empty value' '\n+\tcat >expect <<-\\EOF &&\n+\n+\n+\tfool\n+\troll\n+\tEOF\n+\tcp .git/config .git/config.old &&\n+\ttest_when_finished \"mv .git/config.old .git/config\" &&\n+\tcat >.git/config <<-\\EOF &&\n+\t[foo]\n+\t\tbaz\n+\t\tbaz =\n+\t\tbaz = fool\n+\tEOF\n+\tgit config --add foo.baz roll &&\n+\tgit config --get-all foo.baz >output &&\n+\ttest_cmp expect output\n+'\n+\n test_done\n-- \n1.9.0.GIT\n"},{"id":"247836","messageId":"vpq61hqc6z4.fsf@anie.imag.fr","threadId":"37369","inReplyTo":"1408357077-4745-1-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH] make config --add behave correctly for empty and NULL values","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-08-18T12:33:51Z","receivedAt":"2014-08-18T12:33:51Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra <tanayabh@gmail.com> writes:\n\n> Currently if we have a config file like,\n> [foo]\n>         baz\n>         bar =\n>\n> and we try something like, \"git config --add foo.baz roll\", Git will\n> segfault. Moreover, for \"git config --add foo.bar roll\", it will\n> overwrite the original value instead of appending after the existing\n> empty value.\n>\n> The problem lies with the regexp used for simulating --add in\n> `git_config_set_multivar_in_file()`, \"^$\", which in ideal case should\n> not match with any string but is true for empty strings. Instead use a\n> regexp like \"a^\" which can not be true for any string, empty or not.\n>\n> For removing the segfault add a check for NULL values in `matches()` in\n> config.c.\n\nI would have prefered two separate patches (or even better, 3, the first\none being \"demonstrate failure of ...\" with test_expect_failure) for\neach issues.\n\nBut the fixes are straightforward, and the test actually test what it\nhas to test, so I think we can keep the patch as-is.\n\nThanks,\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247855","messageId":"xmqqvbppwtir.fsf@gitster.dls.corp.google.com","threadId":"37369","inReplyTo":"1408357077-4745-1-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH] make config --add behave correctly for empty and NULL values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-18T18:18:52Z","receivedAt":"2014-08-18T18:18:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tanay Abhra <tanayabh@gmail.com> writes:\n\n> Currently if we have a config file like,\n> [foo]\n>         baz\n>         bar =\n>\n> and we try something like, \"git config --add foo.baz roll\", Git will\n> segfault.\n\nThanks; this is a good find.\n\nThis is a tangent, but people please stop starting their sentence\nwith a somewhat irritating \"Currently\"; it does not help both\ncurrent and future readers very much without some mention of version\nnumbers.\n\nI suspect this bug dates back to pretty much day one of \"git config\"\n(dates at least back to 1.5.3).\n\n> The problem lies with the regexp used for simulating --add in\n> `git_config_set_multivar_in_file()`, \"^$\", which in ideal case should\n> not match with any string but is true for empty strings. Instead use a\n> regexp like \"a^\" which can not be true for any string, empty or not.\n\nYuck, but we cannot pass NULL or some other special value that look\nmore meaningful to signal the fact that we do not want to match\nanything, so this seems to be the easiest way out.  \n\nAre we sure that \"a^\", which cannot be true for any string, will not\nbe caught by anybody's regcomp() as an error?  I know regcomp()\naccepts the expression and regexec() fails to match with GNU libc,\nbut that is not the whole of the world.\n\nAt least, please make it clear for those who read this code later\nwhat is going on with this magic \"a^\", perhaps with\n\n\t#define REGEXP_THAT_NEVER_MATCHES \"a^\"\n\t...\n        return git_config_set_multivar_in_file(given_config_source.file,\n                                              argv[0], value,\n                                              REGEXP_THAT_NEVER_MATCHES, 0);\nand/or with in-code comment.\n\n\t/*\n         * set_multivar_in_file() removes existing values that match\n         * the value_regexp argument and then adds this new value;\n         * pass a pattern that never matches anything, as we do not\n         * want to remove any existing value.\n         */\n\treturn git_config_set_multivar_in_file(given_config_source.file,\n                                              argv[0], value,\n                                              REGEXP_THAT_NEVER_MATCHES, 0);\n\nTo be honest, I'd rather see this done \"right\", by giving an option\nto the caller to tell the function not to call regcomp/regexec in\nmatches().\n\n * Define a global exported via cache.h and defined in config.c\n\n\textern const char CONFIG_SET_MULTIVAR_NO_REPLACE[];\n\n   and pass it from this calling site, instead of an arbitrary\n   literal string e.g. \"a^\"\n\n * Add a bit to the \"store\" struct, e.g. \"unsigned value_never_matches:1\";\n\n * In git_config_set_multivar_in_file() implementation, check for\n   this constant address and set store.value_never_matches to true;\n\n * in matches(), check this bit and always return \"No, this existing\n   value do not match\" when it is set.\n\nor something like that.\n\n> For removing the segfault add a check for NULL values in `matches()` in\n> config.c.\n\nThe fact that you do a check is important, but it equally if not\nmore important what you do with the result.  \"Check for a NULL and\nconsider it as not matching\" is probably what you meant, but I'd\nlike to double check.\n\n> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>\n> ---\n>  builtin/config.c        |  2 +-\n>  config.c                |  2 +-\n>  t/t1303-wacky-config.sh | 20 ++++++++++++++++++++\n>  3 files changed, 22 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/config.c b/builtin/config.c\n> index fcd8474..b9e7dce 100644\n> --- a/builtin/config.c\n> +++ b/builtin/config.c\n> @@ -586,7 +586,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n>  \t\tcheck_argc(argc, 2, 2);\n>  \t\tvalue = normalize_value(argv[0], argv[1]);\n>  \t\treturn git_config_set_multivar_in_file(given_config_source.file,\n> -\t\t\t\t\t\t       argv[0], value, \"^$\", 0);\n> +\t\t\t\t\t\t       argv[0], value, \"a^\", 0);\n>  \t}\n>  \telse if (actions == ACTION_REPLACE_ALL) {\n>  \t\tcheck_write();\n> diff --git a/config.c b/config.c\n> index 058505c..67a7729 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1231,7 +1231,7 @@ static int matches(const char *key, const char *value)\n>  \treturn !strcmp(key, store.key) &&\n>  \t\t(store.value_regex == NULL ||\n>  \t\t (store.do_not_match ^\n> -\t\t  !regexec(store.value_regex, value, 0, NULL, 0)));\n> +\t\t  (value && !regexec(store.value_regex, value, 0, NULL, 0))));\n>  }\n>  \n>  static int store_aux(const char *key, const char *value, void *cb)\n> diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh\n> index 3a2c819..3b92083 100755\n> --- a/t/t1303-wacky-config.sh\n> +++ b/t/t1303-wacky-config.sh\n> @@ -111,4 +111,24 @@ test_expect_success 'unset many entries' '\n>  \ttest_must_fail git config section.key\n>  '\n>  \n> +test_expect_success '--add appends new value after existing empty value' '\n> +\tcat >expect <<-\\EOF &&\n> +\n> +\n> +\tfool\n> +\troll\n> +\tEOF\n> +\tcp .git/config .git/config.old &&\n> +\ttest_when_finished \"mv .git/config.old .git/config\" &&\n> +\tcat >.git/config <<-\\EOF &&\n> +\t[foo]\n> +\t\tbaz\n> +\t\tbaz =\n> +\t\tbaz = fool\n> +\tEOF\n> +\tgit config --add foo.baz roll &&\n> +\tgit config --get-all foo.baz >output &&\n> +\ttest_cmp expect output\n> +'\n> +\n>  test_done\n"},{"id":"247890","messageId":"20140819051732.GA13765@peff.net","threadId":"37369","inReplyTo":"xmqqvbppwtir.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] make config --add behave correctly for empty and NULL values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-19T05:17:32Z","receivedAt":"2014-08-19T05:17:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 18, 2014 at 11:18:52AM -0700, Junio C Hamano wrote:\n\n> Are we sure that \"a^\", which cannot be true for any string, will not\n> be caught by anybody's regcomp() as an error?  I know regcomp()\n> accepts the expression and regexec() fails to match with GNU libc,\n> but that is not the whole of the world.\n\nWe do support negation (ourselves) in the regexp, so \"!$foo\" would work,\nwhere \"$foo\" is some regexp that always matches. But that may be digging\nourselves the opposite hole, trying to find a pattern that reliably\nmatches everything.\n\n> To be honest, I'd rather see this done \"right\", by giving an option\n> to the caller to tell the function not to call regcomp/regexec in\n> matches().\n\nYeah, that was my first thought, too on seeing the patch. I even worked\nup an example before reading your message, but:\n\n>  * Define a global exported via cache.h and defined in config.c\n> \n> \textern const char CONFIG_SET_MULTIVAR_NO_REPLACE[];\n> \n>    and pass it from this calling site, instead of an arbitrary\n>    literal string e.g. \"a^\"\n> \n>  * Add a bit to the \"store\" struct, e.g. \"unsigned value_never_matches:1\";\n> \n>  * In git_config_set_multivar_in_file() implementation, check for\n>    this constant address and set store.value_never_matches to true;\n> \n>  * in matches(), check this bit and always return \"No, this existing\n>    value do not match\" when it is set.\n\nI just used\n\n  #define CONFIG_REGEX_NONE ((void *)1)\n\nas my magic sentinel value, both for the string and compiled regex\nversions. Adding a bit to the store struct is a lot less disgusting and\nerror-prone. So I won't share mine here. :)\n\n-Peff\n"},{"id":"247891","messageId":"xmqqmwb1vwvs.fsf@gitster.dls.corp.google.com","threadId":"37369","inReplyTo":"20140819051732.GA13765@peff.net","subject":"Re: [PATCH] make config --add behave correctly for empty and NULL values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-19T06:03:51Z","receivedAt":"2014-08-19T06:03:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I just used\n>\n>   #define CONFIG_REGEX_NONE ((void *)1)\n>\n> as my magic sentinel value, both for the string and compiled regex\n> versions. Adding a bit to the store struct is a lot less disgusting and\n> error-prone. So I won't share mine here. :)\n\nActually, I wrote something like that aloud but did not type it out\n;-).  Great minds think alike.\n\nWe already have some code paths that use ((void *)1) as a special\npointer value, so in that sense I would say it is not the end of the\nworld if you added a new one.  At the end-user level (i.e. people\nwho write callers to set-multivar-in-file function), I actually like\nyour idea of inventing our own string syntax and parse it at the\nplace where we strip '!' out and remember that the pattern's match\nstatus needs to be negated.  For example, instead of \"a^\" (to which\nI cannot say with confidence that no implementation would match the\nnot-at-the-beginning caret literally), I would not mind if we taught\nset-multivar-in-file that we use \"!*\" as a mark to tell \"this\npattern never matches\", and have it assign your \"never matches\"\nmark, i.e. (void *)1, to store.value_regex.  Then matches() would\nbecome\n\n\tstatic int matches(const char *key, const char *value)\n        {\n        \tif (strcmp(key, store.key))\n                \treturn 0; /* not ours */\n\t\tif (!store.value_regex)\n                \treturn 1; /* always matches */\n\t\tif (store.value_regex == CONFIG_REGEX_NONE)\n                \treturn 0; /* never matches */\n\t\treturn store.do_not_match ^\n                \t!regexec(store.value_regex, value, 0, NULL, 0);\n\t}\n\nor something like that, and the ugly \"magic\" will be localized,\nwhich may make it more palatable.\n"},{"id":"247894","messageId":"20140819062000.GA7805@peff.net","threadId":"37369","inReplyTo":"xmqqmwb1vwvs.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] make config --add behave correctly for empty and NULL values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-19T06:20:00Z","receivedAt":"2014-08-19T06:20:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 18, 2014 at 11:03:51PM -0700, Junio C Hamano wrote:\n\n> We already have some code paths that use ((void *)1) as a special\n> pointer value, so in that sense I would say it is not the end of the\n> world if you added a new one.\n\nNo, but if you use it to replace the regexp, you end up having to check\nfor it in the code paths that regfree() the result. I think a separate\nbit is nicer for that reason.\n\n> At the end-user level (i.e. people who write callers to\n> set-multivar-in-file function), I actually like your idea of inventing\n> our own string syntax and parse it at the place where we strip '!' out\n> and remember that the pattern's match status needs to be negated.\n\nI thought at first that \"!\" by itself might be fine, but that is\nactually a valid regex. And we may feed user input directly to the\nfunction, so we have to be careful not to get too clever.\n\n> For example, instead of \"a^\" (to which\n> I cannot say with confidence that no implementation would match the\n> not-at-the-beginning caret literally), I would not mind if we taught\n> set-multivar-in-file that we use \"!*\" as a mark to tell \"this\n> pattern never matches\",\n\nThat would work, but I think CONFIG_REGEX_NONE or similar is a bit less\ncryptic, and not any harder to implement.\n\nHere is the patch I wrote, for reference (I also think breaking the\n\"matches\" function into a series of conditionals, as you showed, is way\nmore readable):\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex b9e7dce..7bba516 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -586,7 +586,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_argc(argc, 2, 2);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n \t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t       argv[0], value, \"a^\", 0);\n+\t\t\t\t\t\t       argv[0], value,\n+\t\t\t\t\t\t       CONFIG_REGEX_NONE, 0);\n \t}\n \telse if (actions == ACTION_REPLACE_ALL) {\n \t\tcheck_write();\ndiff --git a/cache.h b/cache.h\nindex fcb511d..dcf3a2a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1281,6 +1281,8 @@ extern int update_server_info(int);\n #define CONFIG_INVALID_PATTERN 6\n #define CONFIG_GENERIC_ERROR 7\n \n+#define CONFIG_REGEX_NONE ((void *)1)\n+\n struct git_config_source {\n \tunsigned int use_stdin:1;\n \tconst char *file;\ndiff --git a/config.c b/config.c\nindex 67a7729..1199faf 100644\n--- a/config.c\n+++ b/config.c\n@@ -1229,6 +1229,7 @@ static struct {\n static int matches(const char *key, const char *value)\n {\n \treturn !strcmp(key, store.key) &&\n+\t\tstore.value_regex != CONFIG_REGEX_NONE &&\n \t\t(store.value_regex == NULL ||\n \t\t (store.do_not_match ^\n \t\t  (value && !regexec(store.value_regex, value, 0, NULL, 0))));\n@@ -1493,6 +1494,8 @@ out_free_ret_1:\n /*\n  * If value==NULL, unset in (remove from) config,\n  * if value_regex!=NULL, disregard key/value pairs where value does not match.\n+ * if value_regex==CONFIG_REGEX_NONE, do not match any existing values\n+ *     (only add a new one)\n  * if multi_replace==0, nothing, or only one matching key/value is replaced,\n  *     else all matching key/values (regardless how many) are removed,\n  *     before the new pair is written.\n@@ -1576,6 +1579,8 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \n \t\tif (value_regex == NULL)\n \t\t\tstore.value_regex = NULL;\n+\t\telse if (value_regex == CONFIG_REGEX_NONE)\n+\t\t\tstore.value_regex = CONFIG_REGEX_NONE;\n \t\telse {\n \t\t\tif (value_regex[0] == '!') {\n \t\t\t\tstore.do_not_match = 1;\n@@ -1607,7 +1612,8 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \t\tif (git_config_from_file(store_aux, config_filename, NULL)) {\n \t\t\terror(\"invalid config file %s\", config_filename);\n \t\t\tfree(store.key);\n-\t\t\tif (store.value_regex != NULL) {\n+\t\t\tif (store.value_regex != NULL &&\n+\t\t\t    store.value_regex != CONFIG_REGEX_NONE) {\n \t\t\t\tregfree(store.value_regex);\n \t\t\t\tfree(store.value_regex);\n \t\t\t}\n@@ -1616,7 +1622,8 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \t\t}\n \n \t\tfree(store.key);\n-\t\tif (store.value_regex != NULL) {\n+\t\tif (store.value_regex != NULL &&\n+\t\t    store.value_regex != CONFIG_REGEX_NONE) {\n \t\t\tregfree(store.value_regex);\n \t\t\tfree(store.value_regex);\n \t\t}\n"},{"id":"249271","messageId":"xmqqy4tpbuii.fsf@gitster.dls.corp.google.com","threadId":"37369","inReplyTo":"20140819062000.GA7805@peff.net","subject":"Re: [PATCH] make config --add behave correctly for empty and NULL values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-11T23:35:33Z","receivedAt":"2014-09-11T23:35:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Here is the patch I wrote, for reference (I also think breaking the\n> \"matches\" function into a series of conditionals, as you showed, is way\n> more readable):\n\nOK, while reviewing the today's issue of \"What's cooking\" and making\ntopics graduate to 'master', I got annoyed that the bottom of jch\nbranch still needed to be kept.  Let's do this.\n\n-- >8 --\nFrom: Jeff King <peff@peff.net>\nDate: Tue, 19 Aug 2014 02:20:00 -0400\nSubject: [PATCH] config: avoid a funny sentinel value \"a^\"\n\nIntroduce CONFIG_REGEX_NONE as a more explicit sentinel value to say\n\"we do not want to replace any existing entry\" and use it in the\nimplementation of \"git config --add\".\n\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/config.c |  3 ++-\n cache.h          |  2 ++\n config.c         | 23 +++++++++++++++++------\n 3 files changed, 21 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 8224699..bf1aa6b 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -599,7 +599,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_argc(argc, 2, 2);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n \t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t       argv[0], value, \"a^\", 0);\n+\t\t\t\t\t\t       argv[0], value,\n+\t\t\t\t\t\t       CONFIG_REGEX_NONE, 0);\n \t}\n \telse if (actions == ACTION_REPLACE_ALL) {\n \t\tcheck_write();\ndiff --git a/cache.h b/cache.h\nindex c708062..8356168 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1233,6 +1233,8 @@ extern int update_server_info(int);\n #define CONFIG_INVALID_PATTERN 6\n #define CONFIG_GENERIC_ERROR 7\n \n+#define CONFIG_REGEX_NONE ((void *)1)\n+\n struct git_config_source {\n \tunsigned int use_stdin:1;\n \tconst char *file;\ndiff --git a/config.c b/config.c\nindex ffe0104..2e709bf 100644\n--- a/config.c\n+++ b/config.c\n@@ -1230,10 +1230,15 @@ static struct {\n \n static int matches(const char *key, const char *value)\n {\n-\treturn !strcmp(key, store.key) &&\n-\t\t(store.value_regex == NULL ||\n-\t\t (store.do_not_match ^\n-\t\t  (value && !regexec(store.value_regex, value, 0, NULL, 0))));\n+\tif (strcmp(key, store.key))\n+\t\treturn 0; /* not ours */\n+\tif (!store.value_regex)\n+\t\treturn 1; /* always matches */\n+\tif (store.value_regex == CONFIG_REGEX_NONE)\n+\t\treturn 0; /* never matches */\n+\n+\treturn store.do_not_match ^\n+\t\t(value && !regexec(store.value_regex, value, 0, NULL, 0));\n }\n \n static int store_aux(const char *key, const char *value, void *cb)\n@@ -1495,6 +1500,8 @@ out_free_ret_1:\n /*\n  * If value==NULL, unset in (remove from) config,\n  * if value_regex!=NULL, disregard key/value pairs where value does not match.\n+ * if value_regex==CONFIG_REGEX_NONE, do not match any existing values\n+ *     (only add a new one)\n  * if multi_replace==0, nothing, or only one matching key/value is replaced,\n  *     else all matching key/values (regardless how many) are removed,\n  *     before the new pair is written.\n@@ -1578,6 +1585,8 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \n \t\tif (value_regex == NULL)\n \t\t\tstore.value_regex = NULL;\n+\t\telse if (value_regex == CONFIG_REGEX_NONE)\n+\t\t\tstore.value_regex = CONFIG_REGEX_NONE;\n \t\telse {\n \t\t\tif (value_regex[0] == '!') {\n \t\t\t\tstore.do_not_match = 1;\n@@ -1609,7 +1618,8 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \t\tif (git_config_from_file(store_aux, config_filename, NULL)) {\n \t\t\terror(\"invalid config file %s\", config_filename);\n \t\t\tfree(store.key);\n-\t\t\tif (store.value_regex != NULL) {\n+\t\t\tif (store.value_regex != NULL &&\n+\t\t\t    store.value_regex != CONFIG_REGEX_NONE) {\n \t\t\t\tregfree(store.value_regex);\n \t\t\t\tfree(store.value_regex);\n \t\t\t}\n@@ -1618,7 +1628,8 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \t\t}\n \n \t\tfree(store.key);\n-\t\tif (store.value_regex != NULL) {\n+\t\tif (store.value_regex != NULL &&\n+\t\t    store.value_regex != CONFIG_REGEX_NONE) {\n \t\t\tregfree(store.value_regex);\n \t\t\tfree(store.value_regex);\n \t\t}\n-- \n2.1.0-466-g6597b3e\n"},{"id":"249275","messageId":"20140912022945.GB15519@peff.net","threadId":"37369","inReplyTo":"xmqqy4tpbuii.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] make config --add behave correctly for empty and NULL values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-12T02:29:45Z","receivedAt":"2014-09-12T02:29:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 11, 2014 at 04:35:33PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Here is the patch I wrote, for reference (I also think breaking the\n> > \"matches\" function into a series of conditionals, as you showed, is way\n> > more readable):\n> \n> OK, while reviewing the today's issue of \"What's cooking\" and making\n> topics graduate to 'master', I got annoyed that the bottom of jch\n> branch still needed to be kept.  Let's do this.\n> \n> -- >8 --\n> From: Jeff King <peff@peff.net>\n> Date: Tue, 19 Aug 2014 02:20:00 -0400\n> Subject: [PATCH] config: avoid a funny sentinel value \"a^\"\n> \n> Introduce CONFIG_REGEX_NONE as a more explicit sentinel value to say\n> \"we do not want to replace any existing entry\" and use it in the\n> implementation of \"git config --add\".\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nLooks good, and adding my signoff is fine. Thanks.\n\n-Peff\n"},{"id":"249285","messageId":"54129F66.9080905@gmail.com","threadId":"37369","inReplyTo":"xmqqy4tpbuii.fsf@gitster.dls.corp.google.com","subject":"[PATCH v2 1/2] document irregular config --add behaviour for empty and NULL values","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-09-12T07:23:18Z","receivedAt":"2014-09-12T07:23:18Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"If we have a config file like,\n[foo]\n        baz\n        bar =\n\nand we try something like, \"git config --add foo.baz roll\", Git will\nsegfault. Moreover, for \"git config --add foo.bar roll\", it will\noverwrite the original value instead of appending after the existing\nempty value. Document these deficiencies in form of a test.\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n\nSorry for this very late reply. I was stuck in a flood affected region\nwith no internet connectivity for the past week. I am safe now. :)\n\nFWIW, here is the reroll with a set bit in the store struct and an exported\nglobal. I could have done the reroll as you have done, but Jeff had mentioned\nthat he liked the version with a bit flag more. But you can choose the version\nthat seems better to you.\n\n t/t1303-wacky-config.sh | 20 ++++++++++++++++++++\n 1 file changed, 20 insertions(+)\n\ndiff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh\nindex 3a2c819..e5c0f07 100755\n--- a/t/t1303-wacky-config.sh\n+++ b/t/t1303-wacky-config.sh\n@@ -111,4 +111,24 @@ test_expect_success 'unset many entries' '\n \ttest_must_fail git config section.key\n '\n\n+test_expect_failure '--add appends new value after existing empty value' '\n+\tcat >expect <<-\\EOF &&\n+\n+\n+\tfool\n+\troll\n+\tEOF\n+\tcp .git/config .git/config.old &&\n+\ttest_when_finished \"mv .git/config.old .git/config\" &&\n+\tcat >.git/config <<-\\EOF &&\n+\t[foo]\n+\t\tbaz\n+\t\tbaz =\n+\t\tbaz = fool\n+\tEOF\n+\tgit config --add foo.baz roll &&\n+\tgit config --get-all foo.baz >output &&\n+\ttest_cmp expect output\n+'\n+\n test_done\n-- \n1.9.0.GIT\n"},{"id":"249286","messageId":"54129FE1.6020303@gmail.com","threadId":"37369","inReplyTo":"54129F66.9080905@gmail.com","subject":"[PATCH v2 2/2] make config --add behave correctly for empty and NULL values","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-09-12T07:25:21Z","receivedAt":"2014-09-12T07:25:21Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"The problem lies with the regexp used for simulating --add in\n`git_config_set_multivar_in_file()`, \"^$\", which in ideal case should\nnot match with any string but is true for empty strings. Instead use a\nsentinel value CONFIG_REGEX_NONE to say \"we do not want to replace any\nexisting entry\" and use it in the implementation of \"git config --add\".\n\nFor removing the segfault add a check for NULL values in `matches()` in\nconfig.c.\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n builtin/config.c        |  2 +-\n cache.h                 |  2 ++\n config.c                | 21 +++++++++++++++++----\n t/t1303-wacky-config.sh |  2 +-\n 4 files changed, 21 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex aba7135..195664b 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -611,7 +611,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_argc(argc, 2, 2);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n \t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t       argv[0], value, \"^$\", 0);\n+\t\t\t\t\t\t       argv[0], value, CONFIG_REGEX_NONE, 0);\n \t}\n \telse if (actions == ACTION_REPLACE_ALL) {\n \t\tcheck_write();\ndiff --git a/cache.h b/cache.h\nindex dfa1a56..a09217d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1284,6 +1284,8 @@ extern int update_server_info(int);\n #define CONFIG_INVALID_PATTERN 6\n #define CONFIG_GENERIC_ERROR 7\n\n+extern const char CONFIG_REGEX_NONE[];\n+\n struct git_config_source {\n \tunsigned int use_stdin:1;\n \tconst char *file;\ndiff --git a/config.c b/config.c\nindex 83c913a..20476e0 100644\n--- a/config.c\n+++ b/config.c\n@@ -46,6 +46,8 @@ static int zlib_compression_seen;\n  */\n static struct config_set the_config_set;\n\n+const char CONFIG_REGEX_NONE[] = \"a^\";\n+\n static int config_file_fgetc(struct config_source *conf)\n {\n \treturn fgetc(conf->u.file);\n@@ -1607,14 +1609,20 @@ static struct {\n \tunsigned int offset_alloc;\n \tenum { START, SECTION_SEEN, SECTION_END_SEEN, KEY_SEEN } state;\n \tint seen;\n+\tunsigned value_never_matches:1;\n } store;\n\n static int matches(const char *key, const char *value)\n {\n-\treturn !strcmp(key, store.key) &&\n-\t\t(store.value_regex == NULL ||\n-\t\t (store.do_not_match ^\n-\t\t  !regexec(store.value_regex, value, 0, NULL, 0)));\n+\tif (strcmp(key, store.key))\n+\t\treturn 0; /* not ours */\n+\tif (!store.value_regex)\n+\t\treturn 1; /* always matches */\n+\tif (store.value_never_matches)\n+\t\treturn 0; /* never matches */\n+\n+\treturn store.do_not_match ^\n+\t(value && !regexec(store.value_regex, value, 0, NULL, 0));\n }\n\n static int store_aux(const char *key, const char *value, void *cb)\n@@ -1876,6 +1884,8 @@ out_free_ret_1:\n /*\n  * If value==NULL, unset in (remove from) config,\n  * if value_regex!=NULL, disregard key/value pairs where value does not match.\n+ * if value_regex==CONFIG_REGEX_NONE, do not match any existing values\n+ * (only add a new one)\n  * if multi_replace==0, nothing, or only one matching key/value is replaced,\n  *     else all matching key/values (regardless how many) are removed,\n  *     before the new pair is written.\n@@ -1966,6 +1976,9 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \t\t\t} else\n \t\t\t\tstore.do_not_match = 0;\n\n+\t\t\tif (value_regex == CONFIG_REGEX_NONE)\n+\t\t\t\tstore.value_never_matches = 1;\n+\n \t\t\tstore.value_regex = (regex_t*)xmalloc(sizeof(regex_t));\n \t\t\tif (regcomp(store.value_regex, value_regex,\n \t\t\t\t\tREG_EXTENDED)) {\ndiff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh\nindex e5c0f07..3b92083 100755\n--- a/t/t1303-wacky-config.sh\n+++ b/t/t1303-wacky-config.sh\n@@ -111,7 +111,7 @@ test_expect_success 'unset many entries' '\n \ttest_must_fail git config section.key\n '\n\n-test_expect_failure '--add appends new value after existing empty value' '\n+test_expect_success '--add appends new value after existing empty value' '\n \tcat >expect <<-\\EOF &&\n\n\n-- \n1.9.0.GIT\n"},{"id":"249295","messageId":"vpqegvhfe5p.fsf@anie.imag.fr","threadId":"37369","inReplyTo":"54129FE1.6020303@gmail.com","subject":"Re: [PATCH v2 2/2] make config --add behave correctly for empty and NULL values","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-09-12T08:15:14Z","receivedAt":"2014-09-12T08:15:14Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra <tanayabh@gmail.com> writes:\n\n> +const char CONFIG_REGEX_NONE[] = \"a^\";\n\nI have a slight preference for this version (no magic (void *)1 value,\nand belts-and-suspenders solution since someone actually using the regex\nshould still get a correct behavior.\n\nBut I'm fine with both Junio/Peff's version or this one.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"249311","messageId":"xmqqa964bvd6.fsf@gitster.dls.corp.google.com","threadId":"37369","inReplyTo":"vpqegvhfe5p.fsf@anie.imag.fr","subject":"Re: [PATCH v2 2/2] make config --add behave correctly for empty and NULL values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-12T17:29:25Z","receivedAt":"2014-09-12T17:29:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n> Tanay Abhra <tanayabh@gmail.com> writes:\n>\n>> +const char CONFIG_REGEX_NONE[] = \"a^\";\n>\n> I have a slight preference for this version (no magic (void *)1 value,\n> and belts-and-suspenders solution since someone actually using the regex\n> should still get a correct behavior.\n>\n> But I'm fine with both Junio/Peff's version or this one.\n\nI do not care too deeply either way, to be honest.\n\nBut if we were to redo this in the right way, I suspect that the\nbest solution may be to correct the root cause, which is the design\nmistake in the git_config_set_multivar_in_file API.  The function\ntakes a regexp (possibly NULL) and a multi_replace bit, and with\nthat expresses these three combinations:\n\n    - a non-NULL regexp means only the existing ones that match are\n      subject to replacement;\n\n    - NULL regexp means all of the existing ones that match are\n      subject to replacement;\n\n    - multi-replace bit controls which ones among the replacement\n      candidates are replaced (either the first one or all).\n\nBut we actually want to express three, not two, different handling\nfor the existing entries.  Either (1) use the regexp to decide which\nones are subject to replacement, (2) declare all of them are subject\nto replacement, or (3) declare none of them are to be replaced.  The\nlast one cannot be expressed without coming up with a trick to say\n\"I am giving a regexp that hopefully will not match anything as a\nworkaround because otherwise you will replace all of them but what I\nreally want to say is I do not want you to replace anything\", and\nthis thread discusses a fix to the bug in the implementation that\nfailed to come up with a \"hopefully will not match anything\"\npattern.  And we are still discussing to fix a better workaround.\n\nInstead of polishing the workaround, wouldn't it be better to make\nit unnecessary to work it around?  For exaple, we could turn the\nlast parameter to the function into an \"unsigned flag\" with two\nbits, CONFIG_SET_USE_REGEXP_TO_FILTER (if set, use the regexp to\nfilter which of the existing entries to be replaced) and\nCONFIG_SET_REPLACE_MULTI (if set, replace all the eligible ones),\nand the result would be conceptually a lot cleaner, no?\n\nSome notes:\n\n - Because most callers expect \"replace\" behaviour, instead of\n   adding CONFIG_SET_USE_REGEXP_TO_FILTER to the majority of\n   existing callers, a new flag CONFIG_SET_JUST_APPEND (which is\n   exactly the negation of the USE_REGEXP_TO_FILTER) would be a more\n   practical thing to introduce.\n\n - We can keep using value_regexp==NULL (under !JUST_APPEND) to mean\n   value_regexp=\".*\", i.e. matches anything, as a short-hand.\n\nHmm?\n"}]}