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

[Ian Clatworthy]

Aaron Bentley wrote:
> This patch does not attempt to fix all the problems on Windows, just to
> provide the basic guarantee that if TreeTransform fails, your local
> changes are not lost.
> 
> This patch introduces a TreeTransform helper class called _FileMover.
> Instead of deleting files immediately, _FileMover renames them and marks
> them pending_deletion.  Actual deletions are performed after
> _apply_insertions and _apply_removals have successfully completed.  At
> this point, the tree is in a good state, so any failures on deletion
> will not leave it in a bad state.

bb: tweak

The only change required to merge is being more defensive about the
pending-deletions directory already existing. Some other suggestions
below as well for your consideration ...

> @@ -54,9 +54,9 @@
>          return transform, transform.root
>  
>      def test_existing_limbo(self):
> -        limbo_name = urlutils.local_path_from_url(
> -            self.wt._control_files.controlfilename('limbo'))
>          transform, root = self.get_transform()
> +        limbo_name = transform._limbodir
> +        deletion_path = transform._deletiondir
>          os.mkdir(pathjoin(limbo_name, 'hehe'))
>          self.assertRaises(ImmortalLimbo, transform.apply)
>          self.assertRaises(LockError, self.wt.unlock)
> @@ -64,6 +64,7 @@
>          self.assertRaises(LockError, self.wt.unlock)
>          os.rmdir(pathjoin(limbo_name, 'hehe'))
>          os.rmdir(limbo_name)
> +        os.rmdir(deletion_path)
>          transform, root = self.get_transform()
>          transform.apply()

As Kuno Meyer has suggested previously, I think you ought to be more
defensive about an existing _deletiondir. **it happens. You should test
for an existing _deletiondir here just as you do for _limbodir.

> +class TestFileMover(tests.TestCaseWithTransport):
> +
> +    def test_file_mover(self):
> +        self.build_tree(['a/', 'a/b', 'c/', 'c/d'])
> +        mover = _FileMover()
> +        mover.rename('a', 'q')
> +        self.failUnlessExists('q')
> +        self.failIfExists('a')
> +

Suggestion: test that q/b, c and c/d exist at completion here.

> +    def test_pre_delete_rollback(self):
> +        self.build_tree(['a/'])
> +        mover = _FileMover()
> +        mover.pre_delete('a', 'q')
> +        self.failUnlessExists('q')
> +        self.failIfExists('a')
> +        mover.rollback()
> +        self.failIfExists('q')
> +        self.failUnlessExists('a')
> +
> +    def test_apply_deletions(self):
> +        self.build_tree(['a/'])
> +        mover = _FileMover()
> +        mover.pre_delete('a', 'q')
> +        self.failUnlessExists('q')
> +        self.failIfExists('a')
> +        mover.apply_deletions()
> +        self.failIfExists('q')
> +        self.failIfExists('a')

Suggestion: delete 2 items in one or more of these tests. That would
confirm that more than 1 item is handled correctly, particularly if you
make the deletion order important. (The code does the right thing in
terms of processing deletions in reverse order but the tests don't
exercise that logic.)

> +    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'])
> +        tt = TreeTransform(tree)
> +        self.addCleanup(tt.finalize)
> +        a_id = tt.trans_id_tree_path('a')
> +        tt.adjust_path('c', tt.root, a_id)
> +        tt.adjust_path('d', a_id, tt.trans_id_tree_path('a/b'))
> +        self.assertRaises(Bogus, tt.apply,
> +                          _mover=self.ExceptionFileMover(bad_source='a'))
> +        self.failUnlessExists('a')
> +        self.failUnlessExists('a/b')
> +        tt.apply()
> +        self.failUnlessExists('c')
> +        self.failUnlessExists('c/d')

Suggestion: check c and c/d don't exist post the unsuccessful apply; a
and a/b don't exist on completion.

> +    def test_rollback_rename_into_place(self):
> +        tree = self.make_branch_and_tree('.')
> +        self.build_tree(['a/', 'a/b'])
> +        tt = TreeTransform(tree)
> +        self.addCleanup(tt.finalize)
> +        a_id = tt.trans_id_tree_path('a')
> +        tt.adjust_path('c', tt.root, a_id)
> +        tt.adjust_path('d', a_id, tt.trans_id_tree_path('a/b'))
> +        self.assertRaises(Bogus, tt.apply,
> +                          _mover=self.ExceptionFileMover(bad_target='c/d'))
> +        self.failUnlessExists('a')
> +        self.failUnlessExists('a/b')
> +        tt.apply()
> +        self.failUnlessExists('c')
> +        self.failUnlessExists('c/d')

Suggestion: check c and c/d don't exist post the unsuccessful apply; a
and a/b don't exist on completion.

> +    def test_rollback_deletion(self):
> +        tree = self.make_branch_and_tree('.')
> +        self.build_tree(['a/', 'a/b'])
> +        tt = TreeTransform(tree)
> +        self.addCleanup(tt.finalize)
> +        a_id = tt.trans_id_tree_path('a')
> +        tt.delete_contents(a_id)
> +        tt.adjust_path('d', tt.root, tt.trans_id_tree_path('a/b'))
> +        self.assertRaises(Bogus, tt.apply,
> +                          _mover=self.ExceptionFileMover(bad_target='d'))
> +        self.failUnlessExists('a')
> +        self.failUnlessExists('a/b')

Suggestion: check d doesn't exist on completion.

> === modified file 'bzrlib/transform.py'
> --- bzrlib/transform.py	2007-08-16 05:37:08 +0000
> +++ bzrlib/transform.py	2007-08-21 15:26:59 +0000
> @@ -108,6 +108,9 @@
>              except OSError, e:
>                  if e.errno == errno.EEXIST:
>                      raise ExistingLimbo(self._limbodir)
> +            self._deletiondir = urlutils.local_path_from_url(
> +                control_files.controlfilename('pending-deletion'))
> +            os.mkdir(self._deletiondir)
>          except: 
>              self._tree.unlock()
>              raise

Need to trap unsuccessful mkdir here.

> @@ -170,6 +173,7 @@
>              except OSError:
>                  # We don't especially care *why* the dir is immortal.
>                  raise ImmortalLimbo(self._limbodir)
> +            os.rmdir(self._deletiondir)
>          finally:
>              self._tree.unlock()
>              self._tree = None

Need to trap unsuccessful rmdir here.

> +class _FileMover(object):
> +    """Moves and deletes files for TreeTransform, tracking operations"""
> +
> +    def __init__(self):
> +        self.past_renames = []
> +        self.pending_deletions = []
> +
> +    def rename(self, from_, to):
> +        """Rename a file from one path to another.  Functions like os.rename"""
> +        os.rename(from_, to)
> +        self.past_renames.append((from_, to))
> +
> +    def pre_delete(self, from_, to):
> +        """Rename a file out of the way and mark it for deletion.
> +
> +        Unlike os.unlink, this works equally well for files and directories.
> +        :param from_: The current file path
> +        :param to: A temporary path for the file
> +        """
> +        self.rename(from_, to)
> +        self.pending_deletions.append(to)
> +
> +    def rollback(self):
> +        """Reverse all renames that have been performed"""
> +        for from_, to in reversed(self.past_renames):
> +            os.rename(to, from_)
> +
> +    def apply_deletions(self):
> +        """Apply all marked deletions"""
> +        for path in self.pending_deletions:
> +            delete_any(path)

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 not a big deal but I think that would better reflect the
implicit semantics of this class, i.e. the following sequence shouldn't
rollback the initial changes:

  initial changes ...
  apply_deletions()
  more changes ...
  rollback()

Ian C.