[MERGE][1.6] Real --weave merge

[Ian Clatworthy]

John Arbash Meinel wrote:

> I would really like to get this into 1.6, as ATM mysql is using a custom
> build to get this sort of result, and I'd really like to get them back
> onto official releases.

Here is a start to the review, focussing on code correctness (rather than
algorithms).

bb: comment

> +    * ``bzr (re)merge --weave`` will now use a real Weave algorithm,
> +      rather than the annotation-based merged it was using. It does so by

s/merged it/merge it/

> +            if child_parents is None:
> +                import pdb; pdb.set_trace()
> +            if len(child_parents) != 1:
> +                # This is not its only parent
> +                continue
> +            assert child_parents[0] == node

1st if statement can go. Assert needs to be replaced by an
if ... AssertionError().

> -                include_unchanged=True, specific_files=self.interesting_files,
> +                include_unchanged=False, specific_files=self.interesting_files,

I didn't get as far as working out why this change was made. Can you please
confirm it was meant to be permanent and briefly explain it?

> +        if NULL_REVISION in all_revision_keys:
> +            import pdb; pdb.set_trace()

More droppings to clean up.

> +        ordering = []
> +        for seq_num, key, depth, eom in reversed(tsort.merge_sort(parent_map,
> +                                                                  tip_key)):
> +            if key == tip_key:
> +                continue
> +        # for key in tsort.topo_sort(parent_map):
> +            parent_keys = parent_map[key]
> +            revision_id = key[-1]
> +            ordering.append(revision_id)
> +            parent_ids = [k[-1] for k in parent_keys]
> +            self._weave.add_lines(revision_id, parent_ids,
> +                                  all_texts[revision_id])
> +        mutter('order in weave: %s', ordering)

So "ordering" is never used except for debugging/trace?
I wonder whether it's worth adding yet another -D flag for the
numerous mutters like this one throughout this new code? Up to you.

> +            lines = self.get_lines([self._head_key[-1]])[self._head_key[-1]]

I think it's worth using a local variable here rather than looking up the
last element twice.

> +    def test_collapse_with_multiple_children(self):
> +        #    7
> +        #    |
> +        #    6
> +        #   / \
> +        #  4   5
> +        #  |   |
> +        #  3   2
> +        #   \ /
> +        #    1
> +        #
> +        # 4 and 5 cannot be removed because 6 has 2 children
> +        # 3 and 2 cannot be removed because 1 has 2 parents
> +        d = {1:[2, 3], 2:[4], 4:[6], 3:[5], 5:[6], 6:[7], 7:[]}

You need to swap 2 and 3 in the text graphic in order for the graphic
and definition of 'd' to match.

> -        yield 'unchanged', ''           # terminator
> +        # This doesn't seem to be used anymore
> +        # yield 'unchanged', ''           # terminator

Hmm. If you haven't already, please ask Aaron about that. It's
certainly still documented and used inside versionedfiles.py.

Sorry I didn't get further on this review. I still need to look at
test_merge.py in particular before I can vote on it.

Ian C.