[MERGE] Fix calculation of search recipe in _get_parent_map_rpc when null: has been seen and is cached as missing

[Andrew Bennetts]

This fixes a bug in an edge case of _get_parent_map_rpc.  You can reproduce
it with current bzr.dev by branching bzr.dev from a 1.12 (or earlier) server
with:

  bzr branch -r revid:pqm at pqm.ubuntu.com-20090328053724-souztdxo868gjo3q URL

It will fail with a NoSuchRevision error from the server (after just 4
get_parent_map calls), which is the server's way of signalling the search
recipe in a get_parent_map call is broken (the expected key count does not
match the actual number of included keys matched by that server).  This bug
is only in the bzr.dev client, it's not a release.

This was originally noticed on the weekend when that revision was the tip,
because Launchpad is still running 1.12 (but only for another day I think?).

The problem is that NULL_REVISION is treated just a little bit specially
compared to other revisions, and that interacts badly with the new
cache-missing functionality in _get_parent_map_rpc.

The get_parent_map RPC client uses the cache of the RemoteRepo's
CachingParentsProvider to generate a SearchResult recipe to send to the
server, so the server knows what parts of the graph the client has already
seen (so that the server can avoid resending that data).  But if
NULL_REVISION has already been referenced by some key, *and* the client has
already tried to get_parent_map of NULL_REVISION and found it to be
"missing", then _get_parent_map_rpc will generate a SearchResult recipe
without NULL_REVISION in the stop_keys but without adjusting the expected
key count for that recipe.  So the server will expect NULL_REVISION to be in
the calculated included keys for that search, and thus find that it has 1
more key than the client said it should find, so the server gives an error.

This patch adds a test for this case, which I think happens in practice when
a non-lefthand revision with a parent of NULL_REVISION has been merged
somewhat recently in the history.

And, of course, this patch also adds a fix, which is to adjust the count
when pruning NULL_REVISION from stop_keys because it is missing.  Another
fix might be to never query for NULL_REVISION over the wire (and do what
e.g. the base Repository.get_parent_map does and special case it rather than
pass that key down to the underlying get_parent_map implementation).  It's a
special case either way.  It's a bit of a shame that NULL_REVISION being
special-cased in one place has this sort of knock-on effect.

It's a bit weird to me that NULL_REVISION can considered part of the
calculated included keys of SearchResult while also being missing.  Present
revisions obviously can be included but aren't missing, and ghosts will not
be included and they are missing.  So NULL_REVISION is sort of half-way
between being ghost and being present, which seems bug-prone to me.  Anyway,
we can't change how old servers react to NULL_REVISION in a graph, so we
have to change the client to match.

-Andrew.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: get_parent_map_rpc_ghosts-4219.patch
Type: text/x-diff
Size: 10351 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20090401/593c6038/attachment.bin