threads / bug / 35283
Subject: Bug with git svn fetch? "error:too many matches for svn-remote.svn.added-placeholder"
Hello,
I'm running a git svn fetch of a large svn repo and it is grinding to a halt with thousands of "error: too many matches for svn-remote.svn.added-placeholder". My .git/config has grown to ~50000 lines of added-placeholders.
I also had to up my linux file limit because I have ~2500 open files (tempfiles in .git)
I'm doing a onetime migration so I don't really care about having the added-placeholder entries but I do want to keep empty directories.
Any thoughts for a workaround?
Thanks!
Thomas Rast[PATCH] config: arbitrary number of matches for --unset and --replace-all
git-config used a static match array to hold the matches we want to unset/replace when using --unset or --replace-all. Use a variable-sized array instead.
This in particular fixes the symptoms git-svn had when storing large numbers of svn-remote.*.added-placeholder entries in the config file.
While the tests are rather more paranoid than just --unset and --replace-all, the other operations already worked. Indeed git-svn's usage only breaks the first time *after* creating so many entries, when it wants to unset and re-add them all.
Reported-by: Jess Hottenstein <jess.hottenstein@gmail.com> Signed-off-by: Thomas Rast <tr@thomasrast.ch> ---
This should fix it, though I haven't actually tested with such a funny use-case, nor written a proper git-svn test for it. My analysis about the failure mode is from briefly looking at the code.
git-svn should probably be changed to use another way of storing these, as git-config is not a very efficient database, but I'm leaving that for someone who cares about SVN. (Note that it's also much harder as it'll need a migration plan.)
config.c | 19 ++++++++++----- t/t1303-wacky-config.sh | 64 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 6 deletions(-)
diff --git a/config.c b/config.c index 3c2434a..ef63bf2 100644 --- a/config.c +++ b/config.c @@ -1197,15 +1197,14 @@ int git_config(config_fn_t fn, void *data) * Find all the stuff for git_config_set() below. */ -#define MAX_MATCHES 512 - static struct { int baselen; char *key; int do_not_match; regex_t *value_regex; int multi_replace; - size_t offset[MAX_MATCHES]; + size_t *offset; + unsigned int offset_alloc; enum { START, SECTION_SEEN, SECTION_END_SEEN, KEY_SEEN } state; int seen; } store; @@ -1228,11 +1227,11 @@ static int store_aux(const char *key, const char *value, void *cb) if (matches(key, value)) { if (store.seen == 1 && store.multi_replace == 0) { warning("%s has multiple values", key); - } else if (store.seen >= MAX_MATCHES) { - error("too many matches for %s", key); - return 1; } + ALLOC_GROW(store.offset, store.seen+1, + store.offset_alloc); + store.offset[store.seen] = cf->do_ftell(cf); store.seen++; } @@ -1260,11 +1259,15 @@ static int store_aux(const char *key, const char *value, void *cb) * Do not increment matches: this is no match, but we * just made sure we are in the desired section. */ + ALLOC_GROW(store.offset, store.seen+1, + store.offset_alloc); store.offset[store.seen] = cf->do_ftell(cf); /* fallthru */ case SECTION_END_SEEN: case START: if (matches(key, value)) { + ALLOC_GROW(store.offset, store.seen+1, + store.offset_alloc); store.offset[store.seen] = cf->do_ftell(cf); store.state = KEY_SEEN; store.seen++; @@ -1272,6 +1275,9 @@ static int store_aux(const char *key, const char *value, void *cb) if (strrchr(key, '.') - key == store.baselen && !strncmp(key, store.key, store.baselen)) { store.state = SECTION_SEEN; + ALLOC_GROW(store.offset, + store.seen+1, + store.offset_alloc); store.offset[store.seen] = cf->do_ftell(cf); } } @@ -1570,6 +1576,7 @@ int git_config_set_multivar_in_file(const char *config_filename, } } + ALLOC_GROW(store.offset, 1, store.offset_alloc); store.offset[0] = 0; store.state = START; store.seen = 0; diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh index 46103a1..7d55730 100755 --- a/t/t1303-wacky-config.sh +++ b/t/t1303-wacky-config.sh @@ -3,17 +3,28 @@ test_description='Test wacky input to git config' . ./test-lib.sh +# Leaving off the newline is intentional! setup() { (printf "[section]\n" && printf " key = foo") >.git/config } +# 'check section.key value' verifies that the entry for section.key is +# 'value' check() { echo "$2" >expected git config --get "$1" >actual 2>&1 test_cmp actual expected } +# 'check section.key regex value' verifies that the entry for +# section.key *that matches 'regex'* is 'value' +check_regex() { + echo "$3" >expected + git config --get "$1" "$2" >actual 2>&1 + test_cmp actual expected +} + test_expect_success 'modify same key' ' setup && git config section.key bar && @@ -47,4 +58,57 @@ test_expect_success 'do not crash on special long config line' ' check section.key "$LONG_VALUE" ' +setup_many() { + setup && + # This time we want the newline so that we can tack on more + # entries. + echo >>.git/config && + # Semi-efficient way of concatenating 5^5 = 3125 lines. Note + # that because 'setup' already put one line, this means 3126 + # entries for section.key in the config file. + cat >5to1 <<EOF + key = foo + key = foo + key = foo + key = foo + key = foo +EOF + cat 5to1 5to1 5to1 5to1 5to1 >5to2 && # 25 + cat 5to2 5to2 5to2 5to2 5to2 >5to3 && # 125 + cat 5to3 5to3 5to3 5to3 5to3 >5to4 && # 635 + cat 5to4 5to4 5to4 5to4 5to4 >>.git/config # 3125 +} + +test_expect_success 'get many entries' ' + setup_many && + git config --get-all section.key >actual && + test_line_count = 3126 actual +' + +test_expect_success 'get many entries by regex' ' + setup_many && + git config --get-regexp "sec.*ke." >actual && + test_line_count = 3126 actual +' + +test_expect_success 'add and replace one of many entries' ' + setup_many && + git config --add section.key bar && + check_regex section.key "b.*r" bar && + git config section.key beer "b.*r" && + check_regex section.key "b.*r" beer +' + +test_expect_success 'replace many entries' ' + setup_many && + git config --replace-all section.key bar && + check section.key bar +' + +test_expect_success 'unset many entries' ' + setup_many && + git config --unset-all section.key && + test_must_fail git config section.key +' + test_done
-- 1.8.5.rc0.346.g150976e
Eric SunshineRe: [PATCH] config: arbitrary number of matches for --unset and --replace-all
On Wed, Nov 13, 2013 at 5:19 AM, Thomas Rast <tr@thomasrast.ch> wrote:
> diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh
> index 46103a1..7d55730 100755
> --- a/t/t1303-wacky-config.sh
> +++ b/t/t1303-wacky-config.sh
> @@ -47,4 +58,57 @@ test_expect_success 'do not crash on special long config line' '
> check section.key "$LONG_VALUE"
> '
>
> +setup_many() {
> + setup &&
> + # This time we want the newline so that we can tack on more
> + # entries.
> + echo >>.git/config &&
> + # Semi-efficient way of concatenating 5^5 = 3125 lines. Note
> + # that because 'setup' already put one line, this means 3126
> + # entries for section.key in the config file.
> + cat >5to1 <<EOFBroken &&-chain.
> + key = foo > + key = foo > + key = foo > + key = foo > + key = foo > +EOF > + cat 5to1 5to1 5to1 5to1 5to1 >5to2 && # 25 > + cat 5to2 5to2 5to2 5to2 5to2 >5to3 && # 125 > + cat 5to3 5to3 5to3 5to3 5to3 >5to4 && # 635 > + cat 5to4 5to4 5to4 5to4 5to4 >>.git/config # 3125 > +} > + > +test_expect_success 'get many entries' ' > + setup_many && > + git config --get-all section.key >actual && > + test_line_count = 3126 actual > +' > + > +test_expect_success 'get many entries by regex' ' > + setup_many && > + git config --get-regexp "sec.*ke." >actual && > + test_line_count = 3126 actual > +' > + > +test_expect_success 'add and replace one of many entries' ' > + setup_many && > + git config --add section.key bar && > + check_regex section.key "b.*r" bar && > + git config section.key beer "b.*r" && > + check_regex section.key "b.*r" beer > +' > + > +test_expect_success 'replace many entries' ' > + setup_many && > + git config --replace-all section.key bar && > + check section.key bar > +' > + > +test_expect_success 'unset many entries' ' > + setup_many && > + git config --unset-all section.key && > + test_must_fail git config section.key > +' > + > test_done > -- > 1.8.5.rc0.346.g150976e
Thomas Rast[PATCH] config: arbitrary number of matches for --unset and --replace-all
git-config used a static match array to hold the matches we want to unset/replace when using --unset or --replace-all. Use a variable-sized array instead.
This in particular fixes the symptoms git-svn had when storing large numbers of svn-remote.*.added-placeholder entries in the config file.
While the tests are rather more paranoid than just --unset and --replace-all, the other operations already worked. Indeed git-svn's usage only breaks the first time *after* creating so many entries, when it wants to unset and re-add them all.
Reported-by: Jess Hottenstein <jess.hottenstein@gmail.com> Signed-off-by: Thomas Rast <tr@thomasrast.ch> ---
Eric Sunshine wrote:
> On Wed, Nov 13, 2013 at 5:19 AM, Thomas Rast <tr@thomasrast.ch> wrote:
> > +setup_many() {[...]
> > + cat >5to1 <<EOF > > Broken &&-chain.
Oops, thanks for catching.
config.c | 19 ++++++++++----- t/t1303-wacky-config.sh | 64 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 6 deletions(-)
diff --git a/config.c b/config.c index 3c2434a..ef63bf2 100644 --- a/config.c +++ b/config.c @@ -1197,15 +1197,14 @@ int git_config(config_fn_t fn, void *data) * Find all the stuff for git_config_set() below. */ -#define MAX_MATCHES 512 - static struct { int baselen; char *key; int do_not_match; regex_t *value_regex; int multi_replace; - size_t offset[MAX_MATCHES]; + size_t *offset; + unsigned int offset_alloc; enum { START, SECTION_SEEN, SECTION_END_SEEN, KEY_SEEN } state; int seen; } store; @@ -1228,11 +1227,11 @@ static int store_aux(const char *key, const char *value, void *cb) if (matches(key, value)) { if (store.seen == 1 && store.multi_replace == 0) { warning("%s has multiple values", key); - } else if (store.seen >= MAX_MATCHES) { - error("too many matches for %s", key); - return 1; } + ALLOC_GROW(store.offset, store.seen+1, + store.offset_alloc); + store.offset[store.seen] = cf->do_ftell(cf); store.seen++; } @@ -1260,11 +1259,15 @@ static int store_aux(const char *key, const char *value, void *cb) * Do not increment matches: this is no match, but we * just made sure we are in the desired section. */ + ALLOC_GROW(store.offset, store.seen+1, + store.offset_alloc); store.offset[store.seen] = cf->do_ftell(cf); /* fallthru */ case SECTION_END_SEEN: case START: if (matches(key, value)) { + ALLOC_GROW(store.offset, store.seen+1, + store.offset_alloc); store.offset[store.seen] = cf->do_ftell(cf); store.state = KEY_SEEN; store.seen++; @@ -1272,6 +1275,9 @@ static int store_aux(const char *key, const char *value, void *cb) if (strrchr(key, '.') - key == store.baselen && !strncmp(key, store.key, store.baselen)) { store.state = SECTION_SEEN; + ALLOC_GROW(store.offset, + store.seen+1, + store.offset_alloc); store.offset[store.seen] = cf->do_ftell(cf); } } @@ -1570,6 +1576,7 @@ int git_config_set_multivar_in_file(const char *config_filename, } } + ALLOC_GROW(store.offset, 1, store.offset_alloc); store.offset[0] = 0; store.state = START; store.seen = 0; diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh index 46103a1..c2706ea 100755 --- a/t/t1303-wacky-config.sh +++ b/t/t1303-wacky-config.sh @@ -3,17 +3,28 @@ test_description='Test wacky input to git config' . ./test-lib.sh +# Leaving off the newline is intentional! setup() { (printf "[section]\n" && printf " key = foo") >.git/config } +# 'check section.key value' verifies that the entry for section.key is +# 'value' check() { echo "$2" >expected git config --get "$1" >actual 2>&1 test_cmp actual expected } +# 'check section.key regex value' verifies that the entry for +# section.key *that matches 'regex'* is 'value' +check_regex() { + echo "$3" >expected + git config --get "$1" "$2" >actual 2>&1 + test_cmp actual expected +} + test_expect_success 'modify same key' ' setup && git config section.key bar && @@ -47,4 +58,57 @@ test_expect_success 'do not crash on special long config line' ' check section.key "$LONG_VALUE" ' +setup_many() { + setup && + # This time we want the newline so that we can tack on more + # entries. + echo >>.git/config && + # Semi-efficient way of concatenating 5^5 = 3125 lines. Note + # that because 'setup' already put one line, this means 3126 + # entries for section.key in the config file. + cat >5to1 <<EOF && + key = foo + key = foo + key = foo + key = foo + key = foo +EOF + cat 5to1 5to1 5to1 5to1 5to1 >5to2 && # 25 + cat 5to2 5to2 5to2 5to2 5to2 >5to3 && # 125 + cat 5to3 5to3 5to3 5to3 5to3 >5to4 && # 635 + cat 5to4 5to4 5to4 5to4 5to4 >>.git/config # 3125 +} + +test_expect_success 'get many entries' ' + setup_many && + git config --get-all section.key >actual && + test_line_count = 3126 actual +' + +test_expect_success 'get many entries by regex' ' + setup_many && + git config --get-regexp "sec.*ke." >actual && + test_line_count = 3126 actual +' + +test_expect_success 'add and replace one of many entries' ' + setup_many && + git config --add section.key bar && + check_regex section.key "b.*r" bar && + git config section.key beer "b.*r" && + check_regex section.key "b.*r" beer +' + +test_expect_success 'replace many entries' ' + setup_many && + git config --replace-all section.key bar && + check section.key bar +' + +test_expect_success 'unset many entries' ' + setup_many && + git config --unset-all section.key && + test_must_fail git config section.key +' + test_done
-- 1.8.5.rc1.353.g06ba152
Jeff KingRe: [PATCH] config: arbitrary number of matches for --unset and --replace-all
On Thu, Nov 14, 2013 at 08:50:43AM +0100, Thomas Rast wrote:
> git-config used a static match array to hold the matches we want to > unset/replace when using --unset or --replace-all. Use a > variable-sized array instead. > > This in particular fixes the symptoms git-svn had when storing large > numbers of svn-remote.*.added-placeholder entries in the config file. > > While the tests are rather more paranoid than just --unset and > --replace-all, the other operations already worked. Indeed git-svn's > usage only breaks the first time *after* creating so many entries, > when it wants to unset and re-add them all. > > Reported-by: Jess Hottenstein <jess.hottenstein@gmail.com> > Signed-off-by: Thomas Rast <tr@thomasrast.ch>
This looks good to me.
I agree with your earlier assessment that adding tons of config keys is probably a bad idea. It's not just slow when looking up those keys, but it also slows down every single git operation (which might read config many times, if it is a script). Still, we should at least do the right thing in the face of such config.
> @@ -1260,11 +1259,15 @@ static int store_aux(const char *key, const char *value, void *cb)
> * Do not increment matches: this is no match, but we
> * just made sure we are in the desired section.
> */
> + ALLOC_GROW(store.offset, store.seen+1,
> + store.offset_alloc);
> store.offset[store.seen] = cf->do_ftell(cf);
> /* fallthru */
> case SECTION_END_SEEN:
> case START:
> if (matches(key, value)) {
> + ALLOC_GROW(store.offset, store.seen+1,
> + store.offset_alloc);
> store.offset[store.seen] = cf->do_ftell(cf);
> store.state = KEY_SEEN;
> store.seen++;This code is weird to follow because of the fall-throughs. I do not think you have introduced any bugs with your patch, but it seems weird to me that we set the offset at the top of the hunk. If we hit the conditional in the bottom half, we do actually increment storer.seen, but only _after_ having overwritten the value from above (with the same value, no less).
But if we do not follow that code path, we may end up here:
> @@ -1272,6 +1275,9 @@ static int store_aux(const char *key, const char *value, void *cb)
> if (strrchr(key, '.') - key == store.baselen &&
> !strncmp(key, store.key, store.baselen)) {
> store.state = SECTION_SEEN;
> + ALLOC_GROW(store.offset,
> + store.seen+1,
> + store.offset_alloc);
> store.offset[store.seen] = cf->do_ftell(cf);
> }
> }where we overwrite it again, but do not update store.seen. Or we may trigger neither, and leave the function with our offset stored, but store.seen not incremented.
So it seems odd to me that we would set the offset, but in some cases never actually increment our counter. AFAICT, we do not ever access those values. The reader limits itself to "i < store.seen", which makes sense. And the writers always call a fresh ftell before incrementing store.seen. But maybe I am missing something.
Anyway, it is neither here nor there for your patch. Just something odd I noticed while reading the code.
> +setup_many() {
> + setup &&
> + # This time we want the newline so that we can tack on more
> + # entries.
> + echo >>.git/config &&
> + # Semi-efficient way of concatenating 5^5 = 3125 lines. Note
> + # that because 'setup' already put one line, this means 3126
> + # entries for section.key in the config file.
> + cat >5to1 <<EOF &&
> + key = foo
> + key = foo
> + key = foo
> + key = foo
> + key = foo
> +EOF
> + cat 5to1 5to1 5to1 5to1 5to1 >5to2 && # 25
> + cat 5to2 5to2 5to2 5to2 5to2 >5to3 && # 125
> + cat 5to3 5to3 5to3 5to3 5to3 >5to4 && # 635
> + cat 5to4 5to4 5to4 5to4 5to4 >>.git/config # 3125
> +}You could make this even more semi-efficient by just storing the end result in 5to5, and then copying it into place rather than doing the whole dance for each test. I doubt it matters that much, though.
-Peff
Thomas RastRe: [PATCH] config: arbitrary number of matches for --unset and --replace-all
Jeff King <peff@peff.net> writes:
> This code is weird to follow because of the fall-throughs. I do not
> think you have introduced any bugs with your patch, but it seems weird
> to me that we set the offset at the top of the hunk. If we hit the
> conditional in the bottom half, we do actually increment storer.seen,
> but only _after_ having overwritten the value from above (with the same
> value, no less).
>
> But if we do not follow that code path, we may end up here:
>
>> @@ -1272,6 +1275,9 @@ static int store_aux(const char *key, const char *value, void *cb)
>> if (strrchr(key, '.') - key == store.baselen &&
>> !strncmp(key, store.key, store.baselen)) {
>> store.state = SECTION_SEEN;
>> + ALLOC_GROW(store.offset,
>> + store.seen+1,
>> + store.offset_alloc);
>> store.offset[store.seen] = cf->do_ftell(cf);
>> }
>> }
>
> where we overwrite it again, but do not update store.seen. Or we may
> trigger neither, and leave the function with our offset stored, but
> store.seen not incremented.It's doubly strange that we write in this hunk without any protection against overflow. I was too lazy to think about it long enough to come up with a possible example that triggers this, and instead just put in the defensive ALLOC_GROW(). But if you can trigger it, it will probably cause the algorithm to go off the rails because it overwrote store.state and possibly even store.seen.
-- Thomas Rast tr@thomasrast.ch
Junio C Hamano[PATCH] fixup! config: arbitrary number of matches for --unset and --replace-all
--- * I'll squash this to tr/config-multivalue-lift-max in preparation for merging it to 'master',which should happen by the end of this week.
Thanks.
config.c | 8 ++++---- t/t1303-wacky-config.sh | 14 +++++++------- 2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/config.c b/config.c index 431dcb7..a1e80da 100644 --- a/config.c +++ b/config.c @@ -1192,7 +1192,7 @@ static int store_aux(const char *key, const char *value, void *cb) warning("%s has multiple values", key); } - ALLOC_GROW(store.offset, store.seen+1, + ALLOC_GROW(store.offset, store.seen + 1, store.offset_alloc); store.offset[store.seen] = cf->do_ftell(cf); @@ -1222,14 +1222,14 @@ static int store_aux(const char *key, const char *value, void *cb) * Do not increment matches: this is no match, but we * just made sure we are in the desired section. */ - ALLOC_GROW(store.offset, store.seen+1, + ALLOC_GROW(store.offset, store.seen + 1, store.offset_alloc); store.offset[store.seen] = cf->do_ftell(cf); /* fallthru */ case SECTION_END_SEEN: case START: if (matches(key, value)) { - ALLOC_GROW(store.offset, store.seen+1, + ALLOC_GROW(store.offset, store.seen + 1, store.offset_alloc); store.offset[store.seen] = cf->do_ftell(cf); store.state = KEY_SEEN; @@ -1239,7 +1239,7 @@ static int store_aux(const char *key, const char *value, void *cb) !strncmp(key, store.key, store.baselen)) { store.state = SECTION_SEEN; ALLOC_GROW(store.offset, - store.seen+1, + store.seen + 1, store.offset_alloc); store.offset[store.seen] = cf->do_ftell(cf); } diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh index 7d55730..3a2c819 100755 --- a/t/t1303-wacky-config.sh +++ b/t/t1303-wacky-config.sh @@ -66,13 +66,13 @@ setup_many() { # Semi-efficient way of concatenating 5^5 = 3125 lines. Note # that because 'setup' already put one line, this means 3126 # entries for section.key in the config file. - cat >5to1 <<EOF - key = foo - key = foo - key = foo - key = foo - key = foo -EOF + cat >5to1 <<-\EOF && + key = foo + key = foo + key = foo + key = foo + key = foo + EOF cat 5to1 5to1 5to1 5to1 5to1 >5to2 && # 25 cat 5to2 5to2 5to2 5to2 5to2 >5to3 && # 125 cat 5to3 5to3 5to3 5to3 5to3 >5to4 && # 635
-- 1.8.5.1-402-gdd8f092
Jeff KingRe: [PATCH] fixup! config: arbitrary number of matches for --unset and --replace-all
On Fri, Dec 06, 2013 at 10:46:42AM -0800, Junio C Hamano wrote:
> --- > * I'll squash this to tr/config-multivalue-lift-max in preparation > for merging it to 'master',which should happen by the end of > this week.
Yeah, all makes sense to me. Thanks.
-Peff