Code review comment for lp:~amanica/bzr/rm_dir_with_changed_emigrated_file-129880

Revision history for this message
Andrew Bennetts (spiv) wrote :

8 +* ``bzr rm`` should not refuse to delete directories which contained a file
9 + which have been moved elsewhere in the tree after the previous commit.
10 + (Marius Kruger, Daniel Watkins, #129880)
11 +

Grammar nit: should be "which has", not "which have", to match the singular "file".

26 + self.build_tree_contents([
27 + ('somedir/', ),
28 + ('somedir/file', 'contents')])

Style nit: we don't add whitespace to make brackets line up.

60 - elif content_change and (kind[1] is not None):
61 - # Versioned and changed, but not deleted
62 + elif (content_change and (kind[1] is not None) and
63 + osutils.is_inside_any(files, path[1])):

I wonder a little about the performance implications of this. It's probably ok, and correctness comes before performance anyway.

I'm voting "Needs Fixing" just for those two nits. I'll happily upgrade my vote to Approved once you fix those.

review: Needs Fixing

« Back to merge proposal