Merge lp:~jelmer/bzr/fix-version-info-revision into lp:bzr

[Jelmer Vernooij]

Fix -r argument for 'bzr version-info' to actually use specified tree everywhere.

Follow up to one of my earlier patches, which introduced a -r argument to "bzr version-info". This makes us not mix the working tree with the tree from the -r revision.

Split up version info tests into smaller tests.

[Martin Packman (gz)]

Fix looks sensible, as do test refactorings which pass locally.

+ builder = RioVersionInfoBuilder(wt.branch, working_tree=wt,
+ **kwargs)

Would fit on one line now it's dedented.

+ builder = RioVersionInfoBuilder(wt.branch, working_tree=wt,
+ **kwargs)

Likewise.

+ new_stanzas = list(read_stanzas(
+ StringIO(stanza[key].encode('utf8'))))

This would too, or at least should be slightly less oddly offset.

review: Approve

[Jelmer Vernooij (jelmer)]

sent to pqm by email

=== modified file 'bzrlib/cmd_version_info.py'
--- bzrlib/cmd_version_info.py	2011-12-19 13:23:58 +0000
+++ bzrlib/cmd_version_info.py	2012-01-03 13:16:29 +0000
@@ -107,11 +107,11 @@
         if format is None:
             format = version_info_formats.format_registry.get()
 
-        wt = None
         try:
             wt = workingtree.WorkingTree.open_containing(location)[0]
         except errors.NoWorkingTree:
             b = branch.Branch.open(location)
+            wt = None
         else:
             b = wt.branch
 

=== modified file 'bzrlib/tests/blackbox/test_version_info.py'
--- bzrlib/tests/blackbox/test_version_info.py	2011-10-31 15:49:35 +0000
+++ bzrlib/tests/blackbox/test_version_info.py	2012-01-03 13:16:29 +0000
@@ -18,10 +18,10 @@
 
 import os
 
-from bzrlib.revision import NULL_REVISION
 from bzrlib.tests import TestCaseWithTransport
 from bzrlib.version_info_formats import VersionInfoBuilder
 
+
 class TestVersionInfo(TestCaseWithTransport):
 
     def test_invalid_format(self):
@@ -163,16 +163,16 @@
 
     def test_non_ascii(self):
         """Test that we can output non-ascii data"""
-        
+
         commit_message = u'Non-ascii message with character not in latin-1: \u1234'
-        
+
         tree = self.make_branch_and_tree('.')
         self.build_tree(['a_file'])
         tree.add('a_file')
         tree.commit(commit_message)
         out, err = self.run_bzr(
             ['version-info', '--include-history'], encoding='latin-1')
-        
+
         self.assertContainsString(out, commit_message.encode('utf-8'))
 
     def test_revision(self):

=== modified file 'bzrlib/tests/test_version_info.py'
--- bzrlib/tests/test_version_info.py	2011-06-14 17:01:15 +0000
+++ bzrlib/tests/test_version_info.py	2012-01-03 13:16:29 +0000
@@ -35,7 +35,7 @@
 from bzrlib.version_info_formats.format_python import PythonVersionInfoBuilder
 
 
-class TestVersionInfo(TestCaseWithTransport):
+class VersionInfoTestCase(TestCaseWithTransport):
 
     def create_branch(self):
         wt = self.make_branch_and_tree('branch')
@@ -72,6 +72,9 @@
 
         return wt
 
+
+class TestVersionInfoRio(VersionInfoTestCase):
+
     def test_rio_null(self):
         wt = self.make_branch_and_tree('branch')
 
@@ -91,32 +94,37 @@
         val = sio.getvalue()
         self.assertContainsRe(val, 'revno: 1.1.1')
 
-    def test_rio_version_text(self):
+    def regen_text(self, wt, **kwargs):
+        sio = StringIO()
+        builder = RioVersionInfoBuilder(wt.branch, working_tree=wt, **kwargs)
+        builder.generate(sio)
+        val = sio.getvalue()
+        return val
+
+    def test_simple(self):
         wt = self.create_branch()
 
-        def regen(**kwargs):
-            sio = StringIO()
-            builder = RioVersionInfoBuilder(wt.branch, working_tree=wt,
-                                            **kwargs)
-            builder.generate(sio)
-            val = sio.getvalue()
-            return val
-
-        val = regen()
+        val = self.regen_text(wt)
         self.assertContainsRe(val, 'build-date:')
         self.assertContainsRe(val, 'date:')
         self.assertContainsRe(val, 'revno: 3')
         self.assertContainsRe(val, 'revision-id: r3')
 
-        val = regen(check_for_clean=True)
+    def test_clean(self):
+        wt = self.create_branch()
+        val = self.regen_text(wt, check_for_clean=True)
         self.assertContainsRe(val, 'clean: True')
 
+    def test_no_clean(self):
+        wt = self.create_branch()
         self.build_tree(['branch/c'])
-        val = regen(check_for_clean=True)
+        val = self.regen_text(wt, check_for_clean=True)
         self.assertContainsRe(val, 'clean: False')
-        os.remove('branch/c')
-
-        val = regen(include_revision_history=True)
+
+    def test_history(self):
+        wt = self.create_branch()
+
+        val = self.regen_text(wt, include_revision_history=True)
         self.assertContainsRe(val, 'id: r1')
         self.assertContainsRe(val, 'message: a')
         self.assertContainsRe(val, 'id: r2')
@@ -124,6 +132,15 @@
         self.assertContainsRe(val, 'id: r3')
         self.assertContainsRe(val, 'message: \xc3\xa52') # utf8 encoding '\xe5'
 
+    def regen(self, wt, **kwargs):
+        sio = StringIO()
+        builder = RioVersionInfoBuilder(wt.branch, working_tree=wt, **kwargs)
+        builder.generate(sio)
+        sio.seek(0)
+        stanzas = list(read_stanzas(sio))
+        self.assertEqual(1, len(stanzas))
+        return stanzas[0]
+
     def test_rio_version_hook(self):
         def update_stanza(rev, stanza):
             stanza.add('bla', 'bloe')
@@ -131,86 +148,93 @@
             'revision', update_stanza, None)
         wt = self.create_branch()
 
-        def regen(**kwargs):
-            sio = StringIO()
-            builder = RioVersionInfoBuilder(wt.branch, working_tree=wt,
-                                            **kwargs)
-            builder.generate(sio)
-            sio.seek(0)
-            stanzas = list(read_stanzas(sio))
-            self.assertEqual(1, len(stanzas))
-            return stanzas[0]
-
-        stanza = regen()
+        stanza = self.regen(wt)
         self.assertEqual(['bloe'], stanza.get_all('bla'))
 
-    def test_rio_version(self):
+    def get_one_stanza(self, stanza, key):
+        new_stanzas = list(read_stanzas(StringIO(stanza[key].encode('utf8'))))
+        self.assertEqual(1, len(new_stanzas))
+        return new_stanzas[0]
+
+    def test_build_date(self):
         wt = self.create_branch()
-
-        def regen(**kwargs):
-            sio = StringIO()
-            builder = RioVersionInfoBuilder(wt.branch, working_tree=wt,
-                                            **kwargs)
-            builder.generate(sio)
-            sio.seek(0)
-            stanzas = list(read_stanzas(sio))
-            self.assertEqual(1, len(stanzas))
-            return stanzas[0]
-
-        def get_one_stanza(stanza, key):
-            new_stanzas = list(read_stanzas(
-                                StringIO(stanza[key].encode('utf8'))))
-            self.assertEqual(1, len(new_stanzas))
-            return new_stanzas[0]
-
-        stanza = regen()
+        stanza = self.regen(wt)
         self.assertTrue('date' in stanza)
         self.assertTrue('build-date' in stanza)
         self.assertEqual(['3'], stanza.get_all('revno'))
         self.assertEqual(['r3'], stanza.get_all('revision-id'))
 
-        stanza = regen(check_for_clean=True)
-        self.assertEqual(['True'], stanza.get_all('clean'))
-
+    def test_not_clean(self):
+        wt = self.create_branch()
         self.build_tree(['branch/c'])
-        stanza = regen(check_for_clean=True, include_file_revisions=True)
+        stanza = self.regen(wt, check_for_clean=True, include_file_revisions=True)
         self.assertEqual(['False'], stanza.get_all('clean'))
 
+    def test_file_revisions(self):
+        wt = self.create_branch()
+        self.build_tree(['branch/c'])
+        stanza = self.regen(wt, check_for_clean=True, include_file_revisions=True)
         # This assumes it's being run against a tree that does not update the
         # root revision on every commit.
-        file_rev_stanza = get_one_stanza(stanza, 'file-revisions')
+        file_rev_stanza = self.get_one_stanza(stanza, 'file-revisions')
         self.assertEqual(['', 'a', 'b', 'c'], file_rev_stanza.get_all('path'))
         self.assertEqual(['r1', 'r3', 'r2', 'unversioned'],
             file_rev_stanza.get_all('revision'))
-        os.remove('branch/c')
 
-        stanza = regen(include_revision_history=True)
-        revision_stanza = get_one_stanza(stanza, 'revisions')
+    def test_revision_history(self):
+        wt = self.create_branch()
+        stanza = self.regen(wt, include_revision_history=True)
+        revision_stanza = self.get_one_stanza(stanza, 'revisions')
         self.assertEqual(['r1', 'r2', 'r3'], revision_stanza.get_all('id'))
         self.assertEqual(['a', 'b', u'\xe52'], revision_stanza.get_all('message'))
         self.assertEqual(3, len(revision_stanza.get_all('date')))
 
+    def test_file_revisions_with_rename(self):
         # a was modified, so it should show up modified again
+        wt = self.create_branch()
         self.build_tree(['branch/a', 'branch/c'])
         wt.add('c')
         wt.rename_one('b', 'd')
-        stanza = regen(check_for_clean=True, include_file_revisions=True)
-        file_rev_stanza = get_one_stanza(stanza, 'file-revisions')
+        stanza = self.regen(wt, check_for_clean=True, include_file_revisions=True)
+        file_rev_stanza = self.get_one_stanza(stanza, 'file-revisions')
         self.assertEqual(['', 'a', 'b', 'c', 'd'],
                           file_rev_stanza.get_all('path'))
         self.assertEqual(['r1', 'modified', 'renamed to d', 'new',
                           'renamed from b'],
                          file_rev_stanza.get_all('revision'))
 
+    def test_file_revisions_with_removal(self):
+        wt = self.create_branch()
+        self.build_tree(['branch/a', 'branch/c'])
+        wt.add('c')
+        wt.rename_one('b', 'd')
+
         wt.commit('modified', rev_id='r4')
+
         wt.remove(['c', 'd'])
         os.remove('branch/d')
-        stanza = regen(check_for_clean=True, include_file_revisions=True)
-        file_rev_stanza = get_one_stanza(stanza, 'file-revisions')
+        stanza = self.regen(wt, check_for_clean=True, include_file_revisions=True)
+        file_rev_stanza = self.get_one_stanza(stanza, 'file-revisions')
         self.assertEqual(['', 'a', 'c', 'd'], file_rev_stanza.get_all('path'))
         self.assertEqual(['r1', 'r4', 'unversioned', 'removed'],
                          file_rev_stanza.get_all('revision'))
 
+    def test_revision(self):
+        wt = self.create_branch()
+        self.build_tree(['branch/a', 'branch/c'])
+        wt.add('c')
+        wt.rename_one('b', 'd')
+
+        stanza = self.regen(wt, check_for_clean=True,
+            include_file_revisions=True, revision_id=wt.last_revision())
+        file_rev_stanza = self.get_one_stanza(stanza, 'file-revisions')
+        self.assertEqual(['', 'a', 'b'], file_rev_stanza.get_all('path'))
+        self.assertEqual(['r1', 'r3', 'r2'],
+                         file_rev_stanza.get_all('revision'))
+
+
+class PythonVersionInfoTests(VersionInfoTestCase):
+
     def test_python_null(self):
         wt = self.make_branch_and_tree('branch')
 
@@ -231,45 +255,45 @@
         val = sio.getvalue()
         self.assertContainsRe(val, "'revno': '1.1.1'")
 
+    def regen(self, wt, **kwargs):
+        """Create a test module, import and return it"""
+        outf = open('test_version_information.py', 'wb')
+        try:
+            builder = PythonVersionInfoBuilder(wt.branch, working_tree=wt,
+                                               **kwargs)
+            builder.generate(outf)
+        finally:
+            outf.close()
+        module_info = imp.find_module('test_version_information',
+                                      [os.getcwdu()])
+        tvi = imp.load_module('tvi', *module_info)
+        # Make sure the module isn't cached
+        sys.modules.pop('tvi', None)
+        sys.modules.pop('test_version_information', None)
+        # Delete the compiled versions, because we are generating
+        # a new file fast enough that python doesn't detect it
+        # needs to recompile, and using sleep() just makes the
+        # test slow
+        if os.path.exists('test_version_information.pyc'):
+            os.remove('test_version_information.pyc')
+        if os.path.exists('test_version_information.pyo'):
+            os.remove('test_version_information.pyo')
+        return tvi
+
     def test_python_version(self):
         wt = self.create_branch()
 
-        def regen(**kwargs):
-            """Create a test module, import and return it"""
-            outf = open('test_version_information.py', 'wb')
-            try:
-                builder = PythonVersionInfoBuilder(wt.branch, working_tree=wt,
-                                                   **kwargs)
-                builder.generate(outf)
-            finally:
-                outf.close()
-            module_info = imp.find_module('test_version_information',
-                                          [os.getcwdu()])
-            tvi = imp.load_module('tvi', *module_info)
-            # Make sure the module isn't cached
-            sys.modules.pop('tvi', None)
-            sys.modules.pop('test_version_information', None)
-            # Delete the compiled versions, because we are generating
-            # a new file fast enough that python doesn't detect it
-            # needs to recompile, and using sleep() just makes the
-            # test slow
-            if os.path.exists('test_version_information.pyc'):
-                os.remove('test_version_information.pyc')
-            if os.path.exists('test_version_information.pyo'):
-                os.remove('test_version_information.pyo')
-            return tvi
-
-        tvi = regen()
+        tvi = self.regen(wt)
         self.assertEqual('3', tvi.version_info['revno'])
         self.assertEqual('r3', tvi.version_info['revision_id'])
         self.assertTrue(tvi.version_info.has_key('date'))
         self.assertEqual(None, tvi.version_info['clean'])
 
-        tvi = regen(check_for_clean=True)
+        tvi = self.regen(wt, check_for_clean=True)
         self.assertEqual(True, tvi.version_info['clean'])
 
         self.build_tree(['branch/c'])
-        tvi = regen(check_for_clean=True, include_file_revisions=True)
+        tvi = self.regen(wt, check_for_clean=True, include_file_revisions=True)
         self.assertEqual(False, tvi.version_info['clean'])
         self.assertEqual(['', 'a', 'b', 'c'],
                          sorted(tvi.file_revisions.keys()))
@@ -278,7 +302,7 @@
         self.assertEqual('unversioned', tvi.file_revisions['c'])
         os.remove('branch/c')
 
-        tvi = regen(include_revision_history=True)
+        tvi = self.regen(wt, include_revision_history=True)
 
         rev_info = [(rev, message) for rev, message, timestamp, timezone
                                    in tvi.revisions]
@@ -288,7 +312,7 @@
         self.build_tree(['branch/a', 'branch/c'])
         wt.add('c')
         wt.rename_one('b', 'd')
-        tvi = regen(check_for_clean=True, include_file_revisions=True)
+        tvi = self.regen(wt, check_for_clean=True, include_file_revisions=True)
         self.assertEqual(['', 'a', 'b', 'c', 'd'],
                           sorted(tvi.file_revisions.keys()))
         self.assertEqual('modified', tvi.file_revisions['a'])
@@ -299,13 +323,16 @@
         wt.commit('modified', rev_id='r4')
         wt.remove(['c', 'd'])
         os.remove('branch/d')
-        tvi = regen(check_for_clean=True, include_file_revisions=True)
+        tvi = self.regen(wt, check_for_clean=True, include_file_revisions=True)
         self.assertEqual(['', 'a', 'c', 'd'],
                           sorted(tvi.file_revisions.keys()))
         self.assertEqual('r4', tvi.file_revisions['a'])
         self.assertEqual('unversioned', tvi.file_revisions['c'])
         self.assertEqual('removed', tvi.file_revisions['d'])
 
+
+class CustomVersionInfoTests(VersionInfoTestCase):
+
     def test_custom_null(self):
         sio = StringIO()
         wt = self.make_branch_and_tree('branch')
@@ -328,32 +355,41 @@
         builder.generate(sio)
         self.assertEquals("1.1.1 revid: o2", sio.getvalue())
 
-    def test_custom_version_text(self):
+    def regen(self, wt, tpl, **kwargs):
+        sio = StringIO()
+        builder = CustomVersionInfoBuilder(wt.branch, working_tree=wt,
+                                           template=tpl, **kwargs)
+        builder.generate(sio)
+        val = sio.getvalue()
+        return val
+
+    def test_build_date(self):
         wt = self.create_branch()
 
-        def regen(tpl, **kwargs):
-            sio = StringIO()
-            builder = CustomVersionInfoBuilder(wt.branch, working_tree=wt,
-                                               template=tpl, **kwargs)
-            builder.generate(sio)
-            val = sio.getvalue()
-            return val
-
-        val = regen('build-date: "{build_date}"\ndate: "{date}"')
+        val = self.regen(wt, 'build-date: "{build_date}"\ndate: "{date}"')
         self.assertContainsRe(val, 'build-date: "[0-9-+: ]+"')
         self.assertContainsRe(val, 'date: "[0-9-+: ]+"')
 
-        val = regen('revno: {revno}')
+    def test_revno(self):
+        wt = self.create_branch()
+        val = self.regen(wt, 'revno: {revno}')
         self.assertEqual(val, 'revno: 3')
 
-        val = regen('revision-id: {revision_id}')
+    def test_revision_id(self):
+        wt = self.create_branch()
+        val = self.regen(wt, 'revision-id: {revision_id}')
         self.assertEqual(val, 'revision-id: r3')
 
-        val = regen('clean: {clean}', check_for_clean=True)
+    def test_clean(self):
+        wt = self.create_branch()
+        val = self.regen(wt, 'clean: {clean}', check_for_clean=True)
         self.assertEqual(val, 'clean: 1')
 
+    def test_not_clean(self):
+        wt = self.create_branch()
+
         self.build_tree(['branch/c'])
-        val = regen('clean: {clean}', check_for_clean=True)
+        val = self.regen(wt, 'clean: {clean}', check_for_clean=True)
         self.assertEqual(val, 'clean: 0')
         os.remove('branch/c')
 

=== modified file 'bzrlib/version_info_formats/__init__.py'
--- bzrlib/version_info_formats/__init__.py	2011-12-19 13:23:58 +0000
+++ bzrlib/version_info_formats/__init__.py	2012-01-03 13:16:29 +0000
@@ -70,7 +70,6 @@
         :param revision_id: Revision id to print version for (optional)
         """
         self._branch = branch
-        self._working_tree = working_tree
         self._check = check_for_clean
         self._include_history = include_revision_history
         self._include_file_revs = include_file_revisions
@@ -80,19 +79,27 @@
         self._file_revisions = {}
         self._revision_id = revision_id
 
+        if self._revision_id is None:
+            self._tree = working_tree
+            self._working_tree = working_tree
+        else:
+            self._tree = self._branch.repository.revision_tree(self._revision_id)
+            # the working tree is not relevant if an explicit revision was specified
+            self._working_tree = None
+
     def _extract_file_revisions(self):
         """Extract the working revisions for all files"""
 
         # Things seem clean if we never look :)
         self._clean = True
 
-        if self._working_tree is not None:
+        if self._working_tree is self._tree:
             basis_tree = self._working_tree.basis_tree()
             # TODO: jam 20070215 The working tree should actually be locked at
             #       a higher level, but this will do for now.
             self._working_tree.lock_read()
         else:
-            basis_tree = self._branch.basis_tree()
+            basis_tree = self._branch.repository.revision_tree(self._revision_id)
 
         basis_tree.lock_read()
         try:
@@ -100,7 +107,7 @@
             for info in basis_tree.list_files(include_root=True):
                 self._file_revisions[info[0]] = info[-1].revision
 
-            if not self._check or self._working_tree is None:
+            if not self._check or self._working_tree is not self._tree:
                 return
 
             delta = self._working_tree.changes_from(basis_tree,
@@ -140,11 +147,7 @@
     def _iter_revision_history(self):
         """Find the messages for all revisions in history."""
 
-        # Unfortunately, there is no WorkingTree.revision_history
-        if self._working_tree is not None:
-            last_rev = self._working_tree.last_revision()
-        else:
-            last_rev = self._branch.last_revision()
+        last_rev = self._get_revision_id()
 
         repository =  self._branch.repository
         repository.lock_read()