Bazaar

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

  1. rm_dir_with_changed_emigrated_file-129880
  2. Merge into bzr.dev
Proposed by Marius Kruger
Status: Merged
Approved by: Gary van der Merwe
Approved revision: no longer in the source branch.
Merged at revision: 5209
Proposed branch: lp:~amanica/bzr/rm_dir_with_changed_emigrated_file-129880
Merge into: lp:bzr
Diff against target: 65 lines (+23/-4)
3 files modified
NEWS (+4/-0)
bzrlib/tests/per_workingtree/test_remove.py (+14/-0)
bzrlib/workingtree.py (+5/-4)
To merge this branch: bzr merge lp:~amanica/bzr/rm_dir_with_changed_emigrated_file-129880
Reviewer Review Type Date Requested Status
Gary van der Merwe Approve
Andrew Bennetts Approve
Robert Collins (community) Needs Fixing
Review via email: mp+23528@code.launchpad.net

Commit message

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

Description of the change

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)

To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) 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.

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
Revision history for this message
Marius Kruger (amanica) wrote :

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

Revision history for this message
Marius Kruger (amanica) wrote :

[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

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
Revision history for this message
Marius Kruger (amanica) wrote :

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

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

Thanks. Upgrading my vote to Approve as promised :)

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

review: Approve
Revision history for this message
Marius Kruger (amanica) wrote :

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)

Revision history for this message
Gary van der Merwe (garyvdm) wrote :

I'll submit to pqm.

review: Approve
Revision history for this message
Gary van der Merwe (garyvdm) wrote :

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

Revision history for this message
Gary van der Merwe (garyvdm) wrote :

As per IRC dicussion, Robert is happy with this.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2010-05-03 04:08:50 +0000
3+++ NEWS 2010-05-03 19:53:29 +0000
4@@ -43,6 +43,10 @@
5 difference).
6 (Vincent Ladeuil, #320119)
7
8+* ``bzr rm`` should not refuse to delete directories which contained a file
9+ which has been moved elsewhere in the tree after the previous commit.
10+ (Marius Kruger, Daniel Watkins, #129880)
11+
12 * ``bzr selftest --parallel=fork`` wait for its children avoiding zombies.
13 (Vincent Ladeuil, #566670)
14
15
16=== modified file 'bzrlib/tests/per_workingtree/test_remove.py'
17--- bzrlib/tests/per_workingtree/test_remove.py 2009-07-10 07:14:02 +0000
18+++ bzrlib/tests/per_workingtree/test_remove.py 2010-05-03 19:53:29 +0000
19@@ -278,6 +278,20 @@
20 self.assertRemovedAndDeleted(files)
21 tree._validate()
22
23+ def test_remove_directory_with_changed_emigrated_file(self):
24+ # As per bug #129880
25+ tree = self.make_branch_and_tree('.')
26+ self.build_tree_contents([('somedir/',), ('somedir/file', 'contents')])
27+ tree.add(['somedir', 'somedir/file'])
28+ tree.commit(message="first")
29+ self.build_tree_contents([('somedir/file', 'changed')])
30+ tree.rename_one('somedir/file', 'moved-file')
31+ tree.remove('somedir', keep_files=False)
32+ self.assertNotInWorkingTree('somedir')
33+ self.failIfExists('somedir')
34+ self.assertInWorkingTree('moved-file')
35+ self.failUnlessExists('moved-file')
36+
37 def test_remove_directory_with_renames(self):
38 """Delete directory with renames in or out."""
39
40
41=== modified file 'bzrlib/workingtree.py'
42--- bzrlib/workingtree.py 2010-04-30 11:35:43 +0000
43+++ bzrlib/workingtree.py 2010-05-03 19:53:29 +0000
44@@ -1956,8 +1956,7 @@
45 def recurse_directory_to_add_files(directory):
46 # Recurse directory and add all files
47 # so we can check if they have changed.
48- for parent_info, file_infos in\
49- self.walkdirs(directory):
50+ for parent_info, file_infos in self.walkdirs(directory):
51 for relpath, basename, kind, lstat, fileid, kind in file_infos:
52 # Is it versioned or ignored?
53 if self.path2id(relpath) or self.is_ignored(relpath):
54@@ -1998,8 +1997,10 @@
55 # ... but not ignored
56 has_changed_files = True
57 break
58- elif content_change and (kind[1] is not None):
59- # Versioned and changed, but not deleted
60+ elif (content_change and (kind[1] is not None) and
61+ osutils.is_inside_any(files, path[1])):
62+ # Versioned and changed, but not deleted, and still
63+ # in one of the dirs to be deleted.
64 has_changed_files = True
65 break
66