Merge lp:~jelmer/bzr/result-mentions-tags into lp:bzr

[Jelmer Vernooij]

Mention the number of tags that was pushed/pull, if any.

Mention the number of tags that was pushed/pull, if any.

[Vincent Ladeuil (vila)]

[needsinfo] It seems merge_to changes caused problems in the past
(see bzrlib.tags._merge_tags_if_possible), what's your feeling on
changing the signature again and the possible fallouts ? (loom ?)

Or did I misread the diff and merge_to didn't return anything
before ? Or does it matter nevertheless ?

[needsfixing] There is a lot of duplication between
PullResult.report() and BranchPushResult.report()

But one is using to_file_write.write() and the other
trace.note(). This doesn't feel right, shouldn't they both
provide the same feedback to the user ?

[PEP8 nit]
75 + result.tag_updates, result.tag_conflicts = self.source.tags.merge_to(
76 + self.target.tags, overwrite, ignore_master=not merge_tags_to_master)

lines too long.

[2.4-worthy] Shouldn't that be backported for the benefit of the
package importer ?

review: Needs Fixing

[Jelmer Vernooij (jelmer)]

I've filed a bug about the output inconsistency between "bzr pull" / "bzr push" and added a note in the code. r=vila on IRC.

[Jelmer Vernooij (jelmer)]

sent to pqm by email

[Jelmer Vernooij (jelmer)]

sent to pqm by email

[Jelmer Vernooij (jelmer)]

sent to pqm by email

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2011-08-27 16:59:43 +0000
+++ bzrlib/branch.py	2011-09-02 00:55:08 +0000
@@ -3044,6 +3044,7 @@
     :ivar local_branch: target branch if there is a Master, else None
     :ivar target_branch: Target/destination branch object. (write locked)
     :ivar tag_conflicts: A list of tag conflicts, see BasicTags.merge_to
+    :ivar tag_updates: A dict with new tags, see BasicTags.merge_to
     """
 
     @deprecated_method(deprecated_in((2, 3, 0)))
@@ -3055,11 +3056,18 @@
         return self.new_revno - self.old_revno
 
     def report(self, to_file):
+        tag_conflicts = getattr(self, "tag_conflicts", None)
+        tag_updates = getattr(self, "tag_updates", None)
         if not is_quiet():
-            if self.old_revid == self.new_revid:
-                to_file.write('No revisions to pull.\n')
-            else:
+            if self.old_revid != self.new_revid:
                 to_file.write('Now on revision %d.\n' % self.new_revno)
+            if tag_updates:
+                to_file.write('%d tag(s) updated.\n' % len(tag_updates))
+            if self.old_revid == self.new_revid and not tag_updates:
+                if not tag_conflicts:
+                    to_file.write('No revisions or tags to pull.\n')
+                else:
+                    to_file.write('No revisions to pull.\n')
         self._show_tag_conficts(to_file)
 
 
@@ -3091,11 +3099,22 @@
         return self.new_revno - self.old_revno
 
     def report(self, to_file):
-        """Write a human-readable description of the result."""
-        if self.old_revid == self.new_revid:
-            note('No new revisions to push.')
-        else:
-            note('Pushed up to revision %d.' % self.new_revno)
+        # TODO: This function gets passed a to_file, but then
+        # ignores it and calls note() instead. This is also
+        # inconsistent with PullResult(), which writes to stdout.
+        # -- JRV20110901, bug #838853
+        tag_conflicts = getattr(self, "tag_conflicts", None)
+        tag_updates = getattr(self, "tag_updates", None)
+        if not is_quiet():
+            if self.old_revid != self.new_revid:
+                note('Pushed up to revision %d.' % self.new_revno)
+            if tag_updates:
+                note('%d tag(s) updated.' % len(tag_updates))
+            if self.old_revid == self.new_revid and not tag_updates:
+                if not tag_conflicts:
+                    note('No new revisions or tags to push.')
+                else:
+                    note('No new revisions to push.')
         self._show_tag_conficts(to_file)
 
 
@@ -3409,8 +3428,8 @@
             self._update_revisions(stop_revision, overwrite=overwrite,
                     graph=graph)
         if self.source._push_should_merge_tags():
-            result.tag_conflicts = self.source.tags.merge_to(self.target.tags,
-                overwrite)
+            result.tag_updates, result.tag_conflicts = (
+                self.source.tags.merge_to(self.target.tags, overwrite))
         result.new_revno, result.new_revid = self.target.last_revision_info()
         return result
 
@@ -3499,8 +3518,9 @@
             # TODO: The old revid should be specified when merging tags, 
             # so a tags implementation that versions tags can only 
             # pull in the most recent changes. -- JRV20090506
-            result.tag_conflicts = self.source.tags.merge_to(self.target.tags,
-                overwrite, ignore_master=not merge_tags_to_master)
+            result.tag_updates, result.tag_conflicts = (
+                self.source.tags.merge_to(self.target.tags, overwrite,
+                    ignore_master=not merge_tags_to_master))
             result.new_revno, result.new_revid = self.target.last_revision_info()
             if _hook_master:
                 result.master_branch = _hook_master

=== modified file 'bzrlib/commit.py'
--- bzrlib/commit.py	2011-06-28 17:25:26 +0000
+++ bzrlib/commit.py	2011-09-02 00:55:08 +0000
@@ -464,7 +464,8 @@
         # Merge local tags to remote
         if self.bound_branch:
             self._set_progress_stage("Merging tags to master branch")
-            tag_conflicts = self.branch.tags.merge_to(self.master_branch.tags)
+            tag_updates, tag_conflicts = self.branch.tags.merge_to(
+                self.master_branch.tags)
             if tag_conflicts:
                 warning_lines = ['    ' + name for name, _, _ in tag_conflicts]
                 note("Conflicting tags in bound branch:\n" +

=== modified file 'bzrlib/tag.py'
--- bzrlib/tag.py	2011-05-18 16:42:48 +0000
+++ bzrlib/tag.py	2011-09-02 00:55:08 +0000
@@ -68,7 +68,7 @@
 
     def merge_to(self, to_tags, overwrite=False, ignore_master=False):
         # we never have anything to copy
-        pass
+        return {}, []
 
     def rename_revisions(self, rename_map):
         # No tags, so nothing to rename
@@ -201,7 +201,10 @@
             branch (if any).  Default is false (so the master will be updated).
             New in bzr 2.3.
 
-        :returns: A set of tags that conflicted, each of which is
+        :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.
         """
@@ -211,15 +214,15 @@
     def _merge_to_operation(self, operation, to_tags, overwrite, ignore_master):
         add_cleanup = operation.add_cleanup
         if self.branch == to_tags.branch:
-            return
+            return {}, []
         if not self.branch.supports_tags():
             # obviously nothing to copy
-            return
+            return {}, []
         source_dict = self.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
-            return
+            return {}, []
         # We merge_to both master and child individually.
         #
         # It's possible for master and child to have differing sets of
@@ -239,21 +242,23 @@
             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)
+        updates, conflicts = self._merge_to(to_tags, source_dict, overwrite)
         if master is not None:
-            conflicts += self._merge_to(master.tags, source_dict,
-                overwrite)
+            extra_updates, extra_conflicts = self._merge_to(master.tags,
+                source_dict, overwrite)
+            updates.update(extra_updates)
+            conflicts += extra_conflicts
         # We use set() to remove any duplicate conflicts from the master
         # branch.
-        return set(conflicts)
+        return updates, 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)
+        result, updates, conflicts = self._reconcile_tags(source_dict,
+            dest_dict, overwrite)
         if result != dest_dict:
             to_tags._set_tag_dict(result)
-        return conflicts
+        return updates, conflicts
 
     def rename_revisions(self, rename_map):
         """Rename revisions in this tags dictionary.
@@ -275,19 +280,21 @@
         * different definitions => if overwrite is False, keep destination
           value and give a warning, otherwise use the source value
 
-        :returns: (result_dict,
+        :returns: (result_dict, updates,
             [(conflicting_tag, source_target, dest_target)])
         """
         conflicts = []
+        updates = {}
         result = dict(dest_dict) # copy
         for name, target in source_dict.items():
             if name not in result or overwrite:
                 result[name] = target
+                updates[name] = target
             elif result[name] == target:
                 pass
             else:
                 conflicts.append((name, target, result[name]))
-        return result, conflicts
+        return result, updates, conflicts
 
 
 def _merge_tags_if_possible(from_branch, to_branch, ignore_master=False):

=== modified file 'bzrlib/tests/blackbox/test_non_ascii.py'
--- bzrlib/tests/blackbox/test_non_ascii.py	2011-08-04 00:17:53 +0000
+++ bzrlib/tests/blackbox/test_non_ascii.py	2011-09-02 00:55:08 +0000
@@ -267,7 +267,7 @@
 
         expected = osutils.pathjoin(osutils.getcwd(), dirname1)
         self.assertEqual(u'Using saved parent location: %s/\n'
-                'No revisions to pull.\n' % (expected,), txt)
+                'No revisions or tags to pull.\n' % (expected,), txt)
 
         self.build_tree_contents(
             [(osutils.pathjoin(dirname1, 'a'), 'and yet more\n')])

=== modified file 'bzrlib/tests/blackbox/test_pull.py'
--- bzrlib/tests/blackbox/test_pull.py	2011-08-30 08:46:10 +0000
+++ bzrlib/tests/blackbox/test_pull.py	2011-09-02 00:55:08 +0000
@@ -318,7 +318,7 @@
         # it is legal to attempt to pull an already-merged bundle
         out, err = self.run_bzr('pull ../bundle')
         self.assertEqual(err, '')
-        self.assertEqual(out, 'No revisions to pull.\n')
+        self.assertEqual(out, 'No revisions or tags to pull.\n')
 
     def test_pull_verbose_no_files(self):
         """Pull --verbose should not list modified files"""
@@ -533,3 +533,13 @@
         out = self.run_bzr(['pull','-d','to','from'],retcode=1)
         self.assertEqual(out,
             ('No revisions to pull.\nConflicting tags:\n    mytag\n', ''))
+
+    def test_pull_tag_notification(self):
+        """pulling tags with conflicts will change the exit code"""
+        # create a branch, see that --show-base fails
+        from_tree = self.make_branch_and_tree('from')
+        from_tree.branch.tags.set_tag("mytag", "somerevid")
+        to_tree = self.make_branch_and_tree('to')
+        out = self.run_bzr(['pull', '-d', 'to', 'from'])
+        self.assertEqual(out,
+            ('1 tag(s) updated.\n', ''))

=== modified file 'bzrlib/tests/blackbox/test_push.py'
--- bzrlib/tests/blackbox/test_push.py	2011-08-16 15:03:03 +0000
+++ bzrlib/tests/blackbox/test_push.py	2011-09-02 00:55:08 +0000
@@ -157,7 +157,9 @@
         self.run_bzr('push -d tree pushed-to')
         path = t.branch.get_push_location()
         out, err = self.run_bzr('push', working_dir="tree")
-        self.assertEqual('Using saved push location: %s\nNo new revisions to push.\n' % urlutils.local_path_from_url(path), err)
+        self.assertEqual('Using saved push location: %s\n'
+                         'No new revisions or tags to push.\n' %
+                         urlutils.local_path_from_url(path), err)
         out, err = self.run_bzr('push -q', working_dir="tree")
         self.assertEqual('', out)
         self.assertEqual('', err)

=== modified file 'bzrlib/tests/per_branch/test_pull.py'
--- bzrlib/tests/per_branch/test_pull.py	2011-08-09 14:18:05 +0000
+++ bzrlib/tests/per_branch/test_pull.py	2011-09-02 00:55:08 +0000
@@ -117,7 +117,7 @@
         self.assertEqual('P1', result.old_revid)
         self.assertEqual(2, result.new_revno)
         self.assertEqual('M1', result.new_revid)
-        self.assertEqual(None, result.tag_conflicts)
+        self.assertEqual([], result.tag_conflicts)
 
     def test_pull_overwrite(self):
         tree_a = self.make_branch_and_tree('tree_a')

=== modified file 'bzrlib/tests/per_branch/test_tags.py'
--- bzrlib/tests/per_branch/test_tags.py	2011-08-26 23:59:59 +0000
+++ bzrlib/tests/per_branch/test_tags.py	2011-09-02 00:55:08 +0000
@@ -93,17 +93,19 @@
         # if a tag is in the destination and not in the source, it is not
         # removed when we merge them
         b2.tags.set_tag('in-destination', 'revid')
-        result = b1.tags.merge_to(b2.tags)
-        self.assertEquals(list(result), [])
+        updates, conflicts = b1.tags.merge_to(b2.tags)
+        self.assertEquals(list(conflicts), [])
+        self.assertEquals(updates, {})
         self.assertEquals(b2.tags.lookup_tag('in-destination'), 'revid')
         # if there's a conflicting tag, it's reported -- the command line
         # interface will say "these tags couldn't be copied"
         b1.tags.set_tag('conflicts', 'revid-1')
         b2.tags.set_tag('conflicts', 'revid-2')
-        result = b1.tags.merge_to(b2.tags)
-        self.assertEquals(list(result),
+        updates, conflicts = b1.tags.merge_to(b2.tags)
+        self.assertEquals(list(conflicts),
             [('conflicts', 'revid-1', 'revid-2')])
         # and it keeps the same value
+        self.assertEquals(updates, {})
         self.assertEquals(b2.tags.lookup_tag('conflicts'), 'revid-2')
 
     def test_unicode_tag(self):
@@ -292,9 +294,10 @@
         child.bind(master)
         child.update()
         master.tags.set_tag('foo', 'rev-2')
-        tag_conflicts = other.tags.merge_to(child.tags, overwrite=True)
+        tag_updates, 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.assertEquals({"foo": "rev-1"}, tag_updates)
         self.assertLength(0, tag_conflicts)
 
     def test_merge_to_overwrite_conflict_in_child_and_master(self):
@@ -308,9 +311,11 @@
         child = self.make_branch('child')
         child.bind(master)
         child.update()
-        tag_conflicts = other.tags.merge_to(child.tags, overwrite=True)
+        tag_updates, 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.assertEquals({u'foo': 'rev-1'}, tag_updates)
         self.assertLength(0, tag_conflicts)
 
     def test_merge_to_conflict_in_child_only(self):
@@ -325,13 +330,14 @@
         child.bind(master)
         child.update()
         master.tags.delete_tag('foo')
-        tag_conflicts = other.tags.merge_to(child.tags)
+        tag_updates, 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')], list(tag_conflicts))
+        self.assertEquals({u'foo': 'rev-1'}, tag_updates)
 
     def test_merge_to_conflict_in_master_only(self):
         """When new_tags.merge_to(child.tags) conflicts with the master but not
@@ -344,12 +350,13 @@
         child.bind(master)
         child.update()
         master.tags.set_tag('foo', 'rev-2')
-        tag_conflicts = other.tags.merge_to(child.tags)
+        tag_updates, 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'}, tag_updates)
         self.assertEquals([(u'foo', 'rev-1', 'rev-2')], list(tag_conflicts))
 
     def test_merge_to_same_conflict_in_master_and_child(self):
@@ -363,12 +370,13 @@
         child = self.make_branch('child')
         child.bind(master)
         child.update()
-        tag_conflicts = other.tags.merge_to(child.tags)
+        tag_updates, 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({}, tag_updates)
         self.assertEquals([(u'foo', 'rev-1', 'rev-2')], list(tag_conflicts))
 
     def test_merge_to_different_conflict_in_master_and_child(self):
@@ -385,11 +393,12 @@
         # 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)
+        tag_updates, 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({}, tag_updates)
         self.assertEquals(
             [(u'foo', 'rev-1', 'rev-2'), (u'foo', 'rev-1', 'rev-3')],
             sorted(tag_conflicts))

=== modified file 'bzrlib/tests/per_interbranch/test_pull.py'
--- bzrlib/tests/per_interbranch/test_pull.py	2010-06-17 09:23:19 +0000
+++ bzrlib/tests/per_interbranch/test_pull.py	2011-09-02 00:55:08 +0000
@@ -99,7 +99,7 @@
         self.assertEqual('P1', result.old_revid)
         self.assertEqual(2, result.new_revno)
         self.assertEqual('M1', result.new_revid)
-        self.assertEqual(None, result.tag_conflicts)
+        self.assertEqual([], result.tag_conflicts)
 
     def test_pull_overwrite(self):
         tree_a = self.make_from_branch_and_tree('tree_a')

=== modified file 'bzrlib/tests/test_branch.py'
--- bzrlib/tests/test_branch.py	2011-05-26 08:04:46 +0000
+++ bzrlib/tests/test_branch.py	2011-09-02 00:55:08 +0000
@@ -709,5 +709,5 @@
         r.new_revid = "same-revid"
         f = StringIO()
         r.report(f)
-        self.assertEqual("No revisions to pull.\n", f.getvalue())
+        self.assertEqual("No revisions or tags to pull.\n", f.getvalue())
 

=== modified file 'bzrlib/tests/test_tag.py'
--- bzrlib/tests/test_tag.py	2011-08-06 07:32:51 +0000
+++ bzrlib/tests/test_tag.py	2011-09-02 00:55:08 +0000
@@ -109,12 +109,14 @@
         self.assertRaises(errors.NoSuchTag, a.tags.lookup_tag, 'tag-2')
         # conflicting merge
         a.tags.set_tag('tag-2', 'z')
-        conflicts = a.tags.merge_to(b.tags)
+        updates, conflicts = a.tags.merge_to(b.tags)
+        self.assertEqual({}, updates)
         self.assertEqual(list(conflicts), [('tag-2', 'z', 'y')])
         self.assertEqual('y', b.tags.lookup_tag('tag-2'))
         # overwrite conflicts
-        conflicts = a.tags.merge_to(b.tags, overwrite=True)
+        updates, conflicts = a.tags.merge_to(b.tags, overwrite=True)
         self.assertEqual(list(conflicts), [])
+        self.assertEqual({u'tag-1': 'x', u'tag-2': 'z'}, updates)
         self.assertEqual('z', b.tags.lookup_tag('tag-2'))
 
 

=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2011-09-01 18:11:38 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2011-09-02 00:55:08 +0000
@@ -102,6 +102,9 @@
   Entering an empty commit message in the message editor still triggers
   an error. (Jelmer Vernooij)
 
+* ``bzr pull`` will now mention how many tags it has updated.
+  (Jelmer Vernooij, #164450)
+
 * ``bzr tag`` no longer errors if a tag already exists but refers to the
   same revision. (Jelmer Vernooij)
 
@@ -214,6 +217,9 @@
   value in SI format (i.e. "20MB", "1GB") into its integer equivalent. 
   (Shannon Weyrick)
 
+* ``Tags.merge_to`` now returns a dictionary with the updated tags
+  and a set of conflicts, rather than just conflicts. (Jelmer Vernooij)
+
 * ``Transport`` now has a ``_parsed_url`` attribute instead of
   separate ``_user``, ``_password``, ``_port``, ``_scheme``, ``_host``
   and ``_path`` attributes. Proxies are provided for the moment but