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

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.