[MERGE] new next_with_ghosts call on graph searchers

[Andrew Bennetts]

Robert Collins wrote:
[...]
> > I don't understand the way the “_returning” variable is used.  The values can be
> > 'checked' or 'query', but it's not obvious to me how those words relate to the
> > “next” vs.  “next_with_ghosts” methods.  It doesn't help my confusion that
> > “checked” is an adjective, but “query” is a noun.
> 
> Its a poor man's state object; I didn't feel like doing a whole state
> factoring for this. Perhaps if I used 'next' and 'next_with_ghosts' ?

That's fine.

> > [...]
> > > +            for rev, parents in self._current_parents.iteritems():
> > > +                for parent_id in parents:
> > > +                    try:
> > > +                        stop_rev_references[parent_id] -= 1
> > > +                    except KeyError:
> > > +                        pass
> > 
> > Why not use itervalues here?
> 
> Changed. Is it cheaper?

It would be, if only because there's no need to do tuple-unpacking into “rev,
parents”.  I'd expect it to be a pretty minimal difference, but it's just tidier
:)

> > It's a bit marginal, but I wouldn't mind a “make_graph” helper method on this
> > TestCase to replace these three lines of duplication with a simple:
> > 
> >     graph = self.make_graph({})
> > 
> > Perhaps call it “make_instrumented_graph_from_dict” if “make_graph” is too vague
> > for your tastes.
> > 
> > If you did this, there'd only be a single statement before we got to the meat of
> > the test method.
> 
> Hmm, I think I'll pass on this for now.

Ok.

[...]
> > Why not two separate test methods here, one for next_with_ghosts and one for
> > next?  This method is long and dense enough that it's hard to read, and can
> > easily be broken in two to test the two methods independently.
> > 
> > (I'm a bit tempted to say most of the other tests you add should also be split
> > in two, but I'm not as concerned by those as they aren't as long.)
> 
> I want to keep the tests for the same pattern for the two states in the
> same methods, to make it clear that they are expected to behave
> 'identically'.

I understand that desire.  I don't think this is a particularly satisfying way
to achieve that.  Long, repetitious tests are easy to miss bugs in.  And of
course your stated goal here is only implicit in the code, there's no explicit
note or imposed structure to make sure the tests for the two APIs are 
synchronised, just a convention that a future maintainer may or may not
understand.

I don't have any brilliant ideas to make it better, though.  I suppose you could
have two test cases, one for each method, and then a separate test that asserts
that both test cases have the same set of test_* methods...

-Andrew.