[MERGE] Fix WorkingTree.remove for tree-references and non-cwd trees

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Aaron Bentley wrote:
> Hi all,
> 
> WorkingTree.remove doesn't work properly with tree references, because
> it uses isdir to determine whether a path is a directory, and then calls
> tree.walkdirs.  That mixes "inventory kind" and "disk kind" inappropriately.
> 
> tree.walkdirs operates in terms of inventory kinds, where possible kinds
> are "file", "symlink", "directory" and "tree-reference".  It's not meant
> to recurse into tree references, and if a tree reference is passed as a
> directory, an AttributeError is raised, because a TreeReference has no
> children.
> 
> Since recursion is always desired, I changed it to use osutils.walkdirs,
> which *always* operates in terms of disk kinds.  This means that tree
> references have the kind "directory", and are recursed into.

It would seem that that would mean you should be opening up the sub-tree
and calling subtree.walkdirs().

Also, unless I miss my guess, using osutils.walkdirs() means that we
will walk into unversioned directories.

> 
> I also fixed it to use absolute paths in the appropriate places, so that
> it works properly when the tree root is not the cwd.

+1. Nothing should depend on cwd if we can help it.

> 
> Finally, I removed all use of Tree.inventory from the implementation,
> because we're trying to make the Inventory object an implementation
> detail of WorkingTree3 objects.
> 
> Even if I agreed that it made sense for WorkingTree.remove to delete
> files, I don't think this is a good implementation.

Where would you recommend removing them at? Especially from a "if I'm
not removing them on disk, then I shouldn't remove them from the
inventory" sense (without --force or --keep).

> 
> - keep_files and force are boolean, but clearly keep_files=True,
>   force=True is nonsensical.  This should be tri-modal, e.g.
>   keep_files= "always", "never", or "auto".

I agree that it should be tri-modal.

> 
> - Instead of taking to_file, it should take a reporter object, so we
>   aren't stuck with a particular output format.

Seems reasonable, though we may want to be aware of getting too many
custom reporters. Passing in 'to_file' at least prevents us from forcing
it to sys.stdout.

In general, I think this is an overall UI thing that needs to be done.
In that we should have a ui object that lets us talk in semantic terms,
and not in how it should be formatted for display terms.

> 
> - BzrRemoveChangedFilesError should not take a delta object as an
>   argument.  It would make more sense to take a list of
>   modified/unversioned files.  Requiring a delta restricts the
>   implementation options.
> 
> - It reports on children of unversioned files.  We never do that, and
>   I'm not convinced we should start.

Agreed.

> 
> - It uses "unknown" where it means "unversioned", e.g. the
>   "unknown_files_in_directory" set.

Well, are you saying "unversioned" as in "they were versioned but now
they are not", or just that they are not versioned files.

I'm pretty sure we use the term 'unknown' for that, with 'ignored' being
a subset of 'unknowns'.

> 
> - It appears to ignore symlinks in some places, and it appears to
>   traverse them in others.

That seems like weird behavior.

> 
> - There are PEP8 issues, e.g. lack of spaces around equal signs.

certainly should be fixed

> 
> - It uses the format '"%s does not exist." % (f,)', when our house style
>   is to omit the tuple, and use f directly.

Actually, I always use the tuple. Especially in any circumstance where
we may not know what the object is. (because if it ends up being a
tuple, then you get a python error, rather than just a slightly
different formatting).

So this may be a code style thing we need to discuss. But I can say for
sure that *I* have taken up always using (var,) for formatting.

> 
> - It reimplements TreeDelta.has_changed
> 
> I spent all afternoon yesterday trying to reimplement it from scratch,
> but the desired behavior specified in the test cases is just too weird
> for it to come easily.

Any particular behavior that seems odd to you? I don't think the
behavior (as defined by the tests) is strictly set in stone. But there
are probably some pieces that we do want to keep.

> 
> So I proffer this bugfix instead.
> 
> Aaron


- --- bzrlib/tests/workingtree_implementations/test_remove.py
+++ bzrlib/tests/workingtree_implementations/test_remove.py
@@ -203,3 +203,18 @@
         tree.remove(TestRemove.b, keep_files=False, force=True)
         self.assertNotInWorkingTree(TestRemove.b_c)
         self.failIfExists(TestRemove.b_c)
+
+    def test_remove_subtree(self):
+        tree = self.make_branch_and_tree('.')
+        subtree = self.make_branch_and_tree('subtree')
+        tree.add('subtree', 'subtree-id')
+        tree.remove('subtree')
+        self.assertIs(None, tree.path2id('subtree'))
+

Do all working tree implementations allow you to add a subtree?
Certainly I agree that adding and removing it should end up with it
unversioned in the final tree, but it seems like some trees should be
complaining about trying to add a subtree.

Other than that, I'm very much +1 on your actual patch. There is just
much to discuss about what you proposed.


Oh, and "recurse_directory_to_add_files()" seems like it should be
including sub-directories into its "to-process" list, and should be
removing unversioned directories from the list that it continues to
walk. (That would prevent us from recursing into unversioned directories).


John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGYEwnJdeBCYSNAAMRAtiwAJ9Bk2XnHkvzV/nQcPijCz/TdRdbEgCgkof+
mPqzQCmijl1DaPnDzqfKBNQ=
=29YR
-----END PGP SIGNATURE-----