Julian Foad wrote on Mon, 1 Feb 2010 at 11:54 -0000:
> On Sun, 2010-01-31, Daniel Shahaf wrote:
> > fs_fs.c:recover_body() needs to be taught that the youngest revision
> > does not necessarily have a revprop file.
...
> > Found by: svnadmin_tests 13, when built with
> > -DSVN_FS_FS_DEFAULT_MAX_FILES_PER_DIR=4 and -DPACK_AFTER_EVERY_COMMIT.
>
> Well found!
>
> To help catch these kinds of bugs, it would be a good idea to make the
> test suite use a low fsfs-sharding number all the time, would it not?
>
Yes, it might help. The C/Python tests found a few bugs in this area in
the past. (Just using low shard size won't test packing, of course.)
It's tricky, though: at least in my experience with packing, which bugs
are triggered depends on the shard size (e.g., the bug in this thread
would only surface with shard sizes that divide 4) and on whether
packing is enabled via the compile-time flag or via the 'make
check'-time option.
> [[[
> Index: subversion/tests/cmdline/svntest/main.py
> ===================================================================
> --- subversion/tests/cmdline/svntest/main.py (revision 905239)
> +++ subversion/tests/cmdline/svntest/main.py (working copy)
> @@ -190,7 +190,7 @@
>
> # Global variable: Number of shards to use in FSFS
> # 'None' means "use FSFS's default"
> -fsfs_sharding = None
> +fsfs_sharding = 2
>
Perhaps choose between several values, rather than fix one.
# :-)
fsfs_sharding = random.choice([None, 1, 2, 3, 4, 5, 6, 7])
> # Global variable: automatically pack FSFS repositories after every commit
> fsfs_packing = None
> ]]]
>
> (Golden rule: if an edge case happens seldom, make it happen often.)
>
> This wouldn't have caught the above bug by itself, of course: that
> requires packing after every commit. It would not be right to make the
> test suite default to packing after ever commit, because then the normal
> (partly packed, partly unpacked) case would not get tested.
>
More precisely, it's the "more than one shard is not packed" case that
won't get tested. (The "one shard isn't packed" case is normal:
whenever HEAD+1 is not a multiple of the shard size, the youngest shard
is incomplete and thus not packed.)
I'd be happy to see packing get more testing, though. Both 'make check'
testing, and also field testing.
> - Julian
>
>
>
Received on 2010-02-01 18:24:20 CET