Re: [PATCH] Prevent the modification of the URL [was 1.0.1 veto for r8959]

> This patch guarantees that the parameters of delta_dirs are checked
> *before* open_root is called.

I'm not convinced that drive() is doing anything wrong by calling
open_root before generating an error; it hasn't told the editor to
make any changes yet. Perhaps it would be better if the switch editor
modified the directory URL at close_directory time ("I have made the
necessary changes to this dir to change its location" vs. "I plan to
make the changes to this dir to change its location").

(A potentially larger issue: I'm not sure if an aborted switch
operation can leave directories in a consistent state. I suppose it
might be enough to set the start-empty flag on the directory between
the open-dir and close-dir steps.)

I also feel the patch could be better stylistically; you left the code
with an "if (a) { if (b) c; }" construct, didn't take the opportunity
to remove the braces around a single-line statement, and could have
explained a few things better. Here is my take on the same change.

---
Fix unintended modification of directory entry during "switch".
Patch from <makl@tigris.org>, modified by <ghudson@mit.edu>.
* subversion/libsvn_repos/reporter.c
  (drive): When anchor is target and the source and destination aren't
   both directories, error out before calling open_root.
Index: reporter.c
===================================================================
--- reporter.c	(revision 9078)
+++ reporter.c	(working copy)
@@ -835,19 +835,21 @@
   if (info_is_set_path && !s_entry)
     s_fullpath = NULL;
 
+  /* If the anchor is the operand, the source and target must be dirs.
+     Check this before opening the root to avoid modifying the wc. */
+  if (!*b->s_operand && (!s_entry || s_entry->kind != svn_node_dir
+                         || !t_entry || t_entry->kind != svn_node_dir))
+    return svn_error_create (SVN_ERR_FS_PATH_SYNTAX, NULL,
+                             "Cannot replace a directory from within");
+
   SVN_ERR (b->editor->open_root (b->edit_baton, s_rev, pool, &root_baton));
 
+  /* If the anchor is the operand, diff the two directories; otherwise
+     update the operand within the anchor directory. */
   if (!*b->s_operand)
-    {
-      /* The wc anchor is the operand, so just diff the two directories. */
-      if (!s_entry || s_entry->kind != svn_node_dir
-          || !t_entry || t_entry->kind != svn_node_dir)
-        return svn_error_create (SVN_ERR_FS_PATH_SYNTAX, NULL,
-                                 "Cannot replace a directory from within");
-      SVN_ERR (delta_dirs (b, s_rev, s_fullpath, b->t_path, root_baton,
-                           "", info->start_empty, pool));
-    }
-  else  /* Update the operand within the anchor directory. */
+    SVN_ERR (delta_dirs (b, s_rev, s_fullpath, b->t_path, root_baton,
+                         "", info->start_empty, pool));
+  else
     SVN_ERR (update_entry (b, s_rev, s_fullpath, s_entry, b->t_path,
                            t_entry, root_baton, b->s_operand,
                            (info_is_set_path) ? NULL : info, TRUE, pool));
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org