You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2013/09/25 02:09:07 UTC

svn commit: r1526057 - in /subversion/trunk/subversion: include/svn_fs.h libsvn_fs_fs/fs.h libsvn_fs_fs/tree.c libsvn_fs_fs/util.c libsvn_fs_fs/util.h

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).

* subversion/libsvn_fs_fs/fs.h
  (SVN_FS_FS__MIN_MOVE_SUPPORT_FORMAT): declare new feature conditional

* subversion/libsvn_fs_fs/util.h
  (svn_fs_fs__supports_move): declare new private API

* subversion/libsvn_fs_fs/util.c
  (svn_fs_fs__item_offset): implement

* subversion/include/svn_fs.h
  (svn_fs_move): remove condition on from_root, it's now implementation
                 dependent just as other copy-related API are

* subversion/libsvn_fs_fs/tree.c
  (is_changed_node): node comparison helper
  (copy_helper): fall back to copy if move is not supported;
                 allow moves only from txn base rev or equivalent node

Modified:
    subversion/trunk/subversion/include/svn_fs.h
    subversion/trunk/subversion/libsvn_fs_fs/fs.h
    subversion/trunk/subversion/libsvn_fs_fs/tree.c
    subversion/trunk/subversion/libsvn_fs_fs/util.c
    subversion/trunk/subversion/libsvn_fs_fs/util.h

Modified: subversion/trunk/subversion/include/svn_fs.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_fs.h?rev=1526057&r1=1526056&r2=1526057&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_fs.h (original)
+++ subversion/trunk/subversion/include/svn_fs.h Wed Sep 25 00:09:06 2013
@@ -1949,9 +1949,8 @@ svn_fs_revision_link(svn_fs_root_t *from
  * The move will remember its source; use svn_fs_copied_from() to
  * access this information.
  *
- * @a from_root must be the root of a revision; @a to_root must be the root
- * of a transaction based on that revision. Further, @a to_root and
- * @a from_root must represent the same filesystem.
+ * @a to_root must be the root of a transaction based on that revision.
+ * Further, @a to_root and @a from_root must represent the same filesystem.
  *
  * Do any necessary temporary allocation in @a pool.
  *

Modified: subversion/trunk/subversion/libsvn_fs_fs/fs.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs.h?rev=1526057&r1=1526056&r2=1526057&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/fs.h (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/fs.h Wed Sep 25 00:09:06 2013
@@ -148,6 +148,9 @@ extern "C" {
 /* The minimum format number that supports packed revprops. */
 #define SVN_FS_FS__MIN_PACKED_REVPROP_FORMAT 6
 
+/* Minimum format number that will record moves */
+#define SVN_FS_FS__MIN_MOVE_SUPPORT_FORMAT 7
+
 /* The minimum format number that supports a configuration file (fsfs.conf) */
 #define SVN_FS_FS__MIN_CONFIG_FILE 4
 

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(&copy_from_root1, &copy_from_path1, root,
+                              path, pool));
+  SVN_ERR(svn_fs_closest_copy(&copy_from_root2, &copy_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;
+}
+
+
 /* 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);
+              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));

Modified: subversion/trunk/subversion/libsvn_fs_fs/util.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/util.c?rev=1526057&r1=1526056&r2=1526057&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/util.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/util.c Wed Sep 25 00:09:06 2013
@@ -639,3 +639,11 @@ svn_fs_fs__item_offset(apr_off_t *absolu
 
   return SVN_NO_ERROR;
 }
+
+svn_boolean_t
+svn_fs_fs__supports_move(svn_fs_t *fs)
+{
+  fs_fs_data_t *ffd = fs->fsap_data;
+
+  return ffd->format >= SVN_FS_FS__MIN_MOVE_SUPPORT_FORMAT;
+}

Modified: subversion/trunk/subversion/libsvn_fs_fs/util.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/util.h?rev=1526057&r1=1526056&r2=1526057&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/util.h (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/util.h Wed Sep 25 00:09:06 2013
@@ -361,4 +361,8 @@ svn_fs_fs__item_offset(apr_off_t *absolu
                        apr_off_t offset,
                        apr_pool_t *pool);
 
+/* Return TRUE if FS's format supports moves to be recorded natively. */
+svn_boolean_t
+svn_fs_fs__supports_move(svn_fs_t *fs);
+
 #endif
\ No newline at end of file



Re: svn commit: r1526057 - in /subversion/trunk/subversion: include/svn_fs.h libsvn_fs_fs/fs.h libsvn_fs_fs/tree.c libsvn_fs_fs/util.c libsvn_fs_fs/util.h

Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Sep 25, 2013 at 10:02 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:

> On 25 September 2013 04:09,  <st...@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(&copy_from_root1, &copy_from_path1, root,
> > +                              path, pool));
> > +  SVN_ERR(svn_fs_closest_copy(&copy_from_root2, &copy_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(&copy_from_root1, &copy_from_path1, root,
>                           path, pool));
>   SVN_ERR(fs_closest_copy(&copy_from_root2, &copy_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.

Re: svn commit: r1526057 - in /subversion/trunk/subversion: include/svn_fs.h libsvn_fs_fs/fs.h libsvn_fs_fs/tree.c libsvn_fs_fs/util.c libsvn_fs_fs/util.h

Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 25 September 2013 04:09,  <st...@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(&copy_from_root1, &copy_from_path1, root,
> +                              path, pool));
> +  SVN_ERR(svn_fs_closest_copy(&copy_from_root2, &copy_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(&copy_from_root1, &copy_from_path1, root,
                          path, pool));
  SVN_ERR(fs_closest_copy(&copy_from_root2, &copy_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.

> +
> +
>  /* 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.

> +              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));
>


-- 
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com