Merge lp:~spiv/bzr/fetch-spec-everything-not-in-other into lp:bzr

[Andrew Bennetts]

Add EverythingResult, EverythingNotInOther, and NotInOtherForRevs.

Like <https://code.launchpad.net/~spiv/bzr/sprout-does-not-reopen-repo/+merge/41037> this is a step towards bug 309682 (i.e. is a pre-req for <lp:~spiv/bzr/fetch-all-tags-309682>, as well as being a general improvement.

This patch defines some new “fetch specs” to be passed as the fetch_spec argument of fetch: EverythingResult, EmptySearchResult, EverythingNotInOther, and NotInOtherForRevs. I've created two new abstract classes, AbstractSearchResult, and AbstractSearch, to distinguish between the two different sorts of fetch spec. I think the naming is a bit off, the original fetch spec was called “SearchResult”, which made some sense, but there's also “PendingAncestryResult” which isn't really a “result”. So perhaps we should rename some or all of these things. In the current scheme EverythingNotInOther and NotInOtherForRevs are Searches, rather than Search Results. Suggestions for clearer naming would be very welcome!

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.fetch(source, revision_id=some_rev)" has known the value of some_rev without performing a search, delegating that to the fetch call. I don't think it would it simplify the code much in practice to remove this flexibility, but maybe it would simplify the concepts enough to be worth it?

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_missing_revision_ids' revision_id argument for the new revision_ids (plural).

[John A Meinel (jameinel)]

+ if not isinstance(search, graph.EverythingResult):
^- can we add an attribute, rather than an isinstance check?

if not search.is_everything():

490 + def _present_source_revisions_for(self, revision_ids):
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.get_graph()
501 + present_revs = set(graph.get_parent_map(revision_ids))
502 + missing = revision_ids.difference(present_revs)
503 + if missing:
504 + raise errors.NoSuchRevision(self.source, missing.pop())
505 + source_ids = [rev_id for (rev_id, parents) in
506 + self.source.get_graph().iter_ancestry(revision_ids)
507 + if rev_id != _mod_revision.NULL_REVISION
508 + and parents is not None]
^- you have "graph = self.source.get_graph()" and then you inline "self.source.get_graph()" into the source_ids list comprehension. Better to re-use the graph object.

145 + def get_recipe(self):
146 + raise NotImplementedError(self.get_recipe)
147 +
148 + def get_network_struct(self):
149 + return ('everything',)

Why don't EverythingNotInOther 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...)

review: Needs Fixing

[Andrew Bennetts (spiv)]

John A Meinel wrote:
> Review: Needs Fixing
> + if not isinstance(search, graph.EverythingResult):
> ^- can we add an attribute, rather than an isinstance check?
>
> if not search.is_everything():

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_source_revisions_for(self, revision_ids):
> 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.get_graph()
> 501 + present_revs = set(graph.get_parent_map(revision_ids))
> 502 + missing = revision_ids.difference(present_revs)
> 503 + if missing:
> 504 + raise errors.NoSuchRevision(self.source, missing.pop())
> 505 + source_ids = [rev_id for (rev_id, parents) in
> 506 + self.source.get_graph().iter_ancestry(revision_ids)
> 507 + if rev_id != _mod_revision.NULL_REVISION
> 508 + and parents is not None]
> ^- you have "graph = self.source.get_graph()" and then you inline "self.source.get_graph()" into the source_ids list comprehension. Better to re-use the graph object.

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 NotImplementedError(self.get_recipe)
> 147 +
> 148 + def get_network_struct(self):
> 149 + return ('everything',)
>
> Why don't EverythingNotInOther and NotInOtherForRevs implement these functions?

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, PendingAncestryResult,
   EverythingResult. They implement all the methods SearchResult implements
   (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: EverythingNotInOther, NotInOtherForRevs. They implement a get_search
   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_missing_revision_ids',
> 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-tags-309682, to make it
and easy for the sprout code to request a fetch of all tags at the
same time as the tip.

Separately...

[Martin Pool (mbp)]

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_struct(self):
+ start_keys = ' '.join(self._recipe[1])
+ stop_keys = ' '.join(self._recipe[2])
+ count = str(self._recipe[3])
+ return (self._recipe[0], '\n'.join((start_keys, stop_keys, count)))
+

rather than adhoc encoding, could you send it as bencode?

+ medium._remember_remote_is_before((2, 3))

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.searcH_missing_revision_ids()."""

typo.

+ def test_fetch_everything_backwards_compat(self):
+ """Can fetch with EverythingResult even when the server does not have
+ the Repository.get_stream_2.3 verb.
+ """
+ local = self.make_branch('local')
+ builder = self.make_branch_builder('remote')
+ builder.build_commit(message="Commit.")
+ remote_branch_url = self.smart_server.get_url() + 'remote'
+ remote_branch = bzrdir.BzrDir.open(remote_branch_url).open_branch()
+ self.hpss_calls = []
+ local.repository.fetch(remote_branch.repository,
+ fetch_spec=graph.EverythingResult(remote_branch.repository))
+

I don't understand how this tests that the server doesn't have this verb.

=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt 2010-12-14 23:32:28 +0000
+++ doc/en/release-notes/bzr-2.3.txt 2010-12-15 05:36:46 +0000
@@ -158,6 +158,11 @@
   crashes when encountering private bugs (they are just displayed as such).
   (Vincent Ladeuil, #354985)

+* The ``revision_id`` parameter of
+ ``Repository.search_missing_revision_ids`` and
+ ``InterRepository.search_missing_revision_ids`` is deprecated. It is
+ 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.

review: Needs Information

[Andrew Bennetts (spiv)]

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_struct(self):
> + start_keys = ' '.join(self._recipe[1])
> + stop_keys = ' '.join(self._recipe[2])
> + count = str(self._recipe[3])
> + return (self._recipe[0], '\n'.join((start_keys, stop_keys, count)))
> +
>
> 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 RemoteRepository._serialise_search_recipe.)

> + medium._remember_remote_is_before((2, 3))
>
> 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.searcH_missing_revision_ids()."""
>
> typo.

Fixed, thanks.

> + def test_fetch_everything_backwards_compat(self):
> + """Can fetch with EverythingResult even when the server does not have
> + the Repository.get_stream_2.3 verb.
> + """
> + local = self.make_branch('local')
> + builder = self.make_branch_builder('remote')
> + builder.build_commit(message="Commit.")
> + remote_branch_url = self.smart_server.get_url() + 'remote'
> + remote_branch = bzrdir.BzrDir.open(remote_branch_url).open_branch()
> + self.hpss_calls = []
> + local.repository.fetch(remote_branch.repository,
> + fetch_spec=graph.EverythingResult(remote_branch.repository))
> +
>
> 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)]

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)]

> > 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://code.launchpad.net/~spiv/bzr/fetch-dev-docs/+merge/46053>

I believe all the points/questions raised so far have now been answered.

[Martin Pool (mbp)]

I think it would be worth adding docstrings on AbstractSearchResult and SearchResult because the names are suggestive but not self-explanatory. The docstrings on the members are good. It is pretty well covered in your document (https://code.launchpad.net/~spiv/bzr/fetch-dev-docs/+merge/46053) so a sentence or two should be enough.

+ if symbol_versioning.deprecated_passed(revision_id):
+ symbol_versioning.warn(
+ 'search_missing_revision_ids(revision_id=...) was '
+ '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://bugs.launchpad.net/bzr/+bug/712922) And there's a couple more.

merge-with-tweaks

Thanks, and sorry for the delay.

review: Approve

=== modified file 'bzrlib/fetch.py'
--- bzrlib/fetch.py	2011-01-14 17:18:23 +0000
+++ bzrlib/fetch.py	2011-02-07 02:57:39 +0000
@@ -55,6 +55,8 @@
 
         :param last_revision: If set, try to limit to the data this revision
             references.
+        :param fetch_spec: A SearchResult specifying which revisions to fetch.
+            If set, this overrides last_revision.
         :param find_ghosts: If True search the entire history for ghosts.
         """
         # repository.fetch has the responsibility for short-circuiting
@@ -93,12 +95,12 @@
         pb.show_pct = pb.show_count = False
         try:
             pb.update("Finding revisions", 0, 2)
-            search = self._revids_to_fetch()
-            mutter('fetching: %s', search)
-            if search.is_empty():
+            search_result = self._revids_to_fetch()
+            mutter('fetching: %s', search_result)
+            if search_result.is_empty():
                 return
             pb.update("Fetching revisions", 1, 2)
-            self._fetch_everything_for_search(search)
+            self._fetch_everything_for_search(search_result)
         finally:
             pb.finished()
 
@@ -151,18 +153,11 @@
         install self._last_revision in self.to_repository.
 
         :returns: A SearchResult of some sort.  (Possibly a
-        PendingAncestryResult, EmptySearchResult, etc.)
+            PendingAncestryResult, EmptySearchResult, etc.)
         """
         mutter("self._fetch_spec, self._last_revision: %r, %r",
                 self._fetch_spec, self._last_revision)
-        get_search_result = getattr(self._fetch_spec, 'get_search_result', None)
-        if get_search_result is not None:
-            mutter(
-                'resolving fetch_spec into search result: %s', self._fetch_spec)
-            # This is EverythingNotInOther or a similar kind of fetch_spec.
-            # Turn it into a search result.
-            return get_search_result()
-        elif self._fetch_spec is not None:
+        if self._fetch_spec is not None:
             # The fetch spec is already a concrete search result.
             return self._fetch_spec
         elif self._last_revision == NULL_REVISION:
@@ -172,11 +167,11 @@
         elif self._last_revision is not None:
             return graph.NotInOtherForRevs(self.to_repository,
                 self.from_repository, [self._last_revision],
-                find_ghosts=self.find_ghosts).get_search_result()
+                find_ghosts=self.find_ghosts).execute()
         else: # self._last_revision is None:
             return graph.EverythingNotInOther(self.to_repository,
                 self.from_repository,
-                find_ghosts=self.find_ghosts).get_search_result()
+                find_ghosts=self.find_ghosts).execute()
 
 
 class Inter1and2Helper(object):

=== modified file 'bzrlib/graph.py'
--- bzrlib/graph.py	2011-01-14 17:18:23 +0000
+++ bzrlib/graph.py	2011-02-07 02:57:39 +0000
@@ -1576,14 +1576,15 @@
 
 class AbstractSearch(object):
 
-    def get_search_result(self):
+    def execute(self):
         """Construct a network-ready search result from this search description.
 
         This may take some time to search repositories, etc.
 
-        :return: A search result.
+        :return: A search result (an object that implements
+            AbstractSearchResult's API).
         """
-        raise NotImplementedError(self.get_search_result)
+        raise NotImplementedError(self.execute)
 
 
 class SearchResult(AbstractSearchResult):
@@ -1813,7 +1814,7 @@
         self.from_repo = from_repo
         self.find_ghosts = find_ghosts
 
-    def get_search_result(self):
+    def execute(self):
         return self.to_repo.search_missing_revision_ids(
             self.from_repo, find_ghosts=self.find_ghosts)
 
@@ -1853,7 +1854,7 @@
             self.__class__.__name__, self.from_repo, self.to_repo,
             self.find_ghosts, reqd_revs_repr, ifp_revs_repr)
 
-    def get_search_result(self):
+    def execute(self):
         return self.to_repo.search_missing_revision_ids(
             self.from_repo, revision_ids=self.required_ids,
             if_present_ids=self.if_present_ids, find_ghosts=self.find_ghosts)

=== modified file 'bzrlib/remote.py'
--- bzrlib/remote.py	2011-01-14 17:18:23 +0000
+++ bzrlib/remote.py	2011-02-07 02:57:39 +0000
@@ -1360,7 +1360,7 @@
         if symbol_versioning.deprecated_passed(revision_id):
             symbol_versioning.warn(
                 'search_missing_revision_ids(revision_id=...) was '
-                'deprecated in 2.3.  Use revision_ids=[...] instead.',
+                'deprecated in 2.4.  Use revision_ids=[...] instead.',
                 DeprecationWarning, stacklevel=2)
             if revision_ids is not None:
                 raise AssertionError(
@@ -1991,11 +1991,11 @@
                 if isinstance(search, graph.EverythingResult):
                     error_verb = e.error_from_smart_server.error_verb
                     if error_verb == 'BadSearch':
-                        # Pre-2.3 servers don't support this sort of search.
+                        # Pre-2.4 servers don't support this sort of search.
                         # XXX: perhaps falling back to VFS on BadSearch is a
                         # good idea in general?  It might provide a little bit
                         # of protection against client-side bugs.
-                        medium._remember_remote_is_before((2, 3))
+                        medium._remember_remote_is_before((2, 4))
                         break
                 raise
             else:

=== modified file 'bzrlib/repofmt/knitrepo.py'
--- bzrlib/repofmt/knitrepo.py	2010-12-21 06:10:11 +0000
+++ bzrlib/repofmt/knitrepo.py	2011-02-07 02:57:39 +0000
@@ -542,7 +542,7 @@
         if symbol_versioning.deprecated_passed(revision_id):
             symbol_versioning.warn(
                 'search_missing_revision_ids(revision_id=...) was '
-                'deprecated in 2.3.  Use revision_ids=[...] instead.',
+                'deprecated in 2.4.  Use revision_ids=[...] instead.',
                 DeprecationWarning, stacklevel=2)
             if revision_ids is not None:
                 raise AssertionError(

=== modified file 'bzrlib/repofmt/weaverepo.py'
--- bzrlib/repofmt/weaverepo.py	2011-01-14 17:18:23 +0000
+++ bzrlib/repofmt/weaverepo.py	2011-02-07 02:57:39 +0000
@@ -826,7 +826,7 @@
         if symbol_versioning.deprecated_passed(revision_id):
             symbol_versioning.warn(
                 'search_missing_revision_ids(revision_id=...) was '
-                'deprecated in 2.3.  Use revision_ids=[...] instead.',
+                'deprecated in 2.4.  Use revision_ids=[...] instead.',
                 DeprecationWarning, stacklevel=2)
             if revision_ids is not None:
                 raise AssertionError(

=== modified file 'bzrlib/repository.py'
--- bzrlib/repository.py	2011-01-19 23:00:12 +0000
+++ bzrlib/repository.py	2011-02-07 02:57:39 +0000
@@ -1608,7 +1608,7 @@
         if symbol_versioning.deprecated_passed(revision_id):
             symbol_versioning.warn(
                 'search_missing_revision_ids(revision_id=...) was '
-                'deprecated in 2.3.  Use revision_ids=[...] instead.',
+                'deprecated in 2.4.  Use revision_ids=[...] instead.',
                 DeprecationWarning, stacklevel=3)
             if revision_ids is not None:
                 raise AssertionError(
@@ -3526,7 +3526,7 @@
         if symbol_versioning.deprecated_passed(revision_id):
             symbol_versioning.warn(
                 'search_missing_revision_ids(revision_id=...) was '
-                'deprecated in 2.3.  Use revision_ids=[...] instead.',
+                'deprecated in 2.4.  Use revision_ids=[...] instead.',
                 DeprecationWarning, stacklevel=2)
             if revision_ids is not None:
                 raise AssertionError(

=== modified file 'bzrlib/tests/per_interrepository/test_interrepository.py'
--- bzrlib/tests/per_interrepository/test_interrepository.py	2011-01-14 17:18:23 +0000
+++ bzrlib/tests/per_interrepository/test_interrepository.py	2011-02-07 02:57:39 +0000
@@ -138,7 +138,7 @@
             find_ghosts=False)
         self.callDeprecated(
             ['search_missing_revision_ids(revision_id=...) was deprecated in '
-             '2.3.  Use revision_ids=[...] instead.'],
+             '2.4.  Use revision_ids=[...] instead.'],
             self.assertRaises, errors.NoSuchRevision,
             repo_b.search_missing_revision_ids, repo_a, revision_id='pizza',
             find_ghosts=False)

=== modified file 'bzrlib/tests/test_remote.py'
--- bzrlib/tests/test_remote.py	2011-01-14 17:18:23 +0000
+++ bzrlib/tests/test_remote.py	2011-02-07 02:57:39 +0000
@@ -3211,15 +3211,15 @@
                 override_existing=True)
 
     def test_fetch_everything_backwards_compat(self):
-        """Can fetch with EverythingResult even with pre 2.3 servers.
+        """Can fetch with EverythingResult even with pre 2.4 servers.
         
-        Pre-2.3 do not support 'everything' searches with the
+        Pre-2.4 do not support 'everything' searches with the
         Repository.get_stream_1.19 verb.
         """
         verb_log = []
         class OldGetStreamVerb(SmartServerRepositoryGetStream_1_19):
             """A version of the Repository.get_stream_1.19 verb patched to
-            reject 'everything' searches the way 2.2 and earlier do.
+            reject 'everything' searches the way 2.3 and earlier do.
             """
             def recreate_search(self, repository, search_bytes, discard_excess=False):
                 verb_log.append(search_bytes.split('\n', 1)[0])

=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2011-02-03 05:13:46 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2011-02-07 02:57:39 +0000
@@ -275,11 +275,6 @@
   crashes when encountering private bugs (they are just displayed as such).
   (Vincent Ladeuil, #354985)
 
-* The ``revision_id`` parameter of
-  ``Repository.search_missing_revision_ids`` and
-  ``InterRepository.search_missing_revision_ids`` is deprecated.  It is
-  replaced by the ``revision_ids`` parameter.  (Andrew Bennetts)
-
 Internals
 *********
 

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-02-03 05:46:08 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-02-07 02:57:39 +0000
@@ -94,6 +94,11 @@
 * Added ``bzrlib.mergetools`` module with helper functions for working with
   the list of external merge tools. (Gordon Tyler, #489915)
 
+* The ``revision_id`` parameter of
+  ``Repository.search_missing_revision_ids`` and
+  ``InterRepository.search_missing_revision_ids`` is deprecated.  It is
+  replaced by the ``revision_ids`` parameter.  (Andrew Bennetts)
+
 Internals
 *********