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

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