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

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andrew Bennetts wrote:
> My recent change to _walk_to_common_revisions (bzr.dev r3795) turns out to have
> a bug, due to the way _BreadthFirstSearcher.stop_searching_any behaves.  We
> either need to fix this or revert r3795 for 1.9, as this would be a pretty
> serious regression.
> 
> The change iterates the searcher several times so that the client can batch
> get_parent_map requests.  But for this to work correctly it needs to instruct
> the searcher to stop searching for hits from the previous iterations.  I assumed
> that stop_searching_any already handled this nicely, but it turns out it
> doesn't do what I assumed.
> 
> In particular, it will leave the revisions in the final search result, as can
> been seen with searcher.get_result().get_keys().  This causes fetch to transmit
> revisions that have already been seen, which is obviously quite wasteful.
> 

...

> The graph code is pretty complex, so I would definitely appreciate more eyeballs
> and brains applied to this.  I'm pretty sure it'll give the right result for
> get_keys() on the result, but it doesn't give what you might expect for
> get_recipe().  Maybe that's ok?
> 
> I've added tests for this behaviour that I hope are adequately clear.  The
> second one fails, due to the returned recipe not being what you'd intuitively
> expect (which is what the test currently expects).  Perhaps this is acceptable
> though.  I would appreciate your thoughts.
> 
> -Andrew.


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


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.

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

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.

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.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkkJ0LcACgkQJdeBCYSNAAN1bgCgmPtkNupR1mY/B1dSYeFCNKsc
JdkAnAvGG2ikeFmgVnSPkbtrq+yvH2XJ
=FanK
-----END PGP SIGNATURE-----
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: graph-stop-3812.patch
Url: https://lists.ubuntu.com/archives/bazaar/attachments/20081030/b2145acc/attachment-0001.diff