Merge lp:~garyvdm/bzr/PublicStackedParentsProvider into lp:~bzr/bzr/trunk-old

Proposed by Gary van der Merwe on 2009-05-29
Status: Merged
Approved by: John A Meinel on 2009-06-01
Approved revision: 4386
Merged at revision: not available
Proposed branch: lp:~garyvdm/bzr/PublicStackedParentsProvider
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 224 lines
To merge this branch: bzr merge lp:~garyvdm/bzr/PublicStackedParentsProvider
Reviewer Review Type Date Requested Status
Andrew Bennetts Approve on 2009-06-09
John A Meinel Approve on 2009-06-01
Review via email: mp+6881@code.launchpad.net

This proposal supersedes a proposal from 2009-05-29.

To post a comment you must log in.
Gary van der Merwe (garyvdm) wrote : Posted in a previous version of this proposal

The patch make StackedParentsProvider a public api. It also adds a test to check that overlaping revisions from the different providers are handled correctly.

John A Meinel (jameinel) wrote : Posted in a previous version of this proposal

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Gary van der Merwe wrote:
> Gary van der Merwe has proposed merging lp:~garyvdm/bzr/PublicStackedParentsProvider into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> The patch make StackedParentsProvider a public api. It also adds a test to check that overlaping revisions from the different providers are handled correctly.
>
     def __repr__(self):
- - return "_StackedParentsProvider(%r)" % self._parent_providers
+ return "StackedParentsProvider(%r)" % self._parent_providers

^- we generally try to do:

return "%s(%r)" % (self.__class__.__name__, self._parent_providers)

so that you don't have to update these things, and so that child classes
automatically get named the right thing, too.

We might want to add:

@deprecated((0,1, 16)
def _StackedParentsProvider(*args, **kwargs):
  return StackedParentsProvider(*args, **kwargs)

I realize we don't *need* to by our contract rules. But where we can, it
is a lot nicer to start giving deprecation warnings, rather than causing
existing code to just start breaking.

John
=:->

 Review: approve

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkoeZFMACgkQJdeBCYSNAANTBQCcCGnzpFlAqI74sq5veUieoEOP
5BgAn2Jk/pRDdA+Vh/2ZUDS35bRo8kiC
=12aX
-----END PGP SIGNATURE-----

John A Meinel (jameinel) wrote : Posted in a previous version of this proposal

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Gary van der Merwe wrote:
> Gary van der Merwe has proposed merging lp:~garyvdm/bzr/PublicStackedParentsProvider into lp:bzr.

The only thing I would mention is that we should add a single test that
exercises the deprecated function. So that we

1) Know that the deprecation warning is emitted
2) Know that the api still works.

This would be something like:
+
+ def test_stacked_parents_provider_overlapping(self):
+ # rev2 is availible in both providers.
+ # 1
+ # |
+ # 2
+ parents1 = _mod_graph.DictParentsProvider({'rev2': ['rev1']})
+ parents2 = _mod_graph.DictParentsProvider({'rev2': ['rev1']})
+ stacked = self.callDeprecated(deprecated_in(1, 16, 0),
+ _mod_graph._StackedParentsProvider(
...
+ self.assertEqual({'rev2': ['rev1']},
+ stacked.get_parent_map(['rev2']))

  Review: approve

Sorry I forgot to mention that before, but with that change, I think we
should land it.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkofnqwACgkQJdeBCYSNAAN7ggCgllDFqwDus82fkVKnXUsXHzMI
trAAn3aZTmfxX/LF0AoWOt8Ps6h6gKBu
=HegE
-----END PGP SIGNATURE-----

John A Meinel (jameinel) :
review: Approve
Andrew Bennetts (spiv) wrote :

I'll merge.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-06-05 23:21:51 +0000
3+++ NEWS 2009-06-06 02:36:15 +0000
4@@ -102,6 +102,9 @@
5 ***********
6
7 * Added osutils.parent_directories(). (Ian Clatworthy)
8+* ``graph.StackedParentsProvider`` is now a public API, replacing
9+ ``graph._StackedParentsProvider``. The api is now considered stable and ready
10+ for external users. (Gary van der Merwe)
11
12 * TreeTransformBase no longer assumes that limbo is provided via disk.
13 DiskTreeTransform now provides disk functionality. (Aaron Bentley)
14
15=== modified file 'bzrlib/bzrdir.py'
16--- bzrlib/bzrdir.py 2009-06-04 21:25:46 +0000
17+++ bzrlib/bzrdir.py 2009-06-06 02:36:15 +0000
18@@ -2768,7 +2768,7 @@
19 del ie.text_id
20
21 def get_parent_map(self, revision_ids):
22- """See graph._StackedParentsProvider.get_parent_map"""
23+ """See graph.StackedParentsProvider.get_parent_map"""
24 return dict((revision_id, self.revisions[revision_id])
25 for revision_id in revision_ids
26 if revision_id in self.revisions)
27
28=== modified file 'bzrlib/graph.py'
29--- bzrlib/graph.py 2009-05-27 09:40:33 +0000
30+++ bzrlib/graph.py 2009-06-06 02:36:15 +0000
31@@ -20,10 +20,10 @@
32 debug,
33 errors,
34 revision,
35- symbol_versioning,
36 trace,
37 tsort,
38 )
39+from bzrlib.symbol_versioning import deprecated_function, deprecated_in
40
41 STEP_UNIQUE_SEARCHER_EVERY = 5
42
43@@ -60,18 +60,25 @@
44 return 'DictParentsProvider(%r)' % self.ancestry
45
46 def get_parent_map(self, keys):
47- """See _StackedParentsProvider.get_parent_map"""
48+ """See StackedParentsProvider.get_parent_map"""
49 ancestry = self.ancestry
50 return dict((k, ancestry[k]) for k in keys if k in ancestry)
51
52-
53-class _StackedParentsProvider(object):
54-
55+@deprecated_function(deprecated_in((1, 16, 0)))
56+def _StackedParentsProvider(*args, **kwargs):
57+ return StackedParentsProvider(*args, **kwargs)
58+
59+class StackedParentsProvider(object):
60+ """A parents provider which stacks (or unions) multiple providers.
61+
62+ The providers are queries in the order of the provided parent_providers.
63+ """
64+
65 def __init__(self, parent_providers):
66 self._parent_providers = parent_providers
67
68 def __repr__(self):
69- return "_StackedParentsProvider(%r)" % self._parent_providers
70+ return "%s(%r)" % (self.__class__.__name__, self._parent_providers)
71
72 def get_parent_map(self, keys):
73 """Get a mapping of keys => parents
74@@ -148,7 +155,7 @@
75 return dict(self._cache)
76
77 def get_parent_map(self, keys):
78- """See _StackedParentsProvider.get_parent_map."""
79+ """See StackedParentsProvider.get_parent_map."""
80 cache = self._cache
81 if cache is None:
82 cache = self._get_parent_map(keys)
83
84=== modified file 'bzrlib/index.py'
85--- bzrlib/index.py 2009-03-24 01:53:42 +0000
86+++ bzrlib/index.py 2009-06-06 02:36:15 +0000
87@@ -1192,7 +1192,7 @@
88 ', '.join(map(repr, self._indices)))
89
90 def get_parent_map(self, keys):
91- """See graph._StackedParentsProvider.get_parent_map"""
92+ """See graph.StackedParentsProvider.get_parent_map"""
93 search_keys = set(keys)
94 if NULL_REVISION in search_keys:
95 search_keys.discard(NULL_REVISION)
96
97=== modified file 'bzrlib/remote.py'
98--- bzrlib/remote.py 2009-06-03 01:37:27 +0000
99+++ bzrlib/remote.py 2009-06-06 02:36:15 +0000
100@@ -1576,7 +1576,7 @@
101 providers.insert(0, other)
102 providers.extend(r._make_parents_provider() for r in
103 self._fallback_repositories)
104- return graph._StackedParentsProvider(providers)
105+ return graph.StackedParentsProvider(providers)
106
107 def _serialise_search_recipe(self, recipe):
108 """Serialise a graph search recipe.
109
110=== modified file 'bzrlib/repofmt/knitrepo.py'
111--- bzrlib/repofmt/knitrepo.py 2009-04-09 20:23:07 +0000
112+++ bzrlib/repofmt/knitrepo.py 2009-06-06 02:36:15 +0000
113@@ -54,7 +54,7 @@
114 return 'KnitParentsProvider(%r)' % self._knit
115
116 def get_parent_map(self, keys):
117- """See graph._StackedParentsProvider.get_parent_map"""
118+ """See graph.StackedParentsProvider.get_parent_map"""
119 parent_map = {}
120 for revision_id in keys:
121 if revision_id is None:
122@@ -85,7 +85,7 @@
123 return 'KnitsParentsProvider(%r)' % self._knit
124
125 def get_parent_map(self, keys):
126- """See graph._StackedParentsProvider.get_parent_map"""
127+ """See graph.StackedParentsProvider.get_parent_map"""
128 parent_map = self._knit.get_parent_map(
129 [self._prefix + (key,) for key in keys])
130 result = {}
131
132=== modified file 'bzrlib/repository.py'
133--- bzrlib/repository.py 2009-06-04 21:25:46 +0000
134+++ bzrlib/repository.py 2009-06-06 02:36:15 +0000
135@@ -2384,7 +2384,7 @@
136 return self.control_files.get_transaction()
137
138 def get_parent_map(self, revision_ids):
139- """See graph._StackedParentsProvider.get_parent_map"""
140+ """See graph.StackedParentsProvider.get_parent_map"""
141 # revisions index works in keys; this just works in revisions
142 # therefore wrap and unwrap
143 query_keys = []
144@@ -2413,7 +2413,7 @@
145 parents_provider = self._make_parents_provider()
146 if (other_repository is not None and
147 not self.has_same_location(other_repository)):
148- parents_provider = graph._StackedParentsProvider(
149+ parents_provider = graph.StackedParentsProvider(
150 [parents_provider, other_repository._make_parents_provider()])
151 return graph.Graph(parents_provider)
152
153
154=== modified file 'bzrlib/tests/test_graph.py'
155--- bzrlib/tests/test_graph.py 2009-05-12 02:24:54 +0000
156+++ bzrlib/tests/test_graph.py 2009-06-06 02:36:15 +0000
157@@ -22,6 +22,7 @@
158 )
159 from bzrlib.revision import NULL_REVISION
160 from bzrlib.tests import TestCaseWithMemoryTransport
161+from bzrlib.symbol_versioning import deprecated_in
162
163
164 # Ancestry 1:
165@@ -661,10 +662,36 @@
166 self.assertEqual((set(['e']), set(['f', 'g'])),
167 graph.find_difference('e', 'f'))
168
169+
170 def test_stacked_parents_provider(self):
171 parents1 = _mod_graph.DictParentsProvider({'rev2': ['rev3']})
172 parents2 = _mod_graph.DictParentsProvider({'rev1': ['rev4']})
173- stacked = _mod_graph._StackedParentsProvider([parents1, parents2])
174+ stacked = _mod_graph.StackedParentsProvider([parents1, parents2])
175+ self.assertEqual({'rev1':['rev4'], 'rev2':['rev3']},
176+ stacked.get_parent_map(['rev1', 'rev2']))
177+ self.assertEqual({'rev2':['rev3'], 'rev1':['rev4']},
178+ stacked.get_parent_map(['rev2', 'rev1']))
179+ self.assertEqual({'rev2':['rev3']},
180+ stacked.get_parent_map(['rev2', 'rev2']))
181+ self.assertEqual({'rev1':['rev4']},
182+ stacked.get_parent_map(['rev1', 'rev1']))
183+
184+ def test_stacked_parents_provider_overlapping(self):
185+ # rev2 is availible in both providers.
186+ # 1
187+ # |
188+ # 2
189+ parents1 = _mod_graph.DictParentsProvider({'rev2': ['rev1']})
190+ parents2 = _mod_graph.DictParentsProvider({'rev2': ['rev1']})
191+ stacked = _mod_graph.StackedParentsProvider([parents1, parents2])
192+ self.assertEqual({'rev2': ['rev1']},
193+ stacked.get_parent_map(['rev2']))
194+
195+ def test__stacked_parents_provider_deprecated(self):
196+ parents1 = _mod_graph.DictParentsProvider({'rev2': ['rev3']})
197+ parents2 = _mod_graph.DictParentsProvider({'rev1': ['rev4']})
198+ stacked = self.applyDeprecated(deprecated_in((1, 16, 0)),
199+ _mod_graph._StackedParentsProvider, [parents1, parents2])
200 self.assertEqual({'rev1':['rev4'], 'rev2':['rev3']},
201 stacked.get_parent_map(['rev1', 'rev2']))
202 self.assertEqual({'rev2':['rev3'], 'rev1':['rev4']},
203
204=== modified file 'bzrlib/versionedfile.py'
205--- bzrlib/versionedfile.py 2009-05-26 09:20:17 +0000
206+++ bzrlib/versionedfile.py 2009-06-06 02:36:15 +0000
207@@ -40,7 +40,7 @@
208 revision,
209 ui,
210 )
211-from bzrlib.graph import DictParentsProvider, Graph, _StackedParentsProvider
212+from bzrlib.graph import DictParentsProvider, Graph, StackedParentsProvider
213 from bzrlib.transport.memory import MemoryTransport
214 """)
215 from bzrlib.inter import InterObject
216@@ -1333,7 +1333,7 @@
217 result[revision.NULL_REVISION] = ()
218 self._providers = self._providers[:1] + self.fallback_versionedfiles
219 result.update(
220- _StackedParentsProvider(self._providers).get_parent_map(keys))
221+ StackedParentsProvider(self._providers).get_parent_map(keys))
222 for key, parents in result.iteritems():
223 if parents == ():
224 result[key] = (revision.NULL_REVISION,)