Merge lp:~amanica/bzr/rm_dir_with_changed_emigrated_file-129880 into lp:bzr

[Marius Kruger]

Don't refuse to delete a directory with changed emigrated files. (Marius Kruger, Daniel Watkins, #129880)

When deleting a dir with a changed file that has been moved out of there;
check if the changed file is still in a dir to be removed before refusing to remove it.
(seems it was my oversight that caused this. sorry)

[Robert Collins (lifeless)]

The NEWS entry is confusing, I don't know what 'it' at the end of the sentence is, nor what you've changed, if I just read NEWS.

Your test should check that the file isn't deleted, given what we're changing.

58 + def isStillInDirToBeRemoved(new_path):
59 + for f in files:
60 + if osutils.is_inside(f, path[1]):
61 + return True
62 + return False

This looks trivially buggy: new_path is not used at all. Perhaps you want is_inside_any(files, path[1]) and no helper function?

Even so, I think this might be buggy; we'd normally just check if the parent_id is still in the inventory after iterating. Uhm, I need to think a bit more to really say either way - can you have a think about whether your curernt code could interact with mv'd directories in unexpected ways?

More broadly, perhaps we should stop erroring in this case, and instead just backup the files, in the same ay we want to for merges that delete directories containing unversioned/modified/ignored files. However, this would be a much larger change and is perhaps worth filing as a wishlist bug independent of this merge proposal.

review: Needs Fixing

[Marius Kruger (amanica)]

thanks for the speedy and thorough review.

On 18 April 2010 23:55, Robert Collins <email address hidden> wrote:
> The NEWS entry is confusing, I don't know what 'it' at the end of the sentence is, nor what you've changed, if I just read NEWS.

I tried to improve that now.

> Your test should check that the file isn't deleted, given what we're changing.

done

>
> 58      + def isStillInDirToBeRemoved(new_path):
> 59      +     for f in files:
> 60      +         if osutils.is_inside(f, path[1]):
> 61      +             return True
> 62      +     return False

> This looks trivially buggy: new_path is not used at all.

ouch, should obviously have been "if osutils.is_inside(f, new_path):"

> Perhaps you want is_inside_any(files, path[1]) and no helper function?

yes, I looked for that but could not find it; if it was a snake I
would have been bitten.

> Even so, I think this might be buggy; we'd normally just check if the parent_id is still in the inventory after iterating.

These checks are done before the inventory is altered.

>Uhm, I need to think a bit more to really say either way - can you have a think about whether your curernt code could interact with mv'd directories in unexpected ways?

I can't think of or produce a way in which child dirs cause problems.
Moving them in or out seems to work as I expect.

> More broadly, perhaps we should stop erroring in this case, and instead just backup the files, in the same ay we want to for merges that delete directories containing unversioned/modified/ignored files.

+1
Maybe back it up like revert would do with .~1~ ?
Should deleted directories be renamed to .~#~ too?
Then I suppose we would need an option --no-backup

> However, this would be a much larger change and is perhaps worth filing as a wishlist bug independent of this merge proposal.

+1

[Marius Kruger (amanica)]

[my comments by mail didn't appear on the mp after 30 minutest, so adding it from the web now. sorry if it gets duplicated]

thanks for the speedy and thorough review.

On 18 April 2010 23:55, Robert Collins <email address hidden> wrote:
> The NEWS entry is confusing, I don't know what 'it' at the end of the sentence is, nor what you've changed, if I just read NEWS.

I tried to improve that now.

> Your test should check that the file isn't deleted, given what we're changing.

done

>
> 58 + def isStillInDirToBeRemoved(new_path):
> 59 + for f in files:
> 60 + if osutils.is_inside(f, path[1]):
> 61 + return True
> 62 + return False

> This looks trivially buggy: new_path is not used at all.

ouch, should obviously have been "if osutils.is_inside(f, new_path):"

> Perhaps you want is_inside_any(files, path[1]) and no helper function?

yes, I looked for that but could not find it; if it was a snake I
would have been bitten.

> Even so, I think this might be buggy; we'd normally just check if the parent_id is still in the inventory after iterating.

These checks are done before the inventory is altered.

>Uhm, I need to think a bit more to really say either way - can you have a think about whether your curernt code could interact with mv'd directories in unexpected ways?

I can't think of or produce a way in which child dirs cause problems.
Moving them in or out seems to work as I expect.

> More broadly, perhaps we should stop erroring in this case, and instead just backup the files, in the same ay we want to for merges that delete directories containing unversioned/modified/ignored files.

+1
Maybe back it up like revert would do with .~1~ ?
Should deleted directories be renamed to .~#~ too?
Then I suppose we would need an option --no-backup

> However, this would be a much larger change and is perhaps worth filing as a wishlist bug independent of this merge proposal.

+1

[Andrew Bennetts (spiv)]

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

[lp:~amanica/bzr/rm_dir_with_changed_emigrated_file-129880]

5168. By Marius Kruger <email address hidden> on 2010-04-30

fix andrew's nits

[Marius Kruger (amanica)]

fixed those nits.

I quickly checked the performance and it does not look to degrade too badly (removing 846 files):

bzr revert ; bzr mv bzrlib/export/ export ; time bzr rm `find bzrlib`
== bzr.dev ==
real 0m0.893s
user 0m0.760s
sys 0m0.110s

real 0m0.914s
user 0m0.800s
sys 0m0.100s

real 0m0.902s
user 0m0.800s
sys 0m0.080s

== bzr.dev with my patch ==
real 0m0.900s
user 0m0.800s
sys 0m0.070s

real 0m0.904s
user 0m0.790s
sys 0m0.090s

real 0m0.947s
user 0m0.790s
sys 0m0.110s

[Andrew Bennetts (spiv)]

Thanks. Upgrading my vote to Approve as promised :)

Apparently your NEWS file has a conflict with trunk, btw.

review: Approve

[Marius Kruger (amanica)]

On 3 May 2010 02:04, Andrew Bennetts <email address hidden> wrote:
> Review: Approve
> Thanks.  Upgrading my vote to Approve as promised :)

thanks

> Apparently your NEWS file has a conflict with trunk, btw.

I checked that, but with the news-merge plugin there is no conflicts.
(I can't push a merge now because I'm firewalled at work - no ssh)

[lp:~amanica/bzr/rm_dir_with_changed_emigrated_file-129880]

5169. By Marius Kruger <email address hidden> on 2010-05-03

merge with bzr.dev

[Gary van der Merwe (garyvdm)]

I'll submit to pqm.

review: Approve

[Gary van der Merwe (garyvdm)]

Sorry. I did not notice the review request for Robert Collins. Not submitted.

[Gary van der Merwe (garyvdm)]

As per IRC dicussion, Robert is happy with this.

=== modified file 'NEWS'
--- NEWS	2010-05-03 04:08:50 +0000
+++ NEWS	2010-05-03 19:53:29 +0000
@@ -43,6 +43,10 @@
   difference).
   (Vincent Ladeuil, #320119)
 
+* ``bzr rm`` should not refuse to delete directories which contained a file
+  which has been moved elsewhere in the tree after the previous commit.
+  (Marius Kruger, Daniel Watkins, #129880)
+
 * ``bzr selftest --parallel=fork`` wait for its children avoiding zombies.
   (Vincent Ladeuil, #566670)
 

=== modified file 'bzrlib/tests/per_workingtree/test_remove.py'
--- bzrlib/tests/per_workingtree/test_remove.py	2009-07-10 07:14:02 +0000
+++ bzrlib/tests/per_workingtree/test_remove.py	2010-05-03 19:53:29 +0000
@@ -278,6 +278,20 @@
         self.assertRemovedAndDeleted(files)
         tree._validate()
 
+    def test_remove_directory_with_changed_emigrated_file(self):
+        # As per bug #129880
+        tree = self.make_branch_and_tree('.')
+        self.build_tree_contents([('somedir/',), ('somedir/file', 'contents')])
+        tree.add(['somedir', 'somedir/file'])
+        tree.commit(message="first")
+        self.build_tree_contents([('somedir/file', 'changed')])
+        tree.rename_one('somedir/file', 'moved-file')
+        tree.remove('somedir', keep_files=False)
+        self.assertNotInWorkingTree('somedir')
+        self.failIfExists('somedir')
+        self.assertInWorkingTree('moved-file')
+        self.failUnlessExists('moved-file')
+
     def test_remove_directory_with_renames(self):
         """Delete directory with renames in or out."""
 

=== modified file 'bzrlib/workingtree.py'
--- bzrlib/workingtree.py	2010-04-30 11:35:43 +0000
+++ bzrlib/workingtree.py	2010-05-03 19:53:29 +0000
@@ -1956,8 +1956,7 @@
         def recurse_directory_to_add_files(directory):
             # Recurse directory and add all files
             # so we can check if they have changed.
-            for parent_info, file_infos in\
-                self.walkdirs(directory):
+            for parent_info, file_infos in self.walkdirs(directory):
                 for relpath, basename, kind, lstat, fileid, kind in file_infos:
                     # Is it versioned or ignored?
                     if self.path2id(relpath) or self.is_ignored(relpath):
@@ -1998,8 +1997,10 @@
                             # ... but not ignored
                             has_changed_files = True
                             break
-                    elif content_change and (kind[1] is not None):
-                        # Versioned and changed, but not deleted
+                    elif (content_change and (kind[1] is not None) and
+                            osutils.is_inside_any(files, path[1])):
+                        # Versioned and changed, but not deleted, and still
+                        # in one of the dirs to be deleted.
                         has_changed_files = True
                         break