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 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. As an added wrinkle, EverythingNotInOther an NotInOtherForRevs aren't directly “results” at all, but factories that can be turned into a result. Suggestions for clearer naming would be very welcome!

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...

=== modified file 'bzrlib/fetch.py'
--- bzrlib/fetch.py	2010-09-17 04:35:23 +0000
+++ bzrlib/fetch.py	2010-12-03 07:07:15 +0000
@@ -28,6 +28,7 @@
 from bzrlib.lazy_import import lazy_import
 lazy_import(globals(), """
 from bzrlib import (
+    graph,
     tsort,
     versionedfile,
     )
@@ -93,7 +94,8 @@
         try:
             pb.update("Finding revisions", 0, 2)
             search = self._revids_to_fetch()
-            if search is None:
+            mutter('fetching: %s', search)
+            if search.is_empty():
                 return
             pb.update("Fetching revisions", 1, 2)
             self._fetch_everything_for_search(search)
@@ -126,8 +128,15 @@
             resume_tokens, missing_keys = self.sink.insert_stream(
                 stream, from_format, [])
             if self.to_repository._fallback_repositories:
-                missing_keys.update(
-                    self._parent_inventories(search.get_keys()))
+                if not isinstance(search, graph.EverythingResult):
+                    # If search is EverythingResult this is be unnecessary,
+                    # so we can skip this step.  The source will send us
+                    # every revision it has, and their parent inventories.
+                    # (Unless the source is damaged!  but not really worth
+                    # optimising for that case.  The pack code will reject bad
+                    # streams anyway.)
+                    missing_keys.update(
+                        self._parent_inventories(search.get_keys()))
             if missing_keys:
                 pb.update("Missing keys")
                 stream = source.get_stream_for_missing_keys(missing_keys)
@@ -151,17 +160,33 @@
         """Determines the exact revisions needed from self.from_repository to
         install self._last_revision in self.to_repository.
 
-        If no revisions need to be fetched, then this just returns None.
+        :returns: A SearchResult of some sort.  (Possibly a
+        PendingAncestryResult, EmptySearchResult, etc.)
         """
-        if self._fetch_spec is not None:
+        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:
+            # The fetch spec is already a concrete search result.
             return self._fetch_spec
-        mutter('fetch up to rev {%s}', self._last_revision)
-        if self._last_revision is NULL_REVISION:
+        elif self._last_revision == NULL_REVISION:
+            # fetch_spec is None + last_revision is null => empty fetch.
             # explicit limit of no revisions needed
-            return None
-        return self.to_repository.search_missing_revision_ids(
-            self.from_repository, self._last_revision,
-            find_ghosts=self.find_ghosts)
+            return graph.EmptySearchResult()
+        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()
+        else: # self._last_revision is None:
+            return graph.EverythingNotInOther(self.to_repository,
+                self.from_repository,
+                find_ghosts=self.find_ghosts).get_search_result()
 
     def _parent_inventories(self, revision_ids):
         # Find all the parent revisions referenced by the stream, but

=== modified file 'bzrlib/graph.py'
--- bzrlib/graph.py	2010-08-15 15:20:14 +0000
+++ bzrlib/graph.py	2010-12-03 07:07:15 +0000
@@ -1536,7 +1536,57 @@
             return revs, ghosts
 
 
-class SearchResult(object):
+class AbstractSearchResult(object):
+
+    def get_recipe(self):
+        """Return a recipe that can be used to replay this search.
+
+        The recipe allows reconstruction of the same results at a later date.
+
+        :return: A tuple of (search_kind_str, *details).  The details vary by
+            kind of search result.
+        """
+        raise NotImplementedError(self.get_recipe)
+
+    def get_network_struct(self):
+        """Return a tuple that can be transmitted via the HPSS protocol."""
+        raise NotImplementedError(self.get_network_struct)
+
+    def get_keys(self):
+        """Return the keys found in this search.
+
+        :return: A set of keys.
+        """
+        raise NotImplementedError(self.get_keys)
+
+    def is_empty(self):
+        """Return false if the search lists 1 or more revisions."""
+        raise NotImplementedError(self.is_empty)
+
+    def refine(self, seen, referenced):
+        """Create a new search by refining this search.
+
+        :param seen: Revisions that have been satisfied.
+        :param referenced: Revision references observed while satisfying some
+            of this search.
+        :return: A search result.
+        """
+        raise NotImplementedError(self.refine)
+
+
+class AbstractSearch(object):
+
+    def get_search_result(self):
+        """Construct a network-ready search result from this search description.
+
+        This may take some time to search repositories, etc.
+
+        :return: A search result.
+        """
+        raise NotImplementedError(self.get_search_result)
+
+
+class SearchResult(AbstractSearchResult):
     """The result of a breadth first search.
 
     A SearchResult provides the ability to reconstruct the search or access a
@@ -1557,6 +1607,19 @@
         self._recipe = ('search', start_keys, exclude_keys, key_count)
         self._keys = frozenset(keys)
 
+    def __repr__(self):
+        kind, start_keys, exclude_keys, key_count = self._recipe
+        if len(start_keys) > 5:
+            start_keys_repr = repr(list(start_keys)[:5])[:-1] + ', ...]'
+        else:
+            start_keys_repr = repr(start_keys)
+        if len(exclude_keys) > 5:
+            exclude_keys_repr = repr(list(exclude_keys)[:5])[:-1] + ', ...]'
+        else:
+            exclude_keys_repr = repr(exclude_keys)
+        return '<%s %s:(%s, %s, %d)>' % (self.__class__.__name__,
+            kind, start_keys_repr, exclude_keys_repr, key_count)
+
     def get_recipe(self):
         """Return a recipe that can be used to replay this search.
 
@@ -1580,6 +1643,12 @@
         """
         return self._recipe
 
+    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)))
+
     def get_keys(self):
         """Return the keys found in this search.
 
@@ -1617,7 +1686,7 @@
         return SearchResult(pending_refs, exclude, count, keys)
 
 
-class PendingAncestryResult(object):
+class PendingAncestryResult(AbstractSearchResult):
     """A search result that will reconstruct the ancestry for some graph heads.
 
     Unlike SearchResult, this doesn't hold the complete search result in
@@ -1647,6 +1716,11 @@
         """
         return ('proxy-search', self.heads, set(), -1)
 
+    def get_network_struct(self):
+        parts = ['ancestry-of']
+        parts.extend(self.heads)
+        return parts
+
     def get_keys(self):
         """See SearchResult.get_keys.
 
@@ -1679,6 +1753,104 @@
         return PendingAncestryResult(referenced - seen, self.repo)
 
 
+class EmptySearchResult(AbstractSearchResult):
+    """An empty search result."""
+
+    def is_empty(self):
+        return True
+    
+
+class EverythingResult(AbstractSearchResult):
+    """A search result that simply requests everything in the repository."""
+
+    def __init__(self, repo):
+        self._repo = repo
+
+    def __repr__(self):
+        return '%s(%r)' % (self.__class__.__name__, self._repo)
+
+    def get_recipe(self):
+        raise NotImplementedError(self.get_recipe)
+
+    def get_network_struct(self):
+        return ('everything',)
+
+    def get_keys(self):
+        if 'evil' in debug.debug_flags:
+            from bzrlib import remote
+            if isinstance(self._repo, remote.RemoteRepository):
+                # warn developers (not users) not to do this
+                trace.mutter_callsite(
+                    2, "EverythingResult(RemoteRepository).get_keys() is slow.")
+        return self._repo.all_revision_ids()
+
+    def is_empty(self):
+        # It's ok for this to wrongly return False: the worst that can happen
+        # is that RemoteStreamSource will initiate a get_stream on an empty
+        # repository.  And almost all repositories are non-empty.
+        return False
+
+    def refine(self, seen, referenced):
+        heads = set(self._repo.all_revision_ids())
+        heads.difference_update(seen)
+        heads.update(referenced)
+        return PendingAncestryResult(heads, self._repo)
+
+
+class EverythingNotInOther(AbstractSearch):
+    """Find all revisions in that are in one repo but not the other."""
+
+    def __init__(self, to_repo, from_repo, find_ghosts=False):
+        self.to_repo = to_repo
+        self.from_repo = from_repo
+        self.find_ghosts = find_ghosts
+
+    def get_search_result(self):
+        return self.to_repo.search_missing_revision_ids(
+            self.from_repo, find_ghosts=self.find_ghosts)
+
+
+class NotInOtherForRevs(AbstractSearch):
+    """Find all revisions missing in one repo for a some specific heads."""
+
+    def __init__(self, to_repo, from_repo, required_ids, if_present_ids=None,
+            find_ghosts=False):
+        """Constructor.
+
+        :param required_ids: revision IDs of heads that must be found, or else
+            the search will fail with NoSuchRevision.  All revisions in their
+            ancestry not already in the other repository will be included in
+            the search result.
+        :param if_present_ids: revision IDs of heads that may be absent in the
+            source repository.  If present, then their ancestry not already
+            found in other will be included in the search result.
+        """
+        self.to_repo = to_repo
+        self.from_repo = from_repo
+        self.find_ghosts = find_ghosts
+        self.required_ids = required_ids
+        self.if_present_ids = if_present_ids
+
+    def __repr__(self):
+        if len(self.required_ids) > 5:
+            reqd_revs_repr = repr(list(self.required_ids)[:5])[:-1] + ', ...]'
+        else:
+            reqd_revs_repr = repr(self.required_ids)
+        if self.if_present_ids and len(self.if_present_ids) > 5:
+            ifp_revs_repr = repr(list(self.if_present_ids)[:5])[:-1] + ', ...]'
+        else:
+            ifp_revs_repr = repr(self.if_present_ids)
+
+        return "<%s from:%r to:%r find_ghosts:%r req'd:%r if-present:%r>" % (
+            self.__class__.__name__, self.from_repo, self.to_repo,
+            self.find_ghosts, reqd_revs_repr, ifp_revs_repr)
+
+    def get_search_result(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)
+
+
 def collapse_linear_regions(parent_map):
     """Collapse regions of the graph that are 'linear'.
 

=== modified file 'bzrlib/remote.py'
--- bzrlib/remote.py	2010-12-02 10:41:05 +0000
+++ bzrlib/remote.py	2010-12-03 07:07:15 +0000
@@ -1344,15 +1344,29 @@
         return result
 
     @needs_read_lock
-    def search_missing_revision_ids(self, other, revision_id=None, find_ghosts=True):
+    def search_missing_revision_ids(self, other,
+            revision_id=symbol_versioning.DEPRECATED_PARAMETER,
+            find_ghosts=True, revision_ids=None, if_present_ids=None):
         """Return the revision ids that other has that this does not.
 
         These are returned in topological order.
 
         revision_id: only return revision ids included by revision_id.
         """
-        return repository.InterRepository.get(
-            other, self).search_missing_revision_ids(revision_id, find_ghosts)
+        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)
+            if revision_ids is not None:
+                raise AssertionError(
+                    'revision_ids is mutually exclusive with revision_id')
+            if revision_id is not None:
+                revision_ids = [revision_id]
+        inter_repo = repository.InterRepository.get(other, self)
+        return inter_repo.search_missing_revision_ids(
+            find_ghosts=find_ghosts, revision_ids=revision_ids,
+            if_present_ids=if_present_ids)
 
     def fetch(self, source, revision_id=None, pb=None, find_ghosts=False,
             fetch_spec=None):
@@ -1759,12 +1773,7 @@
         return '\n'.join((start_keys, stop_keys, count))
 
     def _serialise_search_result(self, search_result):
-        if isinstance(search_result, graph.PendingAncestryResult):
-            parts = ['ancestry-of']
-            parts.extend(search_result.heads)
-        else:
-            recipe = search_result.get_recipe()
-            parts = [recipe[0], self._serialise_search_recipe(recipe)]
+        parts = search_result.get_network_struct()
         return '\n'.join(parts)
 
     def autopack(self):
@@ -1964,6 +1973,7 @@
         candidate_verbs = [
             ('Repository.get_stream_1.19', (1, 19)),
             ('Repository.get_stream', (1, 13))]
+
         found_verb = False
         for verb, version in candidate_verbs:
             if medium._is_remote_before(version):
@@ -1973,6 +1983,17 @@
                     verb, args, search_bytes)
             except errors.UnknownSmartMethod:
                 medium._remember_remote_is_before(version)
+            except errors.UnknownErrorFromSmartServer, e:
+                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.
+                        # 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))
+                        break
+                raise
             else:
                 response_tuple, response_handler = response
                 found_verb = True

=== modified file 'bzrlib/repofmt/knitrepo.py'
--- bzrlib/repofmt/knitrepo.py	2010-11-20 21:41:05 +0000
+++ bzrlib/repofmt/knitrepo.py	2010-12-03 07:07:15 +0000
@@ -43,6 +43,7 @@
     RepositoryFormat,
     RootCommitBuilder,
     )
+from bzrlib import symbol_versioning
 
 
 class _KnitParentsProvider(object):
@@ -534,16 +535,23 @@
         return are_knits and InterRepository._same_model(source, target)
 
     @needs_read_lock
-    def search_missing_revision_ids(self, revision_id=None, find_ghosts=True):
-        """See InterRepository.missing_revision_ids()."""
-        if revision_id is not None:
-            source_ids = self.source.get_ancestry(revision_id)
-            if source_ids[0] is not None:
-                raise AssertionError()
-            source_ids.pop(0)
-        else:
-            source_ids = self.source.all_revision_ids()
-        source_ids_set = set(source_ids)
+    def search_missing_revision_ids(self,
+            revision_id=symbol_versioning.DEPRECATED_PARAMETER,
+            find_ghosts=True, revision_ids=None, if_present_ids=None):
+        """See InterRepository.searcH_missing_revision_ids()."""
+        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)
+            if revision_ids is not None:
+                raise AssertionError(
+                    'revision_ids is mutually exclusive with revision_id')
+            if revision_id is not None:
+                revision_ids = [revision_id]
+        del revision_id
+        source_ids_set = self._present_source_revisions_for(
+            revision_ids, if_present_ids)
         # source_ids is the worst possible case we may need to pull.
         # now we want to filter source_ids against what we actually
         # have in target, but don't try to check for existence where we know
@@ -553,7 +561,7 @@
         actually_present_revisions = set(
             self.target._eliminate_revisions_not_present(possibly_present_revisions))
         required_revisions = source_ids_set.difference(actually_present_revisions)
-        if revision_id is not None:
+        if revision_ids is not None:
             # we used get_ancestry to determine source_ids then we are assured all
             # revisions referenced are present as they are installed in topological order.
             # and the tip revision was validated by get_ancestry.

=== modified file 'bzrlib/repofmt/weaverepo.py'
--- bzrlib/repofmt/weaverepo.py	2010-11-20 21:41:05 +0000
+++ bzrlib/repofmt/weaverepo.py	2010-12-03 07:07:15 +0000
@@ -39,6 +39,7 @@
     lockable_files,
     lockdir,
     osutils,
+    symbol_versioning,
     trace,
     urlutils,
     versionedfile,
@@ -803,8 +804,10 @@
             self.target.fetch(self.source, revision_id=revision_id)
 
     @needs_read_lock
-    def search_missing_revision_ids(self, revision_id=None, find_ghosts=True):
-        """See InterRepository.missing_revision_ids()."""
+    def search_missing_revision_ids(self,
+            revision_id=symbol_versioning.DEPRECATED_PARAMETER,
+            find_ghosts=True, revision_ids=None, if_present_ids=None):
+        """See InterRepository.search_missing_revision_ids()."""
         # we want all revisions to satisfy revision_id in source.
         # but we don't want to stat every file here and there.
         # we want then, all revisions other needs to satisfy revision_id
@@ -816,14 +819,19 @@
         # disk format scales terribly for push anyway due to rewriting
         # inventory.weave, this is considered acceptable.
         # - RBC 20060209
-        if revision_id is not None:
-            source_ids = self.source.get_ancestry(revision_id)
-            if source_ids[0] is not None:
-                raise AssertionError()
-            source_ids.pop(0)
-        else:
-            source_ids = self.source._all_possible_ids()
-        source_ids_set = set(source_ids)
+        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)
+            if revision_ids is not None:
+                raise AssertionError(
+                    'revision_ids is mutually exclusive with revision_id')
+            if revision_id is not None:
+                revision_ids = [revision_id]
+        del revision_id
+        source_ids_set = self._present_source_revisions_for(
+            revision_ids, if_present_ids)
         # source_ids is the worst possible case we may need to pull.
         # now we want to filter source_ids against what we actually
         # have in target, but don't try to check for existence where we know
@@ -833,7 +841,7 @@
         actually_present_revisions = set(
             self.target._eliminate_revisions_not_present(possibly_present_revisions))
         required_revisions = source_ids_set.difference(actually_present_revisions)
-        if revision_id is not None:
+        if revision_ids is not None:
             # we used get_ancestry to determine source_ids then we are assured all
             # revisions referenced are present as they are installed in topological order.
             # and the tip revision was validated by get_ancestry.

=== modified file 'bzrlib/repository.py'
--- bzrlib/repository.py	2010-12-02 10:41:05 +0000
+++ bzrlib/repository.py	2010-12-03 07:07:15 +0000
@@ -42,7 +42,6 @@
     pyutils,
     revision as _mod_revision,
     static_tuple,
-    symbol_versioning,
     trace,
     tsort,
     versionedfile,
@@ -57,6 +56,7 @@
 from bzrlib import (
     errors,
     registry,
+    symbol_versioning,
     ui,
     )
 from bzrlib.decorators import needs_read_lock, needs_write_lock, only_raises
@@ -1561,15 +1561,28 @@
         return ret
 
     @needs_read_lock
-    def search_missing_revision_ids(self, other, revision_id=None, find_ghosts=True):
+    def search_missing_revision_ids(self, other,
+            revision_id=symbol_versioning.DEPRECATED_PARAMETER,
+            find_ghosts=True, revision_ids=None, if_present_ids=None):
         """Return the revision ids that other has that this does not.
 
         These are returned in topological order.
 
         revision_id: only return revision ids included by revision_id.
         """
+        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=3)
+            if revision_ids is not None:
+                raise AssertionError(
+                    'revision_ids is mutually exclusive with revision_id')
+            if revision_id is not None:
+                revision_ids = [revision_id]
         return InterRepository.get(other, self).search_missing_revision_ids(
-            revision_id, find_ghosts)
+            find_ghosts=find_ghosts, revision_ids=revision_ids,
+            if_present_ids=if_present_ids)
 
     @staticmethod
     def open(base):
@@ -3430,7 +3443,7 @@
                                fetch_spec=fetch_spec,
                                find_ghosts=find_ghosts)
 
-    def _walk_to_common_revisions(self, revision_ids):
+    def _walk_to_common_revisions(self, revision_ids, if_present_ids=None):
         """Walk out from revision_ids in source to revisions target has.
 
         :param revision_ids: The start point for the search.
@@ -3438,10 +3451,14 @@
         """
         target_graph = self.target.get_graph()
         revision_ids = frozenset(revision_ids)
+        if if_present_ids:
+            all_wanted_revs = revision_ids.union(if_present_ids)
+        else:
+            all_wanted_revs = revision_ids
         missing_revs = set()
         source_graph = self.source.get_graph()
         # ensure we don't pay silly lookup costs.
-        searcher = source_graph._make_breadth_first_searcher(revision_ids)
+        searcher = source_graph._make_breadth_first_searcher(all_wanted_revs)
         null_set = frozenset([_mod_revision.NULL_REVISION])
         searcher_exhausted = False
         while True:
@@ -3483,30 +3500,79 @@
         return searcher.get_result()
 
     @needs_read_lock
-    def search_missing_revision_ids(self, revision_id=None, find_ghosts=True):
+    def search_missing_revision_ids(self,
+            revision_id=symbol_versioning.DEPRECATED_PARAMETER,
+            find_ghosts=True, revision_ids=None, if_present_ids=None):
         """Return the revision ids that source has that target does not.
 
         :param revision_id: only return revision ids included by this
-                            revision_id.
+            revision_id.
+        :param revision_ids: return revision ids included by these
+            revision_ids.  NoSuchRevision will be raised if any of these
+            revisions are not present.
+        :param if_present_ids: like revision_ids, but will not cause
+            NoSuchRevision if any of these are absent, instead they will simply
+            not be in the result.  This is useful for e.g. finding revisions
+            to fetch for tags, which may reference absent revisions.
         :param find_ghosts: If True find missing revisions in deep history
             rather than just finding the surface difference.
         :return: A bzrlib.graph.SearchResult.
         """
+        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)
+            if revision_ids is not None:
+                raise AssertionError(
+                    'revision_ids is mutually exclusive with revision_id')
+            if revision_id is not None:
+                revision_ids = [revision_id]
+        del revision_id
         # stop searching at found target revisions.
-        if not find_ghosts and revision_id is not None:
-            return self._walk_to_common_revisions([revision_id])
+        if not find_ghosts and (revision_ids is not None or if_present_ids is
+                not None):
+            return self._walk_to_common_revisions(revision_ids,
+                    if_present_ids=if_present_ids)
         # generic, possibly worst case, slow code path.
         target_ids = set(self.target.all_revision_ids())
-        if revision_id is not None:
-            source_ids = self.source.get_ancestry(revision_id)
-            if source_ids[0] is not None:
-                raise AssertionError()
-            source_ids.pop(0)
-        else:
-            source_ids = self.source.all_revision_ids()
+        source_ids = self._present_source_revisions_for(
+            revision_ids, if_present_ids)
         result_set = set(source_ids).difference(target_ids)
         return self.source.revision_ids_to_search_result(result_set)
 
+    def _present_source_revisions_for(self, revision_ids, if_present_ids=None):
+        """Returns set of all revisions in ancestry of revision_ids present in
+        the source repo.
+
+        :param revision_ids: if None, all revisions in source are returned.
+        :param if_present_ids: like revision_ids, but if any/all of these are
+            absent no error is raised.
+        """
+        if revision_ids is not None or if_present_ids is not None:
+            # First, ensure all specified revisions exist.  Callers expect
+            # NoSuchRevision when they pass absent revision_ids here.
+            if revision_ids is None:
+                revision_ids = set()
+            if if_present_ids is None:
+                if_present_ids = set()
+            revision_ids = set(revision_ids)
+            if_present_ids = set(if_present_ids)
+            all_wanted_ids = revision_ids.union(if_present_ids)
+            graph = self.source.get_graph()
+            present_revs = set(graph.get_parent_map(all_wanted_ids))
+            missing = revision_ids.difference(present_revs)
+            if missing:
+                raise errors.NoSuchRevision(self.source, missing.pop())
+            found_ids = all_wanted_ids.intersection(present_revs)
+            source_ids = [rev_id for (rev_id, parents) in
+                          graph.iter_ancestry(found_ids)
+                          if rev_id != _mod_revision.NULL_REVISION
+                          and parents is not None]
+        else:
+            source_ids = self.source.all_revision_ids()
+        return set(source_ids)
+
     @staticmethod
     def _same_model(source, target):
         """True if source and target have the same data representation.
@@ -3830,7 +3896,13 @@
             fetch_spec=None):
         """See InterRepository.fetch()."""
         if fetch_spec is not None:
-            raise AssertionError("Not implemented yet...")
+            if (isinstance(fetch_spec, graph.NotInOtherForRevs) and
+                    len(fetch_spec.required_ids) == 1 and not
+                    fetch_spec.if_present_ids):
+                revision_id = list(fetch_spec.required_ids)[0]
+                del fetch_spec
+            else:
+                raise AssertionError("Not implemented yet...")
         ui.ui_factory.warn_experimental_format_fetch(self)
         if (not self.source.supports_rich_root()
             and self.target.supports_rich_root()):
@@ -3843,8 +3915,12 @@
             ui.ui_factory.show_user_warning('cross_format_fetch',
                 from_format=self.source._format,
                 to_format=self.target._format)
+        if revision_id:
+            search_revision_ids = [revision_id]
+        else:
+            search_revision_ids = None
         revision_ids = self.target.search_missing_revision_ids(self.source,
-            revision_id, find_ghosts=find_ghosts).get_keys()
+            revision_ids=search_revision_ids, find_ghosts=find_ghosts).get_keys()
         if not revision_ids:
             return 0, 0
         revision_ids = tsort.topo_sort(

=== modified file 'bzrlib/smart/repository.py'
--- bzrlib/smart/repository.py	2010-11-16 06:06:11 +0000
+++ bzrlib/smart/repository.py	2010-12-03 07:07:15 +0000
@@ -81,6 +81,8 @@
             recreate_search trusts that clients will look for missing things
             they expected and get it from elsewhere.
         """
+        if search_bytes == 'everything':
+            return graph.EverythingResult(repository), None
         lines = search_bytes.split('\n')
         if lines[0] == 'ancestry-of':
             heads = lines[1:]
@@ -412,6 +414,13 @@
     def do_repository_request(self, repository, to_network_name):
         """Get a stream for inserting into a to_format repository.
 
+        The request body is 'search_bytes', a description of the revisions
+        being requested.
+
+        In 2.3 this verb added support for search_bytes == 'everything'.  Older
+        implementations will respond with a BadSearch error, and clients should
+        catch this and fallback appropriately.
+
         :param repository: The repository to stream from.
         :param to_network_name: The network name of the format of the target
             repository.
@@ -489,6 +498,13 @@
 
 
 class SmartServerRepositoryGetStream_1_19(SmartServerRepositoryGetStream):
+    """The same as Repository.get_stream, but will return stream CHK formats to
+    clients.
+
+    See SmartServerRepositoryGetStream._should_fake_unknown.
+    
+    New in 1.19.
+    """
 
     def _should_fake_unknown(self):
         """Returns False; we don't need to workaround bugs in 1.19+ clients."""

=== modified file 'bzrlib/tests/per_interrepository/test_interrepository.py'
--- bzrlib/tests/per_interrepository/test_interrepository.py	2009-07-10 06:46:10 +0000
+++ bzrlib/tests/per_interrepository/test_interrepository.py	2010-12-03 07:07:15 +0000
@@ -139,9 +139,15 @@
         self.assertFalse(repo_b.has_revision('pizza'))
         # Asking specifically for an absent revision errors.
         self.assertRaises(errors.NoSuchRevision,
-            repo_b.search_missing_revision_ids, repo_a, revision_id='pizza',
+            repo_b.search_missing_revision_ids, repo_a, revision_ids=['pizza'],
             find_ghosts=True)
         self.assertRaises(errors.NoSuchRevision,
+            repo_b.search_missing_revision_ids, repo_a, revision_ids=['pizza'],
+            find_ghosts=False)
+        self.callDeprecated(
+            ['search_missing_revision_ids(revision_id=...) was deprecated in '
+             '2.3.  Use revision_ids=[...] instead.'],
+            self.assertRaises, errors.NoSuchRevision,
             repo_b.search_missing_revision_ids, repo_a, revision_id='pizza',
             find_ghosts=False)
 
@@ -151,7 +157,8 @@
         # make a repository to compare against that is empty
         repo_b = self.make_to_repository('empty')
         repo_a = self.bzrdir.open_repository()
-        result = repo_b.search_missing_revision_ids(repo_a, revision_id='rev1')
+        result = repo_b.search_missing_revision_ids(
+            repo_a, revision_ids=['rev1'])
         self.assertEqual(set(['rev1']), result.get_keys())
         self.assertEqual(('search', set(['rev1']), set([NULL_REVISION]), 1),
             result.get_recipe())

=== modified file 'bzrlib/tests/per_repository_reference/test_fetch.py'
--- bzrlib/tests/per_repository_reference/test_fetch.py	2009-06-01 18:13:46 +0000
+++ bzrlib/tests/per_repository_reference/test_fetch.py	2010-12-03 07:07:15 +0000
@@ -18,6 +18,7 @@
 from bzrlib import (
     branch,
     errors,
+    graph,
     )
 from bzrlib.smart import (
     server,
@@ -25,7 +26,7 @@
 from bzrlib.tests.per_repository import TestCaseWithRepository
 
 
-class TestFetch(TestCaseWithRepository):
+class TestFetchBase(TestCaseWithRepository):
 
     def make_source_branch(self):
         # It would be nice if there was a way to force this to be memory-only
@@ -51,6 +52,9 @@
         self.addCleanup(source_b.unlock)
         return content, source_b
 
+
+class TestFetch(TestFetchBase):
+
     def test_sprout_from_stacked_with_short_history(self):
         content, source_b = self.make_source_branch()
         # Split the generated content into a base branch, and a stacked branch
@@ -149,3 +153,38 @@
         source_b.lock_read()
         self.addCleanup(source_b.unlock)
         stacked.pull(source_b, stop_revision='B-id')
+
+
+class TestFetchFromRepoWithUnconfiguredFallbacks(TestFetchBase):
+
+    def make_stacked_source_repo(self):
+        _, source_b = self.make_source_branch()
+        # Use 'make_branch' which gives us a bzr:// branch when appropriate,
+        # rather than creating a branch-on-disk
+        stack_b = self.make_branch('stack-on')
+        stack_b.pull(source_b, stop_revision='B-id')
+        stacked_b = self.make_branch('stacked')
+        stacked_b.set_stacked_on_url('../stack-on')
+        stacked_b.pull(source_b, stop_revision='C-id')
+        return stacked_b.repository
+
+    def test_fetch_everything_includes_parent_invs(self):
+        stacked = self.make_stacked_source_repo()
+        repo_missing_fallbacks = stacked.bzrdir.open_repository()
+        self.addCleanup(repo_missing_fallbacks.lock_read().unlock)
+        target = self.make_repository('target')
+        self.addCleanup(target.lock_write().unlock)
+        target.fetch(
+            repo_missing_fallbacks,
+            fetch_spec=graph.EverythingResult(repo_missing_fallbacks))
+        self.assertEqual(repo_missing_fallbacks.revisions.keys(),
+            target.revisions.keys())
+        self.assertEqual(repo_missing_fallbacks.inventories.keys(),
+            target.inventories.keys())
+        self.assertEqual(['C-id'],
+            sorted(k[-1] for k in target.revisions.keys()))
+        self.assertEqual(['B-id', 'C-id'],
+            sorted(k[-1] for k in target.inventories.keys()))
+
+
+

=== modified file 'bzrlib/tests/test_remote.py'
--- bzrlib/tests/test_remote.py	2010-12-02 10:41:05 +0000
+++ bzrlib/tests/test_remote.py	2010-12-03 07:07:15 +0000
@@ -3182,11 +3182,36 @@
 
     def test_copy_content_into_avoids_revision_history(self):
         local = self.make_branch('local')
-        remote_backing_tree = self.make_branch_and_tree('remote')
-        remote_backing_tree.commit("Commit.")
+        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()
         local.repository.fetch(remote_branch.repository)
         self.hpss_calls = []
         remote_branch.copy_content_into(local)
         self.assertFalse('Branch.revision_history' in self.hpss_calls)
+
+    def test_fetch_everything_needs_just_one_call(self):
+        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))
+        self.assertEqual(['Repository.get_stream_1.19'], self.hpss_calls)
+
+    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))
+

=== modified file 'bzrlib/tests/test_repository.py'
--- bzrlib/tests/test_repository.py	2010-11-22 22:27:58 +0000
+++ bzrlib/tests/test_repository.py	2010-12-03 07:07:15 +0000
@@ -1659,7 +1659,7 @@
         self.orig_pack = target.pack
         target.pack = self.log_pack
         search = target.search_missing_revision_ids(
-            source_tree.branch.repository, tip)
+            source_tree.branch.repository, revision_ids=[tip])
         stream = source.get_stream(search)
         from_format = source_tree.branch.repository._format
         sink = target._get_sink()

=== modified file 'bzrlib/tests/test_smart.py'
--- bzrlib/tests/test_smart.py	2010-05-13 16:17:54 +0000
+++ bzrlib/tests/test_smart.py	2010-12-03 07:07:15 +0000
@@ -25,15 +25,11 @@
 """
 
 import bz2
-from cStringIO import StringIO
-import tarfile
 
 from bzrlib import (
-    bencode,
     branch as _mod_branch,
     bzrdir,
     errors,
-    pack,
     tests,
     transport,
     urlutils,
@@ -45,7 +41,6 @@
     repository as smart_repo,
     packrepository as smart_packrepo,
     request as smart_req,
-    server,
     vfs,
     )
 from bzrlib.tests import test_server
@@ -1474,7 +1469,7 @@
             request.execute('stacked', 1, (3, r3)))
 
 
-class TestSmartServerRepositoryGetStream(tests.TestCaseWithMemoryTransport):
+class GetStreamTestBase(tests.TestCaseWithMemoryTransport):
 
     def make_two_commit_repo(self):
         tree = self.make_branch_and_memory_tree('.')
@@ -1486,6 +1481,9 @@
         repo = tree.branch.repository
         return repo, r1, r2
 
+
+class TestSmartServerRepositoryGetStream(GetStreamTestBase):
+
     def test_ancestry_of(self):
         """The search argument may be a 'ancestry-of' some heads'."""
         backing = self.get_transport()
@@ -1512,6 +1510,18 @@
         stream_bytes = ''.join(response.body_stream)
         self.assertStartsWith(stream_bytes, 'Bazaar pack format 1')
 
+    def test_search_everything(self):
+        """A search of 'everything' returns a stream."""
+        backing = self.get_transport()
+        request = smart_repo.SmartServerRepositoryGetStream_1_19(backing)
+        repo, r1, r2 = self.make_two_commit_repo()
+        serialised_fetch_spec = 'everything'
+        request.execute('', repo._format.network_name())
+        response = request.do_body(serialised_fetch_spec)
+        self.assertEqual(('ok',), response.args)
+        stream_bytes = ''.join(response.body_stream)
+        self.assertStartsWith(stream_bytes, 'Bazaar pack format 1')
+
 
 class TestSmartServerRequestHasRevision(tests.TestCaseWithMemoryTransport):
 
@@ -1911,6 +1921,8 @@
             smart_repo.SmartServerRepositoryGetRevisionGraph)
         self.assertHandlerEqual('Repository.get_stream',
             smart_repo.SmartServerRepositoryGetStream)
+        self.assertHandlerEqual('Repository.get_stream_1.19',
+            smart_repo.SmartServerRepositoryGetStream_1_19)
         self.assertHandlerEqual('Repository.has_revision',
             smart_repo.SmartServerRequestHasRevision)
         self.assertHandlerEqual('Repository.insert_stream',

=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2010-12-02 16:24:54 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2010-12-03 07:07:15 +0000
@@ -128,6 +128,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
 *********