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

On 25 September 2013 04:09, <stefan2_at_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