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

[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