Code review comment for lp:~garyvdm/bzr/bug588698-criss-cross-null-base

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

On 11/17/2010 7:44 AM, Gary van der Merwe wrote:
> Gary van der Merwe has proposed merging lp:~garyvdm/bzr/bug588698-criss-cross-null-base into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #588698 "bzr merge" fails "Branches have no common ancestor"
> https://bugs.launchpad.net/bugs/588698
>
>
> This makes merge choose one of the lcas as the base revision when it cannot locate a unique lca, for example where there is a criss-cross merge of a new root.
>
> It chooses the lca by taking the first lca from find_merge_order(lcas). This seems to give the most sensible for all the user tests I have run.
>
> This has a drive-by fix to make BranchBuilder.build_snapshot accept parent_ids == ['null:']. This was done to make it easier to write a test using BranchBuilder that introduces a new root into the graph.

- - if parent_ids is not None:
+ if parent_ids == [revision.NULL_REVISION]:

^- Are we sure parent_ids is always a list? Could it be a tuple:
(revision.NULL_REVISION,).

I think it usually is. However, you shouldn't need to pass
[NULL_REVISION] either. Just pass the empty list (I think).

+ builder.build_snapshot('B-id', [],
+ [('add', ('', None, 'directory', None))])

I'm not positive if that works, but it does actually match certain bits
of code better. ('null:' isn't saved to the index files, for example).

I'd have to dig a bit deeper to sort out the rest. However, this snippet
looks worrying:

- - self._is_criss_cross = True
+ sorted_lca_keys = self.revision_graph.find_merge_order(

+ revisions[0], lcas)
+ if self.base_rev_id == _mod_revision.NULL_REVISION:
+ self.base_rev_id = sorted_lca_keys[0]
+

graph.find_merge_order is not a cheap operation, because it has to walk
the ancestry. Could we do it only in the 'if' block:

if self.base_rev_id == _mod_revision.NULL_REVISION:
    trace.mutter("hit the NULL_REVISION as the unique LCA."
                 " Picking the 'first' lca as BASE")
    sorted_lca_keys = self.revision_graph.find_merge_order(revisions[0],
lcas)
    self.base_rev_id = sorted_lca_keys[0]

...

                 interesting_revision_ids = [self.base_rev_id]
                 interesting_revision_ids.extend(lcas)
+ interesting_revision_ids =
list(set(interesting_revision_ids))

^- This doesn't preserve the ordering of interesting_revision_ids. But
since it looks like we don't need the specific order, why not just use a
set and not cast to a list in the first place?

I see that 'sorted_lca_keys' is used again right after this point. but
is it used in all possible cases?

Anyway, the basic concept seems fine. Failing down to NULL_REVISION is
bad, picking a 'best guess' LCA seems reasonable. We may want to inform
the user that we did this, in case it would be confusing why X or Y
didn't happen. At least a mutter() helps us debug it if it is happening.

John
=:->

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

iEYEARECAAYFAkzkQ4IACgkQJdeBCYSNAAMgYgCguFfjAzaaqfSrF6Gk6PXUXLhk
2SUAmwX6f8VIy3ysiGDNTpbWf6XCvq3X
=Dw1c
-----END PGP SIGNATURE-----