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
1=== modified file 'bzrlib/graph.py'
2--- bzrlib/graph.py 2009-04-04 02:50:01 +0000
3+++ bzrlib/graph.py 2009-05-14 08:35:10 +0000
4@@ -1556,7 +1556,7 @@
5 def _get_keys(self, graph):
6 NULL_REVISION = revision.NULL_REVISION
7 keys = [key for (key, parents) in graph.iter_ancestry(self.heads)
8- if key != NULL_REVISION]
9+ if key != NULL_REVISION and parents is not None]
10 return keys
11
12 def is_empty(self):
13
14=== modified file 'bzrlib/tests/per_repository/test_fetch.py'
15--- bzrlib/tests/per_repository/test_fetch.py 2009-05-07 05:08:46 +0000
16+++ bzrlib/tests/per_repository/test_fetch.py 2009-05-14 08:35:11 +0000
17@@ -20,6 +20,7 @@
18 bzrdir,
19 errors,
20 gpg,
21+ graph,
22 remote,
23 repository,
24 )
25@@ -298,3 +299,29 @@
26 revision_id = tree.commit('test')
27 repo.fetch(tree.branch.repository)
28 repo.fetch(tree.branch.repository)
29+
30+
31+class TestSource(TestCaseWithRepository):
32+ """Tests for/about the results of Repository._get_source."""
33+
34+ def test_no_absent_records_in_stream_with_ghosts(self):
35+ # XXX: Arguably should be in interrepository_implementations but
36+ # doesn't actually gain coverage there; need a specific set of
37+ # permutations to cover it.
38+ builder = self.make_branch_builder('repo')
39+ builder.start_series()
40+ builder.build_snapshot('tip', ['ghost'],
41+ [('add', ('', 'ROOT_ID', 'directory', ''))],
42+ allow_leftmost_as_ghost=True)
43+ builder.finish_series()
44+ b = builder.get_branch()
45+ b.lock_read()
46+ self.addCleanup(b.unlock)
47+ repo = b.repository
48+ source = repo._get_source(repo._format)
49+ search = graph.PendingAncestryResult(['tip'], repo)
50+ stream = source.get_stream(search)
51+ for substream_type, substream in stream:
52+ for record in substream:
53+ self.assertNotEqual('absent', record.storage_kind,
54+ "Absent record for %s" % (((substream_type,) + record.key),))