rfc: Change get_parent_map signature so that no parents is ()

[John Arbash Meinel]

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

Aaron Bentley wrote:
| Hi all,
|
| Our current implementations of get_parent_map are inconsistent in how
| they handle versions with no parents.
|
| They are supposed to use NULL_REVISION, but many use () instead, and
| NULL_REVISION doesn't make much sense now that get_parent_map has been
| redefined to operate in terms of tuples.
|
| I have put a lot of work into making get_parent_map use NULL_KEY
| instead.  NULL_KEY was defined as (NULL_REVISION,).  But I am finding
| that this approach adds lots and lots of API friction, and causes lots
| of confusion about where NULL_KEY should appear.  You can see my current
| work here:
| http://code.aaronbentley.com/bzr/bzrrepo/fix-plan-merge
|
| I had something like 6000 test cases passing.  Then I fixed a test case,
| and suddenly I was back to 200 test cases passing.  It's like swimming
| in molasses.
|
| I think the simplest fix is to just declare that get_parent_map always
| selects () when a version has no parents.  I realize that this is an API
| break.  But our API is already broken.
|
| Aaron

I agree that the API right now is already broken, with different
implementations giving different results. Some give plain NULL_REVISION,
some give (NULL_REVISION,) etc.

And there is quite a bit of friction in that we don't *record*
NULL_REVISION at the disk layer. So you have to artificially introduce
it up the stream.

And the questions about whether get_parent_map([('file_id', 'tuple')])
returns ('file_id', NULL_REVISION) or (NULL_REVISION,) or NULL_REVISION
or ().

*I* personally prefer to return (). And then we need a consistent way of
presenting ghosts. I think omitting them from the returned map has
actually worked pretty well. As it lets you check for ghosts with a
parent_map request and a set difference.

If we prefer, we could have it return 2 values
~  present_map, ghosts = get_XXXX()

That actually simplifies the _Stacked implementation more, as you can do:

present = {}
remaining = set(requested)

for provider in self._providers:
~  if not remaining:
~    break
~  next_present, remaining = provider.get_XXXX(remaining)
~  present.update(next_present)
return present, remaining

It also simplifies all the code that is doing a get_parent_map and then
doing a set difference to find the ghosts, as we just have them returned.

I'm tempted to call it something else. As Martin mentioned in a review
"get_parent_key_map()" is clearer that it is working in keys rather than
strings. Though the downside is that the Graph code doesn't actually
care, as long as the keys are hashable.

So +1 on returning (), I'm just bringing up that we may want a new api,
and we may want to return ghosts along with the parent map.

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

iEYEARECAAYFAkiF9FsACgkQJdeBCYSNAAObKwCgrUhi0O0KofG2Arv5pQWq2CK2
7y0An204w3688JC/WRzdDSClBB92nY15
=DMEC
-----END PGP SIGNATURE-----