I'm trying to think if Robert's fix is the correct one. We might want to merge it anyway, but I have the feeling this is just layering a workaround on top of a workaround.
We have an issue that ('', '', "TREE_ROOT") is being created by the checkout code. And then when iterating we see TREE_ROOT show up in a subdirectory, and we think there is a problem with that.
At a top level the rough breakdown is:
Dirstate.initialize() always creates a checkout with root id of "TREE_ROOT".
In DirStateWorkingTreeFormat.initialize() we have this code: basis_root_id = basis.get_root_id()
if basis_root_id is not None: wt._set_root_id(basis_root_id) wt.flush()
So *if* the basis tree has a root_id, we force the root_id of the newly created tree to the same root id.
Once _set_root_id has been called, we can see "('', '', 'TREE_ROOT')" in id_index["TREE_ROOT"], even though arguably that entry never actually existed.
And tracing into _set_root_id it calls set_path_id() which has this lovely chunk in it:
# XXX: This was added by Ian, we need to make sure there
# are tests for it, because it isn't in bzr.dev TRUNK
# It looks like the only place it is called is in setting the root
# id of the tree. So probably we never had an _id_index when we
# don't even have a root yet.
if self._id_index is not None: self._add_to_id_index(self._id_index, entry[0])
That XXX leads me to believe that we don't actually want to add the entry to the index. Note that 'set_path_id()' has explicit logic for checking that the path is the root (if len(path): raise NotImplementedError)).
(I'm the one that wrote that XXX, but that was 2 years ago.)
I think the real fix is to just remove that code. I'll try running the test suite with and without it. If nothing actually fails, then I think the bug is that I merged Ian's change without fully vetting the fallout from it.
I'm trying to think if Robert's fix is the correct one. We might want to merge it anyway, but I have the feeling this is just layering a workaround on top of a workaround.
We have an issue that ('', '', "TREE_ROOT") is being created by the checkout code. And then when iterating we see TREE_ROOT show up in a subdirectory, and we think there is a problem with that.
At a top level the rough breakdown is: initialize( ) always creates a checkout with root id of "TREE_ROOT". TreeFormat. initialize( ) we have this code:
basis_ root_id = basis.get_root_id()
wt. _set_root_ id(basis_ root_id)
wt. flush()
Dirstate.
In DirStateWorking
if basis_root_id is not None:
So *if* the basis tree has a root_id, we force the root_id of the newly created tree to the same root id.
Once _set_root_id has been called, we can see "('', '', 'TREE_ROOT')" in id_index[ "TREE_ROOT" ], even though arguably that entry never actually existed.
And tracing into _set_root_id it calls set_path_id() which has this lovely chunk in it:
self. _add_to_ id_index( self._id_ index, entry[0])
# XXX: This was added by Ian, we need to make sure there
# are tests for it, because it isn't in bzr.dev TRUNK
# It looks like the only place it is called is in setting the root
# id of the tree. So probably we never had an _id_index when we
# don't even have a root yet.
if self._id_index is not None:
That XXX leads me to believe that we don't actually want to add the entry to the index. Note that 'set_path_id()' has explicit logic for checking that the path is the root (if len(path): raise NotImplementedE rror)).
(I'm the one that wrote that XXX, but that was 2 years ago.)
I think the real fix is to just remove that code. I'll try running the test suite with and without it. If nothing actually fails, then I think the bug is that I merged Ian's change without fully vetting the fallout from it.