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.
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
> eRemoved( new_path) : is_inside( f, path[1]):
> 58 + def isStillInDirToB
> 59 + for f in files:
> 60 + if osutils.
> 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