[MERGE][#230567] Faster (local) branch

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Ian Clatworthy wrote:
> I've headed back towards a single code path in the attached patch.

Basically, you're not using _apply_insertions at all if you're building
a tree.  You might as well not call it.

> I've also put things back so that resolve_conflicts is always called
> and fixed the issue that was breaking some tests.

Your changes do call resolve_conflicts, but it's not correct to call
resolve_conflicts and then ignore the results.

resolve_conflicts can change the filename that is used, so you cannot
use the filename from the source tree inventory if that happened.

bb:resubmit

Aaron

> === modified file 'bzrlib/transform.py'
> --- bzrlib/transform.py	2008-05-12 02:48:08 +0000
> +++ bzrlib/transform.py	2008-05-30 07:42:07 +0000
> @@ -856,7 +856,8 @@
>      def _set_executability(self, path, entry, trans_id):
>          """Set the executability of versioned files """
>          new_executability = self._new_executability[trans_id]
> -        entry.executable = new_executability
> +        if entry is not None:
> +            entry.executable = new_executability

^^^ It does not make sense to set the executability of an unversioned file.

> @@ -1194,7 +1197,8 @@
>                  child_pb.update('Apply phase', 0, 2)
>                  self._apply_removals(inventory_delta, mover)
>                  child_pb.update('Apply phase', 1, 2)
> -                modified_paths = self._apply_insertions(inventory_delta, mover)
> +                modified_paths = self._apply_insertions(inventory_delta, mover,
> +                    creation_inv_delta)
>              except:
>                  mover.rollback()
>                  raise
> @@ -1246,13 +1250,21 @@
>          finally:
>              child_pb.finished()
>  
> -    def _apply_insertions(self, inventory_delta, mover):
> +    def _apply_insertions(self, inventory_delta, mover, creation_inv_delta):
>          """Perform tree operations that insert directory/inventory names.
>  
>          That is, create any files that need to be created, and restore from
>          limbo any files that needed renaming.  This must be done in strict
>          parent-to-child order.
> +
> +        :param creation_inv_delta: the precomputed inventory delta when
> +          creating a tree from scratch, otherwise None
>          """
> +        if creation_inv_delta is not None:
> +            self._apply_creation(mover)
> +            inventory_delta.extend(creation_inv_delta)
> +            return [new for old, new, file_id, ie in creation_inv_delta]

It seems strange to pass a precomputed delta into this function, only to
 return the same delta.

> @@ -1310,6 +1322,28 @@
>              del self._new_contents[trans_id]
>          return modified_paths
>  
> +    def _apply_creation(self, mover):
> +        """Perform tree operations to create a tree from scratch."""
> +        # Rename the top level directories in limbo
> +        fp = FinalPaths(self)
> +        for trans_id in self._needs_rename:
> +            path = fp.get_path(trans_id)
> +            try:
> +                full_path = self._tree.abspath(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:
> +                    raise
> +            else:
> +                self.rename_count += 1

^^^ This code appears to be identical to the code in _apply_insertions.
 This duplication should be eliminated.

> +        # Set the execute bits for the required files and clean-up
> +        for trans_id in self._new_executability:
> +            path = fp.get_path(trans_id)
> +            self._set_executability(path, None, trans_id)
> +        self._new_contents = {}

The existing implementation removes items from the dict.  If something
was holding onto the dict itself, it would break.  It would be more
correct to call self._new_contents.clear().  Probably doesn't matter,
but it's just as easy to do, and more consistent.

> @@ -1607,7 +1641,12 @@
>      for num, _unused in enumerate(wt.all_file_ids()):
>          if num > 0:  # more than just a root
>              raise errors.WorkingTreeAlreadyPopulated(base=wt.basedir)
> -    file_trans_id = {}
> +    # If there are no existing files outside of .bzr, we can skip
> +    # checking whether we need to reparent them or not.
> +    existing_files = False
> +    for file in wt.list_files():
> +        existing_files = True
> +        break

^^^ alternatively, you could list all files, store them in a set, and
use that to avoid invoking reparent even when there were some files
already present.

>  
>  
> +def _reparent_required(tree, wt, tt, divert, tree_path, entry):
> +    target_path = wt.abspath(tree_path)
> +    try:
> +        kind = file_kind(target_path)
> +    except NoSuchFile:
> +        pass

^^^ You can just return False here.

> +    else:
> +        file_id = entry.file_id
> +        if kind == "directory":
> +            try:
> +                bzrdir.BzrDir.open(target_path)
> +            except errors.NotBranchError:
> +                pass
> +            else:
> +                divert.add(file_id)
> +        if (file_id not in divert and
> +            _content_match(tree, entry, file_id, kind,
> +            target_path)):
> +            tt.delete_contents(tt.trans_id_tree_path(tree_path))

^^^ From the name _reparent_required I don't think anyone would guess
that the contents would be altered.

> +def _check_conflicts_after_build(tt, wt, divert_trans):
> +    resolver = lambda t, c: resolve_checkout(t, c, divert_trans)
> +    raw_conflicts = resolve_conflicts(tt, pass_func=resolver)
> +    conflicts = cook_conflicts(raw_conflicts, tt)
> +    for conflict in conflicts:
> +        warning(conflict)
> +    try:
> +        wt.add_conflicts(conflicts)
> +    except errors.UnsupportedOperation:
> +        pass

^^^ This doesn't seem very specific to a build.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFIRUCW0F+nu1YWqI0RAg3PAJsERVz5MJnHGBNYU/4tB7qN9qRR2ACfQwh4
EmsIlGHIxl8rF9VkXhY0BzI=
=Wpdu
-----END PGP SIGNATURE-----