[MERGE] handle null revision properly for LCA

[Aaron Bentley]

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

Robert Collins wrote:

> I guess fundamentally I don't get the differences between None, an empty
> list, and NULL_REVISION within bzrlib.

In argument lists, None usually means "I choose not to supply this
argument".  For example, WorkingTree.merge_from_branch()

def merge_from_branch(self, branch, to_revision=None)

Similarly, a return value of None typically means "I could not find what
you were looking for."  For example, dict.get.

The null revision is root node of all revision graphs for all imaginable
projects.  Its tree is the empty tree.  We have historically referred to
it using None, but this means that we cannot use None in other contexts.

For example, the null revision would be a valid (if pointless) argument
to merge_from_branch.

Consider this situation

remote = Branch.open('http://example.com')
related = Branch.open('/example1')
tree = WorkingTree.open('tree1')

tree.merge_from_branch(related, remote.last_revision())

If remote's last revision is not the null revision, then this will merge
remote's last-committed revision, using related's repository.

But if remote's last revision is the null revision,
remote.last_revision() will return None.  merge_from_branch will think
no argument was supplied for to_revision, and so it will use
related.last_revision instead.

This will be the wrong behavior.

Similarly, if we store revision ids as dictionary values, we cannot use
dict.get if the null revision is represented as None.

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

I'm not talking about lists here.  You could us NULL_REVISON and leave
it out of lists, not that I think that's a good idea.  I'm saying that
when we name the null revision, we should not use None to do it.

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

That is already the case when None is used to name the null revision.
NULL_REVISION is simply a distinctive name.

We must refer to the null revision in many contexts, so you can't avoid
having to deal with the fact that some revision ids cannot be accessed.
 It's simply a choice between the name None, which interferes with
standard python idioms, and the name NULL_REVISION, which does not.

> OTOH we may need a
> concept for the current trees revision at some point, which will have
> the same defect.

You mean revision.CURRENT_REVISION? That already exists, and gannotate,
at least, already has to deal with that issue.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGeEyp0F+nu1YWqI0RAndZAKCFctgN1jxslho2NFqbtBa6K6cnWACeJQXv
eTGVv4zqKbv/o5jTtHCphOw=
=CnGP
-----END PGP SIGNATURE-----