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

Proposed by Gary van der Merwe
Status: Merged
Approved by: John A Meinel
Approved revision: no longer in the source branch.
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
John A Meinel Approve
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.
Revision history for this message
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.

Revision history for this message
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-----

Revision history for this message
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-----

Revision history for this message
John A Meinel (jameinel) :
review: Approve
Revision history for this message
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
=== modified file 'NEWS'
--- NEWS 2009-06-05 23:21:51 +0000
+++ NEWS 2009-06-06 02:36:15 +0000
@@ -102,6 +102,9 @@
102***********102***********
103103
104* Added osutils.parent_directories(). (Ian Clatworthy)104* Added osutils.parent_directories(). (Ian Clatworthy)
105* ``graph.StackedParentsProvider`` is now a public API, replacing
106 ``graph._StackedParentsProvider``. The api is now considered stable and ready
107 for external users. (Gary van der Merwe)
105108
106* TreeTransformBase no longer assumes that limbo is provided via disk.109* TreeTransformBase no longer assumes that limbo is provided via disk.
107 DiskTreeTransform now provides disk functionality. (Aaron Bentley)110 DiskTreeTransform now provides disk functionality. (Aaron Bentley)
108111
=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py 2009-06-04 21:25:46 +0000
+++ bzrlib/bzrdir.py 2009-06-06 02:36:15 +0000
@@ -2768,7 +2768,7 @@
2768 del ie.text_id2768 del ie.text_id
27692769
2770 def get_parent_map(self, revision_ids):2770 def get_parent_map(self, revision_ids):
2771 """See graph._StackedParentsProvider.get_parent_map"""2771 """See graph.StackedParentsProvider.get_parent_map"""
2772 return dict((revision_id, self.revisions[revision_id])2772 return dict((revision_id, self.revisions[revision_id])
2773 for revision_id in revision_ids2773 for revision_id in revision_ids
2774 if revision_id in self.revisions)2774 if revision_id in self.revisions)
27752775
=== modified file 'bzrlib/graph.py'
--- bzrlib/graph.py 2009-05-27 09:40:33 +0000
+++ bzrlib/graph.py 2009-06-06 02:36:15 +0000
@@ -20,10 +20,10 @@
20 debug,20 debug,
21 errors,21 errors,
22 revision,22 revision,
23 symbol_versioning,
24 trace,23 trace,
25 tsort,24 tsort,
26 )25 )
26from bzrlib.symbol_versioning import deprecated_function, deprecated_in
2727
28STEP_UNIQUE_SEARCHER_EVERY = 528STEP_UNIQUE_SEARCHER_EVERY = 5
2929
@@ -60,18 +60,25 @@
60 return 'DictParentsProvider(%r)' % self.ancestry60 return 'DictParentsProvider(%r)' % self.ancestry
6161
62 def get_parent_map(self, keys):62 def get_parent_map(self, keys):
63 """See _StackedParentsProvider.get_parent_map"""63 """See StackedParentsProvider.get_parent_map"""
64 ancestry = self.ancestry64 ancestry = self.ancestry
65 return dict((k, ancestry[k]) for k in keys if k in ancestry)65 return dict((k, ancestry[k]) for k in keys if k in ancestry)
6666
6767@deprecated_function(deprecated_in((1, 16, 0)))
68class _StackedParentsProvider(object):68def _StackedParentsProvider(*args, **kwargs):
6969 return StackedParentsProvider(*args, **kwargs)
70
71class StackedParentsProvider(object):
72 """A parents provider which stacks (or unions) multiple providers.
73
74 The providers are queries in the order of the provided parent_providers.
75 """
76
70 def __init__(self, parent_providers):77 def __init__(self, parent_providers):
71 self._parent_providers = parent_providers78 self._parent_providers = parent_providers
7279
73 def __repr__(self):80 def __repr__(self):
74 return "_StackedParentsProvider(%r)" % self._parent_providers81 return "%s(%r)" % (self.__class__.__name__, self._parent_providers)
7582
76 def get_parent_map(self, keys):83 def get_parent_map(self, keys):
77 """Get a mapping of keys => parents84 """Get a mapping of keys => parents
@@ -148,7 +155,7 @@
148 return dict(self._cache)155 return dict(self._cache)
149156
150 def get_parent_map(self, keys):157 def get_parent_map(self, keys):
151 """See _StackedParentsProvider.get_parent_map."""158 """See StackedParentsProvider.get_parent_map."""
152 cache = self._cache159 cache = self._cache
153 if cache is None:160 if cache is None:
154 cache = self._get_parent_map(keys)161 cache = self._get_parent_map(keys)
155162
=== modified file 'bzrlib/index.py'
--- bzrlib/index.py 2009-03-24 01:53:42 +0000
+++ bzrlib/index.py 2009-06-06 02:36:15 +0000
@@ -1192,7 +1192,7 @@
1192 ', '.join(map(repr, self._indices)))1192 ', '.join(map(repr, self._indices)))
11931193
1194 def get_parent_map(self, keys):1194 def get_parent_map(self, keys):
1195 """See graph._StackedParentsProvider.get_parent_map"""1195 """See graph.StackedParentsProvider.get_parent_map"""
1196 search_keys = set(keys)1196 search_keys = set(keys)
1197 if NULL_REVISION in search_keys:1197 if NULL_REVISION in search_keys:
1198 search_keys.discard(NULL_REVISION)1198 search_keys.discard(NULL_REVISION)
11991199
=== modified file 'bzrlib/remote.py'
--- bzrlib/remote.py 2009-06-03 01:37:27 +0000
+++ bzrlib/remote.py 2009-06-06 02:36:15 +0000
@@ -1576,7 +1576,7 @@
1576 providers.insert(0, other)1576 providers.insert(0, other)
1577 providers.extend(r._make_parents_provider() for r in1577 providers.extend(r._make_parents_provider() for r in
1578 self._fallback_repositories)1578 self._fallback_repositories)
1579 return graph._StackedParentsProvider(providers)1579 return graph.StackedParentsProvider(providers)
15801580
1581 def _serialise_search_recipe(self, recipe):1581 def _serialise_search_recipe(self, recipe):
1582 """Serialise a graph search recipe.1582 """Serialise a graph search recipe.
15831583
=== modified file 'bzrlib/repofmt/knitrepo.py'
--- bzrlib/repofmt/knitrepo.py 2009-04-09 20:23:07 +0000
+++ bzrlib/repofmt/knitrepo.py 2009-06-06 02:36:15 +0000
@@ -54,7 +54,7 @@
54 return 'KnitParentsProvider(%r)' % self._knit54 return 'KnitParentsProvider(%r)' % self._knit
5555
56 def get_parent_map(self, keys):56 def get_parent_map(self, keys):
57 """See graph._StackedParentsProvider.get_parent_map"""57 """See graph.StackedParentsProvider.get_parent_map"""
58 parent_map = {}58 parent_map = {}
59 for revision_id in keys:59 for revision_id in keys:
60 if revision_id is None:60 if revision_id is None:
@@ -85,7 +85,7 @@
85 return 'KnitsParentsProvider(%r)' % self._knit85 return 'KnitsParentsProvider(%r)' % self._knit
8686
87 def get_parent_map(self, keys):87 def get_parent_map(self, keys):
88 """See graph._StackedParentsProvider.get_parent_map"""88 """See graph.StackedParentsProvider.get_parent_map"""
89 parent_map = self._knit.get_parent_map(89 parent_map = self._knit.get_parent_map(
90 [self._prefix + (key,) for key in keys])90 [self._prefix + (key,) for key in keys])
91 result = {}91 result = {}
9292
=== modified file 'bzrlib/repository.py'
--- bzrlib/repository.py 2009-06-04 21:25:46 +0000
+++ bzrlib/repository.py 2009-06-06 02:36:15 +0000
@@ -2384,7 +2384,7 @@
2384 return self.control_files.get_transaction()2384 return self.control_files.get_transaction()
23852385
2386 def get_parent_map(self, revision_ids):2386 def get_parent_map(self, revision_ids):
2387 """See graph._StackedParentsProvider.get_parent_map"""2387 """See graph.StackedParentsProvider.get_parent_map"""
2388 # revisions index works in keys; this just works in revisions2388 # revisions index works in keys; this just works in revisions
2389 # therefore wrap and unwrap2389 # therefore wrap and unwrap
2390 query_keys = []2390 query_keys = []
@@ -2413,7 +2413,7 @@
2413 parents_provider = self._make_parents_provider()2413 parents_provider = self._make_parents_provider()
2414 if (other_repository is not None and2414 if (other_repository is not None and
2415 not self.has_same_location(other_repository)):2415 not self.has_same_location(other_repository)):
2416 parents_provider = graph._StackedParentsProvider(2416 parents_provider = graph.StackedParentsProvider(
2417 [parents_provider, other_repository._make_parents_provider()])2417 [parents_provider, other_repository._make_parents_provider()])
2418 return graph.Graph(parents_provider)2418 return graph.Graph(parents_provider)
24192419
24202420
=== modified file 'bzrlib/tests/test_graph.py'
--- bzrlib/tests/test_graph.py 2009-05-12 02:24:54 +0000
+++ bzrlib/tests/test_graph.py 2009-06-06 02:36:15 +0000
@@ -22,6 +22,7 @@
22 )22 )
23from bzrlib.revision import NULL_REVISION23from bzrlib.revision import NULL_REVISION
24from bzrlib.tests import TestCaseWithMemoryTransport24from bzrlib.tests import TestCaseWithMemoryTransport
25from bzrlib.symbol_versioning import deprecated_in
2526
2627
27# Ancestry 1:28# Ancestry 1:
@@ -661,10 +662,36 @@
661 self.assertEqual((set(['e']), set(['f', 'g'])),662 self.assertEqual((set(['e']), set(['f', 'g'])),
662 graph.find_difference('e', 'f'))663 graph.find_difference('e', 'f'))
663664
665
664 def test_stacked_parents_provider(self):666 def test_stacked_parents_provider(self):
665 parents1 = _mod_graph.DictParentsProvider({'rev2': ['rev3']})667 parents1 = _mod_graph.DictParentsProvider({'rev2': ['rev3']})
666 parents2 = _mod_graph.DictParentsProvider({'rev1': ['rev4']})668 parents2 = _mod_graph.DictParentsProvider({'rev1': ['rev4']})
667 stacked = _mod_graph._StackedParentsProvider([parents1, parents2])669 stacked = _mod_graph.StackedParentsProvider([parents1, parents2])
670 self.assertEqual({'rev1':['rev4'], 'rev2':['rev3']},
671 stacked.get_parent_map(['rev1', 'rev2']))
672 self.assertEqual({'rev2':['rev3'], 'rev1':['rev4']},
673 stacked.get_parent_map(['rev2', 'rev1']))
674 self.assertEqual({'rev2':['rev3']},
675 stacked.get_parent_map(['rev2', 'rev2']))
676 self.assertEqual({'rev1':['rev4']},
677 stacked.get_parent_map(['rev1', 'rev1']))
678
679 def test_stacked_parents_provider_overlapping(self):
680 # rev2 is availible in both providers.
681 # 1
682 # |
683 # 2
684 parents1 = _mod_graph.DictParentsProvider({'rev2': ['rev1']})
685 parents2 = _mod_graph.DictParentsProvider({'rev2': ['rev1']})
686 stacked = _mod_graph.StackedParentsProvider([parents1, parents2])
687 self.assertEqual({'rev2': ['rev1']},
688 stacked.get_parent_map(['rev2']))
689
690 def test__stacked_parents_provider_deprecated(self):
691 parents1 = _mod_graph.DictParentsProvider({'rev2': ['rev3']})
692 parents2 = _mod_graph.DictParentsProvider({'rev1': ['rev4']})
693 stacked = self.applyDeprecated(deprecated_in((1, 16, 0)),
694 _mod_graph._StackedParentsProvider, [parents1, parents2])
668 self.assertEqual({'rev1':['rev4'], 'rev2':['rev3']},695 self.assertEqual({'rev1':['rev4'], 'rev2':['rev3']},
669 stacked.get_parent_map(['rev1', 'rev2']))696 stacked.get_parent_map(['rev1', 'rev2']))
670 self.assertEqual({'rev2':['rev3'], 'rev1':['rev4']},697 self.assertEqual({'rev2':['rev3'], 'rev1':['rev4']},
671698
=== modified file 'bzrlib/versionedfile.py'
--- bzrlib/versionedfile.py 2009-05-26 09:20:17 +0000
+++ bzrlib/versionedfile.py 2009-06-06 02:36:15 +0000
@@ -40,7 +40,7 @@
40 revision,40 revision,
41 ui,41 ui,
42 )42 )
43from bzrlib.graph import DictParentsProvider, Graph, _StackedParentsProvider43from bzrlib.graph import DictParentsProvider, Graph, StackedParentsProvider
44from bzrlib.transport.memory import MemoryTransport44from bzrlib.transport.memory import MemoryTransport
45""")45""")
46from bzrlib.inter import InterObject46from bzrlib.inter import InterObject
@@ -1333,7 +1333,7 @@
1333 result[revision.NULL_REVISION] = ()1333 result[revision.NULL_REVISION] = ()
1334 self._providers = self._providers[:1] + self.fallback_versionedfiles1334 self._providers = self._providers[:1] + self.fallback_versionedfiles
1335 result.update(1335 result.update(
1336 _StackedParentsProvider(self._providers).get_parent_map(keys))1336 StackedParentsProvider(self._providers).get_parent_map(keys))
1337 for key, parents in result.iteritems():1337 for key, parents in result.iteritems():
1338 if parents == ():1338 if parents == ():
1339 result[key] = (revision.NULL_REVISION,)1339 result[key] = (revision.NULL_REVISION,)