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

[Kuno Meyer]

It's really great news to see a TreeTransform rollback. I played around 
a bit, and I've got the following points:

On 21.08.2007 17:42, Aaron Bentley wrote:

[skip]
> === 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)

Why not complain about an already existing pending-deletion dir? As far 
as I can see, the reasons for complaining are not that different than in 
the case of the already existing limbo dir: if pending-deletion exists, 
then a tree transformation has unexpectedly terminated and name 
conflicts during further operation are very likely to occur.

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

Why not use osutils.rmtree()? It would deal with write-protected files 
which can be moved, but not deleted (on Windows).

>          finally:
>              self._tree.unlock()
>              self._tree = None
> @@ -789,7 +793,7 @@
>              return True
>          return False
>              
> -    def apply(self, no_conflicts=False):
> +    def apply(self, no_conflicts=False, _mover=None):
>          """Apply all changes to the inventory and filesystem.
>          
>          If filesystem or inventory conflicts are present, MalformedTransform
> @@ -799,6 +803,7 @@
>  
>          :param no_conflicts: if True, the caller guarantees there are no
>              conflicts, so no check is made.
> +        :param _mover: Supply an alternate FileMover, for testing
>          """
>          if not no_conflicts:
>              conflicts = self.find_conflicts()
> @@ -808,10 +813,21 @@
>          inventory_delta = []
>          child_pb = bzrlib.ui.ui_factory.nested_progress_bar()
>          try:
> -            child_pb.update('Apply phase', 0, 2)
> -            self._apply_removals(inv, inventory_delta)
> -            child_pb.update('Apply phase', 1, 2)
> -            modified_paths = self._apply_insertions(inv, inventory_delta)
> +            if _mover is None:
> +                mover = _FileMover()
> +            else:
> +                mover = _mover
> +            try:
> +                child_pb.update('Apply phase', 0, 2)
> +                self._apply_removals(inv, inventory_delta, mover)
> +                child_pb.update('Apply phase', 1, 2)
> +                modified_paths = self._apply_insertions(inv, inventory_delta,
> +                                                        mover)
> +            except:
> +                mover.rollback()
> +                raise
> +            else:
> +                mover.apply_deletions()

It would be a big gain in user-friendliness if bazaar would tell the 
user which file causes the conflict, for example by a warning message 
before re-raising exceptions at

- transform.py:898 (in _apply_removals())
- transform.py:939 (in _apply_insertions())

Currently, all what the user receives is a "ERROR: [Errno 13] Permission 
denied".

>          finally:
>              child_pb.finished()
>          self._tree.apply_inventory_delta(inventory_delta)
> @@ -852,7 +868,7 @@
>          self._limbo_files[trans_id] = limbo_name
>          return limbo_name
>  
> -    def _apply_removals(self, inv, inventory_delta):
> +    def _apply_removals(self, inv, inventory_delta, mover):
>          """Perform tree operations that remove directory/inventory names.
>          
>          That is, delete files that are to be deleted, and put any files that
> @@ -868,11 +884,12 @@
>                  child_pb.update('removing file', num, len(tree_paths))
>                  full_path = self._tree.abspath(path)
>                  if trans_id in self._removed_contents:
> -                    delete_any(full_path)
> +                    mover.pre_delete(full_path, os.path.join(self._deletiondir,
> +                                     trans_id))
>                  elif trans_id in self._new_name or trans_id in \
>                      self._new_parent:
>                      try:
> -                        os.rename(full_path, self._limbo_name(trans_id))
> +                        mover.rename(full_path, self._limbo_name(trans_id))
>                      except OSError, e:
>                          if e.errno != errno.ENOENT:
>                              raise
> @@ -888,7 +905,7 @@
>          finally:
>              child_pb.finished()
>  
> -    def _apply_insertions(self, inv, inventory_delta):
> +    def _apply_insertions(self, inv, inventory_delta, mover):
>          """Perform tree operations that insert directory/inventory names.
>          
>          That is, create any files that need to be created, and restore from
> @@ -911,7 +928,7 @@
>                      full_path = self._tree.abspath(path)
>                      if trans_id in self._needs_rename:
>                          try:
> -                            os.rename(self._limbo_name(trans_id), full_path)
> +                            mover.rename(self._limbo_name(trans_id), full_path)
>                          except OSError, e:
>                              # We may be renaming a dangling inventory id
>                              if e.errno != errno.ENOENT:
> @@ -1763,3 +1780,36 @@
>                                     file_id=modified_id, 
>                                     conflict_path=conflicting_path,
>                                     conflict_file_id=conflicting_id)
> +
> +
> +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)