# 
# 
# patch "roster.hh"
#  from [9ec5d3bbd5751f987a7591a3d577002a627a36e3]
#    to [1a84d8d2c3ef6ee7f97f90b4ff0459a59b9c6c6b]
# 
# patch "roster_merge.cc"
#  from [5b6e35ad2190279e4258cbe93e82fe51c65eb292]
#    to [282aa6d4625b5276c3997aa6aca10faf15f69eea]
# 
============================================================
--- roster.hh	9ec5d3bbd5751f987a7591a3d577002a627a36e3
+++ roster.hh	1a84d8d2c3ef6ee7f97f90b4ff0459a59b9c6c6b
@@ -233,12 +233,52 @@
   void do_deep_copy_from(roster_t const & other);
   dir_t root_dir;
   node_map nodes;
-  // this attribute holds the previous location of detached nodes.  when
-  // applying a cset, we pass through intermediate states where some nodes are
-  // detached from the directory tree.  it is illegal to re-attach these nodes
-  // to where they started -- you shouldn't have detached them in the first
-  // place, if you were just going to put them back!  this checking verifies
-  // that csets are in normalized form.
+  // This requires some explanation.  There is a particular kind of
+  // nonsensical behavior which we wish to discourage -- when a node is
+  // detached from some location, and then re-attached at that same location
+  // (or similarly, if a new node is created, then immediately deleted -- this
+  // is like the previous case, if you think of "does not exist" as a
+  // location).  In particular, we _must_ error out if a cset attempts to do
+  // this, because it indicates that the cset had something non-normalized,
+  // like "rename a a" in it, and that is illegal.  There are two options for
+  // detecting this.  The more natural approach, perhaps, is to keep a chunk
+  // of state around while performing any particular operation (like cset
+  // application) for which we wish to detect these kinds of redundant
+  // computations.  The other option is to keep this state directly within the
+  // roster, at all times.  In the first case, we explicitly turn on checking
+  // when we want it; the the latter, we must explicitly turn _off_ checking
+  // when we _don't_ want it.  We choose the latter, because it is more
+  // conservative --- perhaps it will turn out that it is _too_ conservative
+  // and causes problems, in which case we should probably switch to the
+  // former.
+  // 
+  // FIXME: This _is_ all a little nasty, because this can be a source of
+  // abstraction leak -- for instance, roster_merge's contract is that nodes
+  // involved in name-related will be detached in the roster it returns.
+  // Those nodes really should be allowed to be attached anywhere, or dropped,
+  // which is not actually expressible right now.  Worse, whether or not they
+  // are in old_locations map is an implementation detail of roster_merge --
+  // it may temporarily attach and then detach the nodes it creates, but this
+  // is not deterministic or part of its interface.  The main time this would
+  // be a _problem_ is if we add interactive resolution of tree rearrangement
+  // conflicts -- if someone resolves a rename conflict by saying that one
+  // side wins, or by deleting one of the conflicting nodes, and this all
+  // happens in memory, then it may trigger a spurious invariant failure here.
+  // If anyone ever decides to add this kind of functionality, then it would
+  // definitely make sense to move this checking into editable_tree.  For now,
+  // though, no such functionality is planned, so we'll see what happens.
+  //
+  // The implementation itself uses the map old_locations.  A node can be in
+  // the following states:
+  //   -- attached, no entry in old_locations map
+  //   -- detached, no entry in old_locations map
+  //      -- create_dir_node, create_file_node put a node into this state
+  //      -- a node in this state can be attached, anywhere, but may not be
+  //         deleted.
+  //   -- detached, an entry in old_locations map
+  //      -- detach_node puts a node into this state
+  //      -- a node in this state can be attached anywhere _except_ the
+  //         (parent, basename) entry given in the map, or may be deleted.
   std::map<node_id, std::pair<node_id, path_component> > old_locations;
   template <typename T> friend void dump(T const & val, std::string & out);
 };
============================================================
--- roster_merge.cc	5b6e35ad2190279e4258cbe93e82fe51c65eb292
+++ roster_merge.cc	282aa6d4625b5276c3997aa6aca10faf15f69eea
@@ -1795,8 +1795,11 @@
 
     I(result.missing_root_dir);
 
-    result.roster.attach_node(left_root_nid, split(""));
-    result.roster.attach_node(right_root_nid, split("totally_other_name"));
+    // we can't just attach one of these as the root -- see the massive
+    // comment on the old_locations member of roster_t, in roster.hh.
+    result.roster.attach_node(result.roster.create_dir_node(nis), split(""));
+    result.roster.attach_node(left_root_nid, split("totally_left_name"));
+    result.roster.attach_node(right_root_nid, split("totally_right_name"));
     result.rename_target_conflicts.pop_back();
     result.missing_root_dir = false;
     I(result.is_clean());