On Wed, Sep 25, 2013 at 10:02 AM, Ivan Zhakov <i...@visualsvn.com> wrote:
> On 25 September 2013 04:09, <stef...@apache.org> wrote: > > Author: stefan2 > > Date: Wed Sep 25 00:09:06 2013 > > New Revision: 1526057 > > > > URL: http://svn.apache.org/r1526057 > > Log: > > Make native svn_fs_move support in FSFS dependent on a format bump > > (tweak the conditional manually for testing). Fall back to ordinary > > copy-with-history for older format. > > > > Also, relax the condition on svn_fs_move source revision. If a rev > > older than the txn's base revision is specified, there must have been > > no changes at all to that node in meantime and neither the node nor > > nor any of the parents been deleted (and later restored). > > > [...] > Hi, Stefan. See my comment inline. > > > Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c > > URL: > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tree.c?rev=1526057&r1=1526056&r2=1526057&view=diff > > > ============================================================================== > > --- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original) > > +++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Wed Sep 25 00:09:06 > 2013 > > @@ -60,6 +60,7 @@ > > #include "pack.h" > > #include "temp_serializer.h" > > #include "transaction.h" > > +#include "util.h" > > > > #include "private/svn_mergeinfo_private.h" > > #include "private/svn_subr_private.h" > > @@ -2399,6 +2400,55 @@ typedef enum copy_type_t > > copy_type_move > > } copy_type_t; > > > > +/* Set CHANGES to TRUE if PATH in ROOT is unchanged in REVISION if the > > + same files system. If the content is identical, parent path copies > and > > + deletions still count as changes. Use POOL for temporary > allocations. > > + Not that we will return an error if PATH does not exist in ROOT or > > + REVISION- */ > > +static svn_error_t * > > +is_changed_node(svn_boolean_t *changed, > > + svn_fs_root_t *root, > > + const char *path, > > + svn_revnum_t revision, > > + apr_pool_t *pool) > > +{ > > + dag_node_t *node, *rev_node; > > + svn_fs_root_t *rev_root; > > + svn_fs_root_t *copy_from_root1, *copy_from_root2; > > + const char *copy_from_path1, *copy_from_path2; > > + > > + *changed = TRUE; > > + > > + SVN_ERR(svn_fs_fs__revision_root(&rev_root, root->fs, revision, > pool)); > > + > > + /* Get the NODE for FROM_PATH in FROM_ROOT.*/ > > + SVN_ERR(get_dag(&node, root, path, TRUE, pool)); > > + SVN_ERR(get_dag(&rev_node, rev_root, path, TRUE, pool)); > > + > > + /* different ID -> got changed*/ > > + if (!svn_fs_fs__id_eq(svn_fs_fs__dag_get_id(node), > > + svn_fs_fs__dag_get_id(rev_node))) > > + return SVN_NO_ERROR; > > + > > + /* some node. might still be a lazy copy */ > > + SVN_ERR(svn_fs_closest_copy(©_from_root1, ©_from_path1, root, > > + path, pool)); > > + SVN_ERR(svn_fs_closest_copy(©_from_root2, ©_from_path2, > rev_root, > > + path, pool)); > > + > > + if ((copy_from_root1 == NULL) != (copy_from_root2 == NULL)) > > + return SVN_NO_ERROR; > > + > > + if (copy_from_root1) > > + if ( copy_from_root1->rev != copy_from_root2->rev > > + || strcmp(copy_from_path1, copy_from_path2)) > > + return SVN_NO_ERROR; > > + > > + *changed = FALSE; > > + return SVN_NO_ERROR; > > +} > I didn't check that is_changed_node() function doing right thing, but > I suggest to refactor is_changed_node() a bit (see attached patch). > Final code will be: > [[[[ > static svn_error_t * > is_changed_node(svn_boolean_t *changed, > svn_fs_root_t *root, > const char *path, > svn_revnum_t revision, > apr_pool_t *pool) > { > dag_node_t *node, *rev_node; > svn_fs_root_t *rev_root; > svn_fs_root_t *copy_from_root1, *copy_from_root2; > const char *copy_from_path1, *copy_from_path2; > > SVN_ERR(svn_fs_fs__revision_root(&rev_root, root->fs, revision, pool)); > > /* Get the NODE for FROM_PATH in FROM_ROOT.*/ > SVN_ERR(get_dag(&node, root, path, TRUE, pool)); > SVN_ERR(get_dag(&rev_node, rev_root, path, TRUE, pool)); > > /* different ID -> got changed*/ > if (!svn_fs_fs__id_eq(svn_fs_fs__dag_get_id(node), > svn_fs_fs__dag_get_id(rev_node))) > { > *changed = TRUE; > return SVN_NO_ERROR; > } > > /* some node. might still be a lazy copy */ > SVN_ERR(fs_closest_copy(©_from_root1, ©_from_path1, root, > path, pool)); > SVN_ERR(fs_closest_copy(©_from_root2, ©_from_path2, rev_root, > path, pool)); > > if (copy_from_root1 == NULL && copy_from_root2 == NULL) > { > *changed = FALSE; > return SVN_NO_ERROR; > } > else if (copy_from_root1 != NULL && copy_from_root2 != NULL) > { > *changed = (copy_from_root1->rev != copy_from_root2->rev) > || strcmp(copy_from_path1, copy_from_path2); > return SVN_NO_ERROR; > } > else > { > *changed = TRUE; > return SVN_NO_ERROR; > } > } > ]]] > I think it makes behavior more clear. I cannot commit it because there > is no tests for this functionality. Could you please add them. > With some more improvements by me: r1527079. > > + > > + > > /* Copy the node at FROM_PATH under FROM_ROOT to TO_PATH under > > TO_ROOT. COPY_TYPE determines whether then the copy is recorded in > > the copies table and whether it is being marked as a move. > > @@ -2436,10 +2486,33 @@ copy_helper(svn_fs_root_t *from_root, > > (SVN_ERR_UNSUPPORTED_FEATURE, NULL, > > _("Copy immutable tree not supported")); > > > > - if (copy_type == copy_type_move && from_root->rev != txn_id->revision) > > - return svn_error_create > > - (SVN_ERR_UNSUPPORTED_FEATURE, NULL, > > - _("Move from non-HEAD revision not currently supported")); > > + /* move support comes with a number of conditions ... */ > > + if (copy_type == copy_type_move) > > + { > > + /* if we don't copy from the TXN's base rev, check that the path > has > > + not been touched in that revision range */ > > + if (from_root->rev != txn_id->revision) > > + { > > + svn_boolean_t changed = TRUE; > > + svn_error_t *err = is_changed_node(&changed, from_root, > > + from_path, > txn_id->revision, > > + pool); > > + if (err || changed) > > + { > > + svn_error_clear(err); > Converting *any* error to SVN_ERR_FS_OUT_OF_DATE looks wrong for me. > For example SVN_FS_CORRUPTED error returned from is_changed_node() > will be also cleared and converted SVN_ERR_FS_OUT_OF_DATE. I think > this should be changed. > You are right. Fixed in r1527084. > > > + return svn_error_create(SVN_ERR_FS_OUT_OF_DATE, NULL, > > + _("Move-from node is > out-of-date")); > > + } > > + > > + /* always move from the txn's base rev */ > > + SVN_ERR(svn_fs_fs__revision_root(&from_root, from_root->fs, > > + txn_id->revision, pool)); > > + } > > + > > + /* does the FS support moves at all? */ > > + if (!svn_fs_fs__supports_move(to_root->fs)) > > + copy_type = copy_type_add_with_history; > > + } > > > > /* Get the NODE for FROM_PATH in FROM_ROOT.*/ > > SVN_ERR(get_dag(&from_node, from_root, from_path, TRUE, pool)); > > > Thanks for the feedback! -- Stefan^2.
