Code review comment for lp:~spiv/bzr/fetch-spec-everything-not-in-other

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