Merge lp:~lifeless/bzr/repo-source into lp:~bzr/bzr/trunk-old

[Robert Collins]

[Robert Collins (lifeless)]

This fixes bug 376255, which I believe John is looking at vis-a-vis
stacking.

-Rob

[Martin Pool (mbp)]

I'd like the test to have the URL of the bug it relates to, as a pointer in case it regresses or people want more information about the test.

It seems plausible to me but it's not obvious why _get_keys should omit ghosts. Maybe the docstring for get_keys or PendingAncestryResult should say so?

review: Needs Fixing

[Robert Collins (lifeless)]

On Thu, 2009-05-14 at 08:43 +0000, Martin Pool wrote:
> Review: Needs Fixing
> I'd like the test to have the URL of the bug it relates to, as a
> pointer in case it regresses or people want more information about the
> test.

Done.

> It seems plausible to me but it's not obvious why _get_keys should
> omit ghosts. Maybe the docstring for get_keys or
> PendingAncestryResult should say so?

Perhaps; this api is under discussion at the moment, so rather than
muddy the waters, we'll just make it clearer when we reach consensus.

-Rob

[John A Meinel (jameinel)]

So "PendingAncestryResult" is a shorthand for a "SearchResult" which doesn't actually walk the whole graph.

If you actually did the SearchResult, you would end up finding the ghosts, and at that point would explicitly mark them into 'exclude_keys'.

This is essentially just folding that into PendingAncestryResult.get_keys(), so that it gives the same keys as the equivalent SearchResult.get_keys().

I'll probably work on this more later, and actually change PendingAncestryResult to track stuff like ghosts, and thus keep track of 'tails' or 'exclude_keys' or whatever you want to call it.

We certainly need this for CHK stacking, I have it already in my CHK stacking branch, although with a different test. (I test for a non-mainline ghost, but it is the same for PAR).

review: Approve

=== modified file 'bzrlib/graph.py'
--- bzrlib/graph.py	2009-04-04 02:50:01 +0000
+++ bzrlib/graph.py	2009-05-14 08:35:10 +0000
@@ -1556,7 +1556,7 @@
     def _get_keys(self, graph):
         NULL_REVISION = revision.NULL_REVISION
         keys = [key for (key, parents) in graph.iter_ancestry(self.heads)
-                if key != NULL_REVISION]
+                if key != NULL_REVISION and parents is not None]
         return keys
 
     def is_empty(self):

=== modified file 'bzrlib/tests/per_repository/test_fetch.py'
--- bzrlib/tests/per_repository/test_fetch.py	2009-05-07 05:08:46 +0000
+++ bzrlib/tests/per_repository/test_fetch.py	2009-05-14 08:35:11 +0000
@@ -20,6 +20,7 @@
     bzrdir,
     errors,
     gpg,
+    graph,
     remote,
     repository,
     )
@@ -298,3 +299,29 @@
         revision_id = tree.commit('test')
         repo.fetch(tree.branch.repository)
         repo.fetch(tree.branch.repository)
+
+
+class TestSource(TestCaseWithRepository):
+    """Tests for/about the results of Repository._get_source."""
+
+    def test_no_absent_records_in_stream_with_ghosts(self):
+        # XXX: Arguably should be in interrepository_implementations but
+        # doesn't actually gain coverage there; need a specific set of
+        # permutations to cover it.
+        builder = self.make_branch_builder('repo')
+        builder.start_series()
+        builder.build_snapshot('tip', ['ghost'],
+            [('add', ('', 'ROOT_ID', 'directory', ''))],
+            allow_leftmost_as_ghost=True)
+        builder.finish_series()
+        b = builder.get_branch()
+        b.lock_read()
+        self.addCleanup(b.unlock)
+        repo = b.repository
+        source = repo._get_source(repo._format)
+        search = graph.PendingAncestryResult(['tip'], repo)
+        stream = source.get_stream(search)
+        for substream_type, substream in stream:
+            for record in substream:
+                self.assertNotEqual('absent', record.storage_kind,
+                    "Absent record for %s" % (((substream_type,) + record.key),))