[MERGE] bzr rm should delete the working file (Bug #82602)

[John Arbash Meinel]

John Arbash Meinel has voted +0.
Status is now: Waiting
Comment:
This has come quite a ways, and I would really like to see it in 0.16. 
If you need some help, just ask, and I can try to do some of the things 
I mention here


I think this should have a NOTES WHEN UPGRADING: section in NEWS. 
Because it means that 'bzr rm' will change how it works.

I know there have been mention of people who are already using 'bzr rm' 
and expecting it to leave the file alone (James Troup mentioned this for 
versioning files in /etc). So we need to make sure to convey this change 
well. (We aren't destroying the data, you can 'bzr revert' or 'bzr rm 
--keep', but people need to know about it).


I don't quite understand this change:
-    only 'added' files will be removed.  If you specify both, then new 
files
-    in the specified directories will be removed.  If the directories 
are
+    only 'added' files will be removed.  If you specify both, then new
+    files in the specified directories will be removed.  If the 
directories are

It seems you re-wrapped the line, only to cause the next line to become 
*too* long.


I'm not sure about making these take multiple paths:
      def failUnlessExists(self, path):
-        """Fail unless path, which may be abs or relative, exists."""
-        self.failUnless(osutils.lexists(path),path+" does not exist")
+        """Fail unless path or paths, which may be abs or relative, 
exist."""
+        if not isinstance(path, basestring):
+            for p in path:
+                self.failUnlessExists(p)
+        else:
+            self.failUnless(osutils.lexists(path),path+" does not 
exist")

I think we generally avoid apis that use 'isinstance'. I'm probably 
about 75% okay on this, though. Since it is meant as a way to make 
writing tests easier, and as long as the tests are easier to write and 
easier to understand, it seems good.


I prefer 'self.run_bzr' to 'self.runbzr', but that is more of a 
guideline than a requirement.

PEP8 says that:
(out,err) = self.runbzr(
should be
(out, err) = self.



You added a test class to test_workingtree.py, but it should probably be 
a test in workingtree_implementations/test_remove.py

+
+class TestRemove(TestCaseWithTransport):
+    """Tests WorkingTree.remove"""

It needs to be written slightly differently, first by inheriting from 
TestCaseWithWorkingTree.

You can look at other 'workingtree_implementations' tests to get a 
reference for how to do it.

Putting it in bzrlib/tests/test_workingtree.py means it will be tested 
with our current default WT format. Putting it in 
workingtree_implementations means it will get tested against all of our 
WT implementations (currently WT2, WT3, WT4). The biggest benefit is 
that it adds tests for any new implementations, to make sure we maintain 
interface consistency.


I don't see why you needed to do this:
-                if not tree.has_id(file_id):
+                if not tree.has_id(file_id) or not file_id in 
tree.inventory:

If you found a case where "tree.has_id(file_id)" was returning True, but 
file_id not in tree.inventory, then you found a bug. And it should be 
fixed at the 'has_id()' layer.

As the name might imply,
+++ bzrlib/tests/blackbox/test_too_much.py

Should not have tests added to it. If you find the need to update things 
in there, it probably means that they need to be split out into one of 
the other blackbox test files. If the test under focus is already 
handled elsewhere, then the test_too_much.py test can just be removed.


Lines like this:
+                        message="deleted %s"%f

should be:
  message = "deleted %s" % f

And I prefer:
  message = "deleted %s" % (f,)

Just in case 'f' isn't what you think it is. (If f is a tuple, the 
former command will fail with a "Not all arguments handled" error, 
rather than just formatting a string with a tuple in it).


changed_files seems like it should be a set() and not a list, though I 
don't see you actually using it.

Also I'm worried about:
+                if osutils.lexists(f):
+                    if force:
+                        # recursively delete f
+                        if osutils.isdir(f):
+                            osutils.rmtree(f)
+                        else:
+                            os.unlink(f)

I didn't see that you checked that files underneath the directory under 
care are not modified.

So if I do "bzr rm dir" it should fail if "dir/subdir/foo" is currently 
modified. We also should have explicit tests for this, to make sure it 
stays correct.

I didn't get a chance to review all of your test cases, to make sure you 
covered all of the possibilities. But I'll try to do that in the next 
review.


For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3C418c22640704120143i28986910l4c66d97f588651c3%40mail.gmail.com%3E