Merge lp:~spiv/bzr/checkout-tags-propagation-603395-2.2 into lp:bzr/2.2

[Andrew Bennetts]

This is part of the fix for bug 603395. It makes BasicTags.merge_to also merge to the master branch, if there is one. This is consistent with e.g. set_tag. bzr-builddeb directly calls that API, and presumably expects merge_to in a checkout branch to update the master as well. Because bzr-builddeb does call this API directly I don't believe <https://code.launchpad.net/~spiv/bzr/tags-commit-propagation-603395-2.2/+merge/39733> (which has already landed) alone is enough to fix the UDD bug. bzr-svn also calls this API, so it is likely affected too. I know John commented on the other proposal that he thought it alone would be sufficient, but I'm unconvinced. (And even if this isn't necessary it still seems like a reasonable change to me, the previous behaviour was too low on the Rusty API usability scale IMO.)

I've dealt with my reservations on the original merge proposal: merge_to now accepts an optional kwarg, ignore_master, that cmd_merge uses to avoid prematurely propagating changes to the master. So I think this avoids any chance of regressing at all on #99137 (bzr revert will still not remove the new tags from the child branch, but that's not a regression). This does mean that external implementations of merge_to should be updated. I've documented the API change in NEWS, and made sure the callsite in bzr that does pass that kwarg falls back to not passing the kwarg. I've tested the fallback manually with bzr-svn (which has an external implementation of merge_to), and it appears to work as intended.

The tests take care to deal with all the permutations, including those that can only be triggered by an unfixed bzr operating on the checkout. See the test comments for details.

[Andrew Bennetts (spiv)]

I should point out: this does have the perhaps unwanted side-effect that "bzr merge" can cause tags to be added the master even if the next command is "bzr commit". That's really <https://bugs.launchpad.net/bzr/+bug/99137> (tags are "permanently" propagated by merge), and this patch is just making the overall behaviour more consistent. I can see an argument that undoing the accidental limiting of the the effect of bug 99137 might not be acceptable in a stable branch, though.

A compromise would be to add a new optional argument to merge_to (or perhaps a whole new method) that callers can use to explicitly enable/disable propagation. I'm not sure if it would be better to default to enabled (so that plugins like builddeb can automatically benefit), or disabled (to minimise the change in behaviour, and also perhaps to avoid needing to tweak the API layers between cmd_merge and br.tags.merge_to to pass the flag). With 'bzr merge' not propagating we'd then rely on <https://code.launchpad.net/~spiv/bzr/tags-commit-propagation-603395-2.2> to propagate at commit time rather than merge time.

[John A Meinel (jameinel)]

38 - dest_dict = to_tags.get_tag_dict()
39 - result, conflicts = self._reconcile_tags(source_dict, dest_dict,
40 - overwrite)
41 - if result != dest_dict:
42 - to_tags._set_tag_dict(result)
43 + master = to_tags.branch.get_master_branch()
44 + try:
45 + if master is not None:
46 + master.lock_write()
47 + conflicts = self._merge_to(to_tags, source_dict, overwrite)
48 + if master is not None:
49 + conflicts += self._merge_to(master.tags, source_dict,
50 + overwrite)
51 + finally:
52 + if master is not None:
53 + master.unlock()

Isn't this better written as:

master = to_tags.branch.get_master_branch()
if master is not None:
    master.lock_write()
    try:
      conflicts = ...
    finally:
      master.unlock()

Note that the way you wrote it, if we fail to take the write lock in the first place (no such permission), we end up still calling unlock.

I think you wrote it this way because you check the conflicts anyway, but the code looks clearer to me if you just add an 'else: conflicts = ...' to the above statement.

I suppose either way it is a bit clumsy, since you want the merge to fail immediately if you can't propagate the tags to the master branch before you try to merge to the local one...

I think the logic is otherwise ok.

review: Needs Fixing

[Andrew Bennetts (spiv)]

John A Meinel wrote:
[...]
> Note that the way you wrote it, if we fail to take the write lock in
> the first place (no such permission), we end up still calling unlock.

Good point!

> I think you wrote it this way because you check the conflicts anyway,
> but the code looks clearer to me if you just add an 'else: conflicts =
> ...' to the above statement.
>
> I suppose either way it is a bit clumsy, since you want the merge to
> fail immediately if you can't propagate the tags to the master branch
> before you try to merge to the local one...

Really what I want is to use add_cleanup. It seemed like it was perhaps
overkill, so I didn't do that initially. But this review makes it clear
to me that it really would be worthwhile, despite incurring a larger
diff.

[lp:~spiv/bzr/checkout-tags-propagation-603395-2.2]

5115. By Andrew Bennetts on 2010-11-09

Remove inaccurate test comment.

[Vincent Ladeuil (vila)]

Our policy for stable branches is a strict 'no API changes'.

I realize you have gone very far in ensuring compatibility and that this impacts UDD but this still doesn't justify breaking our policy IMHO.

Are there some other ways to propagate the tags to the master branch without this patch ?

I'm otherwise +1 for landing this on trunk.

review: Disapprove

[Vincent Ladeuil (vila)]

More thoughts on this as I realize it's always frustrating to have a mp rejected.

My concerns here of course are: what if we break something ? Would that means we'll get into a whack-a-mole frenzy updating bzr and plugins introducing even more problems ?

Instead of focusing on the right fix on trunk...

What are your thoughts here and what do you plan to do next ?

Is there some way we can have some real-world testing here ? May be asking Jelmer for bzr-svn fallouts ? Do we have a clear understanding of *which* plugins are (or could be) impacted there ?

[John A Meinel (jameinel)]

I think I'm happy with the change, but I would agree with Vincent's hesitancy to land it in 2.2, vs just landing it in trunk.

Unmerged revisions

5115. By Andrew Bennetts on 2010-11-09

Remove inaccurate test comment.

5114. By Andrew Bennetts on 2010-11-09

Add unit test for ignore_master=True, and improve other test comments.

5113. By Andrew Bennetts on 2010-11-09

Cosmetic tweak to deprecation.

5112. By Andrew Bennetts on 2010-11-09

Move bound_branch.tags.merge_to tests to per_branch.

5111. By Andrew Bennetts on 2010-11-09

Move new test to bb.test_tag, to avoid repeating test infrastructure.

5110. By Andrew Bennetts on 2010-11-09

Add blackbox test to demonstrate that 'merge' alone still does not propagate tags to the master.

5109. By Andrew Bennetts on 2010-11-09

Merge lp:bzr/2.2.

5108. By Andrew Bennetts on 2010-11-09

Don't propagate tags to the master branch during cmd_merge.

5107. By Andrew Bennetts on 2010-11-09

Use add_cleanup for simpler and more correct unlocking.

5106. By Andrew Bennetts on 2010-11-01

More tests, more description of test intent, and specify exactly what happens with duplicate vs. different conflicts.

=== modified file 'NEWS'
--- NEWS	2010-11-08 06:10:41 +0000
+++ NEWS	2010-11-09 06:33:49 +0000
@@ -42,6 +42,11 @@
   root. Including the one that wasn't present in 2.1.
   (Vincent Ladeuil, #646133)
 
+* The ``branch.tags.merge_to(target_branch)`` API used by plugins such as
+  ``bzr-builddeb`` now propagates changes to the master branch of the
+  target branch (if there is one).  This makes it consistent with the
+  other tag APIs.  (Andrew Bennetts, #603395)
+
 * Using bzr with `lp:` urls behind an http proxy should work.
   (Robert Collins, #558343)
 
@@ -58,6 +63,14 @@
 API Changes
 ***********
 
+* The ``merge_to`` method of ``branch.tags`` objects is now expected to
+  take a ``ignore_master=False`` keyword parameter.  Plugins that
+  implement this method should add that parameter to their signature.
+  Callsites in bzr itself that pass that flag will still try to work with
+  the old signature (they catch TypeError and retry without the flag), but
+  this support is deprecated and will be removed eventually.
+  (Andrew Bennetts, #603395)
+
 Internals
 *********
 

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2010-08-13 07:32:06 +0000
+++ bzrlib/branch.py	2010-11-09 06:33:49 +0000
@@ -3326,7 +3326,7 @@
         if isinstance(format, remote.RemoteBranchFormat):
             format._ensure_real()
             return format._custom_format
-        return format                                                                                                  
+        return format
 
     @needs_write_lock
     def copy_content_into(self, revision_id=None):

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2010-07-28 07:05:19 +0000
+++ bzrlib/builtins.py	2010-11-09 06:33:49 +0000
@@ -3980,7 +3980,9 @@
         if ((remember or tree.branch.get_submit_branch() is None) and
              user_location is not None):
             tree.branch.set_submit_branch(other_branch.base)
-        _merge_tags_if_possible(other_branch, tree.branch)
+        # Merge tags (but don't set them in the master branch yet, the user
+        # might revert this merge).  Commit will propagate them.
+        _merge_tags_if_possible(other_branch, tree.branch, ignore_master=True)
         merger = _mod_merge.Merger.from_revision_ids(pb, tree,
             other_revision_id, base_revision_id, other_branch, base_branch)
         if other_path != '':

=== modified file 'bzrlib/tag.py'
--- bzrlib/tag.py	2010-03-25 09:39:03 +0000
+++ bzrlib/tag.py	2010-11-09 06:33:49 +0000
@@ -28,7 +28,9 @@
 
 from bzrlib import (
     bencode,
+    cleanup,
     errors,
+    symbol_versioning,
     trace,
     )
 
@@ -57,7 +59,7 @@
     lookup_tag = _not_supported
     delete_tag = _not_supported
 
-    def merge_to(self, to_tags, overwrite=False):
+    def merge_to(self, to_tags, overwrite=False, ignore_master=False):
         # we never have anything to copy
         pass
 
@@ -177,7 +179,7 @@
             raise ValueError("failed to deserialize tag dictionary %r: %s"
                 % (tag_content, e))
 
-    def merge_to(self, to_tags, overwrite=False):
+    def merge_to(self, to_tags, overwrite=False, ignore_master=False):
         """Copy tags between repositories if necessary and possible.
 
         This method has common command-line behaviour about handling
@@ -188,11 +190,19 @@
 
         :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).
+            New in bzr 2.2.2 and bzr 2.3.
 
         :returns: A list of tags that conflicted, each of which is
             (tagname, source_target, dest_target), or None if no copying was
             done.
         """
+        operation = cleanup.OperationWithCleanups(self._merge_to_operation)
+        return operation.run(to_tags, overwrite, ignore_master)
+
+    def _merge_to_operation(self, operation, to_tags, overwrite, ignore_master):
+        add_cleanup = operation.add_cleanup
         if self.branch == to_tags.branch:
             return
         if not self.branch.supports_tags():
@@ -203,15 +213,40 @@
             # no tags in the source, and we don't want to clobber anything
             # that's in the destination
             return
-        to_tags.branch.lock_write()
-        try:
-            dest_dict = to_tags.get_tag_dict()
-            result, conflicts = self._reconcile_tags(source_dict, dest_dict,
-                                                     overwrite)
-            if result != dest_dict:
-                to_tags._set_tag_dict(result)
-        finally:
-            to_tags.branch.unlock()
+        # We merge_to both master and child individually.
+        #
+        # It's possible for master and child to have differing sets of
+        # tags, in which case it's possible to have different sets of
+        # conflicts.  We report the union of both conflict sets.  In
+        # that case it's likely the child and master have accepted
+        # different tags from the source, which may be a surprising result, but
+        # the best we can do in the circumstances.
+        #
+        # 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.
+        add_cleanup(to_tags.branch.lock_write().unlock)
+        if ignore_master:
+            master = None
+        else:
+            master = to_tags.branch.get_master_branch()
+        if master is not None:
+            add_cleanup(master.lock_write().unlock)
+        conflicts = self._merge_to(to_tags, source_dict, overwrite)
+        if master is not None:
+            conflicts += self._merge_to(master.tags, source_dict,
+                overwrite)
+        # We use set() to remove any duplicate conflicts from the master
+        # branch.  We then use list() to keep the behaviour as close to 2.2.1
+        # and earlier as possible, to minimise potential compatibility issues.
+        return list(set(conflicts))
+
+    def _merge_to(self, to_tags, source_dict, overwrite):
+        dest_dict = to_tags.get_tag_dict()
+        result, conflicts = self._reconcile_tags(source_dict, dest_dict,
+                                                 overwrite)
+        if result != dest_dict:
+            to_tags._set_tag_dict(result)
         return conflicts
 
     def rename_revisions(self, rename_map):
@@ -249,6 +284,27 @@
         return result, conflicts
 
 
-def _merge_tags_if_possible(from_branch, to_branch):
-    from_branch.tags.merge_to(to_branch.tags)
+def _merge_tags_if_possible(from_branch, to_branch, ignore_master=False):
+    # Try hard to support merge_to implementations that don't expect
+    # 'ignore_master' (new in bzr 2.2.2/2.3).  First, if the flag isn't set
+    # then we can safely avoid passing ignore_master at all.
+    if not ignore_master:
+        from_branch.tags.merge_to(to_branch.tags)
+        return
+    # If the flag is set, try to pass it, but be ready to catch TypeError.
+    try:
+        from_branch.tags.merge_to(to_branch.tags, ignore_master=ignore_master)
+    except TypeError:
+        # Probably this implementation of 'merge_to' is from a plugin that
+        # doesn't expect the 'ignore_master' keyword argument (e.g. bzr-svn
+        # 1.0.4).  There's a small risk that the TypeError is actually caused
+        # by a completely different problem (which is why we don't catch it for
+        # the ignore_master=False case), but even then there's probably no harm
+        # in calling a second time.
+        symbol_versioning.warn(
+            symbol_versioning.deprecated_in((2,2,2)) % (
+                "Tags.merge_to (of %r) that doesn't accept ignore_master kwarg"
+                % (from_branch.tags,),),
+            DeprecationWarning)
+        from_branch.tags.merge_to(to_branch.tags)
 

=== modified file 'bzrlib/tests/blackbox/test_merge.py'
--- bzrlib/tests/blackbox/test_merge.py	2010-05-20 18:23:17 +0000
+++ bzrlib/tests/blackbox/test_merge.py	2010-11-09 06:33:49 +0000
@@ -23,9 +23,9 @@
 
 from bzrlib import (
     branch,
+    branchbuilder,
     bzrdir,
     conflicts,
-    errors,
     merge_directive,
     osutils,
     tests,

=== modified file 'bzrlib/tests/blackbox/test_tags.py'
--- bzrlib/tests/blackbox/test_tags.py	2010-11-01 05:21:13 +0000
+++ bzrlib/tests/blackbox/test_tags.py	2010-11-09 06:33:49 +0000
@@ -132,6 +132,18 @@
         builder.build_commit(message='Commit in fork.', rev_id='fork-1')
         return fork
 
+    def test_merge_without_commit_does_not_propagate_tags_to_master(self):
+        """'bzr merge' alone does not propagate tags to a master branch.
+
+        (If the user runs 'bzr commit', then that is when the tags from the
+        merge are propagated.)
+        """
+        master, child = self.make_master_and_checkout()
+        fork = self.make_fork(master)
+        fork.tags.set_tag('new-tag', fork.last_revision())
+        self.run_bzr(['merge', '../fork'], working_dir='child')
+        self.assertEqual({}, master.tags.get_tag_dict())
+
     def test_commit_in_heavyweight_checkout_copies_tags_to_master(self):
         master, child = self.make_master_and_checkout()
         fork = self.make_fork(master)

=== modified file 'bzrlib/tests/per_branch/test_tags.py'
--- bzrlib/tests/per_branch/test_tags.py	2010-03-12 02:14:56 +0000
+++ bzrlib/tests/per_branch/test_tags.py	2010-11-09 06:33:49 +0000
@@ -144,6 +144,176 @@
         b1.tags.merge_to(b2.tags)
 
 
+class TestTagsMergeToInCheckouts(per_branch.TestCaseWithBranch):
+    """Tests for checkout.branch.tags.merge_to.
+    
+    In particular this exercises variations in tag conflicts in the master
+    branch and/or the checkout (child).  It may seem strange to have different
+    tags in the child and master, but 'bzr merge' intentionally updates the
+    child and not the master (instead the next 'bzr commit', if the user
+    decides to commit, will update the master).  Also, merge_to in bzr < 2.2.2
+    didn't propagate changes to the master, and current bzr versions may find
+    themselves operating on checkouts touched by older bzrs
+    
+    So we need to make sure bzr copes gracefully with differing tags in the
+    master versus the child.
+
+    See also <https://bugs.launchpad.net/bzr/+bug/603395>.
+    """
+
+    def setUp(self):
+        super(TestTagsMergeToInCheckouts, self).setUp()
+        branch1 = self.make_branch('tags-probe')
+        if not branch1._format.supports_tags():
+            raise tests.TestSkipped(
+                "format %s doesn't support tags" % branch1._format)
+        branch2 = self.make_branch('bind-probe')
+        try:
+            branch2.bind(branch1)
+        except errors.UpgradeRequired:
+            raise tests.TestNotApplicable(
+                "format %s doesn't support bound branches" % branch2._format)
+
+    def test_merge_to_propagates_tags(self):
+        """merge_to(child) also merges tags to the master."""
+        master = self.make_branch('master')
+        other = self.make_branch('other')
+        other.tags.set_tag('foo', 'rev-1')
+        child = self.make_branch('child')
+        child.bind(master)
+        child.update()
+        other.tags.merge_to(child.tags)
+        self.assertEquals('rev-1', child.tags.lookup_tag('foo'))
+        self.assertEquals('rev-1', master.tags.lookup_tag('foo'))
+
+    def test_ignore_master_disables_tag_propagation(self):
+        """merge_to(child, ignore_master=True) does not merge tags to the
+        master.
+        """
+        master = self.make_branch('master')
+        other = self.make_branch('other')
+        other.tags.set_tag('foo', 'rev-1')
+        child = self.make_branch('child')
+        child.bind(master)
+        child.update()
+        other.tags.merge_to(child.tags, ignore_master=True)
+        self.assertEquals('rev-1', child.tags.lookup_tag('foo'))
+        self.assertRaises(errors.NoSuchTag, master.tags.lookup_tag, 'foo')
+
+    def test_merge_to_overwrite_conflict_in_master(self):
+        """merge_to(child, overwrite=True) overwrites any conflicting tags in
+        the master.
+        """
+        master = self.make_branch('master')
+        other = self.make_branch('other')
+        other.tags.set_tag('foo', 'rev-1')
+        child = self.make_branch('child')
+        child.bind(master)
+        child.update()
+        master.tags.set_tag('foo', 'rev-2')
+        tag_conflicts = other.tags.merge_to(child.tags, overwrite=True)
+        self.assertEquals('rev-1', child.tags.lookup_tag('foo'))
+        self.assertEquals('rev-1', master.tags.lookup_tag('foo'))
+        self.assertLength(0, tag_conflicts)
+
+    def test_merge_to_overwrite_conflict_in_child_and_master(self):
+        """merge_to(child, overwrite=True) overwrites any conflicting tags in
+        both the child and the master.
+        """
+        master = self.make_branch('master')
+        master.tags.set_tag('foo', 'rev-2')
+        other = self.make_branch('other')
+        other.tags.set_tag('foo', 'rev-1')
+        child = self.make_branch('child')
+        child.bind(master)
+        child.update()
+        tag_conflicts = other.tags.merge_to(child.tags, overwrite=True)
+        self.assertEquals('rev-1', child.tags.lookup_tag('foo'))
+        self.assertEquals('rev-1', master.tags.lookup_tag('foo'))
+        self.assertLength(0, tag_conflicts)
+
+    def test_merge_to_conflict_in_child_only(self):
+        """When new_tags.merge_to(child.tags) conflicts with the child but not
+        the master, a conflict is reported and the child receives the new tag.
+        """
+        master = self.make_branch('master')
+        master.tags.set_tag('foo', 'rev-2')
+        other = self.make_branch('other')
+        other.tags.set_tag('foo', 'rev-1')
+        child = self.make_branch('child')
+        child.bind(master)
+        child.update()
+        master.tags.delete_tag('foo')
+        tag_conflicts = other.tags.merge_to(child.tags)
+        # Conflict in child, so it is unchanged.
+        self.assertEquals('rev-2', child.tags.lookup_tag('foo'))
+        # No conflict in the master, so the 'foo' tag equals other's value here.
+        self.assertEquals('rev-1', master.tags.lookup_tag('foo'))
+        # The conflict is reported.
+        self.assertEquals([(u'foo', 'rev-1', 'rev-2')], tag_conflicts)
+
+    def test_merge_to_conflict_in_master_only(self):
+        """When new_tags.merge_to(child.tags) conflicts with the master but not
+        the child, a conflict is reported and the child receives the new tag.
+        """
+        master = self.make_branch('master')
+        other = self.make_branch('other')
+        other.tags.set_tag('foo', 'rev-1')
+        child = self.make_branch('child')
+        child.bind(master)
+        child.update()
+        master.tags.set_tag('foo', 'rev-2')
+        tag_conflicts = other.tags.merge_to(child.tags)
+        # No conflict in the child, so the 'foo' tag equals other's value here.
+        self.assertEquals('rev-1', child.tags.lookup_tag('foo'))
+        # Conflict in master, so it is unchanged.
+        self.assertEquals('rev-2', master.tags.lookup_tag('foo'))
+        # The conflict is reported.
+        self.assertEquals([(u'foo', 'rev-1', 'rev-2')], tag_conflicts)
+
+    def test_merge_to_same_conflict_in_master_and_child(self):
+        """When new_tags.merge_to(child.tags) conflicts the same way with the
+        master and the child a single conflict is reported.
+        """
+        master = self.make_branch('master')
+        master.tags.set_tag('foo', 'rev-2')
+        other = self.make_branch('other')
+        other.tags.set_tag('foo', 'rev-1')
+        child = self.make_branch('child')
+        child.bind(master)
+        child.update()
+        tag_conflicts = other.tags.merge_to(child.tags)
+        # Both master and child conflict, so both stay as rev-2
+        self.assertEquals('rev-2', child.tags.lookup_tag('foo'))
+        self.assertEquals('rev-2', master.tags.lookup_tag('foo'))
+        # The conflict is reported exactly once, even though it occurs in both
+        # master and child.
+        self.assertEquals([(u'foo', 'rev-1', 'rev-2')], tag_conflicts)
+
+    def test_merge_to_different_conflict_in_master_and_child(self):
+        """When new_tags.merge_to(child.tags) conflicts differently in the
+        master and the child both conflicts are reported.
+        """
+        master = self.make_branch('master')
+        master.tags.set_tag('foo', 'rev-2')
+        other = self.make_branch('other')
+        other.tags.set_tag('foo', 'rev-1')
+        child = self.make_branch('child')
+        child.bind(master)
+        child.update()
+        # We use the private method _set_tag_dict because normally bzr tries to
+        # avoid this scenario.
+        child.tags._set_tag_dict({'foo': 'rev-3'})
+        tag_conflicts = other.tags.merge_to(child.tags)
+        # Both master and child conflict, so both stay as they were.
+        self.assertEquals('rev-3', child.tags.lookup_tag('foo'))
+        self.assertEquals('rev-2', master.tags.lookup_tag('foo'))
+        # Both conflicts are reported.
+        self.assertEquals(
+            [(u'foo', 'rev-1', 'rev-2'), (u'foo', 'rev-1', 'rev-3')],
+            sorted(tag_conflicts))
+
+
 class TestUnsupportedTags(per_branch.TestCaseWithBranch):
     """Formats that don't support tags should give reasonable errors."""
 

=== modified file 'bzrlib/tests/test_tag.py'
--- bzrlib/tests/test_tag.py	2010-03-25 09:39:03 +0000
+++ bzrlib/tests/test_tag.py	2010-11-09 06:33:49 +0000
@@ -120,6 +120,9 @@
 
 
 class TestTagsInCheckouts(TestCaseWithTransport):
+    """Tests for how tags are synchronised between the master and child branch
+    of a checkout.
+    """
 
     def test_update_tag_into_checkout(self):
         # checkouts are directly connected to the tags of their master branch: