[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: How did this ever work? Re: svn commit: r27751 - in trunk/subversion: libsvn_fs_base libsvn_fs_fs

From: David Glasser <glasser_at_davidglasser.net>
Date: Mon, 1 Dec 2008 10:28:43 -0500

Wow, um, I admit it's been a while since I've ran bdb tests on my Mac, but
they are just ridiculously slow! Even fs-test took minutes to run (way
slower than fsfs), and the python tests are worse. Is this normal?

--dave

On Nov 30, 2008 8:43 PM, "David Glasser" <glasser_at_davidglasser.net> wrote:

Aha, I tracked it down (because of the node origin cache, the node origin
calculation algorithm is basically dead code as far as the test suite is
concerned). I'll commit a fix with copious explanations tonight or tomorrow.

--dave

On Nov 26, 2008 10:35 PM, "David Glasser" <glasser_at_davidglasser.net> wrote:

Hi Mike! Some very-late code review here :) There's a certain line
removal here I'd like to draw our attention to:

On Fri, Nov 9, 2007 at 1:51 PM, <cmpilato_at_tigris.org> wrote:
> Author: cmpilato
> Date: Fri Nov 9 13:51:41 2007
> New Revision: 27751
>
> Log:
> Correct some unconvention subpool usage.
>
> * subversion/libsvn_fs_fs/tree.c
> (fs_node_origin_rev): Wield the subpool per our conventions.
>
> * subversion/libsvn_fs_base/tree.c
> (base_node_origin_rev): Wield the subpool per our conventions.
>
> Found by: dlr
>
> Modified:
> trunk/subversion/libsvn_fs_base/tree.c
> trunk/subversion/libsvn_fs_fs/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=27751&r1=27750&r2=27751
>
==============================================================================
> --- trunk/subversion/libsvn_fs_base/tree.c (original)
> +++ trunk/subversion/libsvn_fs_base/tree.c Fri Nov 9 13:51:41 2007
> @@ -4508,7 +4508,6 @@
> struct get_set_node_origin_args args;
> const svn_fs_id_t *id, *origin_id;
> struct id_created_rev_args icr_args;
> - svn_boolean_t found_copies = FALSE;
>
> SVN_ERR(base_node_id(&id, root, path, pool));
> args.node_id = svn_fs_base__id_node_id(id);
> @@ -4545,8 +4544,14 @@
> svn_revnum_t currev;
> const char *curpath = lastpath->data;
>
> - /* Find the previous location of this object using the
> - closest-copy shortcut. */
> + /* Get a root pointing to LASTREV. (The first time around,
> + LASTREV is invalid, but that's cool because CURROOT is
> + already initialized.) */
> + if (SVN_IS_VALID_REVNUM(lastrev))
> + SVN_ERR(svn_fs_base__revision_root(&curroot, fs,
> + lastrev, subpool));
> +
> + /* Find the previous location using the closest-copy shortcut.
*/
> SVN_ERR(prev_location(&curpath, &currev, fs, curroot,
> curpath, subpool));
> if (! curpath)
> @@ -4554,20 +4559,9 @@
>
> /* Update our LASTPATH and LASTREV variables (which survive
> SUBPOOL). */
> - found_copies = TRUE;
> svn_stringbuf_set(lastpath, curpath);
> - lastrev = currev;

^^^^ this one.

Now look at the current trunk implementation of base_node_origin_rev.
It calls prev_location and gets a curpath and a currev... but currev
is never used!

Thus curroot is *never* changed and is always equal to root. Which
means that if the found lastpath doesn't exist under curroot, there
should be a file-not-found error at the base_node_id line. (And in
fact, when working on Google's fork of libsvn_fs_base, I'm
experiencing that.) This will happen whenever the node we want to
know the origin rev of was the target of a move, right?

The thing that confuses me is that this code works at all. In the
node_origin_rev test in fs-test.c, this exact case seems to be tested
for (with /A/D)!

Any insight? (Otherwise I'll just throw in the one-line patch
restoring "lastrev = currev".)

--dave

> -
> - /* Update our CURROOT from our calculated LASTREV. */
> - svn_pool_clear(subpool);
> - SVN_ERR(svn_fs_base__revision_root(&curroot, fs, lastrev,
subpool));
> }
>
> - /* If we found copies, repoint our CURROOT to the oldest copy
source
> - location. Otherwise, leave it still pointing at ROOT. */
> - if (SVN_IS_VALID_REVNUM(lastrev))
> - SVN_ERR(svn_fs_base__revision_root(&curroot, fs, lastrev, pool));
> -
> /* Walk the predecessor links back to origin. */
> SVN_ERR(base_node_id(&pred_id, curroot, lastpath->data, pool));
> while (1)
>
> Modified: trunk/subversion/libsvn_fs_fs/tree.c
> URL:
http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_fs_fs/tree.c?pathrev=27751&r1=27750&r2=27751
>
==============================================================================
> --- trunk/subversion/libsvn_fs_fs/tree.c (original)
> +++ trunk/subversion/libsvn_fs_fs/tree.c Fri Nov 9 13:51:41 2007
> @@ -2947,8 +2947,15 @@
> svn_revnum_t currev;
> const char *curpath = lastpath->data;
>
> - /* Find the previous location of this object using the
> - closest-copy shortcut. */
> + svn_pool_clear(subpool);
> +
> + /* Get a root pointing to LASTREV. (The first time around,
> + LASTREV is invalid, but that's cool because CURROOT is
> + already initialized.) */
> + if (SVN_IS_VALID_REVNUM(lastrev))
> + SVN_ERR(svn_fs_fs__revision_root(&curroot, fs, lastrev,
subpool));
> +
> + /* Find the previous location using the closest-copy shortcut. */
> SVN_ERR(prev_location(&curpath, &currev, fs, curroot, curpath,
subpool));
> if (! curpath)
> break;
> @@ -2956,17 +2963,8 @@
> /* Update our LASTPATH and LASTREV variables (which survive
SUBPOOL). */
> svn_stringbuf_set(lastpath, curpath);
> lastrev = currev;
> -
> - /* Update our THISROOT from our calculated LASTREV. */
> - svn_pool_clear(subpool);
> - SVN_ERR(svn_fs_fs__revision_root(&curroot, fs, lastrev, subpool));
> }
>
> - /* If we found copies, repoint our CURROOT to the oldest copy source
> - location. Otherwise, leave it still pointing at ROOT. */
> - if (SVN_IS_VALID_REVNUM(lastrev))
> - SVN_ERR(svn_fs_fs__revision_root(&curroot, fs, lastrev, pool));
> -
> /* Walk the predecessor links back to origin. */
> SVN_ERR(fs_node_id(&pred_id, curroot, lastpath->data, predidpool));
> while (pred_id)
>
> ---------------------------------------------------------------------
> 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/
Received on 2008-12-01 16:28:54 CET

This is an archived mail posted to the Subversion Dev mailing list.