Re: [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.hottenst...@gmail.com
Signed-off-by: Thomas Rast t...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] config: arbitrary number of matches for --unset and --replace-all
Jeff King p...@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
t...@thomasrast.ch
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] config: arbitrary number of matches for --unset and --replace-all
On Wed, Nov 13, 2013 at 5:19 AM, Thomas Rast t...@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 EOF
Broken -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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
