[PATCH] Issue 898 atomic rename, implement svn_fs_rename

Hello

While trying to determine what implementing atomic rename would
involve I discovered an interface without an implementation. It
looked fairly straightforward to complete it, but I may well have made
some stupid mistakes. In particular, is it correct to record an add
and a delete in the changes table?

Start issue 898, atomic rename. Implement svn_fs_rename.

* subversion/include/svn_fs.h (svn_fs_rename): Rename parameters to match
  svn_fs_copy.

* subversion/libsvn_fs/tree.c (svn_fs_rename): Implement.

* subversion/tests/libsvn_fs/fs-test.c (rename_test): New test.

Index: subversion/include/svn_fs.h
===================================================================
--- subversion/include/svn_fs.h (revision 4508)
+++ subversion/include/svn_fs.h (working copy)
@@ -1099,16 +1099,16 @@
                                  apr_pool_t *pool);
 
 
-/** Move the node named @a from to @a to, both in @a root.
+/** Move the node named @a from_path to @a to_path, both in @a root.
  *
- * Move the node named @a from to @a to, both in @a root. @a root must be the
- * root of a transaction, not a revision.
+ * Move the node named @a from_path to @a to_path, both in @a root. @a
+ * root must be the root of a transaction, not a revision.
  *
  * Do any necessary temporary allocation in @a pool.
  */
 svn_error_t *svn_fs_rename (svn_fs_root_t *root,
- const char *from,
- const char *to,
+ const char *from_path,
+ const char *to_path,
                             apr_pool_t *pool);
 
 
Index: subversion/libsvn_fs/tree.c
===================================================================
--- subversion/libsvn_fs/tree.c (revision 4508)
+++ subversion/libsvn_fs/tree.c (working copy)
@@ -2639,14 +2639,84 @@
   return svn_fs__retry_txn (root-fs, txn_body_delete, args, pool);
 }
 
+struct rename_args
+{
+ svn_fs_root_t *root;
+ const char *from_path;
+ const char *to_path;
+};
+
+static svn_error_t *
+txn_body_rename (void *baton,
+ trail_t *trail)
+{
+ struct rename_args *args = baton;
+ svn_fs_root_t *root = args-root;
+ const char *from_path = args-from_path;
+ const char *to_path = args-to_path;
+ parent_path_t *from_parent_path, *to_parent_path;
+ svn_fs_path_change_kind_t kind;
+ const char *txn_id = svn_fs_txn_root_name (root, trail-pool);
+ const svn_fs_id_t *from_id;
+
+ /* Sanity checks */
+ if (! svn_fs_is_txn_root (root))
+ return not_txn (root);
+ if (svn_path_is_child (from_path, to_path, trail-pool))
+ return svn_error_createf (SVN_ERR_FS_GENERAL, NULL,
+ cannot move '%s' to '%s', from_path, to_path);
+
+ /* The source must exist */
+ SVN_ERR (open_path (from_parent_path, root, from_path, 0, trail));
+
+ /* The destination is optional, it can be added or replaced */
+ SVN_ERR (open_path (to_parent_path, root, to_path, open_path_last_optional,
+ trail));
+ kind = (to_parent_path-node
+ ? svn_fs_path_change_replace : svn_fs_path_change_add);
+
+ /* Cannot rename the root */
+ if (! from_parent_path-parent || ! to_parent_path-parent)
+ return svn_error_create (SVN_ERR_FS_ROOT_DIR, NULL,
+ the root directory cannot be renamed);
+
+ /* Remove from the source */
+ SVN_ERR (make_path_mutable (root, from_parent_path-parent, from_path,
+ trail));
+ SVN_ERR (svn_fs__dag_delete_tree (from_parent_path-parent-node,
+ from_parent_path-entry,
+ txn_id, trail));
+
+ /* Put in the destination */
+ from_id = svn_fs__dag_get_id (from_parent_path-node);
+ SVN_ERR (make_path_mutable (root, to_parent_path-parent, to_path, trail));
+ SVN_ERR (svn_fs__dag_set_entry (to_parent_path-parent-node,
+ to_parent_path-entry,
+ from_id, txn_id, trail));
+
+ /* Record the changes */
+ SVN_ERR (add_change (svn_fs_root_fs (root), txn_id,
+ to_path, from_id, kind, 0, 0, trail));
+ SVN_ERR (add_change (svn_fs_root_fs (root), txn_id,
+ from_path, from_id,
+ svn_fs_path_change_delete, 0, 0, trail));
+
+ return SVN_NO_ERROR;
+}
 
 svn_error_t *
 svn_fs_rename (svn_fs_root_t *root,
- const char *from,
- const char *to,
+ const char *from_path,
+ const char *to_path,
                apr_pool_t *pool)
 {
- abort ();
+ struct rename_args args;
+
+ args.root = root;
+ args.from_path = from_path;
+ args.to_path = to_path;
+
+ return svn_fs__retry_txn (root-fs, txn_body_rename, args, pool);
 }
 
 
Index: subversion/tests/libsvn_fs/fs-test.c
===================================================================
--- subversion/tests/libsvn_fs/fs-test.c (revision 4508)
+++ subversion/tests/libsvn_fs/fs-test.c (working copy)
@@ -2692,6 +2692,119 @@
 
 
 static svn_error_t *
+rename_test (const char **msg,
+ svn_boolean_t msg_only,
+ apr_pool_t *pool)
+{
+ svn_fs_t *fs;
+ svn_fs_txn_t *txn;
+ svn_fs_root_t *txn_root, *rev_root;
+ svn_revnum_t after_rev;
+ const svn_fs_id_t *id_before, *id_after;
+
+ *msg = rename nodes;
+
+ if (msg_only)
+ return SVN_NO_ERROR;
+
+ /* Prepare a filesystem. */
+ SVN_ERR (svn_test__create_fs (fs, test-repo-rename-test, pool));
+
+ /* First txn, create and commit the greek tree. */
+ SVN_ERR (svn_fs_begin_txn (txn, fs, 0, pool));
+ SVN_ERR (svn_fs_txn_root (txn_root, txn, pool));
+ SVN_ERR (svn_test__create_greek_tree (txn_root, pool));
+ SVN_ERR (test_commit_txn (after_rev, txn, NULL, pool));
+ SVN_ERR (svn_fs_close_txn (txn));
+
+ /* Second txn, rename the file A/D/G/pi to A/D/H/pi2. */
+ SVN_ERR (svn_fs_revision_root (rev_root, fs, after_rev, pool));
+ SVN_ERR (svn_fs_node_id (id_before, rev_root, A/D/G/pi, pool));
+ svn_fs_close_root (rev_root);
+ SVN_ERR (svn_fs_begin_txn (txn, fs, after_rev, pool));
+ SVN_ERR (svn_fs_txn_root (txn_root, txn, pool));
+ SVN_ERR (svn_fs_rename (txn_root, A/D/G/pi, A/D/H/pi2, pool));
+ SVN_ERR (test_commit_txn (after_rev, txn, NULL, pool));
+ SVN_ERR (svn_fs_close_txn (txn));
+ SVN_ERR (svn_fs_revision_root (rev_root, fs, after_rev, pool));
+ SVN_ERR (svn_fs_node_id (id_after, rev_root, A/D/H/pi2, pool));
+ if (svn_fs_compare_ids (id_before, id_after) != 0)
+ return svn_error_create (SVN_ERR_FS_GENERAL, NULL,
+ A/D/G/pi to A/D/H/pi2 changed the node ID);
+ {
+ static svn_test__tree_entry_t expected_entries[] = {
+ /* path, contents (0 = dir) */
+ { iota, This is the file 'iota'.\n },
+ { A, 0 },
+ { A/mu, This is the file 'mu'.\n },
+ { A/B, 0 },
+ { A/B/lambda, This is the file 'lambda'.\n },
+ { A/B/E, 0 },
+ { A/B/E/alpha, This is the file 'alpha'.\n },
+ { A/B/E/beta, This is the file 'beta'.\n },
+ { A/B/F, 0 },
+ { A/C, 0 },
+ { A/D, 0 },
+ { A/D/gamma, This is the file 'gamma'.\n },
+ { A/D/G, 0 },
+ { A/D/G/rho, This is the file 'rho'.\n },
+ { A/D/G/tau, This is the file 'tau'.\n },
+ { A/D/H, 0 },
+ { A/D/H/chi, This is the file 'chi'.\n },
+ { A/D/H/pi2, This is the file 'pi'.\n },
+ { A/D/H/psi, This is the file 'psi'.\n },
+ { A/D/H/omega, This is the file 'omega'.\n }
+ };
+ SVN_ERR (svn_test__validate_tree (rev_root, expected_entries, 20, pool));
+ }
+ svn_fs_close_root (rev_root);
+
+ /* Third txn, rename the directory A/B/E to A/B/F/E. */
+ SVN_ERR (svn_fs_revision_root (rev_root, fs, after_rev, pool));
+ SVN_ERR (svn_fs_node_id (id_before, rev_root, A/B/E, pool));
+ svn_fs_close_root (rev_root);
+ SVN_ERR (svn_fs_begin_txn (txn, fs, after_rev, pool));
+ SVN_ERR (svn_fs_txn_root (txn_root, txn, pool));
+ SVN_ERR (svn_fs_rename (txn_root, A/B/E, A/B/F/E, pool));
+ SVN_ERR (test_commit_txn (after_rev, txn, NULL, pool));
+ SVN_ERR (svn_fs_close_txn (txn));
+ SVN_ERR (svn_fs_revision_root (rev_root, fs, after_rev, pool));
+ SVN_ERR (svn_fs_node_id (id_after, rev_root, A/B/F/E, pool));
+ if (svn_fs_compare_ids (id_before, id_after) != 0)
+ return svn_error_create (SVN_ERR_FS_GENERAL, NULL,
+ A/B/E to A/B/F/E changed the node ID);
+ {
+ static svn_test__tree_entry_t expected_entries[] = {
+ /* path, contents (0 = dir) */
+ { iota, This is the file 'iota'.\n },
+ { A, 0 },
+ { A/mu, This is the file 'mu'.\n },
+ { A/B, 0 },
+ { A/B/lambda, This is the file 'lambda'.\n },
+ { A/B/F, 0 },
+ { A/B/F/E, 0 },
+ { A/B/F/E/alpha, This is the file 'alpha'.\n },
+ { A/B/F/E/beta, This is the file 'beta'.\n },
+ { A/C, 0 },
+ { A/D, 0 },
+ { A/D/gamma, This is the file 'gamma'.\n },
+ { A/D/G, 0 },
+ { A/D/G/rho, This is the file 'rho'.\n },
+ { A/D/G/tau, This is the file 'tau'.\n },
+ { A/D/H, 0 },
+ { A/D/H/chi, This is the file 'chi'.\n },
+ { A/D/H/pi2, This is the file 'pi'.\n },
+ { A/D/H/psi, This is the file 'psi'.\n },
+ { A/D/H/omega, This is the file 'omega'.\n }
+ };
+ SVN_ERR (svn_test__validate_tree (rev_root, expected_entries, 20, pool));
+ }
+ svn_fs_close_root (rev_root);
+
+ return SVN_NO_ERROR;
+}
+
+static svn_error_t *
 link_test (const char **msg,
            svn_boolean_t msg_only,
            apr_pool_t *pool)
@@ -5629,6 +5742,7 @@
     SVN_TEST_PASS (fetch_youngest_rev),
     SVN_TEST_PASS (basic_commit),
     SVN_TEST_PASS (copy_test),
+ SVN_TEST_PASS (rename_test),
     SVN_TEST_XFAIL (link_test),
     SVN_TEST_PASS (merging_commit),
     SVN_TEST_PASS (commit_date),

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org