Merge lp:~spiv/bzr/fetch-spec-everything-not-in-other into lp:bzr
- fetch-spec-everything-not-in-other
- Merge into bzr.dev
Status: | Merged |
---|---|
Merged at revision: | 5648 |
Proposed branch: | lp:~spiv/bzr/fetch-spec-everything-not-in-other |
Merge into: | lp:bzr |
Diff against target: |
247 lines (+32/-36) 10 files modified
bzrlib/fetch.py (+10/-15) bzrlib/graph.py (+6/-5) bzrlib/remote.py (+3/-3) bzrlib/repofmt/knitrepo.py (+1/-1) bzrlib/repofmt/weaverepo.py (+1/-1) bzrlib/repository.py (+2/-2) bzrlib/tests/per_interrepository/test_interrepository.py (+1/-1) bzrlib/tests/test_remote.py (+3/-3) doc/en/release-notes/bzr-2.3.txt (+0/-5) doc/en/release-notes/bzr-2.4.txt (+5/-0) |
To merge this branch: | bzr merge lp:~spiv/bzr/fetch-spec-everything-not-in-other |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Martin Pool | Approve | ||
John A Meinel | Pending | ||
Review via email: mp+42811@code.launchpad.net |
This proposal supersedes a proposal from 2010-11-29.
Commit message
Add EverythingResult, EverythingNotIn
Description of the change
Like <https:/
This patch defines some new “fetch specs” to be passed as the fetch_spec argument of fetch: EverythingResult, EmptySearchResult, EverythingNotIn
Separately I'm not 100% sure we need the ability to have fetch specs that aren't resolved into search results immediately (i.e. how AbstractSearch instances are used). This happens to be effectively how the existing code works: e.g. code that does "target.
A nice thing about adding these objects is that it shifts logic to handle these different cases out of the innards of fetch.py to somewhere more modular. This in turn means we can support these things better over HPSS, and so this patch does that: the 'everything' search is now added to the network protocol. Happily new verb was needed, we can safely interpret a BadSearch error in this case as meaning we need to fallback.
Another change in this patch is the deprecation of search_
John A Meinel (jameinel) wrote : Posted in a previous version of this proposal | # |
Andrew Bennetts (spiv) wrote : Posted in a previous version of this proposal | # |
John A Meinel wrote:
> Review: Needs Fixing
> + if not isinstance(search, graph.Everythin
> ^- can we add an attribute, rather than an isinstance check?
>
> if not search.
Hmm, I can do that, although currently there's no common base class for these
objects, and my gut feeling is that it would be good to keep their interface as
small and simple as possible. So I'm unsure about whether or not to do this. I
think it's probably easier to add this later than it is to remove it later.
> 490 + def _present_
> 491 + """Returns set of all revisions in ancestry of revision_ids present in
> 492 + the source repo.
> 493 +
> 494 + :param revision_ids: if None, all revisions in source are returned.
> 495 + """
> 496 + if revision_ids is not None:
> 497 + # First, ensure all specified revisions exist. Callers expect
> 498 + # NoSuchRevision when they pass absent revision_ids here.
> 499 + revision_ids = set(revision_ids)
> 500 + graph = self.source.
> 501 + present_revs = set(graph.
> 502 + missing = revision_
> 503 + if missing:
> 504 + raise errors.
> 505 + source_ids = [rev_id for (rev_id, parents) in
> 506 + self.source.
> 507 + if rev_id != _mod_revision.
> 508 + and parents is not None]
> ^- you have "graph = self.source.
Fixed, thanks for spotting that. I think it occurred because I initially
copy-and-pasted the list comprehension from graph.py.
> 145 + def get_recipe(self):
> 146 + raise NotImplementedE
> 147 +
> 148 + def get_network_
> 149 + return ('everything',)
>
> Why don't EverythingNotIn
Because they are a different kind of object. Obviously that's not clear enough
in the current code!
The names could stand to be improved, but there are basically two kinds of
“fetch spec” in this patch:
* a network-ready search result: SearchResult, PendingAncestry
EverythingRe
(I'm not certain that's the perfect design, but it's not too far off.)
* a lightweight object that can be resolved into a network-ready search result
later: EverythingNotIn
method that inspects the repository/ies to generate the search result.
> Overall, I think these changes seem good, but I don't really see how this gets
> us closer to computing a search without having one end doing a step-by-step
> search. (Certainly you're still using the same 'search_
> and you don't seem to be serializing anything over the wire...)
Right, this change isn't meant to solve that problem. The concrete performance
issue they help with is in the next patch, fetch-all-
and easy for the sprout code to request a fetch of all tags at the
same time as the tip.
Separately...
Martin Pool (mbp) wrote : | # |
I think I like the concept of splitting these out into something more like objects.
I don't want to hold this hostage but it seems like this would be a good moment to add a little developer documentation to the architectural guide describing at a high level how fetch works.
+ def get_network_
+ start_keys = ' '.join(
+ stop_keys = ' '.join(
+ count = str(self.
+ return (self._recipe[0], '\n'.join(
+
rather than adhoc encoding, could you send it as bencode?
+ medium.
out of scope for this, but concluding it's older than 2.3 because it doesn't support this particular operation is really not accurate.
+ """See InterRepository
typo.
+ def test_fetch_
+ """Can fetch with EverythingResult even when the server does not have
+ the Repository.
+ """
+ local = self.make_
+ builder = self.make_
+ builder.
+ remote_branch_url = self.smart_
+ remote_branch = bzrdir.
+ self.hpss_calls = []
+ local.repositor
+ fetch_spec=
+
I don't understand how this tests that the server doesn't have this verb.
=== modified file 'doc/en/
--- doc/en/
+++ doc/en/
@@ -158,6 +158,11 @@
crashes when encountering private bugs (they are just displayed as such).
(Vincent Ladeuil, #354985)
+* The ``revision_id`` parameter of
+ ``Repository.
+ ``InterReposito
+ replaced by the ``revision_ids`` parameter. (Andrew Bennetts)
+
Internals
*********
spiv said on irc that he's testing this; if it turns out to be faster in practice that should be mentioned too. But I guess this patch alone won't change performance.
Andrew Bennetts (spiv) wrote : | # |
Martin Pool wrote:
> Review: Needs Information
> I think I like the concept of splitting these out into something more like
> objects.
>
> I don't want to hold this hostage but it seems like this would be a good
> moment to add a little developer documentation to the architectural guide
> describing at a high level how fetch works.
Good idea. I'll see what I can write up without spending ages on it.
> + def get_network_
> + start_keys = ' '.join(
> + stop_keys = ' '.join(
> + count = str(self.
> + return (self._recipe[0], '\n'.join(
> +
>
> rather than adhoc encoding, could you send it as bencode?
For this struct: no, because this is how existing clients encode SearchResult in
get_stream requests. (And get_parent_map requests, but that's handled
separately in remote.py. See RemoteRepositor
> + medium.
>
> out of scope for this, but concluding it's older than 2.3 because it doesn't
> support this particular operation is really not accurate.
Yeah. But that is definitely out of scope :)
> + """See InterRepository
>
> typo.
Fixed, thanks.
> + def test_fetch_
> + """Can fetch with EverythingResult even when the server does not have
> + the Repository.
> + """
> + local = self.make_
> + builder = self.make_
> + builder.
> + remote_branch_url = self.smart_
> + remote_branch = bzrdir.
> + self.hpss_calls = []
> + local.repositor
> + fetch_spec=
> +
>
> I don't understand how this tests that the server doesn't have this verb.
Oops, out-of-date docstring and test. I didn't need a new verb in the end, I
just added more search types to the existing get_stream_1.19 verb. I'll fix
that: instead I should be testing that a BadSearch reply to the new search
triggers the old requests (i.e. VFS). Probably that's a bit too hard to mock
(because it involves VFS) so I might have to monkeypatch or add a knob to the
get_stream handler class...
[...]
> spiv said on irc that he's testing this; if it turns out to be faster in
> practice that should be mentioned too. But I guess this patch alone won't
> change performance.
I don't expect it to improve performance (unless compared to fetching all the
tags by hand with the existing bzr CLI!) either.
The concern jam had is that querying for all the tags on every update and other
operations might have a noticeable negative impact on branches with many tags
such as emacs. My guess and preliminary results are that this isn't a problem,
but I'll make sure before landing this.
-Andrew.
Andrew Bennetts (spiv) wrote : | # |
In the case of lp:emacs there's no significant performance difference. The HPSS call count, memory consumption and wall clock time are unchanged (aside from noise in measurement). The HPSS request for the initial 'bzr branch' is larger, as it requests every rev named in the tags, not just tip, but that's a drop in the bucket vs. fetching the 239M of lp:emacs.
Also, lp:emacs has no tags pointing to missing revs (or even non-ancestry revs), so "bzr pull" works out identically with the new code vs. bzr.dev: all the revisions named in the source's tags are already present, so nothing changes. The wall clock time for the new code is again the same (within the noise of my measurements).
The main case I'd be concerned about is when the source branch has tags that refer to missing revisions. The target will copy those tags, and then on every update/pull re-request those tags from the source repository. Because those lookups in the revisions index will be misses, the server will end up opening every *.rix file. In the HPSS case that will happen server side, so it probably isn't a big concern there, but it might be an issue if using a dumb remote transport. The solution for affected branches is to delete the broken tags, or fill in the missing revisions. That's probably worth the benefit this gives, which is making tags Just Work in more cases. Branches and tags made with this new bzr version will of course fetch all tags by default, so that scenario won't be a concern for them.
In short: I'm confident. If it copes with a branch with as much history and tags as lp:emacs, it probably copes with everything else.
Andrew Bennetts (spiv) wrote : | # |
> > I don't want to hold this hostage but it seems like this would be a good
> > moment to add a little developer documentation to the architectural guide
> > describing at a high level how fetch works.
>
> Good idea. I'll see what I can write up without spending ages on it.
Done: <https:/
I believe all the points/questions raised so far have now been answered.
Martin Pool (mbp) wrote : | # |
I think it would be worth adding docstrings on AbstractSearchR
+ if symbol_
+ symbol_
+ 'search_
+ 'deprecated in 2.3. Use revision_ids=[...] instead.',
+ DeprecationWarning, stacklevel=2)
That will now be 2.4. It seems like we ought to have common handling of deprecated parameters. (https:/
merge-with-tweaks
Thanks, and sorry for the delay.
Preview Diff
1 | === modified file 'bzrlib/fetch.py' | |||
2 | --- bzrlib/fetch.py 2011-01-14 17:18:23 +0000 | |||
3 | +++ bzrlib/fetch.py 2011-02-07 02:57:39 +0000 | |||
4 | @@ -55,6 +55,8 @@ | |||
5 | 55 | 55 | ||
6 | 56 | :param last_revision: If set, try to limit to the data this revision | 56 | :param last_revision: If set, try to limit to the data this revision |
7 | 57 | references. | 57 | references. |
8 | 58 | :param fetch_spec: A SearchResult specifying which revisions to fetch. | ||
9 | 59 | If set, this overrides last_revision. | ||
10 | 58 | :param find_ghosts: If True search the entire history for ghosts. | 60 | :param find_ghosts: If True search the entire history for ghosts. |
11 | 59 | """ | 61 | """ |
12 | 60 | # repository.fetch has the responsibility for short-circuiting | 62 | # repository.fetch has the responsibility for short-circuiting |
13 | @@ -93,12 +95,12 @@ | |||
14 | 93 | pb.show_pct = pb.show_count = False | 95 | pb.show_pct = pb.show_count = False |
15 | 94 | try: | 96 | try: |
16 | 95 | pb.update("Finding revisions", 0, 2) | 97 | pb.update("Finding revisions", 0, 2) |
20 | 96 | search = self._revids_to_fetch() | 98 | search_result = self._revids_to_fetch() |
21 | 97 | mutter('fetching: %s', search) | 99 | mutter('fetching: %s', search_result) |
22 | 98 | if search.is_empty(): | 100 | if search_result.is_empty(): |
23 | 99 | return | 101 | return |
24 | 100 | pb.update("Fetching revisions", 1, 2) | 102 | pb.update("Fetching revisions", 1, 2) |
26 | 101 | self._fetch_everything_for_search(search) | 103 | self._fetch_everything_for_search(search_result) |
27 | 102 | finally: | 104 | finally: |
28 | 103 | pb.finished() | 105 | pb.finished() |
29 | 104 | 106 | ||
30 | @@ -151,18 +153,11 @@ | |||
31 | 151 | install self._last_revision in self.to_repository. | 153 | install self._last_revision in self.to_repository. |
32 | 152 | 154 | ||
33 | 153 | :returns: A SearchResult of some sort. (Possibly a | 155 | :returns: A SearchResult of some sort. (Possibly a |
35 | 154 | PendingAncestryResult, EmptySearchResult, etc.) | 156 | PendingAncestryResult, EmptySearchResult, etc.) |
36 | 155 | """ | 157 | """ |
37 | 156 | mutter("self._fetch_spec, self._last_revision: %r, %r", | 158 | mutter("self._fetch_spec, self._last_revision: %r, %r", |
38 | 157 | self._fetch_spec, self._last_revision) | 159 | self._fetch_spec, self._last_revision) |
47 | 158 | get_search_result = getattr(self._fetch_spec, 'get_search_result', None) | 160 | if self._fetch_spec is not None: |
40 | 159 | if get_search_result is not None: | ||
41 | 160 | mutter( | ||
42 | 161 | 'resolving fetch_spec into search result: %s', self._fetch_spec) | ||
43 | 162 | # This is EverythingNotInOther or a similar kind of fetch_spec. | ||
44 | 163 | # Turn it into a search result. | ||
45 | 164 | return get_search_result() | ||
46 | 165 | elif self._fetch_spec is not None: | ||
48 | 166 | # The fetch spec is already a concrete search result. | 161 | # The fetch spec is already a concrete search result. |
49 | 167 | return self._fetch_spec | 162 | return self._fetch_spec |
50 | 168 | elif self._last_revision == NULL_REVISION: | 163 | elif self._last_revision == NULL_REVISION: |
51 | @@ -172,11 +167,11 @@ | |||
52 | 172 | elif self._last_revision is not None: | 167 | elif self._last_revision is not None: |
53 | 173 | return graph.NotInOtherForRevs(self.to_repository, | 168 | return graph.NotInOtherForRevs(self.to_repository, |
54 | 174 | self.from_repository, [self._last_revision], | 169 | self.from_repository, [self._last_revision], |
56 | 175 | find_ghosts=self.find_ghosts).get_search_result() | 170 | find_ghosts=self.find_ghosts).execute() |
57 | 176 | else: # self._last_revision is None: | 171 | else: # self._last_revision is None: |
58 | 177 | return graph.EverythingNotInOther(self.to_repository, | 172 | return graph.EverythingNotInOther(self.to_repository, |
59 | 178 | self.from_repository, | 173 | self.from_repository, |
61 | 179 | find_ghosts=self.find_ghosts).get_search_result() | 174 | find_ghosts=self.find_ghosts).execute() |
62 | 180 | 175 | ||
63 | 181 | 176 | ||
64 | 182 | class Inter1and2Helper(object): | 177 | class Inter1and2Helper(object): |
65 | 183 | 178 | ||
66 | === modified file 'bzrlib/graph.py' | |||
67 | --- bzrlib/graph.py 2011-01-14 17:18:23 +0000 | |||
68 | +++ bzrlib/graph.py 2011-02-07 02:57:39 +0000 | |||
69 | @@ -1576,14 +1576,15 @@ | |||
70 | 1576 | 1576 | ||
71 | 1577 | class AbstractSearch(object): | 1577 | class AbstractSearch(object): |
72 | 1578 | 1578 | ||
74 | 1579 | def get_search_result(self): | 1579 | def execute(self): |
75 | 1580 | """Construct a network-ready search result from this search description. | 1580 | """Construct a network-ready search result from this search description. |
76 | 1581 | 1581 | ||
77 | 1582 | This may take some time to search repositories, etc. | 1582 | This may take some time to search repositories, etc. |
78 | 1583 | 1583 | ||
80 | 1584 | :return: A search result. | 1584 | :return: A search result (an object that implements |
81 | 1585 | AbstractSearchResult's API). | ||
82 | 1585 | """ | 1586 | """ |
84 | 1586 | raise NotImplementedError(self.get_search_result) | 1587 | raise NotImplementedError(self.execute) |
85 | 1587 | 1588 | ||
86 | 1588 | 1589 | ||
87 | 1589 | class SearchResult(AbstractSearchResult): | 1590 | class SearchResult(AbstractSearchResult): |
88 | @@ -1813,7 +1814,7 @@ | |||
89 | 1813 | self.from_repo = from_repo | 1814 | self.from_repo = from_repo |
90 | 1814 | self.find_ghosts = find_ghosts | 1815 | self.find_ghosts = find_ghosts |
91 | 1815 | 1816 | ||
93 | 1816 | def get_search_result(self): | 1817 | def execute(self): |
94 | 1817 | return self.to_repo.search_missing_revision_ids( | 1818 | return self.to_repo.search_missing_revision_ids( |
95 | 1818 | self.from_repo, find_ghosts=self.find_ghosts) | 1819 | self.from_repo, find_ghosts=self.find_ghosts) |
96 | 1819 | 1820 | ||
97 | @@ -1853,7 +1854,7 @@ | |||
98 | 1853 | self.__class__.__name__, self.from_repo, self.to_repo, | 1854 | self.__class__.__name__, self.from_repo, self.to_repo, |
99 | 1854 | self.find_ghosts, reqd_revs_repr, ifp_revs_repr) | 1855 | self.find_ghosts, reqd_revs_repr, ifp_revs_repr) |
100 | 1855 | 1856 | ||
102 | 1856 | def get_search_result(self): | 1857 | def execute(self): |
103 | 1857 | return self.to_repo.search_missing_revision_ids( | 1858 | return self.to_repo.search_missing_revision_ids( |
104 | 1858 | self.from_repo, revision_ids=self.required_ids, | 1859 | self.from_repo, revision_ids=self.required_ids, |
105 | 1859 | if_present_ids=self.if_present_ids, find_ghosts=self.find_ghosts) | 1860 | if_present_ids=self.if_present_ids, find_ghosts=self.find_ghosts) |
106 | 1860 | 1861 | ||
107 | === modified file 'bzrlib/remote.py' | |||
108 | --- bzrlib/remote.py 2011-01-14 17:18:23 +0000 | |||
109 | +++ bzrlib/remote.py 2011-02-07 02:57:39 +0000 | |||
110 | @@ -1360,7 +1360,7 @@ | |||
111 | 1360 | if symbol_versioning.deprecated_passed(revision_id): | 1360 | if symbol_versioning.deprecated_passed(revision_id): |
112 | 1361 | symbol_versioning.warn( | 1361 | symbol_versioning.warn( |
113 | 1362 | 'search_missing_revision_ids(revision_id=...) was ' | 1362 | 'search_missing_revision_ids(revision_id=...) was ' |
115 | 1363 | 'deprecated in 2.3. Use revision_ids=[...] instead.', | 1363 | 'deprecated in 2.4. Use revision_ids=[...] instead.', |
116 | 1364 | DeprecationWarning, stacklevel=2) | 1364 | DeprecationWarning, stacklevel=2) |
117 | 1365 | if revision_ids is not None: | 1365 | if revision_ids is not None: |
118 | 1366 | raise AssertionError( | 1366 | raise AssertionError( |
119 | @@ -1991,11 +1991,11 @@ | |||
120 | 1991 | if isinstance(search, graph.EverythingResult): | 1991 | if isinstance(search, graph.EverythingResult): |
121 | 1992 | error_verb = e.error_from_smart_server.error_verb | 1992 | error_verb = e.error_from_smart_server.error_verb |
122 | 1993 | if error_verb == 'BadSearch': | 1993 | if error_verb == 'BadSearch': |
124 | 1994 | # Pre-2.3 servers don't support this sort of search. | 1994 | # Pre-2.4 servers don't support this sort of search. |
125 | 1995 | # XXX: perhaps falling back to VFS on BadSearch is a | 1995 | # XXX: perhaps falling back to VFS on BadSearch is a |
126 | 1996 | # good idea in general? It might provide a little bit | 1996 | # good idea in general? It might provide a little bit |
127 | 1997 | # of protection against client-side bugs. | 1997 | # of protection against client-side bugs. |
129 | 1998 | medium._remember_remote_is_before((2, 3)) | 1998 | medium._remember_remote_is_before((2, 4)) |
130 | 1999 | break | 1999 | break |
131 | 2000 | raise | 2000 | raise |
132 | 2001 | else: | 2001 | else: |
133 | 2002 | 2002 | ||
134 | === modified file 'bzrlib/repofmt/knitrepo.py' | |||
135 | --- bzrlib/repofmt/knitrepo.py 2010-12-21 06:10:11 +0000 | |||
136 | +++ bzrlib/repofmt/knitrepo.py 2011-02-07 02:57:39 +0000 | |||
137 | @@ -542,7 +542,7 @@ | |||
138 | 542 | if symbol_versioning.deprecated_passed(revision_id): | 542 | if symbol_versioning.deprecated_passed(revision_id): |
139 | 543 | symbol_versioning.warn( | 543 | symbol_versioning.warn( |
140 | 544 | 'search_missing_revision_ids(revision_id=...) was ' | 544 | 'search_missing_revision_ids(revision_id=...) was ' |
142 | 545 | 'deprecated in 2.3. Use revision_ids=[...] instead.', | 545 | 'deprecated in 2.4. Use revision_ids=[...] instead.', |
143 | 546 | DeprecationWarning, stacklevel=2) | 546 | DeprecationWarning, stacklevel=2) |
144 | 547 | if revision_ids is not None: | 547 | if revision_ids is not None: |
145 | 548 | raise AssertionError( | 548 | raise AssertionError( |
146 | 549 | 549 | ||
147 | === modified file 'bzrlib/repofmt/weaverepo.py' | |||
148 | --- bzrlib/repofmt/weaverepo.py 2011-01-14 17:18:23 +0000 | |||
149 | +++ bzrlib/repofmt/weaverepo.py 2011-02-07 02:57:39 +0000 | |||
150 | @@ -826,7 +826,7 @@ | |||
151 | 826 | if symbol_versioning.deprecated_passed(revision_id): | 826 | if symbol_versioning.deprecated_passed(revision_id): |
152 | 827 | symbol_versioning.warn( | 827 | symbol_versioning.warn( |
153 | 828 | 'search_missing_revision_ids(revision_id=...) was ' | 828 | 'search_missing_revision_ids(revision_id=...) was ' |
155 | 829 | 'deprecated in 2.3. Use revision_ids=[...] instead.', | 829 | 'deprecated in 2.4. Use revision_ids=[...] instead.', |
156 | 830 | DeprecationWarning, stacklevel=2) | 830 | DeprecationWarning, stacklevel=2) |
157 | 831 | if revision_ids is not None: | 831 | if revision_ids is not None: |
158 | 832 | raise AssertionError( | 832 | raise AssertionError( |
159 | 833 | 833 | ||
160 | === modified file 'bzrlib/repository.py' | |||
161 | --- bzrlib/repository.py 2011-01-19 23:00:12 +0000 | |||
162 | +++ bzrlib/repository.py 2011-02-07 02:57:39 +0000 | |||
163 | @@ -1608,7 +1608,7 @@ | |||
164 | 1608 | if symbol_versioning.deprecated_passed(revision_id): | 1608 | if symbol_versioning.deprecated_passed(revision_id): |
165 | 1609 | symbol_versioning.warn( | 1609 | symbol_versioning.warn( |
166 | 1610 | 'search_missing_revision_ids(revision_id=...) was ' | 1610 | 'search_missing_revision_ids(revision_id=...) was ' |
168 | 1611 | 'deprecated in 2.3. Use revision_ids=[...] instead.', | 1611 | 'deprecated in 2.4. Use revision_ids=[...] instead.', |
169 | 1612 | DeprecationWarning, stacklevel=3) | 1612 | DeprecationWarning, stacklevel=3) |
170 | 1613 | if revision_ids is not None: | 1613 | if revision_ids is not None: |
171 | 1614 | raise AssertionError( | 1614 | raise AssertionError( |
172 | @@ -3526,7 +3526,7 @@ | |||
173 | 3526 | if symbol_versioning.deprecated_passed(revision_id): | 3526 | if symbol_versioning.deprecated_passed(revision_id): |
174 | 3527 | symbol_versioning.warn( | 3527 | symbol_versioning.warn( |
175 | 3528 | 'search_missing_revision_ids(revision_id=...) was ' | 3528 | 'search_missing_revision_ids(revision_id=...) was ' |
177 | 3529 | 'deprecated in 2.3. Use revision_ids=[...] instead.', | 3529 | 'deprecated in 2.4. Use revision_ids=[...] instead.', |
178 | 3530 | DeprecationWarning, stacklevel=2) | 3530 | DeprecationWarning, stacklevel=2) |
179 | 3531 | if revision_ids is not None: | 3531 | if revision_ids is not None: |
180 | 3532 | raise AssertionError( | 3532 | raise AssertionError( |
181 | 3533 | 3533 | ||
182 | === modified file 'bzrlib/tests/per_interrepository/test_interrepository.py' | |||
183 | --- bzrlib/tests/per_interrepository/test_interrepository.py 2011-01-14 17:18:23 +0000 | |||
184 | +++ bzrlib/tests/per_interrepository/test_interrepository.py 2011-02-07 02:57:39 +0000 | |||
185 | @@ -138,7 +138,7 @@ | |||
186 | 138 | find_ghosts=False) | 138 | find_ghosts=False) |
187 | 139 | self.callDeprecated( | 139 | self.callDeprecated( |
188 | 140 | ['search_missing_revision_ids(revision_id=...) was deprecated in ' | 140 | ['search_missing_revision_ids(revision_id=...) was deprecated in ' |
190 | 141 | '2.3. Use revision_ids=[...] instead.'], | 141 | '2.4. Use revision_ids=[...] instead.'], |
191 | 142 | self.assertRaises, errors.NoSuchRevision, | 142 | self.assertRaises, errors.NoSuchRevision, |
192 | 143 | repo_b.search_missing_revision_ids, repo_a, revision_id='pizza', | 143 | repo_b.search_missing_revision_ids, repo_a, revision_id='pizza', |
193 | 144 | find_ghosts=False) | 144 | find_ghosts=False) |
194 | 145 | 145 | ||
195 | === modified file 'bzrlib/tests/test_remote.py' | |||
196 | --- bzrlib/tests/test_remote.py 2011-01-14 17:18:23 +0000 | |||
197 | +++ bzrlib/tests/test_remote.py 2011-02-07 02:57:39 +0000 | |||
198 | @@ -3211,15 +3211,15 @@ | |||
199 | 3211 | override_existing=True) | 3211 | override_existing=True) |
200 | 3212 | 3212 | ||
201 | 3213 | def test_fetch_everything_backwards_compat(self): | 3213 | def test_fetch_everything_backwards_compat(self): |
203 | 3214 | """Can fetch with EverythingResult even with pre 2.3 servers. | 3214 | """Can fetch with EverythingResult even with pre 2.4 servers. |
204 | 3215 | 3215 | ||
206 | 3216 | Pre-2.3 do not support 'everything' searches with the | 3216 | Pre-2.4 do not support 'everything' searches with the |
207 | 3217 | Repository.get_stream_1.19 verb. | 3217 | Repository.get_stream_1.19 verb. |
208 | 3218 | """ | 3218 | """ |
209 | 3219 | verb_log = [] | 3219 | verb_log = [] |
210 | 3220 | class OldGetStreamVerb(SmartServerRepositoryGetStream_1_19): | 3220 | class OldGetStreamVerb(SmartServerRepositoryGetStream_1_19): |
211 | 3221 | """A version of the Repository.get_stream_1.19 verb patched to | 3221 | """A version of the Repository.get_stream_1.19 verb patched to |
213 | 3222 | reject 'everything' searches the way 2.2 and earlier do. | 3222 | reject 'everything' searches the way 2.3 and earlier do. |
214 | 3223 | """ | 3223 | """ |
215 | 3224 | def recreate_search(self, repository, search_bytes, discard_excess=False): | 3224 | def recreate_search(self, repository, search_bytes, discard_excess=False): |
216 | 3225 | verb_log.append(search_bytes.split('\n', 1)[0]) | 3225 | verb_log.append(search_bytes.split('\n', 1)[0]) |
217 | 3226 | 3226 | ||
218 | === modified file 'doc/en/release-notes/bzr-2.3.txt' | |||
219 | --- doc/en/release-notes/bzr-2.3.txt 2011-02-03 05:13:46 +0000 | |||
220 | +++ doc/en/release-notes/bzr-2.3.txt 2011-02-07 02:57:39 +0000 | |||
221 | @@ -275,11 +275,6 @@ | |||
222 | 275 | crashes when encountering private bugs (they are just displayed as such). | 275 | crashes when encountering private bugs (they are just displayed as such). |
223 | 276 | (Vincent Ladeuil, #354985) | 276 | (Vincent Ladeuil, #354985) |
224 | 277 | 277 | ||
225 | 278 | * The ``revision_id`` parameter of | ||
226 | 279 | ``Repository.search_missing_revision_ids`` and | ||
227 | 280 | ``InterRepository.search_missing_revision_ids`` is deprecated. It is | ||
228 | 281 | replaced by the ``revision_ids`` parameter. (Andrew Bennetts) | ||
229 | 282 | |||
230 | 283 | Internals | 278 | Internals |
231 | 284 | ********* | 279 | ********* |
232 | 285 | 280 | ||
233 | 286 | 281 | ||
234 | === modified file 'doc/en/release-notes/bzr-2.4.txt' | |||
235 | --- doc/en/release-notes/bzr-2.4.txt 2011-02-03 05:46:08 +0000 | |||
236 | +++ doc/en/release-notes/bzr-2.4.txt 2011-02-07 02:57:39 +0000 | |||
237 | @@ -94,6 +94,11 @@ | |||
238 | 94 | * Added ``bzrlib.mergetools`` module with helper functions for working with | 94 | * Added ``bzrlib.mergetools`` module with helper functions for working with |
239 | 95 | the list of external merge tools. (Gordon Tyler, #489915) | 95 | the list of external merge tools. (Gordon Tyler, #489915) |
240 | 96 | 96 | ||
241 | 97 | * The ``revision_id`` parameter of | ||
242 | 98 | ``Repository.search_missing_revision_ids`` and | ||
243 | 99 | ``InterRepository.search_missing_revision_ids`` is deprecated. It is | ||
244 | 100 | replaced by the ``revision_ids`` parameter. (Andrew Bennetts) | ||
245 | 101 | |||
246 | 97 | Internals | 102 | Internals |
247 | 98 | ********* | 103 | ********* |
248 | 99 | 104 |
+ if not isinstance(search, graph.Everythin gResult) :
^- can we add an attribute, rather than an isinstance check?
if not search. is_everything( ):
490 + def _present_ source_ revisions_ for(self, revision_ids): get_graph( ) get_parent_ map(revision_ ids)) ids.difference( present_ revs) NoSuchRevision( self.source, missing.pop()) get_graph( ).iter_ ancestry( revision_ ids) NULL_REVISION get_graph( )" and then you inline "self.source. get_graph( )" into the source_ids list comprehension. Better to re-use the graph object.
491 + """Returns set of all revisions in ancestry of revision_ids present in
492 + the source repo.
493 +
494 + :param revision_ids: if None, all revisions in source are returned.
495 + """
496 + if revision_ids is not None:
497 + # First, ensure all specified revisions exist. Callers expect
498 + # NoSuchRevision when they pass absent revision_ids here.
499 + revision_ids = set(revision_ids)
500 + graph = self.source.
501 + present_revs = set(graph.
502 + missing = revision_
503 + if missing:
504 + raise errors.
505 + source_ids = [rev_id for (rev_id, parents) in
506 + self.source.
507 + if rev_id != _mod_revision.
508 + and parents is not None]
^- you have "graph = self.source.
145 + def get_recipe(self): rror(self. get_recipe) struct( self):
146 + raise NotImplementedE
147 +
148 + def get_network_
149 + return ('everything',)
Why don't EverythingNotIn Other and NotInOtherForRevs implement these functions?
Overall, I think these changes seem good, but I don't really see how this gets us closer to computing a search without having one end doing a step-by-step search. (Certainly you're still using the same 'search_ missing_ revision_ ids', and you don't seem to be serializing anything over the wire...)