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

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, I think this is a better design: instead of having many parameters
on sprout/fetch/etc that interact in complex ways to decide what to fetch, I've
shifted that logic out of fetch into relatively simple objects.

I'm going to put this back in work-in-progress briefly while I update this to
add an if_present_ids optional param to NotInOtherForRevs (and
search_missing_revision_ids), so that I can use this nicely even when there may
be tags referring to absent revisions. I have the code for this already in a
later branch, but it really belongs in this branch.

(Hmm, it appears there's no support for setting the status to WIP from email...)