[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: Ben Collins-Sussman <sussman_at_red-bean.com>
Date: Mon, 1 Dec 2008 10:38:24 -0600

Bad entropy? Is APR using /dev/random instead of /dev/urandom?

On Mon, Dec 1, 2008 at 9:28 AM, David Glasser <glasser_at_davidglasser.net> wrote:
> 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/
>

---------------------------------------------------------------------
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 17:38:38 CET

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