Merge lp:~jelmer/brz/move-reconcile-1 into lp:brz

[Jelmer Vernooij]

Move bzr-specific reconcile bits to breezy.bzr.reconcile.

Move bzr-specific reconcile bits to breezy.bzr.reconcile.

[Martin Packman (gz)]

Somewhat rubber-stamping code move, motive seems reasonable.

review: Approve

[The Breezy Bot (the-breezy-bot)]

Running landing tests failed
https://ci.breezy-vcs.org/job/brz/job/brz-land/158/

[The Breezy Bot (the-breezy-bot)]

Running landing tests failed
https://ci.breezy-vcs.org/job/brz/job/brz-land/160/

[The Breezy Bot (the-breezy-bot)]

Running landing tests failed
https://ci.breezy-vcs.org/job/brz/job/brz-land/163/

[The Breezy Bot (the-breezy-bot)]

Running landing tests failed
https://ci.breezy-vcs.org/job/brz/job/brz-land/165/

=== modified file 'breezy/annotate.py'
--- breezy/annotate.py	2018-11-16 18:33:17 +0000
+++ breezy/annotate.py	2019-01-01 23:11:42 +0000
@@ -54,7 +54,7 @@
 
 def annotate_file_tree(tree, path, to_file, verbose=False, full=False,
                        show_ids=False, branch=None):
-    """Annotate file_id in a tree.
+    """Annotate path in a tree.
 
     The tree should already be read_locked() when annotate_file_tree is called.
 

=== modified file 'breezy/branch.py'
--- breezy/branch.py	2018-11-16 18:40:46 +0000
+++ breezy/branch.py	2019-01-01 23:11:42 +0000
@@ -1414,12 +1414,11 @@
         return tree
 
     def reconcile(self, thorough=True):
-        """Make sure the data stored in this branch is consistent."""
-        from breezy.reconcile import BranchReconciler
-        with self.lock_write():
-            reconciler = BranchReconciler(self, thorough=thorough)
-            reconciler.reconcile()
-            return reconciler
+        """Make sure the data stored in this branch is consistent.
+
+        :return: A `ReconcileResult` object.
+        """
+        raise NotImplementedError(self.reconcile)
 
     def reference_parent(self, path, file_id=None, possible_transports=None):
         """Return the parent branch for a tree-reference file_id

=== modified file 'breezy/builtins.py'
--- breezy/builtins.py	2019-01-01 21:53:56 +0000
+++ breezy/builtins.py	2019-01-01 23:11:42 +0000
@@ -5224,17 +5224,15 @@
         tree = _get_one_revision_tree('annotate', revision, branch=branch)
         self.add_cleanup(tree.lock_read().unlock)
         if wt is not None and revision is None:
-            file_id = wt.path2id(relpath)
-        else:
-            file_id = tree.path2id(relpath)
-        if file_id is None:
-            raise errors.NotVersionedError(filename)
-        if wt is not None and revision is None:
+            if not wt.is_versioned(relpath):
+                raise errors.NotVersionedError(relpath)
             # If there is a tree and we're not annotating historical
             # versions, annotate the working tree's content.
             annotate_file_tree(wt, relpath, self.outf, long, all,
                                show_ids=show_ids)
         else:
+            if not tree.is_versioned(relpath):
+                raise errors.NotVersionedError(relpath)
             annotate_file_tree(tree, relpath, self.outf, long, all,
                                show_ids=show_ids, branch=branch)
 

=== modified file 'breezy/bzr/branch.py'
--- breezy/bzr/branch.py	2018-11-11 15:40:12 +0000
+++ breezy/bzr/branch.py	2019-01-01 23:11:42 +0000
@@ -443,6 +443,13 @@
         super(BzrBranch, self)._clear_cached_state()
         self._tags_bytes = None
 
+    def reconcile(self, thorough=True):
+        """Make sure the data stored in this branch is consistent."""
+        from .reconcile import BranchReconciler
+        with self.lock_write():
+            reconciler = BranchReconciler(self, thorough=thorough)
+            return reconciler.reconcile()
+
 
 class BzrBranch8(BzrBranch):
     """A branch that stores tree-reference locations."""

=== modified file 'breezy/bzr/groupcompress_repo.py'
--- breezy/bzr/groupcompress_repo.py	2018-11-11 04:08:32 +0000
+++ breezy/bzr/groupcompress_repo.py	2019-01-01 23:11:42 +0000
@@ -1098,12 +1098,11 @@
         """Reconcile this repository to make sure all CHKs are in canonical
         form.
         """
-        from breezy.reconcile import PackReconciler
+        from .reconcile import PackReconciler
         with self.lock_write():
             reconciler = PackReconciler(
                 self, thorough=True, canonicalize_chks=True)
-            reconciler.reconcile()
-            return reconciler
+            return reconciler.reconcile()
 
     def _reconcile_pack(self, collection, packs, extension, revs, pb):
         packer = GCCHKReconcilePacker(collection, packs, extension)

=== modified file 'breezy/bzr/knitrepo.py'
--- breezy/bzr/knitrepo.py	2018-11-11 04:08:32 +0000
+++ breezy/bzr/knitrepo.py	2019-01-01 23:11:42 +0000
@@ -206,11 +206,10 @@
 
     def reconcile(self, other=None, thorough=False):
         """Reconcile this repository."""
-        from breezy.reconcile import KnitReconciler
+        from .reconcile import KnitReconciler
         with self.lock_write():
             reconciler = KnitReconciler(self, thorough=thorough)
-            reconciler.reconcile()
-            return reconciler
+            return reconciler.reconcile()
 
     def _make_parents_provider(self):
         return _KnitsParentsProvider(self.revisions)

=== modified file 'breezy/bzr/pack_repo.py'
--- breezy/bzr/pack_repo.py	2018-11-12 01:41:38 +0000
+++ breezy/bzr/pack_repo.py	2019-01-01 23:11:42 +0000
@@ -1819,11 +1819,10 @@
 
     def reconcile(self, other=None, thorough=False):
         """Reconcile this repository."""
-        from breezy.reconcile import PackReconciler
+        from .reconcile import PackReconciler
         with self.lock_write():
             reconciler = PackReconciler(self, thorough=thorough)
-            reconciler.reconcile()
-            return reconciler
+            return reconciler.reconcile()
 
     def _reconcile_pack(self, collection, packs, extension, revs, pb):
         raise NotImplementedError(self._reconcile_pack)

=== added file 'breezy/bzr/reconcile.py'
--- breezy/bzr/reconcile.py	1970-01-01 00:00:00 +0000
+++ breezy/bzr/reconcile.py	2019-01-01 23:11:42 +0000
@@ -0,0 +1,471 @@
+# Copyright (C) 2006-2010 Canonical Ltd
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+"""Reconcilers are able to fix some potential data errors in a branch."""
+
+from __future__ import absolute_import
+
+__all__ = [
+    'BranchReconciler',
+    'KnitReconciler',
+    'PackReconciler',
+    'VersionedFileRepoReconciler',
+    ]
+
+from .. import (
+    cleanup,
+    errors,
+    revision as _mod_revision,
+    ui,
+    )
+from ..reconcile import ReconcileResult
+from ..i18n import gettext
+from ..trace import mutter
+from ..tsort import topo_sort
+from .versionedfile import AdapterFactory, FulltextContentFactory
+
+
+class VersionedFileRepoReconciler(object):
+    """Reconciler that reconciles a repository.
+
+    The goal of repository reconciliation is to make any derived data
+    consistent with the core data committed by a user. This can involve
+    reindexing, or removing unreferenced data if that can interfere with
+    queries in a given repository.
+
+    Currently this consists of an inventory reweave with revision cross-checks.
+    """
+
+    def __init__(self, repo, other=None, thorough=False):
+        """Construct a RepoReconciler.
+
+        :param thorough: perform a thorough check which may take longer but
+                         will correct non-data loss issues such as incorrect
+                         cached data.
+        """
+        self.garbage_inventories = 0
+        self.inconsistent_parents = 0
+        self.aborted = False
+        self.repo = repo
+        self.thorough = thorough
+
+    def reconcile(self):
+        """Perform reconciliation.
+
+        After reconciliation the following attributes document found issues:
+
+        * `inconsistent_parents`: The number of revisions in the repository
+          whose ancestry was being reported incorrectly.
+        * `garbage_inventories`: The number of inventory objects without
+          revisions that were garbage collected.
+        """
+        with self.repo.lock_write(), \
+                ui.ui_factory.nested_progress_bar() as self.pb:
+            self._reconcile_steps()
+            ret = ReconcileResult()
+            ret.aborted = self.aborted
+            ret.garbage_inventories = self.garbage_inventories
+            ret.inconsistent_parents = self.inconsistent_parents
+            return ret
+
+    def _reconcile_steps(self):
+        """Perform the steps to reconcile this repository."""
+        self._reweave_inventory()
+
+    def _reweave_inventory(self):
+        """Regenerate the inventory weave for the repository from scratch.
+
+        This is a smart function: it will only do the reweave if doing it
+        will correct data issues. The self.thorough flag controls whether
+        only data-loss causing issues (!self.thorough) or all issues
+        (self.thorough) are treated as requiring the reweave.
+        """
+        transaction = self.repo.get_transaction()
+        self.pb.update(gettext('Reading inventory data'))
+        self.inventory = self.repo.inventories
+        self.revisions = self.repo.revisions
+        # the total set of revisions to process
+        self.pending = {key[-1] for key in self.revisions.keys()}
+
+        # mapping from revision_id to parents
+        self._rev_graph = {}
+        # errors that we detect
+        self.inconsistent_parents = 0
+        # we need the revision id of each revision and its available parents list
+        self._setup_steps(len(self.pending))
+        for rev_id in self.pending:
+            # put a revision into the graph.
+            self._graph_revision(rev_id)
+        self._check_garbage_inventories()
+        # if there are no inconsistent_parents and
+        # (no garbage inventories or we are not doing a thorough check)
+        if (not self.inconsistent_parents
+                and (not self.garbage_inventories or not self.thorough)):
+            ui.ui_factory.note(gettext('Inventory ok.'))
+            return
+        self.pb.update(gettext('Backing up inventory'), 0, 0)
+        self.repo._backup_inventory()
+        ui.ui_factory.note(gettext('Backup inventory created.'))
+        new_inventories = self.repo._temp_inventories()
+
+        # we have topological order of revisions and non ghost parents ready.
+        self._setup_steps(len(self._rev_graph))
+        revision_keys = [(rev_id,) for rev_id in topo_sort(self._rev_graph)]
+        stream = self._change_inv_parents(
+            self.inventory.get_record_stream(revision_keys, 'unordered', True),
+            self._new_inv_parents,
+            set(revision_keys))
+        new_inventories.insert_record_stream(stream)
+        # if this worked, the set of new_inventories.keys should equal
+        # self.pending
+        if not (set(new_inventories.keys())
+                == {(revid,) for revid in self.pending}):
+            raise AssertionError()
+        self.pb.update(gettext('Writing weave'))
+        self.repo._activate_new_inventory()
+        self.inventory = None
+        ui.ui_factory.note(gettext('Inventory regenerated.'))
+
+    def _new_inv_parents(self, revision_key):
+        """Lookup ghost-filtered parents for revision_key."""
+        # Use the filtered ghostless parents list:
+        return tuple([(revid,) for revid in self._rev_graph[revision_key[-1]]])
+
+    def _change_inv_parents(self, stream, get_parents, all_revision_keys):
+        """Adapt a record stream to reconcile the parents."""
+        for record in stream:
+            wanted_parents = get_parents(record.key)
+            if wanted_parents and wanted_parents[0] not in all_revision_keys:
+                # The check for the left most parent only handles knit
+                # compressors, but this code only applies to knit and weave
+                # repositories anyway.
+                bytes = record.get_bytes_as('fulltext')
+                yield FulltextContentFactory(record.key, wanted_parents, record.sha1, bytes)
+            else:
+                adapted_record = AdapterFactory(
+                    record.key, wanted_parents, record)
+                yield adapted_record
+            self._reweave_step('adding inventories')
+
+    def _setup_steps(self, new_total):
+        """Setup the markers we need to control the progress bar."""
+        self.total = new_total
+        self.count = 0
+
+    def _graph_revision(self, rev_id):
+        """Load a revision into the revision graph."""
+        # pick a random revision
+        # analyse revision id rev_id and put it in the stack.
+        self._reweave_step('loading revisions')
+        rev = self.repo.get_revision_reconcile(rev_id)
+        parents = []
+        for parent in rev.parent_ids:
+            if self._parent_is_available(parent):
+                parents.append(parent)
+            else:
+                mutter('found ghost %s', parent)
+        self._rev_graph[rev_id] = parents
+
+    def _check_garbage_inventories(self):
+        """Check for garbage inventories which we cannot trust
+
+        We cant trust them because their pre-requisite file data may not
+        be present - all we know is that their revision was not installed.
+        """
+        if not self.thorough:
+            return
+        inventories = set(self.inventory.keys())
+        revisions = set(self.revisions.keys())
+        garbage = inventories.difference(revisions)
+        self.garbage_inventories = len(garbage)
+        for revision_key in garbage:
+            mutter('Garbage inventory {%s} found.', revision_key[-1])
+
+    def _parent_is_available(self, parent):
+        """True if parent is a fully available revision
+
+        A fully available revision has a inventory and a revision object in the
+        repository.
+        """
+        if parent in self._rev_graph:
+            return True
+        inv_present = (1 == len(self.inventory.get_parent_map([(parent,)])))
+        return (inv_present and self.repo.has_revision(parent))
+
+    def _reweave_step(self, message):
+        """Mark a single step of regeneration complete."""
+        self.pb.update(message, self.count, self.total)
+        self.count += 1
+
+
+class KnitReconciler(VersionedFileRepoReconciler):
+    """Reconciler that reconciles a knit format repository.
+
+    This will detect garbage inventories and remove them in thorough mode.
+    """
+
+    def _reconcile_steps(self):
+        """Perform the steps to reconcile this repository."""
+        if self.thorough:
+            try:
+                self._load_indexes()
+            except errors.BzrCheckError:
+                self.aborted = True
+                return
+            # knits never suffer this
+            self._gc_inventory()
+            self._fix_text_parents()
+
+    def _load_indexes(self):
+        """Load indexes for the reconciliation."""
+        self.transaction = self.repo.get_transaction()
+        self.pb.update(gettext('Reading indexes'), 0, 2)
+        self.inventory = self.repo.inventories
+        self.pb.update(gettext('Reading indexes'), 1, 2)
+        self.repo._check_for_inconsistent_revision_parents()
+        self.revisions = self.repo.revisions
+        self.pb.update(gettext('Reading indexes'), 2, 2)
+
+    def _gc_inventory(self):
+        """Remove inventories that are not referenced from the revision store."""
+        self.pb.update(gettext('Checking unused inventories'), 0, 1)
+        self._check_garbage_inventories()
+        self.pb.update(gettext('Checking unused inventories'), 1, 3)
+        if not self.garbage_inventories:
+            ui.ui_factory.note(gettext('Inventory ok.'))
+            return
+        self.pb.update(gettext('Backing up inventory'), 0, 0)
+        self.repo._backup_inventory()
+        ui.ui_factory.note(gettext('Backup Inventory created'))
+        # asking for '' should never return a non-empty weave
+        new_inventories = self.repo._temp_inventories()
+        # we have topological order of revisions and non ghost parents ready.
+        graph = self.revisions.get_parent_map(self.revisions.keys())
+        revision_keys = topo_sort(graph)
+        revision_ids = [key[-1] for key in revision_keys]
+        self._setup_steps(len(revision_keys))
+        stream = self._change_inv_parents(
+            self.inventory.get_record_stream(revision_keys, 'unordered', True),
+            graph.__getitem__,
+            set(revision_keys))
+        new_inventories.insert_record_stream(stream)
+        # if this worked, the set of new_inventory_vf.names should equal
+        # the revisionds list
+        if not(set(new_inventories.keys()) == set(revision_keys)):
+            raise AssertionError()
+        self.pb.update(gettext('Writing weave'))
+        self.repo._activate_new_inventory()
+        self.inventory = None
+        ui.ui_factory.note(gettext('Inventory regenerated.'))
+
+    def _fix_text_parents(self):
+        """Fix bad versionedfile parent entries.
+
+        It is possible for the parents entry in a versionedfile entry to be
+        inconsistent with the values in the revision and inventory.
+
+        This method finds entries with such inconsistencies, corrects their
+        parent lists, and replaces the versionedfile with a corrected version.
+        """
+        transaction = self.repo.get_transaction()
+        versions = [key[-1] for key in self.revisions.keys()]
+        mutter('Prepopulating revision text cache with %d revisions',
+               len(versions))
+        vf_checker = self.repo._get_versioned_file_checker()
+        bad_parents, unused_versions = vf_checker.check_file_version_parents(
+            self.repo.texts, self.pb)
+        text_index = vf_checker.text_index
+        per_id_bad_parents = {}
+        for key in unused_versions:
+            # Ensure that every file with unused versions gets rewritten.
+            # NB: This is really not needed, reconcile != pack.
+            per_id_bad_parents[key[0]] = {}
+        # Generate per-knit/weave data.
+        for key, details in bad_parents.items():
+            file_id = key[0]
+            rev_id = key[1]
+            knit_parents = tuple([parent[-1] for parent in details[0]])
+            correct_parents = tuple([parent[-1] for parent in details[1]])
+            file_details = per_id_bad_parents.setdefault(file_id, {})
+            file_details[rev_id] = (knit_parents, correct_parents)
+        file_id_versions = {}
+        for text_key in text_index:
+            versions_list = file_id_versions.setdefault(text_key[0], [])
+            versions_list.append(text_key[1])
+        # Do the reconcile of individual weaves.
+        for num, file_id in enumerate(per_id_bad_parents):
+            self.pb.update(gettext('Fixing text parents'), num,
+                           len(per_id_bad_parents))
+            versions_with_bad_parents = per_id_bad_parents[file_id]
+            id_unused_versions = set(key[-1] for key in unused_versions
+                                     if key[0] == file_id)
+            if file_id in file_id_versions:
+                file_versions = file_id_versions[file_id]
+            else:
+                # This id was present in the disk store but is not referenced
+                # by any revision at all.
+                file_versions = []
+            self._fix_text_parent(file_id, versions_with_bad_parents,
+                                  id_unused_versions, file_versions)
+
+    def _fix_text_parent(self, file_id, versions_with_bad_parents,
+                         unused_versions, all_versions):
+        """Fix bad versionedfile entries in a single versioned file."""
+        mutter('fixing text parent: %r (%d versions)', file_id,
+               len(versions_with_bad_parents))
+        mutter('(%d are unused)', len(unused_versions))
+        new_file_id = b'temp:%s' % file_id
+        new_parents = {}
+        needed_keys = set()
+        for version in all_versions:
+            if version in unused_versions:
+                continue
+            elif version in versions_with_bad_parents:
+                parents = versions_with_bad_parents[version][1]
+            else:
+                pmap = self.repo.texts.get_parent_map([(file_id, version)])
+                parents = [key[-1] for key in pmap[(file_id, version)]]
+            new_parents[(new_file_id, version)] = [
+                (new_file_id, parent) for parent in parents]
+            needed_keys.add((file_id, version))
+
+        def fix_parents(stream):
+            for record in stream:
+                bytes = record.get_bytes_as('fulltext')
+                new_key = (new_file_id, record.key[-1])
+                parents = new_parents[new_key]
+                yield FulltextContentFactory(new_key, parents, record.sha1, bytes)
+        stream = self.repo.texts.get_record_stream(
+            needed_keys, 'topological', True)
+        self.repo._remove_file_id(new_file_id)
+        self.repo.texts.insert_record_stream(fix_parents(stream))
+        self.repo._remove_file_id(file_id)
+        if len(new_parents):
+            self.repo._move_file_id(new_file_id, file_id)
+
+
+class PackReconciler(VersionedFileRepoReconciler):
+    """Reconciler that reconciles a pack based repository.
+
+    Garbage inventories do not affect ancestry queries, and removal is
+    considerably more expensive as there is no separate versioned file for
+    them, so they are not cleaned. In short it is currently a no-op.
+
+    In future this may be a good place to hook in annotation cache checking,
+    index recreation etc.
+    """
+
+    # XXX: The index corruption that _fix_text_parents performs is needed for
+    # packs, but not yet implemented. The basic approach is to:
+    #  - lock the names list
+    #  - perform a customised pack() that regenerates data as needed
+    #  - unlock the names list
+    # https://bugs.launchpad.net/bzr/+bug/154173
+
+    def __init__(self, repo, other=None, thorough=False,
+                 canonicalize_chks=False):
+        super(PackReconciler, self).__init__(repo, other=other,
+                                             thorough=thorough)
+        self.canonicalize_chks = canonicalize_chks
+
+    def _reconcile_steps(self):
+        """Perform the steps to reconcile this repository."""
+        if not self.thorough:
+            return
+        collection = self.repo._pack_collection
+        collection.ensure_loaded()
+        collection.lock_names()
+        try:
+            packs = collection.all_packs()
+            all_revisions = self.repo.all_revision_ids()
+            total_inventories = len(list(
+                collection.inventory_index.combined_index.iter_all_entries()))
+            if len(all_revisions):
+                if self.canonicalize_chks:
+                    reconcile_meth = self.repo._canonicalize_chks_pack
+                else:
+                    reconcile_meth = self.repo._reconcile_pack
+                new_pack = reconcile_meth(collection, packs, ".reconcile",
+                                          all_revisions, self.pb)
+                if new_pack is not None:
+                    self._discard_and_save(packs)
+            else:
+                # only make a new pack when there is data to copy.
+                self._discard_and_save(packs)
+            self.garbage_inventories = total_inventories - len(list(
+                collection.inventory_index.combined_index.iter_all_entries()))
+        finally:
+            collection._unlock_names()
+
+    def _discard_and_save(self, packs):
+        """Discard some packs from the repository.
+
+        This removes them from the memory index, saves the in-memory index
+        which makes the newly reconciled pack visible and hides the packs to be
+        discarded, and finally renames the packs being discarded into the
+        obsolete packs directory.
+
+        :param packs: The packs to discard.
+        """
+        for pack in packs:
+            self.repo._pack_collection._remove_pack_from_memory(pack)
+        self.repo._pack_collection._save_pack_names()
+        self.repo._pack_collection._obsolete_packs(packs)
+
+
+class BranchReconciler(object):
+    """Reconciler that works on a branch."""
+
+    def __init__(self, a_branch, thorough=False):
+        self.fixed_history = None
+        self.thorough = thorough
+        self.branch = a_branch
+
+    def reconcile(self):
+        with self.branch.lock_write(), \
+                ui.ui_factory.nested_progress_bar() as self.pb:
+            ret = ReconcileResult()
+            ret.fixed_history = self._reconcile_steps()
+            return ret
+
+    def _reconcile_steps(self):
+        return self._reconcile_revision_history()
+
+    def _reconcile_revision_history(self):
+        last_revno, last_revision_id = self.branch.last_revision_info()
+        real_history = []
+        graph = self.branch.repository.get_graph()
+        try:
+            for revid in graph.iter_lefthand_ancestry(
+                    last_revision_id, (_mod_revision.NULL_REVISION,)):
+                real_history.append(revid)
+        except errors.RevisionNotPresent:
+            pass  # Hit a ghost left hand parent
+        real_history.reverse()
+        if last_revno != len(real_history):
+            # Technically for Branch5 formats, it is more efficient to use
+            # set_revision_history, as this will regenerate it again.
+            # Not really worth a whole BranchReconciler class just for this,
+            # though.
+            ui.ui_factory.note(gettext('Fixing last revision info {0} '
+                                       ' => {1}').format(
+                last_revno, len(real_history)))
+            self.branch.set_last_revision_info(len(real_history),
+                                               last_revision_id)
+            return True
+        else:
+            ui.ui_factory.note(gettext('revision_history ok.'))
+            return False

=== modified file 'breezy/bzr/remote.py'
--- breezy/bzr/remote.py	2019-01-01 21:23:40 +0000
+++ breezy/bzr/remote.py	2019-01-01 23:11:42 +0000
@@ -2474,7 +2474,7 @@
             return self._real_repository._get_inventory_xml(revision_id)
 
     def reconcile(self, other=None, thorough=False):
-        from ..reconcile import RepoReconciler
+        from ..reconcile import ReconcileResult
         with self.lock_write():
             path = self.controldir._path_for_remote_call(self._client)
             try:
@@ -2486,7 +2486,10 @@
             if response != (b'ok', ):
                 raise errors.UnexpectedSmartServerResponse(response)
             body = handler.read_body_bytes()
-            result = RepoReconciler(self)
+            result = ReconcileResult()
+            result.garbage_inventories = None
+            result.inconsistent_parents = None
+            result.aborted = None
             for line in body.split(b'\n'):
                 if not line:
                     continue
@@ -4139,6 +4142,13 @@
         self._ensure_real()
         return self._real_branch.heads_to_fetch()
 
+    def reconcile(self, thorough=True):
+        """Make sure the data stored in this branch is consistent."""
+        from .reconcile import BranchReconciler
+        with self.lock_write():
+            reconciler = BranchReconciler(self, thorough=thorough)
+            return reconciler.reconcile()
+
 
 class RemoteConfig(object):
     """A Config that reads and writes from smart verbs.

=== modified file 'breezy/bzr/vf_repository.py'
--- breezy/bzr/vf_repository.py	2018-11-29 23:42:41 +0000
+++ breezy/bzr/vf_repository.py	2019-01-01 23:11:42 +0000
@@ -294,7 +294,7 @@
             or errored-on before record_iter_changes sees the item.
         :param _entry_factory: Private method to bind entry_factory locally for
             performance.
-        :return: A generator of (file_id, relpath, fs_hash) tuples for use with
+        :return: A generator of (relpath, fs_hash) tuples for use with
             tree._observed_sha1.
         """
         # Create an inventory delta based on deltas between all the parents and
@@ -487,7 +487,7 @@
                     try:
                         entry.text_sha1, entry.text_size = self._add_file_to_weave(
                             file_id, file_obj, heads, nostore_sha)
-                        yield file_id, change[1][1], (entry.text_sha1, stat_value)
+                        yield change[1][1], (entry.text_sha1, stat_value)
                     except errors.ExistingContent:
                         # No content change against a carry_over parent
                         # Perhaps this should also yield a fs hash update?
@@ -1737,6 +1737,13 @@
         """Return a source for streaming from this repository."""
         return StreamSource(self, to_format)
 
+    def reconcile(self, other=None, thorough=False):
+        """Reconcile this repository."""
+        from .reconcile import VersionedFileRepoReconciler
+        with self.lock_write():
+            reconciler = VersionedFileRepoReconciler(self, thorough=thorough)
+            return reconciler.reconcile()
+
 
 class MetaDirVersionedFileRepository(MetaDirRepository,
                                      VersionedFileRepository):

=== modified file 'breezy/bzr/workingtree_4.py'
--- breezy/bzr/workingtree_4.py	2018-11-21 03:20:30 +0000
+++ breezy/bzr/workingtree_4.py	2019-01-01 23:11:42 +0000
@@ -1935,7 +1935,7 @@
         for path, identifier in desired_files:
             entry = self._get_entry(path=path)
             if entry == (None, None):
-                raise errors.NoSuchFile(self, path)
+                raise errors.NoSuchFile(path)
             repo_desired_files.append((entry[0][2], entry[1][parent_index][4],
                                        identifier))
         return self._repository.iter_files_bytes(repo_desired_files)

=== modified file 'breezy/commit.py'
--- breezy/commit.py	2018-11-16 23:15:15 +0000
+++ breezy/commit.py	2019-01-01 23:11:42 +0000
@@ -681,7 +681,7 @@
         if self.exclude:
             iter_changes = filter_excluded(iter_changes, self.exclude)
         iter_changes = self._filter_iter_changes(iter_changes)
-        for file_id, path, fs_hash in self.builder.record_iter_changes(
+        for path, fs_hash in self.builder.record_iter_changes(
                 self.work_tree, self.basis_revid, iter_changes):
             self.work_tree._observed_sha1(path, fs_hash)
 

=== modified file 'breezy/diff.py'
--- breezy/diff.py	2018-11-18 19:48:57 +0000
+++ breezy/diff.py	2019-01-01 23:11:42 +0000
@@ -477,7 +477,7 @@
                     tree.unlock()
 
 
-def _patch_header_date(tree, file_id, path):
+def _patch_header_date(tree, path):
     """Returns a timestamp suitable for use in a patch header."""
     try:
         mtime = tree.get_file_mtime(path)
@@ -526,10 +526,9 @@
                      diff_tree.to_file, diff_tree.path_encoding)
 
     @staticmethod
-    def _diff_many(differs, file_id, old_path, new_path, old_kind, new_kind):
+    def _diff_many(differs, old_path, new_path, old_kind, new_kind):
         for file_differ in differs:
-            result = file_differ.diff(file_id, old_path, new_path, old_kind,
-                                      new_kind)
+            result = file_differ.diff(old_path, new_path, old_kind, new_kind)
             if result is not DiffPath.CANNOT_DIFF:
                 return result
         else:
@@ -553,10 +552,9 @@
     def from_diff_tree(klass, diff_tree):
         return klass(diff_tree.differs)
 
-    def diff(self, file_id, old_path, new_path, old_kind, new_kind):
+    def diff(self, old_path, new_path, old_kind, new_kind):
         """Perform comparison
 
-        :param file_id: The file_id of the file to compare
         :param old_path: Path of the file in the old tree
         :param new_path: Path of the file in the new tree
         :param old_kind: Old file-kind of the file
@@ -564,17 +562,17 @@
         """
         if None in (old_kind, new_kind):
             return DiffPath.CANNOT_DIFF
-        result = DiffPath._diff_many(self.differs, file_id, old_path,
-                                     new_path, old_kind, None)
+        result = DiffPath._diff_many(
+            self.differs, old_path, new_path, old_kind, None)
         if result is DiffPath.CANNOT_DIFF:
             return result
-        return DiffPath._diff_many(self.differs, file_id, old_path, new_path,
-                                   None, new_kind)
+        return DiffPath._diff_many(
+            self.differs, old_path, new_path, None, new_kind)
 
 
 class DiffDirectory(DiffPath):
 
-    def diff(self, file_id, old_path, new_path, old_kind, new_kind):
+    def diff(self, old_path, new_path, old_kind, new_kind):
         """Perform comparison between two directories.  (dummy)
 
         """
@@ -589,10 +587,9 @@
 
 class DiffSymlink(DiffPath):
 
-    def diff(self, file_id, old_path, new_path, old_kind, new_kind):
+    def diff(self, old_path, new_path, old_kind, new_kind):
         """Perform comparison between two symlinks
 
-        :param file_id: The file_id of the file to compare
         :param old_path: Path of the file in the old tree
         :param new_path: Path of the file in the new tree
         :param old_kind: Old file-kind of the file
@@ -644,10 +641,9 @@
         self.path_encoding = path_encoding
         self.context_lines = context_lines
 
-    def diff(self, file_id, old_path, new_path, old_kind, new_kind):
+    def diff(self, old_path, new_path, old_kind, new_kind):
         """Compare two files in unified diff format
 
-        :param file_id: The file_id of the file to compare
         :param old_path: Path of the file in the old tree
         :param new_path: Path of the file in the new tree
         :param old_kind: Old file-kind of the file
@@ -655,30 +651,25 @@
         """
         if 'file' not in (old_kind, new_kind):
             return self.CANNOT_DIFF
-        from_file_id = to_file_id = file_id
         if old_kind == 'file':
-            old_date = _patch_header_date(self.old_tree, file_id, old_path)
+            old_date = _patch_header_date(self.old_tree, old_path)
         elif old_kind is None:
             old_date = self.EPOCH_DATE
-            from_file_id = None
         else:
             return self.CANNOT_DIFF
         if new_kind == 'file':
-            new_date = _patch_header_date(self.new_tree, file_id, new_path)
+            new_date = _patch_header_date(self.new_tree, new_path)
         elif new_kind is None:
             new_date = self.EPOCH_DATE
-            to_file_id = None
         else:
             return self.CANNOT_DIFF
         from_label = '%s%s\t%s' % (self.old_label, old_path,
                                    old_date)
         to_label = '%s%s\t%s' % (self.new_label, new_path,
                                  new_date)
-        return self.diff_text(old_path, new_path, from_label, to_label,
-                              from_file_id, to_file_id)
+        return self.diff_text(old_path, new_path, from_label, to_label)
 
-    def diff_text(self, from_path, to_path, from_label, to_label,
-                  from_file_id=None, to_file_id=None):
+    def diff_text(self, from_path, to_path, from_label, to_label):
         """Diff the content of given files in two trees
 
         :param from_path: The path in the from tree. If None,
@@ -686,18 +677,17 @@
         :param to_path: The path in the to tree. This may refer
             to a different file from from_path.  If None,
             the file is not present in the to tree.
-        :param from_file_id: The id of the file in the from tree or None if
-            unknown.
-        :param to_file_id: The id of the file in the to tree or None if
-            unknown.
         """
-        def _get_text(tree, file_id, path):
-            if file_id is None:
-                return []
-            return tree.get_file_lines(path)
+        def _get_text(tree, path):
+            if path is None:
+                return []
+            try:
+                return tree.get_file_lines(path)
+            except errors.NoSuchFile:
+                return []
         try:
-            from_text = _get_text(self.old_tree, from_file_id, from_path)
-            to_text = _get_text(self.new_tree, to_file_id, to_path)
+            from_text = _get_text(self.old_tree, from_path)
+            to_text = _get_text(self.new_tree, to_path)
             self.text_differ(from_label, from_text, to_label, to_text,
                              self.to_file, path_encoding=self.path_encoding,
                              context_lines=self.context_lines)
@@ -804,7 +794,7 @@
         return osutils.pathjoin(self._root, prefix, relpath_tmp)
 
     def _write_file(self, relpath, tree, prefix, force_temp=False,
-                    allow_write=False, file_id=None):
+                    allow_write=False):
         if not force_temp and isinstance(tree, WorkingTree):
             full_path = tree.abspath(relpath)
             if self._is_safepath(full_path):
@@ -836,12 +826,12 @@
         return full_path
 
     def _prepare_files(self, old_path, new_path, force_temp=False,
-                       allow_write_new=False, file_id=None):
-        old_disk_path = self._write_file(old_path, self.old_tree, 'old',
-                                         force_temp, file_id=file_id)
-        new_disk_path = self._write_file(new_path, self.new_tree, 'new',
-                                         force_temp, file_id=file_id,
-                                         allow_write=allow_write_new)
+                       allow_write_new=False):
+        old_disk_path = self._write_file(
+            old_path, self.old_tree, 'old', force_temp)
+        new_disk_path = self._write_file(
+            new_path, self.new_tree, 'new', force_temp,
+            allow_write=allow_write_new)
         return old_disk_path, new_disk_path
 
     def finish(self):
@@ -852,25 +842,23 @@
                 mutter("The temporary directory \"%s\" was not "
                        "cleanly removed: %s." % (self._root, e))
 
-    def diff(self, file_id, old_path, new_path, old_kind, new_kind):
+    def diff(self, old_path, new_path, old_kind, new_kind):
         if (old_kind, new_kind) != ('file', 'file'):
             return DiffPath.CANNOT_DIFF
         (old_disk_path, new_disk_path) = self._prepare_files(
-            old_path, new_path, file_id=file_id)
+            old_path, new_path)
         self._execute(old_disk_path, new_disk_path)
 
-    def edit_file(self, old_path, new_path, file_id=None):
+    def edit_file(self, old_path, new_path):
         """Use this tool to edit a file.
 
         A temporary copy will be edited, and the new contents will be
         returned.
 
-        :param file_id: The id of the file to edit.
         :return: The new contents of the file.
         """
         old_abs_path, new_abs_path = self._prepare_files(
-            old_path, new_path, allow_write_new=True, force_temp=True,
-            file_id=file_id)
+            old_path, new_path, allow_write_new=True, force_temp=True)
         command = self._get_command(old_abs_path, new_abs_path)
         subprocess.call(command, cwd=self._root)
         with open(new_abs_path, 'rb') as new_file:
@@ -1028,16 +1016,15 @@
                 self.to_file.write(b"=== modified %s '%s'%s\n" % (kind[0].encode('ascii'),
                                                                   newpath_encoded, prop_str))
             if changed_content:
-                self._diff(oldpath, newpath, kind[0], kind[1], file_id=file_id)
+                self._diff(oldpath, newpath, kind[0], kind[1])
                 has_changes = 1
             if renamed:
                 has_changes = 1
         return has_changes
 
-    def diff(self, file_id, old_path, new_path):
+    def diff(self, old_path, new_path):
         """Perform a diff of a single file
 
-        :param file_id: file-id of the file
         :param old_path: The path of the file in the old tree
         :param new_path: The path of the file in the new tree
         """
@@ -1049,11 +1036,11 @@
             new_kind = None
         else:
             new_kind = self.new_tree.kind(new_path)
-        self._diff(old_path, new_path, old_kind, new_kind, file_id=file_id)
+        self._diff(old_path, new_path, old_kind, new_kind)
 
-    def _diff(self, old_path, new_path, old_kind, new_kind, file_id):
-        result = DiffPath._diff_many(self.differs, file_id, old_path,
-                                     new_path, old_kind, new_kind)
+    def _diff(self, old_path, new_path, old_kind, new_kind):
+        result = DiffPath._diff_many(
+            self.differs, old_path, new_path, old_kind, new_kind)
         if result is DiffPath.CANNOT_DIFF:
             error_path = new_path
             if error_path is None:

=== modified file 'breezy/git/branch.py'
--- breezy/git/branch.py	2018-11-16 23:15:15 +0000
+++ breezy/git/branch.py	2019-01-01 23:11:42 +0000
@@ -615,6 +615,12 @@
             self, stop_revision=revid, lossy=lossy, _stop_revno=revno)
         return (push_result.new_revno, push_result.new_revid)
 
+    def reconcile(self, thorough=True):
+        """Make sure the data stored in this branch is consistent."""
+        from ..reconcile import ReconcileResult
+        # Nothing to do here
+        return ReconcileResult()
+
 
 class LocalGitBranch(GitBranch):
     """A local Git branch."""

=== modified file 'breezy/git/commit.py'
--- breezy/git/commit.py	2018-11-22 03:04:59 +0000
+++ breezy/git/commit.py	2019-01-01 23:11:42 +0000
@@ -142,7 +142,7 @@
             encoded_new_path = path[1].encode("utf-8")
             self._blobs[encoded_new_path] = (mode, sha)
             if st is not None:
-                yield file_id, path[1], (entry.text_sha1, st)
+                yield path[1], (entry.text_sha1, st)
             if self._mapping.generate_file_id(encoded_new_path) != file_id:
                 self._override_fileids[encoded_new_path] = file_id
             else:

=== modified file 'breezy/git/repository.py'
--- breezy/git/repository.py	2018-11-11 14:23:06 +0000
+++ breezy/git/repository.py	2019-01-01 23:11:42 +0000
@@ -68,31 +68,6 @@
     )
 
 
-class RepoReconciler(object):
-    """Reconciler that reconciles a repository.
-
-    """
-
-    def __init__(self, repo, other=None, thorough=False):
-        """Construct a RepoReconciler.
-
-        :param thorough: perform a thorough check which may take longer but
-                         will correct non-data loss issues such as incorrect
-                         cached data.
-        """
-        self.repo = repo
-
-    def reconcile(self):
-        """Perform reconciliation.
-
-        After reconciliation the following attributes document found issues:
-        inconsistent_parents: The number of revisions in the repository whose
-                              ancestry was being reported incorrectly.
-        garbage_inventories: The number of inventory objects without revisions
-                             that were garbage collected.
-        """
-
-
 class GitCheck(check.Check):
 
     def __init__(self, repository, check_repo=True):
@@ -241,9 +216,10 @@
 
     def reconcile(self, other=None, thorough=False):
         """Reconcile this repository."""
-        reconciler = RepoReconciler(self, thorough=thorough)
-        reconciler.reconcile()
-        return reconciler
+        from ..reconcile import ReconcileResult
+        ret = ReconcileResult()
+        ret.aborted = False
+        return ret
 
     def supports_rich_root(self):
         return True

=== modified file 'breezy/git/tests/test_blackbox.py'
--- breezy/git/tests/test_blackbox.py	2018-12-18 19:51:52 +0000
+++ breezy/git/tests/test_blackbox.py	2019-01-01 23:11:42 +0000
@@ -395,6 +395,21 @@
         self.assertEqual(error, '')
 
 
+class ReconcileTests(ExternalBase):
+
+    def test_simple_reconcile(self):
+        tree = self.make_branch_and_tree('.', format='git')
+        self.build_tree_contents([('a', 'text for a\n')])
+        tree.add(['a'])
+        output, error = self.run_bzr('reconcile')
+        self.assertContainsRe(
+            output,
+            'Reconciling branch file://.*\n'
+            'Reconciling repository file://.*\n'
+            'Reconciliation complete.\n')
+        self.assertEqual(error, '')
+
+
 class StatusTests(ExternalBase):
 
     def test_empty_dir(self):

=== modified file 'breezy/git/tree.py'
--- breezy/git/tree.py	2018-12-11 17:29:18 +0000
+++ breezy/git/tree.py	2019-01-01 23:11:42 +0000
@@ -995,7 +995,7 @@
             raise errors.NoSuchId(self, file_id)
 
     def _set_root_id(self, file_id):
-        self._fileid_map.set_file_id("", file_id)
+        raise errors.UnsupportedOperation(self._set_root_id, self)
 
     def get_root_id(self):
         return self.path2id(u"")

=== modified file 'breezy/plugins/fastimport/revision_store.py'
--- breezy/plugins/fastimport/revision_store.py	2018-11-16 18:33:17 +0000
+++ breezy/plugins/fastimport/revision_store.py	2019-01-01 23:11:42 +0000
@@ -367,7 +367,7 @@
             basis_rev_id = _mod_revision.NULL_REVISION
         tree = _TreeShim(self.repo, basis_inv, inv_delta, text_provider)
         changes = tree._delta_to_iter_changes()
-        for (file_id, path, fs_hash) in builder.record_iter_changes(
+        for (path, fs_hash) in builder.record_iter_changes(
                 tree, basis_rev_id, changes):
             # So far, we don't *do* anything with the result
             pass

=== modified file 'breezy/reconcile.py'
--- breezy/reconcile.py	2018-11-11 04:08:32 +0000
+++ breezy/reconcile.py	2019-01-01 23:11:42 +0000
@@ -19,23 +19,17 @@
 from __future__ import absolute_import
 
 __all__ = [
-    'KnitReconciler',
-    'PackReconciler',
     'reconcile',
     'Reconciler',
-    'RepoReconciler',
     ]
 
 
 from . import (
     cleanup,
     errors,
-    revision as _mod_revision,
     ui,
     )
 from .trace import mutter
-from .tsort import topo_sort
-from .bzr.versionedfile import AdapterFactory, FulltextContentFactory
 from .i18n import gettext
 
 
@@ -52,7 +46,11 @@
     :param canonicalize_chks: Make sure CHKs are in canonical form.
     """
     reconciler = Reconciler(dir, canonicalize_chks=canonicalize_chks)
-    reconciler.reconcile()
+    return reconciler.reconcile()
+
+
+class ReconcileResult(object):
+    """Class describing the result of a reconcile operation."""
 
 
 class Reconciler(object):
@@ -65,38 +63,29 @@
 
     def reconcile(self):
         """Perform reconciliation.
-
-        After reconciliation the following attributes document found issues:
-
-        * `inconsistent_parents`: The number of revisions in the repository
-          whose ancestry was being reported incorrectly.
-        * `garbage_inventories`: The number of inventory objects without
-          revisions that were garbage collected.
-        * `fixed_branch_history`: None if there was no branch, False if the
-          branch history was correct, True if the branch history needed to be
-          re-normalized.
         """
-        operation = cleanup.OperationWithCleanups(self._reconcile)
-        self.add_cleanup = operation.add_cleanup
-        operation.run_simple()
-
-    def _reconcile(self):
-        """Helper function for performing reconciliation."""
-        self.pb = ui.ui_factory.nested_progress_bar()
-        self.add_cleanup(self.pb.finished)
-        self._reconcile_branch()
-        self._reconcile_repository()
+        with ui.ui_factory.nested_progress_bar() as self.pb:
+            result = ReconcileResult()
+            branch_result = self._reconcile_branch()
+            repo_result = self._reconcile_repository()
+            # TODO(jelmer): Don't hardcode supported attributes here
+            result.inconsistent_parents = getattr(
+                repo_result, 'inconsistent_parents', None)
+            result.aborted = getattr(repo_result, 'aborted', None)
+            result.garbage_inventories = getattr(
+                repo_result, 'garbage_inventories', None)
+            result.fixed_branch_history = getattr(
+                branch_result, 'fixed_history', None)
+            return result
 
     def _reconcile_branch(self):
         try:
             self.branch = self.controldir.open_branch()
         except errors.NotBranchError:
             # Nothing to check here
-            self.fixed_branch_history = None
             return
         ui.ui_factory.note(gettext('Reconciling branch %s') % self.branch.base)
-        branch_reconciler = self.branch.reconcile(thorough=True)
-        self.fixed_branch_history = branch_reconciler.fixed_history
+        return self.branch.reconcile(thorough=True)
 
     def _reconcile_repository(self):
         self.repo = self.controldir.find_repository()
@@ -109,453 +98,14 @@
             except AttributeError:
                 raise errors.BzrError(
                     gettext("%s cannot canonicalize CHKs.") % (self.repo,))
-            repo_reconciler = self.repo.reconcile_canonicalize_chks()
+            reconcile_result = self.repo.reconcile_canonicalize_chks()
         else:
-            repo_reconciler = self.repo.reconcile(thorough=True)
-        self.inconsistent_parents = repo_reconciler.inconsistent_parents
-        self.garbage_inventories = repo_reconciler.garbage_inventories
-        if repo_reconciler.aborted:
+            reconcile_result = self.repo.reconcile(thorough=True)
+        if reconcile_result.aborted:
             ui.ui_factory.note(gettext(
                 'Reconcile aborted: revision index has inconsistent parents.'))
             ui.ui_factory.note(gettext(
                 'Run "brz check" for more details.'))
         else:
             ui.ui_factory.note(gettext('Reconciliation complete.'))
-
-
-class BranchReconciler(object):
-    """Reconciler that works on a branch."""
-
-    def __init__(self, a_branch, thorough=False):
-        self.fixed_history = None
-        self.thorough = thorough
-        self.branch = a_branch
-
-    def reconcile(self):
-        operation = cleanup.OperationWithCleanups(self._reconcile)
-        self.add_cleanup = operation.add_cleanup
-        operation.run_simple()
-
-    def _reconcile(self):
-        self.branch.lock_write()
-        self.add_cleanup(self.branch.unlock)
-        self.pb = ui.ui_factory.nested_progress_bar()
-        self.add_cleanup(self.pb.finished)
-        self._reconcile_steps()
-
-    def _reconcile_steps(self):
-        self._reconcile_revision_history()
-
-    def _reconcile_revision_history(self):
-        last_revno, last_revision_id = self.branch.last_revision_info()
-        real_history = []
-        graph = self.branch.repository.get_graph()
-        try:
-            for revid in graph.iter_lefthand_ancestry(
-                    last_revision_id, (_mod_revision.NULL_REVISION,)):
-                real_history.append(revid)
-        except errors.RevisionNotPresent:
-            pass  # Hit a ghost left hand parent
-        real_history.reverse()
-        if last_revno != len(real_history):
-            self.fixed_history = True
-            # Technically for Branch5 formats, it is more efficient to use
-            # set_revision_history, as this will regenerate it again.
-            # Not really worth a whole BranchReconciler class just for this,
-            # though.
-            ui.ui_factory.note(gettext('Fixing last revision info {0} '
-                                       ' => {1}').format(
-                last_revno, len(real_history)))
-            self.branch.set_last_revision_info(len(real_history),
-                                               last_revision_id)
-        else:
-            self.fixed_history = False
-            ui.ui_factory.note(gettext('revision_history ok.'))
-
-
-class RepoReconciler(object):
-    """Reconciler that reconciles a repository.
-
-    The goal of repository reconciliation is to make any derived data
-    consistent with the core data committed by a user. This can involve
-    reindexing, or removing unreferenced data if that can interfere with
-    queries in a given repository.
-
-    Currently this consists of an inventory reweave with revision cross-checks.
-    """
-
-    def __init__(self, repo, other=None, thorough=False):
-        """Construct a RepoReconciler.
-
-        :param thorough: perform a thorough check which may take longer but
-                         will correct non-data loss issues such as incorrect
-                         cached data.
-        """
-        self.garbage_inventories = 0
-        self.inconsistent_parents = 0
-        self.aborted = False
-        self.repo = repo
-        self.thorough = thorough
-
-    def reconcile(self):
-        """Perform reconciliation.
-
-        After reconciliation the following attributes document found issues:
-
-        * `inconsistent_parents`: The number of revisions in the repository
-          whose ancestry was being reported incorrectly.
-        * `garbage_inventories`: The number of inventory objects without
-          revisions that were garbage collected.
-        """
-        operation = cleanup.OperationWithCleanups(self._reconcile)
-        self.add_cleanup = operation.add_cleanup
-        operation.run_simple()
-
-    def _reconcile(self):
-        self.repo.lock_write()
-        self.add_cleanup(self.repo.unlock)
-        self.pb = ui.ui_factory.nested_progress_bar()
-        self.add_cleanup(self.pb.finished)
-        self._reconcile_steps()
-
-    def _reconcile_steps(self):
-        """Perform the steps to reconcile this repository."""
-        self._reweave_inventory()
-
-    def _reweave_inventory(self):
-        """Regenerate the inventory weave for the repository from scratch.
-
-        This is a smart function: it will only do the reweave if doing it
-        will correct data issues. The self.thorough flag controls whether
-        only data-loss causing issues (!self.thorough) or all issues
-        (self.thorough) are treated as requiring the reweave.
-        """
-        transaction = self.repo.get_transaction()
-        self.pb.update(gettext('Reading inventory data'))
-        self.inventory = self.repo.inventories
-        self.revisions = self.repo.revisions
-        # the total set of revisions to process
-        self.pending = {key[-1] for key in self.revisions.keys()}
-
-        # mapping from revision_id to parents
-        self._rev_graph = {}
-        # errors that we detect
-        self.inconsistent_parents = 0
-        # we need the revision id of each revision and its available parents list
-        self._setup_steps(len(self.pending))
-        for rev_id in self.pending:
-            # put a revision into the graph.
-            self._graph_revision(rev_id)
-        self._check_garbage_inventories()
-        # if there are no inconsistent_parents and
-        # (no garbage inventories or we are not doing a thorough check)
-        if (not self.inconsistent_parents
-                and (not self.garbage_inventories or not self.thorough)):
-            ui.ui_factory.note(gettext('Inventory ok.'))
-            return
-        self.pb.update(gettext('Backing up inventory'), 0, 0)
-        self.repo._backup_inventory()
-        ui.ui_factory.note(gettext('Backup inventory created.'))
-        new_inventories = self.repo._temp_inventories()
-
-        # we have topological order of revisions and non ghost parents ready.
-        self._setup_steps(len(self._rev_graph))
-        revision_keys = [(rev_id,) for rev_id in topo_sort(self._rev_graph)]
-        stream = self._change_inv_parents(
-            self.inventory.get_record_stream(revision_keys, 'unordered', True),
-            self._new_inv_parents,
-            set(revision_keys))
-        new_inventories.insert_record_stream(stream)
-        # if this worked, the set of new_inventories.keys should equal
-        # self.pending
-        if not (set(new_inventories.keys())
-                == {(revid,) for revid in self.pending}):
-            raise AssertionError()
-        self.pb.update(gettext('Writing weave'))
-        self.repo._activate_new_inventory()
-        self.inventory = None
-        ui.ui_factory.note(gettext('Inventory regenerated.'))
-
-    def _new_inv_parents(self, revision_key):
-        """Lookup ghost-filtered parents for revision_key."""
-        # Use the filtered ghostless parents list:
-        return tuple([(revid,) for revid in self._rev_graph[revision_key[-1]]])
-
-    def _change_inv_parents(self, stream, get_parents, all_revision_keys):
-        """Adapt a record stream to reconcile the parents."""
-        for record in stream:
-            wanted_parents = get_parents(record.key)
-            if wanted_parents and wanted_parents[0] not in all_revision_keys:
-                # The check for the left most parent only handles knit
-                # compressors, but this code only applies to knit and weave
-                # repositories anyway.
-                bytes = record.get_bytes_as('fulltext')
-                yield FulltextContentFactory(record.key, wanted_parents, record.sha1, bytes)
-            else:
-                adapted_record = AdapterFactory(
-                    record.key, wanted_parents, record)
-                yield adapted_record
-            self._reweave_step('adding inventories')
-
-    def _setup_steps(self, new_total):
-        """Setup the markers we need to control the progress bar."""
-        self.total = new_total
-        self.count = 0
-
-    def _graph_revision(self, rev_id):
-        """Load a revision into the revision graph."""
-        # pick a random revision
-        # analyse revision id rev_id and put it in the stack.
-        self._reweave_step('loading revisions')
-        rev = self.repo.get_revision_reconcile(rev_id)
-        parents = []
-        for parent in rev.parent_ids:
-            if self._parent_is_available(parent):
-                parents.append(parent)
-            else:
-                mutter('found ghost %s', parent)
-        self._rev_graph[rev_id] = parents
-
-    def _check_garbage_inventories(self):
-        """Check for garbage inventories which we cannot trust
-
-        We cant trust them because their pre-requisite file data may not
-        be present - all we know is that their revision was not installed.
-        """
-        if not self.thorough:
-            return
-        inventories = set(self.inventory.keys())
-        revisions = set(self.revisions.keys())
-        garbage = inventories.difference(revisions)
-        self.garbage_inventories = len(garbage)
-        for revision_key in garbage:
-            mutter('Garbage inventory {%s} found.', revision_key[-1])
-
-    def _parent_is_available(self, parent):
-        """True if parent is a fully available revision
-
-        A fully available revision has a inventory and a revision object in the
-        repository.
-        """
-        if parent in self._rev_graph:
-            return True
-        inv_present = (1 == len(self.inventory.get_parent_map([(parent,)])))
-        return (inv_present and self.repo.has_revision(parent))
-
-    def _reweave_step(self, message):
-        """Mark a single step of regeneration complete."""
-        self.pb.update(message, self.count, self.total)
-        self.count += 1
-
-
-class KnitReconciler(RepoReconciler):
-    """Reconciler that reconciles a knit format repository.
-
-    This will detect garbage inventories and remove them in thorough mode.
-    """
-
-    def _reconcile_steps(self):
-        """Perform the steps to reconcile this repository."""
-        if self.thorough:
-            try:
-                self._load_indexes()
-            except errors.BzrCheckError:
-                self.aborted = True
-                return
-            # knits never suffer this
-            self._gc_inventory()
-            self._fix_text_parents()
-
-    def _load_indexes(self):
-        """Load indexes for the reconciliation."""
-        self.transaction = self.repo.get_transaction()
-        self.pb.update(gettext('Reading indexes'), 0, 2)
-        self.inventory = self.repo.inventories
-        self.pb.update(gettext('Reading indexes'), 1, 2)
-        self.repo._check_for_inconsistent_revision_parents()
-        self.revisions = self.repo.revisions
-        self.pb.update(gettext('Reading indexes'), 2, 2)
-
-    def _gc_inventory(self):
-        """Remove inventories that are not referenced from the revision store."""
-        self.pb.update(gettext('Checking unused inventories'), 0, 1)
-        self._check_garbage_inventories()
-        self.pb.update(gettext('Checking unused inventories'), 1, 3)
-        if not self.garbage_inventories:
-            ui.ui_factory.note(gettext('Inventory ok.'))
-            return
-        self.pb.update(gettext('Backing up inventory'), 0, 0)
-        self.repo._backup_inventory()
-        ui.ui_factory.note(gettext('Backup Inventory created'))
-        # asking for '' should never return a non-empty weave
-        new_inventories = self.repo._temp_inventories()
-        # we have topological order of revisions and non ghost parents ready.
-        graph = self.revisions.get_parent_map(self.revisions.keys())
-        revision_keys = topo_sort(graph)
-        revision_ids = [key[-1] for key in revision_keys]
-        self._setup_steps(len(revision_keys))
-        stream = self._change_inv_parents(
-            self.inventory.get_record_stream(revision_keys, 'unordered', True),
-            graph.__getitem__,
-            set(revision_keys))
-        new_inventories.insert_record_stream(stream)
-        # if this worked, the set of new_inventory_vf.names should equal
-        # the revisionds list
-        if not(set(new_inventories.keys()) == set(revision_keys)):
-            raise AssertionError()
-        self.pb.update(gettext('Writing weave'))
-        self.repo._activate_new_inventory()
-        self.inventory = None
-        ui.ui_factory.note(gettext('Inventory regenerated.'))
-
-    def _fix_text_parents(self):
-        """Fix bad versionedfile parent entries.
-
-        It is possible for the parents entry in a versionedfile entry to be
-        inconsistent with the values in the revision and inventory.
-
-        This method finds entries with such inconsistencies, corrects their
-        parent lists, and replaces the versionedfile with a corrected version.
-        """
-        transaction = self.repo.get_transaction()
-        versions = [key[-1] for key in self.revisions.keys()]
-        mutter('Prepopulating revision text cache with %d revisions',
-               len(versions))
-        vf_checker = self.repo._get_versioned_file_checker()
-        bad_parents, unused_versions = vf_checker.check_file_version_parents(
-            self.repo.texts, self.pb)
-        text_index = vf_checker.text_index
-        per_id_bad_parents = {}
-        for key in unused_versions:
-            # Ensure that every file with unused versions gets rewritten.
-            # NB: This is really not needed, reconcile != pack.
-            per_id_bad_parents[key[0]] = {}
-        # Generate per-knit/weave data.
-        for key, details in bad_parents.items():
-            file_id = key[0]
-            rev_id = key[1]
-            knit_parents = tuple([parent[-1] for parent in details[0]])
-            correct_parents = tuple([parent[-1] for parent in details[1]])
-            file_details = per_id_bad_parents.setdefault(file_id, {})
-            file_details[rev_id] = (knit_parents, correct_parents)
-        file_id_versions = {}
-        for text_key in text_index:
-            versions_list = file_id_versions.setdefault(text_key[0], [])
-            versions_list.append(text_key[1])
-        # Do the reconcile of individual weaves.
-        for num, file_id in enumerate(per_id_bad_parents):
-            self.pb.update(gettext('Fixing text parents'), num,
-                           len(per_id_bad_parents))
-            versions_with_bad_parents = per_id_bad_parents[file_id]
-            id_unused_versions = set(key[-1] for key in unused_versions
-                                     if key[0] == file_id)
-            if file_id in file_id_versions:
-                file_versions = file_id_versions[file_id]
-            else:
-                # This id was present in the disk store but is not referenced
-                # by any revision at all.
-                file_versions = []
-            self._fix_text_parent(file_id, versions_with_bad_parents,
-                                  id_unused_versions, file_versions)
-
-    def _fix_text_parent(self, file_id, versions_with_bad_parents,
-                         unused_versions, all_versions):
-        """Fix bad versionedfile entries in a single versioned file."""
-        mutter('fixing text parent: %r (%d versions)', file_id,
-               len(versions_with_bad_parents))
-        mutter('(%d are unused)', len(unused_versions))
-        new_file_id = b'temp:%s' % file_id
-        new_parents = {}
-        needed_keys = set()
-        for version in all_versions:
-            if version in unused_versions:
-                continue
-            elif version in versions_with_bad_parents:
-                parents = versions_with_bad_parents[version][1]
-            else:
-                pmap = self.repo.texts.get_parent_map([(file_id, version)])
-                parents = [key[-1] for key in pmap[(file_id, version)]]
-            new_parents[(new_file_id, version)] = [
-                (new_file_id, parent) for parent in parents]
-            needed_keys.add((file_id, version))
-
-        def fix_parents(stream):
-            for record in stream:
-                bytes = record.get_bytes_as('fulltext')
-                new_key = (new_file_id, record.key[-1])
-                parents = new_parents[new_key]
-                yield FulltextContentFactory(new_key, parents, record.sha1, bytes)
-        stream = self.repo.texts.get_record_stream(
-            needed_keys, 'topological', True)
-        self.repo._remove_file_id(new_file_id)
-        self.repo.texts.insert_record_stream(fix_parents(stream))
-        self.repo._remove_file_id(file_id)
-        if len(new_parents):
-            self.repo._move_file_id(new_file_id, file_id)
-
-
-class PackReconciler(RepoReconciler):
-    """Reconciler that reconciles a pack based repository.
-
-    Garbage inventories do not affect ancestry queries, and removal is
-    considerably more expensive as there is no separate versioned file for
-    them, so they are not cleaned. In short it is currently a no-op.
-
-    In future this may be a good place to hook in annotation cache checking,
-    index recreation etc.
-    """
-
-    # XXX: The index corruption that _fix_text_parents performs is needed for
-    # packs, but not yet implemented. The basic approach is to:
-    #  - lock the names list
-    #  - perform a customised pack() that regenerates data as needed
-    #  - unlock the names list
-    # https://bugs.launchpad.net/bzr/+bug/154173
-
-    def __init__(self, repo, other=None, thorough=False,
-                 canonicalize_chks=False):
-        super(PackReconciler, self).__init__(repo, other=other,
-                                             thorough=thorough)
-        self.canonicalize_chks = canonicalize_chks
-
-    def _reconcile_steps(self):
-        """Perform the steps to reconcile this repository."""
-        if not self.thorough:
-            return
-        collection = self.repo._pack_collection
-        collection.ensure_loaded()
-        collection.lock_names()
-        self.add_cleanup(collection._unlock_names)
-        packs = collection.all_packs()
-        all_revisions = self.repo.all_revision_ids()
-        total_inventories = len(list(
-            collection.inventory_index.combined_index.iter_all_entries()))
-        if len(all_revisions):
-            if self.canonicalize_chks:
-                reconcile_meth = self.repo._canonicalize_chks_pack
-            else:
-                reconcile_meth = self.repo._reconcile_pack
-            new_pack = reconcile_meth(collection, packs, ".reconcile",
-                                      all_revisions, self.pb)
-            if new_pack is not None:
-                self._discard_and_save(packs)
-        else:
-            # only make a new pack when there is data to copy.
-            self._discard_and_save(packs)
-        self.garbage_inventories = total_inventories - len(list(
-            collection.inventory_index.combined_index.iter_all_entries()))
-
-    def _discard_and_save(self, packs):
-        """Discard some packs from the repository.
-
-        This removes them from the memory index, saves the in-memory index
-        which makes the newly reconciled pack visible and hides the packs to be
-        discarded, and finally renames the packs being discarded into the
-        obsolete packs directory.
-
-        :param packs: The packs to discard.
-        """
-        for pack in packs:
-            self.repo._pack_collection._remove_pack_from_memory(pack)
-        self.repo._pack_collection._save_pack_names()
-        self.repo._pack_collection._obsolete_packs(packs)
+        return reconcile_result

=== modified file 'breezy/repository.py'
--- breezy/repository.py	2018-11-29 23:42:41 +0000
+++ breezy/repository.py	2019-01-01 23:11:42 +0000
@@ -218,7 +218,7 @@
             to basis_revision_id. The iterator must not include any items with
             a current kind of None - missing items must be either filtered out
             or errored-on beefore record_iter_changes sees the item.
-        :return: A generator of (file_id, relpath, fs_hash) tuples for use with
+        :return: A generator of (relpath, fs_hash) tuples for use with
             tree._observed_sha1.
         """
         raise NotImplementedError(self.record_iter_changes)
@@ -953,11 +953,7 @@
 
     def reconcile(self, other=None, thorough=False):
         """Reconcile this repository."""
-        from .reconcile import RepoReconciler
-        with self.lock_write():
-            reconciler = RepoReconciler(self, thorough=thorough)
-            reconciler.reconcile()
-            return reconciler
+        raise NotImplementedError(self.reconcile)
 
     def _refresh_data(self):
         """Helper called from lock_* to ensure coherency with disk.

=== modified file 'breezy/shelf_ui.py'
--- breezy/shelf_ui.py	2018-11-16 18:33:17 +0000
+++ breezy/shelf_ui.py	2019-01-01 23:11:42 +0000
@@ -250,7 +250,7 @@
         path_encoding = osutils.get_terminal_encoding()
         text_differ = diff.DiffText(old_tree, new_tree, diff_file,
                                     path_encoding=path_encoding)
-        patch = text_differ.diff(file_id, old_path, new_path, 'file', 'file')
+        patch = text_differ.diff(old_path, new_path, 'file', 'file')
         diff_file.seek(0)
         return patches.parse_patch(diff_file)
 
@@ -365,8 +365,7 @@
         """
         lines = osutils.split_lines(self.change_editor.edit_file(
             self.change_editor.old_tree.id2path(file_id),
-            self.change_editor.new_tree.id2path(file_id),
-            file_id=file_id))
+            self.change_editor.new_tree.id2path(file_id)))
         return lines, self._count_changed_regions(work_tree_lines, lines)
 
     @staticmethod

=== modified file 'breezy/tests/per_branch/test_reconcile.py'
--- breezy/tests/per_branch/test_reconcile.py	2018-11-11 04:08:32 +0000
+++ breezy/tests/per_branch/test_reconcile.py	2019-01-01 23:11:42 +0000
@@ -68,9 +68,9 @@
     def test_reconcile_returns_reconciler(self):
         a_branch = self.make_branch('a_branch')
         result = a_branch.reconcile()
-        self.assertIsInstance(result, reconcile.BranchReconciler)
+        self.assertIsInstance(result, reconcile.ReconcileResult)
         # No history to fix
-        self.assertIs(False, result.fixed_history)
+        self.assertIs(False, getattr(result, 'fixed_history', False))
 
     def test_reconcile_supports_thorough(self):
         a_branch = self.make_branch('a_branch')

=== modified file 'breezy/tests/per_intertree/test_compare.py'
--- breezy/tests/per_intertree/test_compare.py	2018-11-22 03:51:03 +0000
+++ breezy/tests/per_intertree/test_compare.py	2019-01-01 23:11:42 +0000
@@ -798,13 +798,15 @@
         tree1.mkdir('changing', b'parent-id')
         tree1.mkdir('changing/unchanging', b'mid-id')
         tree1.add(['changing/unchanging/file'], [b'file-id'], ['file'])
-        tree1.put_file_bytes_non_atomic('changing/unchanging/file', b'a file')
+        tree1.put_file_bytes_non_atomic(
+            'changing/unchanging/file', b'a file')
         tree2 = self.make_to_branch_and_tree('2')
         tree2.set_root_id(tree1.get_root_id())
         tree2.mkdir('changed', b'parent-id')
         tree2.mkdir('changed/unchanging', b'mid-id')
         tree2.add(['changed/unchanging/file'], [b'file-id'], ['file'])
-        tree2.put_file_bytes_non_atomic('changed/unchanging/file', b'changed content')
+        tree2.put_file_bytes_non_atomic(
+            'changed/unchanging/file', b'changed content')
         tree1, tree2 = self.mutable_trees_to_test_trees(self, tree1, tree2)
         # parent-id has changed, as has file-id
         root_id = tree1.path2id('')

=== modified file 'breezy/tests/per_repository/test_commit_builder.py'
--- breezy/tests/per_repository/test_commit_builder.py	2018-11-16 18:33:17 +0000
+++ breezy/tests/per_repository/test_commit_builder.py	2019-01-01 23:11:42 +0000
@@ -49,8 +49,7 @@
 
     def test_finish_inventory_record_iter_changes(self):
         tree = self.make_branch_and_tree(".")
-        tree.lock_write()
-        try:
+        with tree.lock_write():
             builder = tree.branch.get_commit_builder([])
             try:
                 list(builder.record_iter_changes(tree, tree.last_revision(),
@@ -61,13 +60,10 @@
                 raise
             repo = tree.branch.repository
             repo.commit_write_group()
-        finally:
-            tree.unlock()
 
     def test_abort_record_iter_changes(self):
         tree = self.make_branch_and_tree(".")
-        tree.lock_write()
-        try:
+        with tree.lock_write():
             builder = tree.branch.get_commit_builder([])
             try:
                 basis = tree.basis_tree()
@@ -77,56 +73,44 @@
                 builder.finish_inventory()
             finally:
                 builder.abort()
-        finally:
-            tree.unlock()
 
     def test_commit_lossy(self):
         tree = self.make_branch_and_tree(".")
-        tree.lock_write()
-        try:
+        with tree.lock_write():
             builder = tree.branch.get_commit_builder([], lossy=True)
             list(builder.record_iter_changes(tree, tree.last_revision(),
                                              tree.iter_changes(tree.basis_tree())))
             builder.finish_inventory()
             rev_id = builder.commit('foo bar blah')
-        finally:
-            tree.unlock()
         rev = tree.branch.repository.get_revision(rev_id)
         self.assertEqual('foo bar blah', rev.message)
 
     def test_commit_message(self):
         tree = self.make_branch_and_tree(".")
-        tree.lock_write()
-        try:
+        with tree.lock_write():
             builder = tree.branch.get_commit_builder([])
             list(builder.record_iter_changes(tree, tree.last_revision(),
                                              tree.iter_changes(tree.basis_tree())))
             builder.finish_inventory()
             rev_id = builder.commit('foo bar blah')
-        finally:
-            tree.unlock()
         rev = tree.branch.repository.get_revision(rev_id)
         self.assertEqual('foo bar blah', rev.message)
 
     def test_updates_branch(self):
         tree = self.make_branch_and_tree(".")
-        tree.lock_write()
-        try:
+        with tree.lock_write():
             builder = tree.branch.get_commit_builder([])
             list(builder.record_iter_changes(tree, tree.last_revision(),
                                              tree.iter_changes(tree.basis_tree())))
             builder.finish_inventory()
             will_update_branch = builder.updates_branch
             rev_id = builder.commit('might update the branch')
-        finally:
-            tree.unlock()
         actually_updated_branch = (tree.branch.last_revision() == rev_id)
         self.assertEqual(actually_updated_branch, will_update_branch)
 
     def test_commit_with_revision_id_record_iter_changes(self):
         tree = self.make_branch_and_tree(".")
-        tree.lock_write()
-        try:
+        with tree.lock_write():
             # use a unicode revision id to test more corner cases.
             # The repository layer is meant to handle this.
             revision_id = u'\xc8abc'.encode('utf8')
@@ -150,20 +134,18 @@
                 builder.abort()
                 raise
             self.assertEqual(revision_id, builder.commit('foo bar'))
-        finally:
-            tree.unlock()
         self.assertTrue(tree.branch.repository.has_revision(revision_id))
         # the revision id must be set on the inventory when saving it. This
         # does not precisely test that - a repository that wants to can add it
         # on deserialisation, but thats all the current contract guarantees
         # anyway.
-        self.assertEqual(revision_id,
-                         tree.branch.repository.revision_tree(revision_id).get_revision_id())
+        self.assertEqual(
+            revision_id,
+            tree.branch.repository.revision_tree(revision_id).get_revision_id())
 
     def test_commit_without_root_errors(self):
         tree = self.make_branch_and_tree(".")
-        tree.lock_write()
-        try:
+        with tree.lock_write():
             builder = tree.branch.get_commit_builder([])
 
             def do_commit():
@@ -177,8 +159,6 @@
                 else:
                     builder.commit("msg")
             self.assertRaises(errors.RootMissing, do_commit)
-        finally:
-            tree.unlock()
 
     def test_commit_unchanged_root_record_iter_changes(self):
         tree = self.make_branch_and_tree(".")
@@ -210,8 +190,7 @@
         tree.add(["foo"])
         foo_id = tree.path2id('foo')
         rev_id = tree.commit("added foo")
-        tree.lock_write()
-        try:
+        with tree.lock_write():
             builder = tree.branch.get_commit_builder([rev_id])
             try:
                 delete_change = (foo_id, ('foo', None), True, (True, False),
@@ -228,8 +207,6 @@
             except:
                 builder.abort()
                 raise
-        finally:
-            tree.unlock()
         rev_tree = builder.revision_tree()
         rev_tree.lock_read()
         self.addCleanup(rev_tree.unlock)
@@ -462,7 +439,9 @@
         self.assertFileGraph(expected_graph, tree, (file_id, rev2))
 
     def mini_commit_record_iter_changes(self, tree, name, new_name,
-                                        records_version=True, delta_against_basis=True, expect_fs_hash=False):
+                                        records_version=True,
+                                        delta_against_basis=True,
+                                        expect_fs_hash=False):
         """Perform a miniature commit looking for record entry results.
 
         This version uses the record_iter_changes interface.
@@ -498,10 +477,10 @@
                         tree_file_stat[0].close()
                         self.assertLength(1, result)
                         result = result[0]
-                        self.assertEqual(result[:2], (file_id, new_name))
+                        self.assertEqual(result[0], new_name)
                         self.assertEqual(
-                            result[2][0], tree.get_file_sha1(new_name))
-                        self.assertEqualStat(result[2][1], tree_file_stat[1])
+                            result[1][0], tree.get_file_sha1(new_name))
+                        self.assertEqualStat(result[1][1], tree_file_stat[1])
                     else:
                         self.assertEqual([], result)
                 builder.finish_inventory()
@@ -600,9 +579,10 @@
         rev2 = self._rename_in_tree(tree1, name, 'rev2')
         rev3 = self._rename_in_tree(tree2, name, 'rev3')
         tree1.merge_from_branch(tree2.branch)
-        rev4 = self.mini_commit_record_iter_changes(tree1, 'new_' + name, 'new_' + name,
-                                                    expect_fs_hash=expect_fs_hash,
-                                                    delta_against_basis=tree1.supports_rename_tracking())
+        rev4 = self.mini_commit_record_iter_changes(
+            tree1, 'new_' + name, 'new_' + name,
+            expect_fs_hash=expect_fs_hash,
+            delta_against_basis=tree1.supports_rename_tracking())
         tree3, = self._get_revtrees(tree1, [rev4])
         expected_graph = {}
         if tree1.supports_rename_tracking():
@@ -873,8 +853,7 @@
         self.overrideAttr(config, '_auto_user_id',
                           lambda: (None, None))
         tree = self.make_branch_and_tree(".")
-        tree.lock_write()
-        try:
+        with tree.lock_write():
             # Make sure no username is available.
             self.assertRaises(config.NoWhoami, tree.branch.get_commit_builder,
                               [])
@@ -889,5 +868,3 @@
                 raise
             repo = tree.branch.repository
             repo.commit_write_group()
-        finally:
-            tree.unlock()

=== modified file 'breezy/tests/per_repository_vf/test_reconcile.py'
--- breezy/tests/per_repository_vf/test_reconcile.py	2018-11-11 04:08:32 +0000
+++ breezy/tests/per_repository_vf/test_reconcile.py	2019-01-01 23:11:42 +0000
@@ -73,14 +73,14 @@
         make sure we safely detect this problem.
         """
         repo = self.make_repo_with_extra_ghost_index()
-        reconciler = repo.reconcile(thorough=True)
-        self.assertTrue(reconciler.aborted,
+        result = repo.reconcile(thorough=True)
+        self.assertTrue(result.aborted,
                         "reconcile should have aborted due to bad parents.")
 
     def test_does_not_abort_on_clean_repo(self):
         repo = self.make_repository('.')
-        reconciler = repo.reconcile(thorough=True)
-        self.assertFalse(reconciler.aborted,
+        result = repo.reconcile(thorough=True)
+        self.assertFalse(result.aborted,
                          "reconcile should not have aborted on an unbroken repository.")
 
 
@@ -147,11 +147,11 @@
         self.make_repository('empty')
         d = BzrDir.open(self.get_url('empty'))
         # calling on a empty repository should do nothing
-        reconciler = d.find_repository().reconcile(**kwargs)
+        result = d.find_repository().reconcile(**kwargs)
         # no inconsistent parents should have been found
-        self.assertEqual(0, reconciler.inconsistent_parents)
+        self.assertEqual(0, result.inconsistent_parents)
         # and no garbage inventories
-        self.assertEqual(0, reconciler.garbage_inventories)
+        self.assertEqual(0, result.garbage_inventories)
         # and no backup weave should have been needed/made.
         self.checkNoBackupInventory(d)
 
@@ -187,11 +187,11 @@
         if not repo._reconcile_does_inventory_gc:
             raise TestSkipped('Irrelevant test')
         self.checkUnreconciled(d, repo.reconcile())
-        reconciler = repo.reconcile(thorough=True)
+        result = repo.reconcile(thorough=True)
         # no bad parents
-        self.assertEqual(0, reconciler.inconsistent_parents)
+        self.assertEqual(0, result.inconsistent_parents)
         # and one garbage inventory
-        self.assertEqual(1, reconciler.garbage_inventories)
+        self.assertEqual(1, result.garbage_inventories)
         self.check_missing_was_removed(repo)
 
     def check_thorough_reweave_missing_revision(self, aBzrDir, reconcile,
@@ -241,8 +241,7 @@
 
         def reconcile():
             reconciler = Reconciler(d)
-            reconciler.reconcile()
-            return reconciler
+            return reconciler.reconcile()
         self.check_thorough_reweave_missing_revision(d, reconcile)
 
     def test_reweave_inventory_without_revision_and_ghost(self):

=== modified file 'breezy/tests/per_repository_vf/test_repository.py'
--- breezy/tests/per_repository_vf/test_repository.py	2018-11-18 01:02:16 +0000
+++ breezy/tests/per_repository_vf/test_repository.py	2019-01-01 23:11:42 +0000
@@ -271,7 +271,8 @@
         with tree.lock_write():
             self.assertEqual(set(), set(repo.texts.keys()))
             tree.add(['foo'], [file_id], ['file'])
-            tree.put_file_bytes_non_atomic('foo', b'content\n')
+            tree.put_file_bytes_non_atomic(
+                'foo', b'content\n')
             try:
                 rev_key = (tree.commit("foo"),)
             except errors.IllegalPath:

=== modified file 'breezy/tests/per_workingtree/test_parents.py'
--- breezy/tests/per_workingtree/test_parents.py	2018-11-18 00:25:19 +0000
+++ breezy/tests/per_workingtree/test_parents.py	2019-01-01 23:11:42 +0000
@@ -466,8 +466,8 @@
                 _mod_revision.NULL_REVISION)
             changes = shape_tree.iter_changes(
                 base_tree)
-            list(builder.record_iter_changes(shape_tree,
-                                             base_tree.get_revision_id(), changes))
+            list(builder.record_iter_changes(
+                shape_tree, base_tree.get_revision_id(), changes))
             builder.finish_inventory()
             builder.commit("Message")
 

=== modified file 'breezy/tests/test_diff.py'
--- breezy/tests/test_diff.py	2018-11-11 04:08:32 +0000
+++ breezy/tests/test_diff.py	2019-01-01 23:11:42 +0000
@@ -699,7 +699,7 @@
 
 class DiffWasIs(diff.DiffPath):
 
-    def diff(self, file_id, old_path, new_path, old_kind, new_kind):
+    def diff(self, old_path, new_path, old_kind, new_kind):
         self.to_file.write(b'was: ')
         self.to_file.write(self.old_tree.get_file(old_path).read())
         self.to_file.write(b'is: ')
@@ -728,20 +728,19 @@
         self.new_tree.add('newdir')
         self.new_tree.add('newdir/newfile', b'file-id')
         differ = diff.DiffText(self.old_tree, self.new_tree, BytesIO())
-        differ.diff_text('olddir/oldfile', None, 'old label',
-                         'new label', b'file-id', None)
+        differ.diff_text('olddir/oldfile', None, 'old label', 'new label')
         self.assertEqual(
             b'--- old label\n+++ new label\n@@ -1,1 +0,0 @@\n-old\n\n',
             differ.to_file.getvalue())
         differ.to_file.seek(0)
         differ.diff_text(None, 'newdir/newfile',
-                         'old label', 'new label', None, b'file-id')
+                         'old label', 'new label')
         self.assertEqual(
             b'--- old label\n+++ new label\n@@ -0,0 +1,1 @@\n+new\n\n',
             differ.to_file.getvalue())
         differ.to_file.seek(0)
         differ.diff_text('olddir/oldfile', 'newdir/newfile',
-                         'old label', 'new label', b'file-id', b'file-id')
+                         'old label', 'new label')
         self.assertEqual(
             b'--- old label\n+++ new label\n@@ -1,1 +1,1 @@\n-old\n+new\n\n',
             differ.to_file.getvalue())
@@ -789,7 +788,7 @@
                                   ('new-tree/newdir/newfile', b'new\n')])
         self.new_tree.add('newdir')
         self.new_tree.add('newdir/newfile', b'file-id')
-        self.differ.diff(b'file-id', 'olddir/oldfile', 'newdir/newfile')
+        self.differ.diff('olddir/oldfile', 'newdir/newfile')
         self.assertContainsRe(
             self.differ.to_file.getvalue(),
             br'--- olddir/oldfile.*\n\+\+\+ newdir/newfile.*\n\@\@ -1,1 \+1,1'
@@ -805,7 +804,7 @@
         os.symlink('new', 'new-tree/newdir/newfile')
         self.new_tree.add('newdir')
         self.new_tree.add('newdir/newfile', b'file-id')
-        self.differ.diff(b'file-id', 'olddir/oldfile', 'newdir/newfile')
+        self.differ.diff('olddir/oldfile', 'newdir/newfile')
         self.assertContainsRe(
             self.differ.to_file.getvalue(),
             br'--- olddir/oldfile.*\n\+\+\+ newdir/newfile.*\n\@\@ -1,1 \+0,0'
@@ -816,7 +815,7 @@
     def test_diff_directory(self):
         self.build_tree(['new-tree/new-dir/'])
         self.new_tree.add('new-dir', b'new-dir-id')
-        self.differ.diff(b'new-dir-id', None, 'new-dir')
+        self.differ.diff(None, 'new-dir')
         self.assertEqual(self.differ.to_file.getvalue(), b'')
 
     def create_old_new(self):
@@ -838,7 +837,7 @@
             differ = diff.DiffTree(self.old_tree, self.new_tree, BytesIO())
         finally:
             diff.DiffTree.diff_factories = old_diff_factories
-        differ.diff(b'file-id', 'olddir/oldfile', 'newdir/newfile')
+        differ.diff('olddir/oldfile', 'newdir/newfile')
         self.assertNotContainsRe(
             differ.to_file.getvalue(),
             br'--- olddir/oldfile.*\n\+\+\+ newdir/newfile.*\n\@\@ -1,1 \+1,1'
@@ -850,7 +849,7 @@
         self.create_old_new()
         differ = diff.DiffTree(self.old_tree, self.new_tree, BytesIO(),
                                extra_factories=[DiffWasIs.from_diff_tree])
-        differ.diff(b'file-id', 'olddir/oldfile', 'newdir/newfile')
+        differ.diff('olddir/oldfile', 'newdir/newfile')
         self.assertNotContainsRe(
             differ.to_file.getvalue(),
             br'--- olddir/oldfile.*\n\+\+\+ newdir/newfile.*\n\@\@ -1,1 \+1,1'
@@ -1492,7 +1491,7 @@
         self.addCleanup(diff_obj.finish)
         self.assertContainsRe(diff_obj._root, 'brz-diff-[^/]*')
         old_path, new_path = diff_obj._prepare_files(
-            'oldname', 'newname', file_id=b'file-id')
+            'oldname', 'newname')
         self.assertContainsRe(old_path, 'old/oldname$')
         self.assertEqual(315532800, os.stat(old_path).st_mtime)
         self.assertContainsRe(new_path, 'tree/newname$')
@@ -1501,7 +1500,7 @@
         if osutils.host_os_dereferences_symlinks():
             self.assertTrue(os.path.samefile('tree/newname', new_path))
         # make sure we can create files with the same parent directories
-        diff_obj._prepare_files('oldname2', 'newname2', file_id=b'file2-id')
+        diff_obj._prepare_files('oldname2', 'newname2')
 
 
 class TestDiffFromToolEncodedFilename(tests.TestCaseWithTransport):

=== modified file 'breezy/tests/test_foreign.py'
--- breezy/tests/test_foreign.py	2018-11-16 18:33:17 +0000
+++ breezy/tests/test_foreign.py	2019-01-01 23:11:42 +0000
@@ -205,10 +205,10 @@
                     parent_revids = []
                 else:
                     parent_revids = [parent_revid]
-                builder = self.target.get_commit_builder(parent_revids,
-                                                         self.target.get_config_stack(), rev.timestamp,
-                                                         rev.timezone, rev.committer, rev.properties,
-                                                         new_revid)
+                builder = self.target.get_commit_builder(
+                    parent_revids, self.target.get_config_stack(), rev.timestamp,
+                    rev.timezone, rev.committer, rev.properties,
+                    new_revid)
                 try:
                     parent_tree = self.target.repository.revision_tree(
                         parent_revid)

=== modified file 'breezy/tests/test_reconcile.py'
--- breezy/tests/test_reconcile.py	2017-06-10 16:40:42 +0000
+++ breezy/tests/test_reconcile.py	2019-01-01 23:11:42 +0000
@@ -38,14 +38,14 @@
         child = bzrdir.BzrDirMetaFormat1().initialize('child')
         self.assertRaises(errors.NoRepositoryPresent, child.open_repository)
         reconciler = Reconciler(child)
-        reconciler.reconcile()
+        result = reconciler.reconcile()
         # smoke test for reconcile appears to work too.
         reconcile(child)
         # no inconsistent parents should have been found
         # but the values should have been set.
-        self.assertEqual(0, reconciler.inconsistent_parents)
+        self.assertEqual(0, result.inconsistent_parents)
         # and no garbage inventories
-        self.assertEqual(0, reconciler.garbage_inventories)
+        self.assertEqual(0, result.garbage_inventories)
 
 
 class TestReconciler(tests.TestCaseWithTransport):
@@ -53,20 +53,20 @@
     def test_reconciler_with_no_branch(self):
         repo = self.make_repository('repo')
         reconciler = Reconciler(repo.controldir)
-        reconciler.reconcile()
+        result = reconciler.reconcile()
         # no inconsistent parents should have been found
         # but the values should have been set.
-        self.assertEqual(0, reconciler.inconsistent_parents)
+        self.assertEqual(0, result.inconsistent_parents)
         # and no garbage inventories
-        self.assertEqual(0, reconciler.garbage_inventories)
-        self.assertIs(None, reconciler.fixed_branch_history)
+        self.assertEqual(0, result.garbage_inventories)
+        self.assertIs(None, result.fixed_branch_history)
 
     def test_reconciler_finds_branch(self):
         a_branch = self.make_branch('a_branch')
         reconciler = Reconciler(a_branch.controldir)
-        reconciler.reconcile()
+        result = reconciler.reconcile()
 
         # It should have checked the repository, and the branch
-        self.assertEqual(0, reconciler.inconsistent_parents)
-        self.assertEqual(0, reconciler.garbage_inventories)
-        self.assertIs(False, reconciler.fixed_branch_history)
+        self.assertEqual(0, result.inconsistent_parents)
+        self.assertEqual(0, result.garbage_inventories)
+        self.assertIs(False, result.fixed_branch_history)