[MERGE] TreeTransform rolls back on exception (#67699)

[Ian Clatworthy]

Aaron Bentley wrote:

> The code does not guarantee deletion in any particular order.
> pending-deletions has no nesting-- all the items are in
> pending-deletions, not in subdirectories.  This means that delete-order
> is not important.  In fact, they are deleted in *forward* order, not
> reverse order.

Of course - good point. I still think you need a test deleting two items
somewhere though.

>>> +    def test_file_mover_rollback(self):
>>> +        self.build_tree(['a/', 'a/b', 'c/', 'c/d/', 'c/e/'])
>>> +        mover = _FileMover()
>>> +        mover.rename('c/d', 'c/f')
>>> +        mover.rename('c/e', 'c/d')
>>> +        try:
>>> +            mover.rename('a', 'c')
>>> +        except OSError, e:
>>> +            mover.rollback()
>>> +        self.failUnlessExists('a')
>>> +        self.failUnlessExists('c/d')
>> Suggestion: test that c/f doesn't exist on completion.
> 
>>> +    def test_rollback_rename(self):
>>> +        tree = self.make_branch_and_tree('.')
>>> +        self.build_tree(['a/', 'a/b'])
> 
>> Suggestion: check c and c/d don't exist post the unsuccessful apply; a
>> and a/b don't exist on completion.
> 
> We shouldn't be testing everything in every test.  They should build on
> one another.  The unit tests for _FileMover should establish what a
> rollback entails.  The purpose of this test is to establish that
> tt.apply will do a rollback.

I don't disagree. The suggestions were just that. When I have my
'tester/reviewer' hat on, I try to look at how things could break and to
confirm that there were no unwanted side effects. With my programmer hat
on, I'm typically (sadly) a little more focussed on just the 'see - it
works' angle. I think your tests are sufficient.

>> Suggestion: I think past_renames and pending_deletions should be cleared
>> on completion of both rollback() and apply_deletions(). Perhaps add a
>> _clear_queues() method that gets called from __init__() and those
>> routines?
> 
> It's supposed to be a run-once operation.  I'd rather set them to None.

That would do. At least it would error then on subsequent attempts after
an apply_deletions or rollback to rename/pre_delete more files/dirs.

Ian C.