[MERGE/RFC][1.9] Better behaviour when calling stop_searching_any with a revision from an earlier iteration

[Andrew Bennetts]

John Arbash Meinel wrote:
[...]
> 
> For starters I think we need to update the tests so that instead of using:
> +        expected = [
> +            (set(['head']), (set(['head']), set(['middle']), 1),
> +             ['head'], None, None),
> 
> we use:
> 
> instructions = []
> 
> At least, it would make it easier for *me* to realize what the values
> really are. (I realize you probably just copied it from elsewhere.)

Right.  I don't find the current format particularly readable, but
consistency with the existing tests comes first.

I think it might be clearer to make a small API instead, so that it
would look like:

    search_driver.iterate_expecting_results(
	seen=set(...), recipe=..., keys=...)
    search_driver.start_searching(...)
    search_driver.stop_searching_any(...)

It's not as compact, but the keywords args make things clearer.  And
it'd allow simply omitting things you didn't want to test (e.g. just
don't call stop_searching_any rather than passing None as the 5th tuple
element).  I haven't given it a bunch of thought, and it's possible the
increased verbosity would be a net loss.

Anyway, that's just cosmetics that we can worry about another time...

> Anyway, I feel like your "stop_searching_late" test is actually
> incorrect, and the "stop_searching_late_more_keys" is more relevant.
> 
> Specifically, I think it is the responsibility of
> _walk_to_common_ancestors to call "stop = find_seen_ancestors(stop)" and
> make a call to ensure that all ancestors of the stop set are properly
> stopped.

Ah, nice.  I somehow didn't notice find_seen_ancestors, but that sounds
like a good idea.

> Also, while tracking down the effect of a recipe, I found we were double
> serializing, so I cleaned up that code.

Thanks!

> The important thing about recipes is that they should be able to
> reproduce the search result with no extra information. Which means that
> having extra keys in "stop_searching" doesn't hurt anything, and only
> costs us a few more bytes in the request we are sending.

Right.  Well, the other important thing about recipes (from HPSS's point
of view, anyway) is that they should be shorter than just listing all
the desired revisions explicitly, but I think this will still be true.

> So even though when applying the recipe remotely we will stop searching
> at 'middle' and never get to 'child', the local graph wouldn't know that
> unless we did more work. I think you could do:
> 
>   have_revs = XXX
>   all_stop_revs = searcher.find_seen_ancestors(have_revs)
>   searcher.stop_searching_any(all_stop_revs)
>   stopped_ancestors = all_stop_revs.difference(have_revs)
>   searcher.pretend_you_didnt_search(stopped_ancestors)
> 
> For now, I don't really care about the 'pretend_you_didnt_search" part.
> 
> I went ahead and updated the patch as I think it should be.

Thanks a bunch!  I think your patch looks fine.

bb:approve

I've chatted a bit with Robert about it (in broad terms, rather than
about the actual details of the patch) and he agrees this sounds sane.
So I'm going to land it now.

Thanks again for the review with complete patch, it's very helpful :)

-Andrew.