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

Proposed by Robert Collins on 2009-05-14
Status: Merged
Approved by: John A Meinel on 2009-05-14
Approved revision: 4361
Merged at revision: not available
Proposed branch: lp:~lifeless/bzr/repo-source
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 54 lines
To merge this branch: bzr merge lp:~lifeless/bzr/repo-source
Reviewer Review Type Date Requested Status
John A Meinel Approve on 2009-05-14
Martin Pool 2009-05-14 Needs Fixing on 2009-05-14
Review via email: mp+6566@code.launchpad.net
To post a comment you must log in.
Robert Collins (lifeless) wrote :

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

-Rob

Martin Pool (mbp) wrote :

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) wrote :

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) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
1556 def _get_keys(self, graph):1556 def _get_keys(self, graph):
1557 NULL_REVISION = revision.NULL_REVISION1557 NULL_REVISION = revision.NULL_REVISION
1558 keys = [key for (key, parents) in graph.iter_ancestry(self.heads)1558 keys = [key for (key, parents) in graph.iter_ancestry(self.heads)
1559 if key != NULL_REVISION]1559 if key != NULL_REVISION and parents is not None]
1560 return keys1560 return keys
15611561
1562 def is_empty(self):1562 def is_empty(self):
15631563
=== 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 @@
20 bzrdir,20 bzrdir,
21 errors,21 errors,
22 gpg,22 gpg,
23 graph,
23 remote,24 remote,
24 repository,25 repository,
25 )26 )
@@ -298,3 +299,29 @@
298 revision_id = tree.commit('test')299 revision_id = tree.commit('test')
299 repo.fetch(tree.branch.repository)300 repo.fetch(tree.branch.repository)
300 repo.fetch(tree.branch.repository)301 repo.fetch(tree.branch.repository)
302
303
304class TestSource(TestCaseWithRepository):
305 """Tests for/about the results of Repository._get_source."""
306
307 def test_no_absent_records_in_stream_with_ghosts(self):
308 # XXX: Arguably should be in interrepository_implementations but
309 # doesn't actually gain coverage there; need a specific set of
310 # permutations to cover it.
311 builder = self.make_branch_builder('repo')
312 builder.start_series()
313 builder.build_snapshot('tip', ['ghost'],
314 [('add', ('', 'ROOT_ID', 'directory', ''))],
315 allow_leftmost_as_ghost=True)
316 builder.finish_series()
317 b = builder.get_branch()
318 b.lock_read()
319 self.addCleanup(b.unlock)
320 repo = b.repository
321 source = repo._get_source(repo._format)
322 search = graph.PendingAncestryResult(['tip'], repo)
323 stream = source.get_stream(search)
324 for substream_type, substream in stream:
325 for record in substream:
326 self.assertNotEqual('absent', record.storage_kind,
327 "Absent record for %s" % (((substream_type,) + record.key),))