Merge lp:~jelmer/bzr/uncommit-remove-tags into lp:bzr

[Jelmer Vernooij]

Remove tags in `bzr uncommit`.

Finishing off some code that I still had around in my working area...

Remove tags when uncommitting revisions. A new option --keep-tags prevents
this behaviour.

[Martin Pool (mbp)]

That's good.

[tweak] It's a bit safer to add new keyword parameters at the end.

[tweak] Please mention it in the user guide under uncommit.

review: Approve

[John A Meinel (jameinel)]

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

On 8/24/2011 7:30 AM, Martin Pool wrote:
> Review: Approve That's good.
>
> [tweak] It's a bit safer to add new keyword parameters at the end.
>
> [tweak] Please mention it in the user guide under uncommit.

I'm a bit concerned that we do a graph search to find what tags to
remove. I guess we've only been doing a linear search to resolve what
revisions to display?

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5UyAIACgkQJdeBCYSNAAPBegCfVrVDCtKFCvmBgJ7OEg7WGgWc
k8oAoNeYK5humf4yWFpI2lD+t4oh2GbX
=j8AV
-----END PGP SIGNATURE-----

[Jelmer Vernooij (jelmer)]

On 08/24/2011 11:46 AM, John A Meinel wrote:
> On 8/24/2011 7:30 AM, Martin Pool wrote:
>> Review: Approve That's good.
>>
>> [tweak] It's a bit safer to add new keyword parameters at the end.
>>
>> [tweak] Please mention it in the user guide under uncommit.
> I'm a bit concerned that we do a graph search to find what tags to
> remove. I guess we've only been doing a linear search to resolve what
> revisions to display?
>
Yep. We could potentially avoid redoing the get_parent_map calls for the
revisions on the mainline, but at the cost of more code complexity.

Cheers,

Jelmer

[John A Meinel (jameinel)]

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

On 8/24/2011 12:10 PM, Jelmer Vernooij wrote:
> On 08/24/2011 11:46 AM, John A Meinel wrote:
>> On 8/24/2011 7:30 AM, Martin Pool wrote:
>>> Review: Approve That's good.
>>>
>>> [tweak] It's a bit safer to add new keyword parameters at the
>>> end.
>>>
>>> [tweak] Please mention it in the user guide under uncommit.
>> I'm a bit concerned that we do a graph search to find what tags
>> to remove. I guess we've only been doing a linear search to
>> resolve what revisions to display?
>>
> Yep. We could potentially avoid redoing the get_parent_map calls
> for the revisions on the mainline, but at the cost of more code
> complexity.
>
> Cheers,
>
> Jelmer
>

Right, if we only know about mainline, it isn't worth worrying about.
As long as things are well cached, etc.

I'd worry a bit about performance, but uncommit should usually be
local enough in scope it should be ok. Though I wish our
find_unique_ancestors code didn't have some really bad behaviors
(landing bzr-2.1 into bzr-2.4 causes a lot of graph searching, though
some of that is just because we are 'skipping over' 2 years of history.)

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5U1J8ACgkQJdeBCYSNAAP8LwCeJYZt9z82Vc0tH6bYzSy6kStN
eI0An09oAtrCdzasSV4UVpFNZ1GlhogZ
=qfkU
-----END PGP SIGNATURE-----

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2011-08-19 22:26:03 +0000
+++ bzrlib/builtins.py	2011-08-24 09:34:52 +0000
@@ -4979,6 +4979,8 @@
     takes_options = ['verbose', 'revision',
                     Option('dry-run', help='Don\'t actually make changes.'),
                     Option('force', help='Say yes to all questions.'),
+                    Option('keep-tags',
+                           help='Keep tags that point to removed revisions.'),
                     Option('local',
                            help="Only remove the commits from the local branch"
                                 " when in a checkout."
@@ -4988,9 +4990,8 @@
     aliases = []
     encoding_type = 'replace'
 
-    def run(self, location=None,
-            dry_run=False, verbose=False,
-            revision=None, force=False, local=False):
+    def run(self, location=None, dry_run=False, verbose=False,
+            revision=None, force=False, local=False, keep_tags=False):
         if location is None:
             location = u'.'
         control, relpath = bzrdir.BzrDir.open_containing(location)
@@ -5005,9 +5006,11 @@
             self.add_cleanup(tree.lock_write().unlock)
         else:
             self.add_cleanup(b.lock_write().unlock)
-        return self._run(b, tree, dry_run, verbose, revision, force, local=local)
+        return self._run(b, tree, dry_run, verbose, revision, force,
+                         local, keep_tags)
 
-    def _run(self, b, tree, dry_run, verbose, revision, force, local=False):
+    def _run(self, b, tree, dry_run, verbose, revision, force, local,
+             keep_tags):
         from bzrlib.log import log_formatter, show_log
         from bzrlib.uncommit import uncommit
 
@@ -5059,7 +5062,7 @@
         mutter('Uncommitting from {%s} to {%s}',
                last_rev_id, rev_id)
         uncommit(b, tree=tree, dry_run=dry_run, verbose=verbose,
-                 revno=revno, local=local)
+                 revno=revno, local=local, keep_tags=keep_tags)
         self.outf.write('You can restore the old tip by running:\n'
              '  bzr pull . -r revid:%s\n' % last_rev_id)
 
 
=== modified file 'bzrlib/tests/blackbox/test_uncommit.py'
--- bzrlib/tests/blackbox/test_uncommit.py	2010-09-15 09:35:42 +0000
+++ bzrlib/tests/blackbox/test_uncommit.py	2011-08-24 09:34:52 +0000
@@ -18,9 +18,9 @@
 
 import os
 
-from bzrlib import uncommit, workingtree
+from bzrlib import uncommit
 from bzrlib.bzrdir import BzrDirMetaFormat1
-from bzrlib.errors import BzrError, BoundBranchOutOfDate
+from bzrlib.errors import BoundBranchOutOfDate
 from bzrlib.tests import TestCaseWithTransport
 from bzrlib.tests.script import (
     run_script,
@@ -280,3 +280,17 @@
         tree.commit(u'\u1234 message')
         out, err = self.run_bzr('uncommit --force tree', encoding='ascii')
         self.assertContainsRe(out, r'\? message')
+
+    def test_uncommit_removes_tags(self):
+        tree = self.make_branch_and_tree('tree')
+        revid = tree.commit('message')
+        tree.branch.tags.set_tag("atag", revid)
+        out, err = self.run_bzr('uncommit --force tree')
+        self.assertEquals({}, tree.branch.tags.get_tag_dict())
+
+    def test_uncommit_keep_tags(self):
+        tree = self.make_branch_and_tree('tree')
+        revid = tree.commit('message')
+        tree.branch.tags.set_tag("atag", revid)
+        out, err = self.run_bzr('uncommit --keep-tags --force tree')
+        self.assertEquals({"atag": revid}, tree.branch.tags.get_tag_dict())
 
=== modified file 'bzrlib/tests/test_uncommit.py'
--- bzrlib/tests/test_uncommit.py	2011-05-13 12:51:05 +0000
+++ bzrlib/tests/test_uncommit.py	2011-08-24 09:34:52 +0000
@@ -96,3 +96,30 @@
         # If this tree isn't bound, local=True raises an exception
         self.assertRaises(errors.LocalRequiresBoundBranch,
             uncommit.uncommit, tree.branch, tree=tree, local=True)
+
+    def test_uncommit_remove_tags(self):
+        tree, history = self.make_linear_tree()
+        self.assertEqual(history[1], tree.last_revision())
+        self.assertEqual((2, history[1]), tree.branch.last_revision_info())
+        tree.branch.tags.set_tag(u"pointsatexisting", history[0])
+        tree.branch.tags.set_tag(u"pointsatremoved", history[1])
+        uncommit.uncommit(tree.branch, tree=tree)
+        self.assertEqual(history[0], tree.last_revision())
+        self.assertEqual((1, history[0]), tree.branch.last_revision_info())
+        self.assertEqual({
+            "pointsatexisting": history[0]
+            }, tree.branch.tags.get_tag_dict())
+
+    def test_uncommit_keep_tags(self):
+        tree, history = self.make_linear_tree()
+        self.assertEqual(history[1], tree.last_revision())
+        self.assertEqual((2, history[1]), tree.branch.last_revision_info())
+        tree.branch.tags.set_tag(u"pointsatexisting", history[0])
+        tree.branch.tags.set_tag(u"pointsatremoved", history[1])
+        uncommit.uncommit(tree.branch, tree=tree, keep_tags=True)
+        self.assertEqual(history[0], tree.last_revision())
+        self.assertEqual((1, history[0]), tree.branch.last_revision_info())
+        self.assertEqual({
+            "pointsatexisting": history[0],
+            "pointsatremoved": history[1],
+            }, tree.branch.tags.get_tag_dict())
 
=== modified file 'bzrlib/uncommit.py'
--- bzrlib/uncommit.py	2011-06-16 12:50:32 +0000
+++ bzrlib/uncommit.py	2011-08-24 09:34:52 +0000
@@ -26,8 +26,29 @@
 from bzrlib.errors import BoundBranchOutOfDate
 
 
+def remove_tags(branch, graph, old_tip, new_tip):
+    """Remove tags on revisions between old_tip and new_tip.
+
+    :param branch: Branch to remove tags from
+    :param graph: Graph object for branch repository
+    :param old_tip: Old branch tip
+    :param new_tip: New branch tip
+    :return: Names of the removed tags
+    """
+    reverse_tags = branch.tags.get_reverse_tag_dict()
+    ancestors = graph.find_unique_ancestors(old_tip, [new_tip])
+    removed_tags = []
+    for revid, tags in reverse_tags.iteritems():
+        if not revid in ancestors:
+            continue
+        for tag in tags:
+            branch.tags.delete_tag(tag)
+            removed_tags.append(tag)
+    return removed_tags
+
+
 def uncommit(branch, dry_run=False, verbose=False, revno=None, tree=None,
-             local=False):
+             local=False, keep_tags=False):
     """Remove the last revision from the supplied branch.
 
     :param dry_run: Don't actually change anything
@@ -36,6 +57,8 @@
     :param local: If this branch is bound, only remove the revisions from the
         local branch. If this branch is not bound, it is an error to pass
         local=True.
+    :param keep_tags: Whether to keep tags pointing at the removed revisions
+        around.
     """
     unlockable = []
     try:
@@ -105,6 +128,8 @@
                     hook_new_tip = None
                 hook(hook_local, hook_master, old_revno, old_tip, new_revno,
                      hook_new_tip)
+            if branch.supports_tags() and not keep_tags:
+                remove_tags(branch, graph, old_tip, new_revision_id)
             if tree is not None:
                 if not _mod_revision.is_null(new_revision_id):
                     parents = [new_revision_id]
 
=== modified file 'bzrlib/vf_repository.py'
--- bzrlib/vf_repository.py	2011-08-02 11:18:43 +0000
+++ bzrlib/vf_repository.py	2011-08-24 09:34:52 +0000
@@ -419,8 +419,8 @@
                 return None, False, None
         # XXX: Friction: parent_candidates should return a list not a dict
         #      so that we don't have to walk the inventories again.
-        parent_candiate_entries = ie.parent_candidates(parent_invs)
-        head_set = self._heads(ie.file_id, parent_candiate_entries.keys())
+        parent_candidate_entries = ie.parent_candidates(parent_invs)
+        head_set = self._heads(ie.file_id, parent_candidate_entries.keys())
         heads = []
         for inv in parent_invs:
             if inv.has_id(ie.file_id):
@@ -441,7 +441,7 @@
             store = True
         if not store:
             # There is a single head, look it up for comparison
-            parent_entry = parent_candiate_entries[heads[0]]
+            parent_entry = parent_candidate_entries[heads[0]]
             # if the non-content specific data has changed, we'll be writing a
             # node:
             if (parent_entry.parent_id != ie.parent_id or
@@ -559,7 +559,7 @@
         :param iter_changes: An iter_changes iterator with the changes to apply
             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.
+            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
 
=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2011-08-23 10:27:54 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2011-08-24 09:34:52 +0000
@@ -100,6 +100,10 @@
   Entering an empty commit message in the message editor still triggers
   an error. (Jelmer Vernooij)
 
+* ``bzr uncommit`` will now remove tags that refer to removed revisions.
+  The ``--keep-tags`` option can be used to prevent this behaviour.
+  (Jelmer Vernooij, #605814)
+
 * Locations printed by ``bzr upgrade`` are now formatted before display.
   (Jelmer Vernooij)
 
 
=== modified file 'doc/en/user-guide/undoing_mistakes.txt'
--- doc/en/user-guide/undoing_mistakes.txt	2009-11-23 15:29:24 +0000
+++ doc/en/user-guide/undoing_mistakes.txt	2011-08-24 09:34:52 +0000
@@ -93,6 +93,9 @@
 one or more files. Some users like to alias ``commit`` to ``commit --strict``
 so that commits fail if unknown files are found in the tree.
 
+Tags for uncommitted revisions are removed from the branch unless
+``--keep-tags`` was specified.
+
 Note: While the ``merge`` command is not introduced until the next
 chapter, it is worth noting now that ``uncommit`` restores any pending
 merges. (Running ``bzr status`` after ``uncommit`` will show these.)