This should probably be backported to 1.5, but I don't have any more
time to put into a bugfix for nearly-dead untested code in a backend I
never use.
Oh, and by the way, the second bugfix below means that theoretically,
some node-origin calculations could have been made incorrectly and
cached in legacy repositories, which means that theoretically, the
incorrect answer will still be given even after upgrading to a version
with this fix. So theoretically, you might want to worry about coming
up with a way to wipe out old node origin caches.
--dave
On Mon, Dec 1, 2008 at 9:45 AM, <glasser_at_tigris.org> wrote:
> Author: glasser
> Date: Mon Dec 1 09:45:39 2008
> New Revision: 34506
>
> Log:
> Fix a few bugs (one which can lead to a crash and one to an incorrect
> error) in the BDB implementation of node-origin-rev calculation.
>
> These bugs should have been caught by the C tests for
> svn_fs_node_origin_rev, but the code is practically dead code: it's
> essentially only used for legacy repositories, since any node created
> in a 1.5-era repository will immediately have its origin ID stored in
> the node-origins cache. To reveal these bugs, go into
> svn_fs_bdb__get_node_origin, and comment out the "if (db_err ==
> DB_NOTFOUND)" line (ie, make it always be a cache miss).
>
> Then run fs-test 34: you'll get a segfault in the last svn_path_join
> of prev_location. This is because "path" isn't canonicalized ("A/D")
> but "copy_path" is ("/A/D"), so the svn_path_is_child call returns
> NULL for remainder. The first change to base_node_origin_rev fixes
> this. (Though perhaps the correct answer is to change the test to
> always pass absolute paths? It's unclear to me if
> base_node_origin_rev's use of relative paths is actually legal.)
>
> However, you then end up with a "File not found: revision 7, path
> '/A/D2'" error; this is because the loop calling prev_location
> accidentally fails to update the revision that it's looking at with
> the revision returned from prev_location, despite a comment to the
> contrary. The second change to base_node_origin_rev fixes this. Now
> the test happily passes without the cache, and you can remove the hack
> to svn_fs_bdb__get_node_origin.
>
> This change was tested by running "make check FS_TYPE=bdb" with the
> svn_fs_bdb__get_node_origin hack still in. Modifying the C test suite
> so that it manages to test this code without a manual hack is left as
> an exercise to the reader. (OK, that's a lie: after about 8 hours of
> running the test suite on my Mac (with no failures, and fs-test having
> passed) I got bored of waiting and killed it. Then I asked on dev@,
> and people said that bdb tests were just known to be slow on OS X, so
> I decided to try it on my Linux box instead. But after 45 minutes of
> trying to figure out how to build trunk Subversion on a Dapper box
> with SQLite 3.2 installed in /usr/lib and failing, I figured I might
> as well just commit this fix. It's not like anybody noticed the
> segfault bug in the past year. Maybe nobody uses fs_base anymore
> anyway. I don't know why they would.)
>
> * subversion/libsvn_fs_base/tree.c
> (base_node_origin_rev): Canonicalize the input path as an absolute
> path. Actually update a revision variable in a loop instead of
> staying stuck on the revision it started with.
>
> Modified:
> trunk/subversion/libsvn_fs_base/tree.c
>
> Modified: trunk/subversion/libsvn_fs_base/tree.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_fs_base/tree.c?pathrev=34506&r1=34505&r2=34506
> ==============================================================================
> --- trunk/subversion/libsvn_fs_base/tree.c Mon Dec 1 09:15:13 2008 (r34505)
> +++ trunk/subversion/libsvn_fs_base/tree.c Mon Dec 1 09:45:39 2008 (r34506)
> @@ -4795,6 +4795,8 @@ base_node_origin_rev(svn_revnum_t *revis
> SVN_ERR(svn_fs_base__test_required_feature_format
> (fs, "node-origins", SVN_FS_BASE__MIN_NODE_ORIGINS_FORMAT));
>
> + path = svn_fs__canonicalize_abspath(path, pool);
> +
> SVN_ERR(base_node_id(&id, root, path, pool));
> args.node_id = svn_fs_base__id_node_id(id);
> err = svn_fs_base__retry_txn(root->fs, txn_body_get_node_origin,
> @@ -4847,6 +4849,7 @@ base_node_origin_rev(svn_revnum_t *revis
> /* Update our LASTPATH and LASTREV variables (which survive
> SUBPOOL). */
> svn_stringbuf_set(lastpath, curpath);
> + lastrev = currev;
> }
>
> /* Walk the predecessor links back to origin. */
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: svn-unsubscribe_at_subversion.tigris.org
> For additional commands, e-mail: svn-help_at_subversion.tigris.org
>
>
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-12-01 18:49:07 CET