{"thread":{"id":"35283","subject":"Bug with git svn fetch? \"error:too many matches for svn-remote.svn.added-placeholder\"","startedAt":"2013-11-05T15:31:01Z","lastAt":"2013-12-06T21:10:35Z","messageCount":8,"participants":["Jess Hottenstein","Thomas Rast","Eric Sunshine","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"230118","messageId":"CAB8C745oJjw6pZ1MFy73Wy=WM-8n=aRY7VUh73u__VLB5e8mQA@mail.gmail.com","threadId":"35283","inReplyTo":null,"subject":"Bug with git svn fetch? \"error:too many matches for svn-remote.svn.added-placeholder\"","fromName":"Jess Hottenstein","fromEmail":"jess.hottenstein@gmail.com","sentAt":"2013-11-05T15:31:01Z","receivedAt":"2013-11-05T15:31:01Z","isPatch":false,"sender":{"key":"jess.hottenstein@gmail.com","avatar":null},"body":"Hello,\n\nI'm running a git svn fetch of a large svn repo and it is grinding to\na halt with thousands of \"error: too many matches for\nsvn-remote.svn.added-placeholder\".  My .git/config has grown to ~50000\nlines of added-placeholders.\n\nI also had to up my linux file limit because I have ~2500 open files\n(tempfiles in .git)\n\nI'm doing a onetime migration so I don't really care about having the\nadded-placeholder entries but I do want to keep empty directories.\n\nAny thoughts for a workaround?\n\nThanks!\n"},{"id":"230562","messageId":"06ba1524cbe4fa31b6e1a8d644882521aeaff4f4.1384337608.git.tr@thomasrast.ch","threadId":"35283","inReplyTo":"CAB8C745oJjw6pZ1MFy73Wy=WM-8n=aRY7VUh73u__VLB5e8mQA@mail.gmail.com","subject":"[PATCH] config: arbitrary number of matches for --unset and --replace-all","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2013-11-13T10:19:00Z","receivedAt":"2013-11-13T10:19:00Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"git-config used a static match array to hold the matches we want to\nunset/replace when using --unset or --replace-all.  Use a\nvariable-sized array instead.\n\nThis in particular fixes the symptoms git-svn had when storing large\nnumbers of svn-remote.*.added-placeholder entries in the config file.\n\nWhile the tests are rather more paranoid than just --unset and\n--replace-all, the other operations already worked.  Indeed git-svn's\nusage only breaks the first time *after* creating so many entries,\nwhen it wants to unset and re-add them all.\n\nReported-by: Jess Hottenstein <jess.hottenstein@gmail.com>\nSigned-off-by: Thomas Rast <tr@thomasrast.ch>\n---\n\nThis should fix it, though I haven't actually tested with such a funny\nuse-case, nor written a proper git-svn test for it.  My analysis about\nthe failure mode is from briefly looking at the code.\n\ngit-svn should probably be changed to use another way of storing\nthese, as git-config is not a very efficient database, but I'm leaving\nthat for someone who cares about SVN.  (Note that it's also much\nharder as it'll need a migration plan.)\n\n\n config.c                | 19 ++++++++++-----\n t/t1303-wacky-config.sh | 64 +++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 77 insertions(+), 6 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 3c2434a..ef63bf2 100644\n--- a/config.c\n+++ b/config.c\n@@ -1197,15 +1197,14 @@ int git_config(config_fn_t fn, void *data)\n  * Find all the stuff for git_config_set() below.\n  */\n \n-#define MAX_MATCHES 512\n-\n static struct {\n \tint baselen;\n \tchar *key;\n \tint do_not_match;\n \tregex_t *value_regex;\n \tint multi_replace;\n-\tsize_t offset[MAX_MATCHES];\n+\tsize_t *offset;\n+\tunsigned int offset_alloc;\n \tenum { START, SECTION_SEEN, SECTION_END_SEEN, KEY_SEEN } state;\n \tint seen;\n } store;\n@@ -1228,11 +1227,11 @@ static int store_aux(const char *key, const char *value, void *cb)\n \t\tif (matches(key, value)) {\n \t\t\tif (store.seen == 1 && store.multi_replace == 0) {\n \t\t\t\twarning(\"%s has multiple values\", key);\n-\t\t\t} else if (store.seen >= MAX_MATCHES) {\n-\t\t\t\terror(\"too many matches for %s\", key);\n-\t\t\t\treturn 1;\n \t\t\t}\n \n+\t\t\tALLOC_GROW(store.offset, store.seen+1,\n+\t\t\t\t   store.offset_alloc);\n+\n \t\t\tstore.offset[store.seen] = cf->do_ftell(cf);\n \t\t\tstore.seen++;\n \t\t}\n@@ -1260,11 +1259,15 @@ static int store_aux(const char *key, const char *value, void *cb)\n \t\t * Do not increment matches: this is no match, but we\n \t\t * just made sure we are in the desired section.\n \t\t */\n+\t\tALLOC_GROW(store.offset, store.seen+1,\n+\t\t\t   store.offset_alloc);\n \t\tstore.offset[store.seen] = cf->do_ftell(cf);\n \t\t/* fallthru */\n \tcase SECTION_END_SEEN:\n \tcase START:\n \t\tif (matches(key, value)) {\n+\t\t\tALLOC_GROW(store.offset, store.seen+1,\n+\t\t\t\t   store.offset_alloc);\n \t\t\tstore.offset[store.seen] = cf->do_ftell(cf);\n \t\t\tstore.state = KEY_SEEN;\n \t\t\tstore.seen++;\n@@ -1272,6 +1275,9 @@ static int store_aux(const char *key, const char *value, void *cb)\n \t\t\tif (strrchr(key, '.') - key == store.baselen &&\n \t\t\t      !strncmp(key, store.key, store.baselen)) {\n \t\t\t\t\tstore.state = SECTION_SEEN;\n+\t\t\t\t\tALLOC_GROW(store.offset,\n+\t\t\t\t\t\t   store.seen+1,\n+\t\t\t\t\t\t   store.offset_alloc);\n \t\t\t\t\tstore.offset[store.seen] = cf->do_ftell(cf);\n \t\t\t}\n \t\t}\n@@ -1570,6 +1576,7 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \t\t\t}\n \t\t}\n \n+\t\tALLOC_GROW(store.offset, 1, store.offset_alloc);\n \t\tstore.offset[0] = 0;\n \t\tstore.state = START;\n \t\tstore.seen = 0;\ndiff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh\nindex 46103a1..7d55730 100755\n--- a/t/t1303-wacky-config.sh\n+++ b/t/t1303-wacky-config.sh\n@@ -3,17 +3,28 @@\n test_description='Test wacky input to git config'\n . ./test-lib.sh\n \n+# Leaving off the newline is intentional!\n setup() {\n \t(printf \"[section]\\n\" &&\n \tprintf \"  key = foo\") >.git/config\n }\n \n+# 'check section.key value' verifies that the entry for section.key is\n+# 'value'\n check() {\n \techo \"$2\" >expected\n \tgit config --get \"$1\" >actual 2>&1\n \ttest_cmp actual expected\n }\n \n+# 'check section.key regex value' verifies that the entry for\n+# section.key *that matches 'regex'* is 'value'\n+check_regex() {\n+\techo \"$3\" >expected\n+\tgit config --get \"$1\" \"$2\" >actual 2>&1\n+\ttest_cmp actual expected\n+}\n+\n test_expect_success 'modify same key' '\n \tsetup &&\n \tgit config section.key bar &&\n@@ -47,4 +58,57 @@ test_expect_success 'do not crash on special long config line' '\n \tcheck section.key \"$LONG_VALUE\"\n '\n \n+setup_many() {\n+\tsetup &&\n+\t# This time we want the newline so that we can tack on more\n+\t# entries.\n+\techo >>.git/config &&\n+\t# Semi-efficient way of concatenating 5^5 = 3125 lines. Note\n+\t# that because 'setup' already put one line, this means 3126\n+\t# entries for section.key in the config file.\n+\tcat >5to1 <<EOF\n+  key = foo\n+  key = foo\n+  key = foo\n+  key = foo\n+  key = foo\n+EOF\n+\tcat 5to1 5to1 5to1 5to1 5to1 >5to2 &&\t   # 25\n+\tcat 5to2 5to2 5to2 5to2 5to2 >5to3 &&\t   # 125\n+\tcat 5to3 5to3 5to3 5to3 5to3 >5to4 &&\t   # 635\n+\tcat 5to4 5to4 5to4 5to4 5to4 >>.git/config # 3125\n+}\n+\n+test_expect_success 'get many entries' '\n+\tsetup_many &&\n+\tgit config --get-all section.key >actual &&\n+\ttest_line_count = 3126 actual\n+'\n+\n+test_expect_success 'get many entries by regex' '\n+\tsetup_many &&\n+\tgit config --get-regexp \"sec.*ke.\" >actual &&\n+\ttest_line_count = 3126 actual\n+'\n+\n+test_expect_success 'add and replace one of many entries' '\n+\tsetup_many &&\n+\tgit config --add section.key bar &&\n+\tcheck_regex section.key \"b.*r\" bar &&\n+\tgit config section.key beer \"b.*r\" &&\n+\tcheck_regex section.key \"b.*r\" beer\n+'\n+\n+test_expect_success 'replace many entries' '\n+\tsetup_many &&\n+\tgit config --replace-all section.key bar &&\n+\tcheck section.key bar\n+'\n+\n+test_expect_success 'unset many entries' '\n+\tsetup_many &&\n+\tgit config --unset-all section.key &&\n+\ttest_must_fail git config section.key\n+'\n+\n test_done\n-- \n1.8.5.rc0.346.g150976e\n"},{"id":"230581","messageId":"CAPig+cQZo0R3q=J2BygTfdJ1uuiT1HPDCjTxt8mykxOXM1uf2Q@mail.gmail.com","threadId":"35283","inReplyTo":"06ba1524cbe4fa31b6e1a8d644882521aeaff4f4.1384337608.git.tr@thomasrast.ch","subject":"Re: [PATCH] config: arbitrary number of matches for --unset and --replace-all","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-11-13T23:22:38Z","receivedAt":"2013-11-13T23:22:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 13, 2013 at 5:19 AM, Thomas Rast <tr@thomasrast.ch> wrote:\n> diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh\n> index 46103a1..7d55730 100755\n> --- a/t/t1303-wacky-config.sh\n> +++ b/t/t1303-wacky-config.sh\n> @@ -47,4 +58,57 @@ test_expect_success 'do not crash on special long config line' '\n>         check section.key \"$LONG_VALUE\"\n>  '\n>\n> +setup_many() {\n> +       setup &&\n> +       # This time we want the newline so that we can tack on more\n> +       # entries.\n> +       echo >>.git/config &&\n> +       # Semi-efficient way of concatenating 5^5 = 3125 lines. Note\n> +       # that because 'setup' already put one line, this means 3126\n> +       # entries for section.key in the config file.\n> +       cat >5to1 <<EOF\n\nBroken &&-chain.\n\n> +  key = foo\n> +  key = foo\n> +  key = foo\n> +  key = foo\n> +  key = foo\n> +EOF\n> +       cat 5to1 5to1 5to1 5to1 5to1 >5to2 &&      # 25\n> +       cat 5to2 5to2 5to2 5to2 5to2 >5to3 &&      # 125\n> +       cat 5to3 5to3 5to3 5to3 5to3 >5to4 &&      # 635\n> +       cat 5to4 5to4 5to4 5to4 5to4 >>.git/config # 3125\n> +}\n> +\n> +test_expect_success 'get many entries' '\n> +       setup_many &&\n> +       git config --get-all section.key >actual &&\n> +       test_line_count = 3126 actual\n> +'\n> +\n> +test_expect_success 'get many entries by regex' '\n> +       setup_many &&\n> +       git config --get-regexp \"sec.*ke.\" >actual &&\n> +       test_line_count = 3126 actual\n> +'\n> +\n> +test_expect_success 'add and replace one of many entries' '\n> +       setup_many &&\n> +       git config --add section.key bar &&\n> +       check_regex section.key \"b.*r\" bar &&\n> +       git config section.key beer \"b.*r\" &&\n> +       check_regex section.key \"b.*r\" beer\n> +'\n> +\n> +test_expect_success 'replace many entries' '\n> +       setup_many &&\n> +       git config --replace-all section.key bar &&\n> +       check section.key bar\n> +'\n> +\n> +test_expect_success 'unset many entries' '\n> +       setup_many &&\n> +       git config --unset-all section.key &&\n> +       test_must_fail git config section.key\n> +'\n> +\n>  test_done\n> --\n> 1.8.5.rc0.346.g150976e\n"},{"id":"230588","messageId":"9bc62ec0072a0513865f39ba287819dd0d9d606d.1384415180.git.tr@thomasrast.ch","threadId":"35283","inReplyTo":"CAPig+cQZo0R3q=J2BygTfdJ1uuiT1HPDCjTxt8mykxOXM1uf2Q@mail.gmail.com","subject":"[PATCH] config: arbitrary number of matches for --unset and --replace-all","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2013-11-14T07:50:43Z","receivedAt":"2013-11-14T07:50:43Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"git-config used a static match array to hold the matches we want to\nunset/replace when using --unset or --replace-all.  Use a\nvariable-sized array instead.\n\nThis in particular fixes the symptoms git-svn had when storing large\nnumbers of svn-remote.*.added-placeholder entries in the config file.\n\nWhile the tests are rather more paranoid than just --unset and\n--replace-all, the other operations already worked.  Indeed git-svn's\nusage only breaks the first time *after* creating so many entries,\nwhen it wants to unset and re-add them all.\n\nReported-by: Jess Hottenstein <jess.hottenstein@gmail.com>\nSigned-off-by: Thomas Rast <tr@thomasrast.ch>\n---\n\nEric Sunshine wrote:\n> On Wed, Nov 13, 2013 at 5:19 AM, Thomas Rast <tr@thomasrast.ch> wrote:\n> > +setup_many() {\n[...]\n> > +       cat >5to1 <<EOF\n> \n> Broken &&-chain.\n\nOops, thanks for catching.\n\n\n config.c                | 19 ++++++++++-----\n t/t1303-wacky-config.sh | 64 +++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 77 insertions(+), 6 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 3c2434a..ef63bf2 100644\n--- a/config.c\n+++ b/config.c\n@@ -1197,15 +1197,14 @@ int git_config(config_fn_t fn, void *data)\n  * Find all the stuff for git_config_set() below.\n  */\n \n-#define MAX_MATCHES 512\n-\n static struct {\n \tint baselen;\n \tchar *key;\n \tint do_not_match;\n \tregex_t *value_regex;\n \tint multi_replace;\n-\tsize_t offset[MAX_MATCHES];\n+\tsize_t *offset;\n+\tunsigned int offset_alloc;\n \tenum { START, SECTION_SEEN, SECTION_END_SEEN, KEY_SEEN } state;\n \tint seen;\n } store;\n@@ -1228,11 +1227,11 @@ static int store_aux(const char *key, const char *value, void *cb)\n \t\tif (matches(key, value)) {\n \t\t\tif (store.seen == 1 && store.multi_replace == 0) {\n \t\t\t\twarning(\"%s has multiple values\", key);\n-\t\t\t} else if (store.seen >= MAX_MATCHES) {\n-\t\t\t\terror(\"too many matches for %s\", key);\n-\t\t\t\treturn 1;\n \t\t\t}\n \n+\t\t\tALLOC_GROW(store.offset, store.seen+1,\n+\t\t\t\t   store.offset_alloc);\n+\n \t\t\tstore.offset[store.seen] = cf->do_ftell(cf);\n \t\t\tstore.seen++;\n \t\t}\n@@ -1260,11 +1259,15 @@ static int store_aux(const char *key, const char *value, void *cb)\n \t\t * Do not increment matches: this is no match, but we\n \t\t * just made sure we are in the desired section.\n \t\t */\n+\t\tALLOC_GROW(store.offset, store.seen+1,\n+\t\t\t   store.offset_alloc);\n \t\tstore.offset[store.seen] = cf->do_ftell(cf);\n \t\t/* fallthru */\n \tcase SECTION_END_SEEN:\n \tcase START:\n \t\tif (matches(key, value)) {\n+\t\t\tALLOC_GROW(store.offset, store.seen+1,\n+\t\t\t\t   store.offset_alloc);\n \t\t\tstore.offset[store.seen] = cf->do_ftell(cf);\n \t\t\tstore.state = KEY_SEEN;\n \t\t\tstore.seen++;\n@@ -1272,6 +1275,9 @@ static int store_aux(const char *key, const char *value, void *cb)\n \t\t\tif (strrchr(key, '.') - key == store.baselen &&\n \t\t\t      !strncmp(key, store.key, store.baselen)) {\n \t\t\t\t\tstore.state = SECTION_SEEN;\n+\t\t\t\t\tALLOC_GROW(store.offset,\n+\t\t\t\t\t\t   store.seen+1,\n+\t\t\t\t\t\t   store.offset_alloc);\n \t\t\t\t\tstore.offset[store.seen] = cf->do_ftell(cf);\n \t\t\t}\n \t\t}\n@@ -1570,6 +1576,7 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \t\t\t}\n \t\t}\n \n+\t\tALLOC_GROW(store.offset, 1, store.offset_alloc);\n \t\tstore.offset[0] = 0;\n \t\tstore.state = START;\n \t\tstore.seen = 0;\ndiff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh\nindex 46103a1..c2706ea 100755\n--- a/t/t1303-wacky-config.sh\n+++ b/t/t1303-wacky-config.sh\n@@ -3,17 +3,28 @@\n test_description='Test wacky input to git config'\n . ./test-lib.sh\n \n+# Leaving off the newline is intentional!\n setup() {\n \t(printf \"[section]\\n\" &&\n \tprintf \"  key = foo\") >.git/config\n }\n \n+# 'check section.key value' verifies that the entry for section.key is\n+# 'value'\n check() {\n \techo \"$2\" >expected\n \tgit config --get \"$1\" >actual 2>&1\n \ttest_cmp actual expected\n }\n \n+# 'check section.key regex value' verifies that the entry for\n+# section.key *that matches 'regex'* is 'value'\n+check_regex() {\n+\techo \"$3\" >expected\n+\tgit config --get \"$1\" \"$2\" >actual 2>&1\n+\ttest_cmp actual expected\n+}\n+\n test_expect_success 'modify same key' '\n \tsetup &&\n \tgit config section.key bar &&\n@@ -47,4 +58,57 @@ test_expect_success 'do not crash on special long config line' '\n \tcheck section.key \"$LONG_VALUE\"\n '\n \n+setup_many() {\n+\tsetup &&\n+\t# This time we want the newline so that we can tack on more\n+\t# entries.\n+\techo >>.git/config &&\n+\t# Semi-efficient way of concatenating 5^5 = 3125 lines. Note\n+\t# that because 'setup' already put one line, this means 3126\n+\t# entries for section.key in the config file.\n+\tcat >5to1 <<EOF &&\n+  key = foo\n+  key = foo\n+  key = foo\n+  key = foo\n+  key = foo\n+EOF\n+\tcat 5to1 5to1 5to1 5to1 5to1 >5to2 &&\t   # 25\n+\tcat 5to2 5to2 5to2 5to2 5to2 >5to3 &&\t   # 125\n+\tcat 5to3 5to3 5to3 5to3 5to3 >5to4 &&\t   # 635\n+\tcat 5to4 5to4 5to4 5to4 5to4 >>.git/config # 3125\n+}\n+\n+test_expect_success 'get many entries' '\n+\tsetup_many &&\n+\tgit config --get-all section.key >actual &&\n+\ttest_line_count = 3126 actual\n+'\n+\n+test_expect_success 'get many entries by regex' '\n+\tsetup_many &&\n+\tgit config --get-regexp \"sec.*ke.\" >actual &&\n+\ttest_line_count = 3126 actual\n+'\n+\n+test_expect_success 'add and replace one of many entries' '\n+\tsetup_many &&\n+\tgit config --add section.key bar &&\n+\tcheck_regex section.key \"b.*r\" bar &&\n+\tgit config section.key beer \"b.*r\" &&\n+\tcheck_regex section.key \"b.*r\" beer\n+'\n+\n+test_expect_success 'replace many entries' '\n+\tsetup_many &&\n+\tgit config --replace-all section.key bar &&\n+\tcheck section.key bar\n+'\n+\n+test_expect_success 'unset many entries' '\n+\tsetup_many &&\n+\tgit config --unset-all section.key &&\n+\ttest_must_fail git config section.key\n+'\n+\n test_done\n-- \n1.8.5.rc1.353.g06ba152\n"},{"id":"230594","messageId":"20131114083747.GD16327@sigill.intra.peff.net","threadId":"35283","inReplyTo":"9bc62ec0072a0513865f39ba287819dd0d9d606d.1384415180.git.tr@thomasrast.ch","subject":"Re: [PATCH] config: arbitrary number of matches for --unset and --replace-all","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-11-14T08:37:47Z","receivedAt":"2013-11-14T08:37:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 14, 2013 at 08:50:43AM +0100, Thomas Rast wrote:\n\n> git-config used a static match array to hold the matches we want to\n> unset/replace when using --unset or --replace-all.  Use a\n> variable-sized array instead.\n> \n> This in particular fixes the symptoms git-svn had when storing large\n> numbers of svn-remote.*.added-placeholder entries in the config file.\n> \n> While the tests are rather more paranoid than just --unset and\n> --replace-all, the other operations already worked.  Indeed git-svn's\n> usage only breaks the first time *after* creating so many entries,\n> when it wants to unset and re-add them all.\n> \n> Reported-by: Jess Hottenstein <jess.hottenstein@gmail.com>\n> Signed-off-by: Thomas Rast <tr@thomasrast.ch>\n\nThis looks good to me.\n\nI agree with your earlier assessment that adding tons of config keys is\nprobably a bad idea. It's not just slow when looking up those keys, but\nit also slows down every single git operation (which might read config\nmany times, if it is a script).  Still, we should at least do the right\nthing in the face of such config.\n\n> @@ -1260,11 +1259,15 @@ static int store_aux(const char *key, const char *value, void *cb)\n>  \t\t * Do not increment matches: this is no match, but we\n>  \t\t * just made sure we are in the desired section.\n>  \t\t */\n> +\t\tALLOC_GROW(store.offset, store.seen+1,\n> +\t\t\t   store.offset_alloc);\n>  \t\tstore.offset[store.seen] = cf->do_ftell(cf);\n>  \t\t/* fallthru */\n>  \tcase SECTION_END_SEEN:\n>  \tcase START:\n>  \t\tif (matches(key, value)) {\n> +\t\t\tALLOC_GROW(store.offset, store.seen+1,\n> +\t\t\t\t   store.offset_alloc);\n>  \t\t\tstore.offset[store.seen] = cf->do_ftell(cf);\n>  \t\t\tstore.state = KEY_SEEN;\n>  \t\t\tstore.seen++;\n\nThis code is weird to follow because of the fall-throughs. I do not\nthink you have introduced any bugs with your patch, but it seems weird\nto me that we set the offset at the top of the hunk. If we hit the\nconditional in the bottom half, we do actually increment storer.seen,\nbut only _after_ having overwritten the value from above (with the same\nvalue, no less).\n\nBut if we do not follow that code path, we may end up here:\n\n> @@ -1272,6 +1275,9 @@ static int store_aux(const char *key, const char *value, void *cb)\n>  \t\t\tif (strrchr(key, '.') - key == store.baselen &&\n>  \t\t\t      !strncmp(key, store.key, store.baselen)) {\n>  \t\t\t\t\tstore.state = SECTION_SEEN;\n> +\t\t\t\t\tALLOC_GROW(store.offset,\n> +\t\t\t\t\t\t   store.seen+1,\n> +\t\t\t\t\t\t   store.offset_alloc);\n>  \t\t\t\t\tstore.offset[store.seen] = cf->do_ftell(cf);\n>  \t\t\t}\n>  \t\t}\n\nwhere we overwrite it again, but do not update store.seen. Or we may\ntrigger neither, and leave the function with our offset stored, but\nstore.seen not incremented.\n\nSo it seems odd to me that we would set the offset, but in some cases\nnever actually increment our counter. AFAICT, we do not ever access\nthose values. The reader limits itself to \"i < store.seen\", which makes\nsense. And the writers always call a fresh ftell before incrementing\nstore.seen.  But maybe I am missing something.\n\nAnyway, it is neither here nor there for your patch. Just something odd\nI noticed while reading the code.\n\n> +setup_many() {\n> +\tsetup &&\n> +\t# This time we want the newline so that we can tack on more\n> +\t# entries.\n> +\techo >>.git/config &&\n> +\t# Semi-efficient way of concatenating 5^5 = 3125 lines. Note\n> +\t# that because 'setup' already put one line, this means 3126\n> +\t# entries for section.key in the config file.\n> +\tcat >5to1 <<EOF &&\n> +  key = foo\n> +  key = foo\n> +  key = foo\n> +  key = foo\n> +  key = foo\n> +EOF\n> +\tcat 5to1 5to1 5to1 5to1 5to1 >5to2 &&\t   # 25\n> +\tcat 5to2 5to2 5to2 5to2 5to2 >5to3 &&\t   # 125\n> +\tcat 5to3 5to3 5to3 5to3 5to3 >5to4 &&\t   # 635\n> +\tcat 5to4 5to4 5to4 5to4 5to4 >>.git/config # 3125\n> +}\n\nYou could make this even more semi-efficient by just storing the end\nresult in 5to5, and then copying it into place rather than doing the\nwhole dance for each test. I doubt it matters that much, though.\n\n-Peff\n"},{"id":"230651","messageId":"87zjp6loiz.fsf@linux-k42r.v.cablecom.net","threadId":"35283","inReplyTo":"20131114083747.GD16327@sigill.intra.peff.net","subject":"Re: [PATCH] config: arbitrary number of matches for --unset and --replace-all","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2013-11-14T20:24:52Z","receivedAt":"2013-11-14T20:24:52Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This code is weird to follow because of the fall-throughs. I do not\n> think you have introduced any bugs with your patch, but it seems weird\n> to me that we set the offset at the top of the hunk. If we hit the\n> conditional in the bottom half, we do actually increment storer.seen,\n> but only _after_ having overwritten the value from above (with the same\n> value, no less).\n>\n> But if we do not follow that code path, we may end up here:\n>\n>> @@ -1272,6 +1275,9 @@ static int store_aux(const char *key, const char *value, void *cb)\n>>  \t\t\tif (strrchr(key, '.') - key == store.baselen &&\n>>  \t\t\t      !strncmp(key, store.key, store.baselen)) {\n>>  \t\t\t\t\tstore.state = SECTION_SEEN;\n>> +\t\t\t\t\tALLOC_GROW(store.offset,\n>> +\t\t\t\t\t\t   store.seen+1,\n>> +\t\t\t\t\t\t   store.offset_alloc);\n>>  \t\t\t\t\tstore.offset[store.seen] = cf->do_ftell(cf);\n>>  \t\t\t}\n>>  \t\t}\n>\n> where we overwrite it again, but do not update store.seen. Or we may\n> trigger neither, and leave the function with our offset stored, but\n> store.seen not incremented.\n\nIt's doubly strange that we write in this hunk without any protection\nagainst overflow.  I was too lazy to think about it long enough to come\nup with a possible example that triggers this, and instead just put in\nthe defensive ALLOC_GROW().  But if you can trigger it, it will probably\ncause the algorithm to go off the rails because it overwrote store.state\nand possibly even store.seen.\n\n-- \nThomas Rast\ntr@thomasrast.ch\n"},{"id":"231667","messageId":"1386355602-21818-1-git-send-email-gitster@pobox.com","threadId":"35283","inReplyTo":"06ba1524cbe4fa31b6e1a8d644882521aeaff4f4.1384337608.git.tr@thomasrast.ch","subject":"[PATCH] fixup! config: arbitrary number of matches for --unset and --replace-all","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-06T18:46:42Z","receivedAt":"2013-12-06T18:46:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"---\n * I'll squash this to tr/config-multivalue-lift-max in preparation\n   for merging it to 'master',which should happen by the end of\n   this week.\n\n   Thanks.\n\n config.c                |  8 ++++----\n t/t1303-wacky-config.sh | 14 +++++++-------\n 2 files changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 431dcb7..a1e80da 100644\n--- a/config.c\n+++ b/config.c\n@@ -1192,7 +1192,7 @@ static int store_aux(const char *key, const char *value, void *cb)\n \t\t\t\twarning(\"%s has multiple values\", key);\n \t\t\t}\n \n-\t\t\tALLOC_GROW(store.offset, store.seen+1,\n+\t\t\tALLOC_GROW(store.offset, store.seen + 1,\n \t\t\t\t   store.offset_alloc);\n \n \t\t\tstore.offset[store.seen] = cf->do_ftell(cf);\n@@ -1222,14 +1222,14 @@ static int store_aux(const char *key, const char *value, void *cb)\n \t\t * Do not increment matches: this is no match, but we\n \t\t * just made sure we are in the desired section.\n \t\t */\n-\t\tALLOC_GROW(store.offset, store.seen+1,\n+\t\tALLOC_GROW(store.offset, store.seen + 1,\n \t\t\t   store.offset_alloc);\n \t\tstore.offset[store.seen] = cf->do_ftell(cf);\n \t\t/* fallthru */\n \tcase SECTION_END_SEEN:\n \tcase START:\n \t\tif (matches(key, value)) {\n-\t\t\tALLOC_GROW(store.offset, store.seen+1,\n+\t\t\tALLOC_GROW(store.offset, store.seen + 1,\n \t\t\t\t   store.offset_alloc);\n \t\t\tstore.offset[store.seen] = cf->do_ftell(cf);\n \t\t\tstore.state = KEY_SEEN;\n@@ -1239,7 +1239,7 @@ static int store_aux(const char *key, const char *value, void *cb)\n \t\t\t      !strncmp(key, store.key, store.baselen)) {\n \t\t\t\t\tstore.state = SECTION_SEEN;\n \t\t\t\t\tALLOC_GROW(store.offset,\n-\t\t\t\t\t\t   store.seen+1,\n+\t\t\t\t\t\t   store.seen + 1,\n \t\t\t\t\t\t   store.offset_alloc);\n \t\t\t\t\tstore.offset[store.seen] = cf->do_ftell(cf);\n \t\t\t}\ndiff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh\nindex 7d55730..3a2c819 100755\n--- a/t/t1303-wacky-config.sh\n+++ b/t/t1303-wacky-config.sh\n@@ -66,13 +66,13 @@ setup_many() {\n \t# Semi-efficient way of concatenating 5^5 = 3125 lines. Note\n \t# that because 'setup' already put one line, this means 3126\n \t# entries for section.key in the config file.\n-\tcat >5to1 <<EOF\n-  key = foo\n-  key = foo\n-  key = foo\n-  key = foo\n-  key = foo\n-EOF\n+\tcat >5to1 <<-\\EOF &&\n+\t  key = foo\n+\t  key = foo\n+\t  key = foo\n+\t  key = foo\n+\t  key = foo\n+\tEOF\n \tcat 5to1 5to1 5to1 5to1 5to1 >5to2 &&\t   # 25\n \tcat 5to2 5to2 5to2 5to2 5to2 >5to3 &&\t   # 125\n \tcat 5to3 5to3 5to3 5to3 5to3 >5to4 &&\t   # 635\n-- \n1.8.5.1-402-gdd8f092\n"},{"id":"231673","messageId":"20131206211035.GA20482@sigill.intra.peff.net","threadId":"35283","inReplyTo":"1386355602-21818-1-git-send-email-gitster@pobox.com","subject":"Re: [PATCH] fixup! config: arbitrary number of matches for --unset and --replace-all","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-06T21:10:35Z","receivedAt":"2013-12-06T21:10:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 06, 2013 at 10:46:42AM -0800, Junio C Hamano wrote:\n\n> ---\n>  * I'll squash this to tr/config-multivalue-lift-max in preparation\n>    for merging it to 'master',which should happen by the end of\n>    this week.\n\nYeah, all makes sense to me. Thanks.\n\n-Peff\n"}]}