Merge lp:~jameinel/bzr/1.18-stack-and-annotate-393366 into lp:~bzr/bzr/trunk-old

[John A Meinel]

[John A Meinel (jameinel)]

We have two bugs when it comes to annotate and get_record_stream.

1) If doing VF.get_record_stream(.., 'topological', True), it was not passing the 'topological' flag down to fallback repositories. This meant that 'bzr annotate' would fail pretty much any time there was a graph that crossed a stacking boundary. As it would properly return the stacked-on revisions, but the fallback repository would return the keys in 'unordered' mode.

2) It isn't quite sufficient to just return fallback revs in 'topological' and then stacked revs in 'topological', as some of the stacked revs may actually be ancestors of some of the fallback revs.

However, I didn't fix (2) because --2a works correctly today, and I think (1) is sufficient to dramatically reduce the number of times people run into this, and is much easier to fix.

This adds tests for the behavior of get_record_stream() across all repositories that support stacking, and includes an expectFailure() for formats where I know it is broken in the complex case.

[Martin Pool (mbp)]

review: Approve

=== modified file 'NEWS'
--- NEWS	2009-07-24 03:15:56 +0000
+++ NEWS	2009-07-24 19:35:12 +0000
@@ -26,6 +26,10 @@
 Bug Fixes
 *********
 
+* Annotating on a stacked branch will now succeed in simple scenarios.
+  There are still some complex scenarios where it will fail (bug #399884)
+  (John Arbash Meinel, #393366)
+
 * Authenticating against an ssh server now uses ``auth_none`` to determine
   if password authentication is even supported. This fixes a bug where
   users would be prompted for a launchpad password, even though launchpad

=== modified file 'bzrlib/knit.py'
--- bzrlib/knit.py	2009-07-08 23:10:47 +0000
+++ bzrlib/knit.py	2009-07-24 19:35:12 +0000
@@ -1489,7 +1489,8 @@
                                                                 non_local_keys,
                                                                 positions):
                 generator = _VFContentMapGenerator(self, keys, non_local_keys,
-                                                   global_map)
+                                                   global_map,
+                                                   ordering=ordering)
                 for record in generator.get_record_stream():
                     yield record
         else:
@@ -1993,6 +1994,9 @@
 class _ContentMapGenerator(object):
     """Generate texts or expose raw deltas for a set of texts."""
 
+    def __init__(self, ordering='unordered'):
+        self._ordering = ordering
+
     def _get_content(self, key):
         """Get the content object for key."""
         # Note that _get_content is only called when the _ContentMapGenerator
@@ -2032,7 +2036,7 @@
             # Loop over fallback repositories asking them for texts - ignore
             # any missing from a particular fallback.
             for record in source.get_record_stream(missing_keys,
-                'unordered', True):
+                self._ordering, True):
                 if record.storage_kind == 'absent':
                     # Not in thie particular stream, may be in one of the
                     # other fallback vfs objects.
@@ -2170,7 +2174,7 @@
     """Content map generator reading from a VersionedFiles object."""
 
     def __init__(self, versioned_files, keys, nonlocal_keys=None,
-        global_map=None, raw_record_map=None):
+        global_map=None, raw_record_map=None, ordering='unordered'):
         """Create a _ContentMapGenerator.
 
         :param versioned_files: The versioned files that the texts are being
@@ -2184,6 +2188,7 @@
         :param raw_record_map: A unparsed raw record map to use for answering
             contents.
         """
+        _ContentMapGenerator.__init__(self, ordering=ordering)
         # The vf to source data from
         self.vf = versioned_files
         # The keys desired

=== modified file 'bzrlib/tests/per_repository_reference/__init__.py'
--- bzrlib/tests/per_repository_reference/__init__.py	2009-06-15 06:47:14 +0000
+++ bzrlib/tests/per_repository_reference/__init__.py	2009-07-24 19:35:12 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2008 Canonical Ltd
+# Copyright (C) 2008, 2009 Canonical Ltd
 #
 # 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
@@ -98,6 +98,7 @@
         'bzrlib.tests.per_repository_reference.test_check',
         'bzrlib.tests.per_repository_reference.test_default_stacking',
         'bzrlib.tests.per_repository_reference.test_fetch',
+        'bzrlib.tests.per_repository_reference.test_get_record_stream',
         'bzrlib.tests.per_repository_reference.test_get_rev_id_for_revno',
         'bzrlib.tests.per_repository_reference.test_initialize',
         'bzrlib.tests.per_repository_reference.test_unlock',

=== added file 'bzrlib/tests/per_repository_reference/test_get_record_stream.py'
--- bzrlib/tests/per_repository_reference/test_get_record_stream.py	1970-01-01 00:00:00 +0000
+++ bzrlib/tests/per_repository_reference/test_get_record_stream.py	2009-07-24 19:35:12 +0000
@@ -0,0 +1,201 @@
+# Copyright (C) 2008, 2009 Canonical Ltd
+#
+# 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
+
+"""Tests that get_record_stream() behaves itself properly when stacked."""
+
+from bzrlib import (
+    errors,
+    knit,
+    )
+from bzrlib.tests.per_repository_reference import (
+    TestCaseWithExternalReferenceRepository,
+    )
+
+
+class TestGetRecordStream(TestCaseWithExternalReferenceRepository):
+
+    def setUp(self):
+        super(TestGetRecordStream, self).setUp()
+        builder = self.make_branch_builder('all')
+        builder.start_series()
+        # Graph of revisions:
+        #
+        #   A
+        #   |\
+        #   B C
+        #   |/|
+        #   D E
+        #   |\|
+        #   F G
+        # These can be split up among the different repos as desired
+        #
+
+        builder.build_snapshot('A', None, [
+            ('add', ('', 'root-id', 'directory', None)),
+            ('add', ('file', 'f-id', 'file', 'initial content\n')),
+            ])
+        builder.build_snapshot('B', ['A'], [
+            ('modify', ('f-id', 'initial content\n'
+                                'and B content\n')),
+            ])
+        builder.build_snapshot('C', ['A'], [
+            ('modify', ('f-id', 'initial content\n'
+                                'and C content\n')),
+            ])
+        builder.build_snapshot('D', ['B', 'C'], [
+            ('modify', ('f-id', 'initial content\n'
+                                'and B content\n'
+                                'and C content\n')),
+            ])
+        builder.build_snapshot('E', ['C'], [
+            ('modify', ('f-id', 'initial content\n'
+                                'and C content\n'
+                                'and E content\n')),
+            ])
+        builder.build_snapshot('F', ['D'], [
+            ('modify', ('f-id', 'initial content\n'
+                                'and B content\n'
+                                'and C content\n'
+                                'and F content\n')),
+            ])
+        builder.build_snapshot('G', ['E', 'D'], [
+            ('modify', ('f-id', 'initial content\n'
+                                'and B content\n'
+                                'and C content\n'
+                                'and E content\n')),
+            ])
+        builder.finish_series()
+        self.all_repo = builder.get_branch().repository
+        self.all_repo.lock_read()
+        self.addCleanup(self.all_repo.unlock)
+        self.base_repo = self.make_repository('base')
+        self.stacked_repo = self.make_referring('referring', 'base')
+
+    def make_simple_split(self):
+        """Set up the repositories so that everything is in base except F"""
+        self.base_repo.fetch(self.all_repo, revision_id='G')
+        self.stacked_repo.fetch(self.all_repo, revision_id='F')
+
+    def make_complex_split(self):
+        """intermix the revisions so that base holds left stacked holds right.
+
+        base will hold
+            A B D F (and C because it is a parent of D)
+        referring will hold
+            C E G (only)
+        """
+        self.base_repo.fetch(self.all_repo, revision_id='B')
+        self.stacked_repo.fetch(self.all_repo, revision_id='C')
+        self.base_repo.fetch(self.all_repo, revision_id='F')
+        self.stacked_repo.fetch(self.all_repo, revision_id='G')
+
+    def test_unordered_fetch_simple_split(self):
+        self.make_simple_split()
+        keys = [('f-id', r) for r in 'ABCDF']
+        self.stacked_repo.lock_read()
+        self.addCleanup(self.stacked_repo.unlock)
+        stream = self.stacked_repo.texts.get_record_stream(
+            keys, 'unordered', False)
+        record_keys = set()
+        for record in stream:
+            if record.storage_kind == 'absent':
+                raise ValueError('absent record: %s' % (record.key,))
+            record_keys.add(record.key)
+        # everything should be present, we don't care about the order
+        self.assertEqual(keys, sorted(record_keys))
+
+    def test_unordered_fetch_complex_split(self):
+        self.make_complex_split()
+        keys = [('f-id', r) for r in 'ABCDEG']
+        self.stacked_repo.lock_read()
+        self.addCleanup(self.stacked_repo.unlock)
+        stream = self.stacked_repo.texts.get_record_stream(
+            keys, 'unordered', False)
+        record_keys = set()
+        for record in stream:
+            if record.storage_kind == 'absent':
+                raise ValueError('absent record: %s' % (record.key,))
+            record_keys.add(record.key)
+        # everything should be present, we don't care about the order
+        self.assertEqual(keys, sorted(record_keys))
+
+    def test_ordered_no_closure(self):
+        self.make_complex_split()
+        # Topological ordering allows B & C and D & E to be returned with
+        # either one first, so the required ordering is:
+        # [A (B C) (D E) G]
+        keys = [('f-id', r) for r in 'ABCDEG']
+        alt_1 = [('f-id', r) for r in 'ACBDEG']
+        alt_2 = [('f-id', r) for r in 'ABCEDG']
+        alt_3 = [('f-id', r) for r in 'ACBEDG']
+        self.stacked_repo.lock_read()
+        self.addCleanup(self.stacked_repo.unlock)
+        stream = self.stacked_repo.texts.get_record_stream(
+            keys, 'topological', False)
+        record_keys = []
+        for record in stream:
+            if record.storage_kind == 'absent':
+                raise ValueError('absent record: %s' % (record.key,))
+            record_keys.append(record.key)
+        self.assertTrue(record_keys in (keys, alt_1, alt_2, alt_3))
+
+    def test_ordered_fulltext_simple(self):
+        self.make_simple_split()
+        # This is a common case in asking to annotate a file that exists on a
+        # stacked branch.
+        # See https://bugs.launchpad.net/bzr/+bug/393366
+        # Topological ordering allows B & C and D & E to be returned with
+        # either one first, so the required ordering is:
+        # [A (B C) D F]
+        keys = [('f-id', r) for r in 'ABCDF']
+        alt_1 = [('f-id', r) for r in 'ACBDF']
+        self.stacked_repo.lock_read()
+        self.addCleanup(self.stacked_repo.unlock)
+        stream = self.stacked_repo.texts.get_record_stream(
+            keys, 'topological', True)
+        record_keys = []
+        for record in stream:
+            if record.storage_kind == 'absent':
+                raise ValueError('absent record: %s' % (record.key,))
+            record_keys.append(record.key)
+        self.assertTrue(record_keys in (keys, alt_1))
+
+    def test_ordered_fulltext_complex(self):
+        self.make_complex_split()
+        # Topological ordering allows B & C and D & E to be returned with
+        # either one first, so the required ordering is:
+        # [A (B C) (D E) G]
+        keys = [('f-id', r) for r in 'ABCDEG']
+        alt_1 = [('f-id', r) for r in 'ACBDEG']
+        alt_2 = [('f-id', r) for r in 'ABCEDG']
+        alt_3 = [('f-id', r) for r in 'ACBEDG']
+        self.stacked_repo.lock_read()
+        self.addCleanup(self.stacked_repo.unlock)
+        stream = self.stacked_repo.texts.get_record_stream(
+            keys, 'topological', True)
+        record_keys = []
+        for record in stream:
+            if record.storage_kind == 'absent':
+                raise ValueError('absent record: %s' % (record.key,))
+            record_keys.append(record.key)
+        # Note that currently --2a format repositories do this correctly, but
+        # KnitPack format repositories do not.
+        if isinstance(self.stacked_repo.texts, knit.KnitVersionedFiles):
+            # See https://bugs.launchpad.net/bzr/+bug/399884
+            self.expectFailure('KVF does not weave fulltexts from fallback'
+                ' repositories to preserve perfect order',
+                self.assertTrue, record_keys in (keys, alt_1, alt_2, alt_3))
+        self.assertTrue(record_keys in (keys, alt_1, alt_2, alt_3))

=== modified file 'bzrlib/tests/test_knit.py'
--- bzrlib/tests/test_knit.py	2009-07-06 19:37:07 +0000
+++ bzrlib/tests/test_knit.py	2009-07-24 19:35:12 +0000
@@ -2230,7 +2230,7 @@
         # self.assertEqual([("annotate", key_basis)], basis.calls)
         self.assertEqual([('get_parent_map', set([key_basis])),
             ('get_parent_map', set([key_basis])),
-            ('get_record_stream', [key_basis], 'unordered', True)],
+            ('get_record_stream', [key_basis], 'topological', True)],
             basis.calls)
 
     def test_check(self):
@@ -2342,9 +2342,9 @@
         # ask which fallbacks have which parents.
         self.assertEqual([
             ("get_parent_map", set([key_basis, key_basis_2, key_missing])),
-            # unordered is asked for by the underlying worker as it still
-            # buffers everything while answering - which is a problem!
-            ("get_record_stream", [key_basis_2, key_basis], 'unordered', True)],
+            # topological is requested from the fallback, because that is what
+            # was requested at the top level.
+            ("get_record_stream", [key_basis_2, key_basis], 'topological', True)],
             calls)
 
     def test_get_record_stream_unordered_deltas(self):
@@ -2571,7 +2571,7 @@
         last_call = basis.calls[-1]
         self.assertEqual('get_record_stream', last_call[0])
         self.assertEqual(set([key_left, key_right]), set(last_call[1]))
-        self.assertEqual('unordered', last_call[2])
+        self.assertEqual('topological', last_call[2])
         self.assertEqual(True, last_call[3])
 
 

=== modified file 'bzrlib/ui/text.py'
--- bzrlib/ui/text.py	2009-07-19 01:05:42 +0000
+++ bzrlib/ui/text.py	2009-07-24 19:35:13 +0000
@@ -18,6 +18,7 @@
 """Text UI, write output to the console.
 """
 
+import getpass
 import os
 import sys
 import time