Merge lp:~jelmer/brz/intertags into lp:brz/3.1

[Jelmer Vernooij]

Factor out an InterTags object.

Factor out an InterTags object.

[Jelmer Vernooij (jelmer)]

review: Approve

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

There was nothing to merge
https://ci.breezy-vcs.org/job/brz-3.1/job/brz-3.1-land/6/

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

Merging failed
https://ci.breezy-vcs.org/job/brz-3.1/job/brz-3.1-land/7/

=== modified file 'breezy/bzr/branch.py'
--- breezy/bzr/branch.py	2020-01-19 14:49:03 +0000
+++ breezy/bzr/branch.py	2020-02-19 03:31:03 +0000
@@ -28,6 +28,8 @@
     lockdir,
     rio,
     shelf,
+    )
+from breezy.bzr import (
     tag as _mod_tag,
     )
 """)

=== added file 'breezy/bzr/tag.py'
--- breezy/bzr/tag.py	1970-01-01 00:00:00 +0000
+++ breezy/bzr/tag.py	2020-02-19 03:31:03 +0000
@@ -0,0 +1,113 @@
+# Copyright (C) 2005-2012 Canonical Ltd
+# Copyright (C) 2020 Breezy Developers
+#
+# 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
+
+from __future__ import absolute_import
+
+from ..tag import Tags
+
+from .. import (
+    bencode,
+    errors,
+    trace,
+    )
+
+
+class BasicTags(Tags):
+    """Tag storage in an unversioned branch control file.
+    """
+
+    def set_tag(self, tag_name, tag_target):
+        """Add a tag definition to the branch.
+
+        Behaviour if the tag is already present is not defined (yet).
+        """
+        # all done with a write lock held, so this looks atomic
+        with self.branch.lock_write():
+            master = self.branch.get_master_branch()
+            if master is not None:
+                master.tags.set_tag(tag_name, tag_target)
+            td = self.get_tag_dict()
+            td[tag_name] = tag_target
+            self._set_tag_dict(td)
+
+    def lookup_tag(self, tag_name):
+        """Return the referent string of a tag"""
+        td = self.get_tag_dict()
+        try:
+            return td[tag_name]
+        except KeyError:
+            raise errors.NoSuchTag(tag_name)
+
+    def get_tag_dict(self):
+        with self.branch.lock_read():
+            try:
+                tag_content = self.branch._get_tags_bytes()
+            except errors.NoSuchFile:
+                # ugly, but only abentley should see this :)
+                trace.warning('No branch/tags file in %s.  '
+                              'This branch was probably created by bzr 0.15pre.  '
+                              'Create an empty file to silence this message.'
+                              % (self.branch, ))
+                return {}
+            return self._deserialize_tag_dict(tag_content)
+
+    def delete_tag(self, tag_name):
+        """Delete a tag definition.
+        """
+        with self.branch.lock_write():
+            d = self.get_tag_dict()
+            try:
+                del d[tag_name]
+            except KeyError:
+                raise errors.NoSuchTag(tag_name)
+            master = self.branch.get_master_branch()
+            if master is not None:
+                try:
+                    master.tags.delete_tag(tag_name)
+                except errors.NoSuchTag:
+                    pass
+            self._set_tag_dict(d)
+
+    def _set_tag_dict(self, new_dict):
+        """Replace all tag definitions
+
+        WARNING: Calling this on an unlocked branch will lock it, and will
+        replace the tags without warning on conflicts.
+
+        :param new_dict: Dictionary from tag name to target.
+        """
+        return self.branch._set_tags_bytes(self._serialize_tag_dict(new_dict))
+
+    def _serialize_tag_dict(self, tag_dict):
+        td = dict((k.encode('utf-8'), v)
+                  for k, v in tag_dict.items())
+        return bencode.bencode(td)
+
+    def _deserialize_tag_dict(self, tag_content):
+        """Convert the tag file into a dictionary of tags"""
+        # was a special case to make initialization easy, an empty definition
+        # is an empty dictionary
+        if tag_content == b'':
+            return {}
+        try:
+            r = {}
+            for k, v in bencode.bdecode(tag_content).items():
+                r[k.decode('utf-8')] = v
+            return r
+        except ValueError as e:
+            raise ValueError("failed to deserialize tag dictionary %r: %s"
+                             % (tag_content, e))

=== modified file 'breezy/git/branch.py'
--- breezy/git/branch.py	2020-01-24 00:06:05 +0000
+++ breezy/git/branch.py	2020-02-19 03:31:03 +0000
@@ -42,7 +42,6 @@
     lock,
     repository as _mod_repository,
     revision,
-    tag,
     trace,
     transport,
     urlutils,
@@ -55,6 +54,10 @@
     text_type,
     viewitems,
     )
+from ..tag import (
+    Tags,
+    InterTags,
+    )
 from ..trace import (
     is_quiet,
     mutter,
@@ -116,17 +119,24 @@
         return self._lookup_revno(self.new_revid)
 
 
-class GitTags(tag.BasicTags):
-    """Ref-based tag dictionary."""
-
-    def __init__(self, branch):
-        self.branch = branch
-        self.repository = branch.repository
-
-    def _merge_to_remote_git(self, target_repo, source_tag_refs,
-                             overwrite=False):
+class InterTagsFromGitToRemoteGit(InterTags):
+
+    @classmethod
+    def is_compatible(klass, source, target):
+        if not isinstance(source, GitTags):
+            return False
+        if not isinstance(target, GitTags):
+            return False
+        if getattr(target.branch.repository, "_git", None) is not None:
+            return False
+        return True
+
+    def merge(self, overwrite=False, ignore_master=False):
+        if self.source.branch.repository.has_same_location(self.target.branch.repository):
+            return {}, []
         updates = {}
         conflicts = []
+        source_tag_refs = self.source.branch.get_tag_refs()
 
         def get_changed_refs(old_refs):
             ret = dict(old_refs)
@@ -136,23 +146,43 @@
                     pass
                 elif overwrite or ref_name not in old_refs:
                     ret[ref_name] = unpeeled
-                    updates[tag_name] = target_repo.lookup_foreign_revision_id(
+                    updates[tag_name] = self.target.branch.repository.lookup_foreign_revision_id(
                         peeled)
+                    self.target.branch._tag_refs = None
                 else:
                     conflicts.append(
                         (tag_name,
                          self.repository.lookup_foreign_revision_id(peeled),
-                         target_repo.lookup_foreign_revision_id(
+                         self.target.branch.repository.lookup_foreign_revision_id(
                              old_refs[ref_name])))
             return ret
-        target_repo.controldir.send_pack(
+        self.target.branch.repository.controldir.send_pack(
             get_changed_refs, lambda have, want: [])
         return updates, conflicts
 
-    def _merge_to_local_git(self, target_repo, source_tag_refs,
-                            overwrite=False):
+
+class InterTagsFromGitToLocalGit(InterTags):
+
+    @classmethod
+    def is_compatible(klass, source, target):
+        if not isinstance(source, GitTags):
+            return False
+        if not isinstance(target, GitTags):
+            return False
+        if getattr(target.branch.repository, "_git", None) is None:
+            return False
+        return True
+
+    def merge(self, overwrite=False, ignore_master=False):
+        if self.source.branch.repository.has_same_location(self.target.branch.repository):
+            return {}, []
+
         conflicts = []
         updates = {}
+        source_tag_refs = self.source.branch.get_tag_refs()
+
+        target_repo = self.target.branch.repository
+
         for ref_name, tag_name, peeled, unpeeled in source_tag_refs:
             if target_repo._git.refs.get(ref_name) == unpeeled:
                 pass
@@ -169,9 +199,10 @@
                                   tag_name)
                     continue
                 target_repo._git.refs[ref_name] = unpeeled or peeled
+                self.target.branch._tag_refs = None
             else:
                 try:
-                    source_revid = self.repository.lookup_foreign_revision_id(
+                    source_revid = self.source.branch.repository.lookup_foreign_revision_id(
                         peeled)
                     target_revid = target_repo.lookup_foreign_revision_id(
                         target_repo._git.refs[ref_name])
@@ -186,21 +217,39 @@
                 conflicts.append((tag_name, source_revid, target_revid))
         return updates, conflicts
 
-    def _merge_to_git(self, to_tags, source_tag_refs, overwrite=False):
-        target_repo = to_tags.repository
-        if self.repository.has_same_location(target_repo):
-            return {}, []
-        try:
-            if getattr(target_repo, "_git", None):
-                return self._merge_to_local_git(
-                    target_repo, source_tag_refs, overwrite)
-            else:
-                return self._merge_to_remote_git(
-                    target_repo, source_tag_refs, overwrite)
-        finally:
-            to_tags.branch._tag_refs = None
-
-    def _merge_to_non_git(self, to_tags, source_tag_refs, overwrite=False):
+
+class InterTagsFromGitToNonGit(InterTags):
+
+    @classmethod
+    def is_compatible(klass, source, target):
+        if not isinstance(source, GitTags):
+            return False
+        if isinstance(target, GitTags):
+            return False
+        return True
+
+    def merge(self, overwrite=False, ignore_master=False):
+        """See Tags.merge_to."""
+        source_tag_refs = self.source.branch.get_tag_refs()
+        if ignore_master:
+            master = None
+        else:
+            master = self.target.branch.get_master_branch()
+        with cleanup.ExitStack() as es:
+            if master is not None:
+                es.enter_context(master.lock_write())
+            updates, conflicts = self._merge_to(
+                self.target, source_tag_refs, overwrite=overwrite)
+            if master is not None:
+                extra_updates, extra_conflicts = self._merge_to(
+                    master.tags, overwrite=overwrite,
+                    source_tag_refs=source_tag_refs,
+                    ignore_master=ignore_master)
+                updates.update(extra_updates)
+                conflicts += extra_conflicts
+            return updates, conflicts
+
+    def _merge_to(self, to_tags, source_tag_refs, overwrite=False):
         unpeeled_map = defaultdict(set)
         conflicts = []
         updates = {}
@@ -209,7 +258,7 @@
             if unpeeled is not None:
                 unpeeled_map[peeled].add(unpeeled)
             try:
-                bzr_revid = self.branch.lookup_foreign_revision_id(peeled)
+                bzr_revid = self.source.branch.lookup_foreign_revision_id(peeled)
             except NotCommitError:
                 continue
             if result.get(tag_name) == bzr_revid:
@@ -226,34 +275,18 @@
             map_file.save_in_repository(to_tags.branch.repository)
         return updates, conflicts
 
-    def merge_to(self, to_tags, overwrite=False, ignore_master=False,
-                 source_tag_refs=None):
-        """See Tags.merge_to."""
-        if source_tag_refs is None:
-            source_tag_refs = self.branch.get_tag_refs()
-        if self == to_tags:
-            return {}, []
-        if isinstance(to_tags, GitTags):
-            return self._merge_to_git(to_tags, source_tag_refs,
-                                      overwrite=overwrite)
-        else:
-            if ignore_master:
-                master = None
-            else:
-                master = to_tags.branch.get_master_branch()
-            with cleanup.ExitStack() as es:
-                if master is not None:
-                    es.enter_context(master.lock_write())
-                updates, conflicts = self._merge_to_non_git(
-                    to_tags, source_tag_refs, overwrite=overwrite)
-                if master is not None:
-                    extra_updates, extra_conflicts = self.merge_to(
-                        master.tags, overwrite=overwrite,
-                        source_tag_refs=source_tag_refs,
-                        ignore_master=ignore_master)
-                    updates.update(extra_updates)
-                    conflicts += extra_conflicts
-                return updates, conflicts
+
+InterTags.register_optimiser(InterTagsFromGitToRemoteGit)
+InterTags.register_optimiser(InterTagsFromGitToLocalGit)
+InterTags.register_optimiser(InterTagsFromGitToNonGit)
+
+
+class GitTags(Tags):
+    """Ref-based tag dictionary."""
+
+    def __init__(self, branch):
+        self.branch = branch
+        self.repository = branch.repository
 
     def get_tag_dict(self):
         ret = {}
@@ -267,6 +300,15 @@
                 ret[tag_name] = bzr_revid
         return ret
 
+    def lookup_tag(self, tag_name):
+        """Return the referent string of a tag"""
+        # TODO(jelmer): Replace with something more efficient for local tags.
+        td = self.get_tag_dict()
+        try:
+            return td[tag_name]
+        except KeyError:
+            raise errors.NoSuchTag(tag_name)
+
 
 class LocalGitTagDict(GitTags):
     """Dictionary with tags in a local repository."""
@@ -1194,7 +1236,6 @@
             other_branch=self.source)
         tags_ret = self.source.tags.merge_to(
             self.target.tags,
-            source_tag_refs=remote_refs_dict_to_tag_refs(refs),
             overwrite=("tags" in overwrite))
         if isinstance(tags_ret, tuple):
             (result.tag_updates, result.tag_conflicts) = tags_ret
@@ -1243,8 +1284,7 @@
                 (result.old_revid if ("history" not in overwrite) else None),
                 other_branch=self.source)
             tags_ret = self.source.tags.merge_to(
-                self.target.tags, overwrite=("tags" in overwrite),
-                source_tag_refs=remote_refs_dict_to_tag_refs(refs))
+                self.target.tags, overwrite=("tags" in overwrite))
             if isinstance(tags_ret, tuple):
                 (result.tag_updates, result.tag_conflicts) = tags_ret
             else:

=== modified file 'breezy/tag.py'
--- breezy/tag.py	2020-02-18 00:46:00 +0000
+++ breezy/tag.py	2020-02-19 03:31:03 +0000
@@ -25,22 +25,21 @@
 from __future__ import absolute_import
 
 from collections import defaultdict
+import itertools
+import re
+import sys
 
 # NOTE: I was going to call this tags.py, but vim seems to think all files
 # called tags* are ctags files... mbp 20070220.
 
+from .inter import InterObject
 from .registry import Registry
 from .sixish import text_type
 from .lazy_import import lazy_import
 lazy_import(globals(), """
-import itertools
-import re
-import sys
 
 from breezy import (
-    bencode,
     cleanup,
-    trace,
     )
 """)
 
@@ -75,7 +74,7 @@
     return result, updates, conflicts
 
 
-class _Tags(object):
+class Tags(object):
 
     def __init__(self, branch):
         self.branch = branch
@@ -86,20 +85,37 @@
         raise NotImplementedError(self.get_tag_dict)
 
     def get_reverse_tag_dict(self):
-        """Return a dictionary mapping revision ids to list of tags.
-        """
-        raise NotImplementedError(self.get_reverse_tag_dict)
+        """Returns a dict with revisions as keys
+           and a list of tags for that revision as value"""
+        d = self.get_tag_dict()
+        rev = defaultdict(set)
+        for key in d:
+            rev[d[key]].add(key)
+        return rev
 
     def merge_to(self, to_tags, overwrite=False, ignore_master=False):
-        """Merge new tags from this tags container into another.
-
-        :param to_tags: Tags container to merge into
-        :param overwrite: Whether to overwrite existing, divergent, tags.
+        """Copy tags between repositories if necessary and possible.
+
+        This method has common command-line behaviour about handling
+        error cases.
+
+        All new definitions are copied across, except that tags that already
+        exist keep their existing definitions.
+
+        :param to_tags: Branch to receive these tags
+        :param overwrite: Overwrite conflicting tags in the target branch
         :param ignore_master: Do not modify the tags in the target's master
             branch (if any).  Default is false (so the master will be updated).
-        :return: Tuple with tag updates as dictionary and tag conflicts
+
+        :returns: Tuple with tag_updates and tag_conflicts.
+            tag_updates is a dictionary with new tags, None is used for
+            removed tags
+            tag_conflicts is a set of tags that conflicted, each of which is
+            (tagname, source_target, dest_target), or None if no copying was
+            done.
         """
-        raise NotImplementedError(self.merge_to)
+        intertags = InterTags.get(self, to_tags)
+        return intertags.merge(overwrite=overwrite, ignore_master=ignore_master)
 
     def set_tag(self, tag_name, revision):
         """Set a tag.
@@ -129,18 +145,21 @@
         raise NotImplementedError(self.delete_tag)
 
     def rename_revisions(self, rename_map):
-        """Replace revision ids according to a rename map.
+        """Rename revisions in this tags dictionary.
 
-        :param rename_map: Dictionary mapping old revision ids to
-            new revision ids.
+        :param rename_map: Dictionary mapping old revids to new revids
         """
-        raise NotImplementedError(self.rename_revisions)
+        reverse_tags = self.get_reverse_tag_dict()
+        for revid, names in reverse_tags.items():
+            if revid in rename_map:
+                for name in names:
+                    self.set_tag(name, rename_map[revid])
 
     def has_tag(self, tag_name):
         return tag_name in self.get_tag_dict()
 
 
-class DisabledTags(_Tags):
+class DisabledTags(Tags):
     """Tag storage that refuses to store anything.
 
     This is used by older formats that can't store tags.
@@ -168,102 +187,19 @@
         return {}
 
 
-class BasicTags(_Tags):
-    """Tag storage in an unversioned branch control file.
+class InterTags(InterObject):
+    """Operations between sets of tags.
     """
 
-    def set_tag(self, tag_name, tag_target):
-        """Add a tag definition to the branch.
-
-        Behaviour if the tag is already present is not defined (yet).
-        """
-        # all done with a write lock held, so this looks atomic
-        with self.branch.lock_write():
-            master = self.branch.get_master_branch()
-            if master is not None:
-                master.tags.set_tag(tag_name, tag_target)
-            td = self.get_tag_dict()
-            td[tag_name] = tag_target
-            self._set_tag_dict(td)
-
-    def lookup_tag(self, tag_name):
-        """Return the referent string of a tag"""
-        td = self.get_tag_dict()
-        try:
-            return td[tag_name]
-        except KeyError:
-            raise errors.NoSuchTag(tag_name)
-
-    def get_tag_dict(self):
-        with self.branch.lock_read():
-            try:
-                tag_content = self.branch._get_tags_bytes()
-            except errors.NoSuchFile:
-                # ugly, but only abentley should see this :)
-                trace.warning('No branch/tags file in %s.  '
-                              'This branch was probably created by bzr 0.15pre.  '
-                              'Create an empty file to silence this message.'
-                              % (self.branch, ))
-                return {}
-            return self._deserialize_tag_dict(tag_content)
-
-    def get_reverse_tag_dict(self):
-        """Returns a dict with revisions as keys
-           and a list of tags for that revision as value"""
-        d = self.get_tag_dict()
-        rev = defaultdict(set)
-        for key in d:
-            rev[d[key]].add(key)
-        return rev
-
-    def delete_tag(self, tag_name):
-        """Delete a tag definition.
-        """
-        with self.branch.lock_write():
-            d = self.get_tag_dict()
-            try:
-                del d[tag_name]
-            except KeyError:
-                raise errors.NoSuchTag(tag_name)
-            master = self.branch.get_master_branch()
-            if master is not None:
-                try:
-                    master.tags.delete_tag(tag_name)
-                except errors.NoSuchTag:
-                    pass
-            self._set_tag_dict(d)
-
-    def _set_tag_dict(self, new_dict):
-        """Replace all tag definitions
-
-        WARNING: Calling this on an unlocked branch will lock it, and will
-        replace the tags without warning on conflicts.
-
-        :param new_dict: Dictionary from tag name to target.
-        """
-        return self.branch._set_tags_bytes(self._serialize_tag_dict(new_dict))
-
-    def _serialize_tag_dict(self, tag_dict):
-        td = dict((k.encode('utf-8'), v)
-                  for k, v in tag_dict.items())
-        return bencode.bencode(td)
-
-    def _deserialize_tag_dict(self, tag_content):
-        """Convert the tag file into a dictionary of tags"""
-        # was a special case to make initialization easy, an empty definition
-        # is an empty dictionary
-        if tag_content == b'':
-            return {}
-        try:
-            r = {}
-            for k, v in bencode.bdecode(tag_content).items():
-                r[k.decode('utf-8')] = v
-            return r
-        except ValueError as e:
-            raise ValueError("failed to deserialize tag dictionary %r: %s"
-                             % (tag_content, e))
-
-    def merge_to(self, to_tags, overwrite=False, ignore_master=False):
+    _optimisers = []
+    """The available optimised InterTags types."""
+
+    @classmethod
+    def is_compatible(klass, source, target):
+        # This is the default implementation
+        return True
+
+    def merge(self, overwrite=False, ignore_master=False):
         """Copy tags between repositories if necessary and possible.
 
         This method has common command-line behaviour about handling
@@ -285,12 +221,12 @@
             done.
         """
         with cleanup.ExitStack() as stack:
-            if self.branch == to_tags.branch:
+            if self.source.branch == self.target.branch:
                 return {}, []
-            if not self.branch.supports_tags():
+            if not self.source.branch.supports_tags():
                 # obviously nothing to copy
                 return {}, []
-            source_dict = self.get_tag_dict()
+            source_dict = self.source.get_tag_dict()
             if not source_dict:
                 # no tags in the source, and we don't want to clobber anything
                 # that's in the destination
@@ -307,14 +243,14 @@
             # Ideally we'd improve this API to report the different conflicts
             # more clearly to the caller, but we don't want to break plugins
             # such as bzr-builddeb that use this API.
-            stack.enter_context(to_tags.branch.lock_write())
+            stack.enter_context(self.target.branch.lock_write())
             if ignore_master:
                 master = None
             else:
-                master = to_tags.branch.get_master_branch()
+                master = self.target.branch.get_master_branch()
             if master is not None:
                 stack.enter_context(master.lock_write())
-            updates, conflicts = self._merge_to(to_tags, source_dict, overwrite)
+            updates, conflicts = self._merge_to(self.target, source_dict, overwrite)
             if master is not None:
                 extra_updates, extra_conflicts = self._merge_to(master.tags,
                                                                 source_dict, overwrite)
@@ -324,7 +260,8 @@
             # branch.
             return updates, set(conflicts)
 
-    def _merge_to(self, to_tags, source_dict, overwrite):
+    @classmethod
+    def _merge_to(cls, to_tags, source_dict, overwrite):
         dest_dict = to_tags.get_tag_dict()
         result, updates, conflicts = _reconcile_tags(
             source_dict, dest_dict, overwrite)
@@ -332,19 +269,8 @@
             to_tags._set_tag_dict(result)
         return updates, conflicts
 
-    def rename_revisions(self, rename_map):
-        """Rename revisions in this tags dictionary.
-
-        :param rename_map: Dictionary mapping old revids to new revids
-        """
-        reverse_tags = self.get_reverse_tag_dict()
-        for revid, names in reverse_tags.items():
-            if revid in rename_map:
-                for name in names:
-                    self.set_tag(name, rename_map[revid])
-
-
-class MemoryTags(_Tags):
+
+class MemoryTags(Tags):
 
     def __init__(self, tag_dict):
         self._tag_dict = tag_dict
@@ -360,15 +286,6 @@
         except KeyError:
             raise errors.NoSuchTag(tag_name)
 
-    def get_reverse_tag_dict(self):
-        """Returns a dict with revisions as keys
-           and a list of tags for that revision as value"""
-        d = self.get_tag_dict()
-        rev = defaultdict(set)
-        for key in d:
-            rev[d[key]].add(key)
-        return rev
-
     def set_tag(self, name, revid):
         self._tag_dict[name] = revid
 
@@ -442,3 +359,6 @@
 tag_sort_methods.register("alpha", sort_alpha, 'Sort tags lexicographically.')
 tag_sort_methods.register("time", sort_time, 'Sort tags chronologically.')
 tag_sort_methods.default_key = "natural"
+
+
+InterTags.register_optimiser(InterTags)

=== modified file 'breezy/tests/test_tag.py'
--- breezy/tests/test_tag.py	2019-02-01 16:29:45 +0000
+++ breezy/tests/test_tag.py	2020-02-19 03:31:03 +0000
@@ -22,10 +22,12 @@
     errors,
     )
 from breezy.tag import (
-    BasicTags,
     DisabledTags,
     MemoryTags,
     )
+from breezy.bzr.tag import (
+    BasicTags,
+    )
 from breezy.tests import (
     TestCase,
     TestCaseWithTransport,