[MERGE] handle null revision properly for LCA

[John Arbash Meinel]

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

Robert Collins wrote:
> On Tue, 2007-06-19 at 10:57 -0400, Aaron Bentley wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Hi all,
>>
>> The new Graph methods require that the null revision be expressed as
>> 'null:' not None.  Using None has turned out to be a bad idea in our
>> APIs, so I propose that all new APIs accept only 'null:'.
> 
> Can we examine this a bit more? How/where has None been a problem. I do
> remember that we had a confusion with ghosts in the past - 
> A:[] (no parents) 
> was confused with
> B:[] (all parents are ghosts)
> 
> these days you get
> A:[] (no parents)
> B:[ghost, ghost, ghost]
> 
> if you use the ghost aware API
> 
> I guess fundamentally I don't get the differences between None, an empty
> list, and NULL_REVISION within bzrlib.
> 
> It just seems more work to say "every revision will always have a non
> empty list of parents except the pseudo revision 'NULL_REVISION'" rather
> than "Every revision has a list of parents which may be empty".
> 
> Specifically having NULL_REVISION as a parent returned in queries means
> that some revision ids returned cannot be accessed - theres no 'commit
> date' for NULL_REVISION (and there shouldn't be). OTOH we may need a
> concept for the current trees revision at some point, which will have
> the same defect.
> 
> -Rob
> 

bzrlib.repository.get_revision_graph(None)

Are you asking for the whole graph, or the graph of the null revision?

We have "solved" it in the past by using a

_marker = object()

def get_revision_graph(revision_id=_marker):

I believe we have this explicit problem with:

bzr branch branch_with_no_revs_in_shared_repo target_not_in_repo

Which copies the entire repository into 'target_not_in_repo', rather than
recognizing that you don't want to copy anything (Vincent found this, there is
a bug).

It isn't the only place. But in general passing "None" is done because you
weren't given anything (so use the last/first/all/none depending on context),
rather than sometimes you have a branch with no revisions so you should be
passing 'null:'. Or you are recursing up through history and you run into
'null:' for some reason. (Arguably [] is better for the parents list).

A hypothetical reason is apis like "Branch.last_revision()" are you returning
None because you don't know, or because you actually meant 'null:' (arguably it
isn't confusing for last_revision() but when you pass that off to
get_revision_graph() what used to be unambiguous suddenly is ambiguous)

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGeEejJdeBCYSNAAMRAh9YAJ9mWTKmnQdSxyDHtwMcMpIiM1Tc4QCeNOI0
iw45IbCKLS3k6e67fSD/n6o=
=XDx0
-----END PGP SIGNATURE-----