[MERGE][bug #172657] Graph.find_differences() and status after merge

[Ian Clatworthy]

John Arbash Meinel wrote:
> Attached is an updated form of my previous submission, which I now
> consider at least ready for review.

Here's a partial review of the test changes.

bb:comment

> -# but the common searcher won't have time to find that one branch is actually
> -# in common. The extra nodes at the top are because we want to avoid
> +# but the common searcher won't have time to find that one branche is actually
> +# in common. The extra nodes at the beginning are because we want to avoid
,
branch ->branche is a typo.

> -        self.expectFailure('find_difference cannot handle shortcuts',
> -            self.assertEqual, (set(['m']), set(['h', 'n'])),
> -                graph.find_difference('m', 'n'))
> -        self.assertEqual((set(['m']), set(['h', 'n'])),
> +        # self.expectFailure('find_difference cannot handle shortcuts',
> +        #     self.assertEqual, (set(['m']), set(['h', 'n'])),
> +        #         graph.find_difference('m', 'n'))
> +        self.assertEqual((set(['m', 'i', 'e']), set(['n', 'h'])),

Rather than commenting out the expectFailure calls, I think the code
ought to be removed in the 3 or 4 cases this is done. We have a VCS
after all. :-) :-)

> +        def get_parent_map(keys):
> +            result = {}
> +            for key in keys:
> +                if key == 'deeper':
> +                    self.fail('key deeper was accessed')
> +                result[key] = graph_dict[key]
> +            return result

This is a redundant duplicate of the existing get_parent_map method, yes?

> -        show_pending_merges(tree2, output)
> +        tree2.lock_read()
> +        try:
> +            show_pending_merges(tree2, output)
> +        finally:
> +            tree2.unlock()

If the new implementation of show_pending_merges requires the tree to be
locked but the old one didn't, then I think you ought to document that
in NEWS as an API break, given show_pending_merges is a public function.

> +    def test_multiple_pending(self):
> +        config.GlobalConfig().set_user_option('email', 'Joe Foo <joe at foo.com>')
> +        tree = self.make_branch_and_tree('a')
> +        tree.commit('commit 1', timestamp=1196796819, timezone=0)
> +        tree2 = tree.bzrdir.clone('b').open_workingtree()
> +        tree.commit('commit 2', timestamp=1196796819, timezone=0)
> +        tree2.commit('commit 2b', timestamp=1196796819, timezone=0)
> +        tree3 = tree.bzrdir.clone('c').open_workingtree()
> +        tree2.commit('commit 3b', timestamp=1196796819, timezone=0)
> +        tree3.commit('commit 3c', timestamp=1196796819, timezone=0)
> +        tree.merge_from_branch(tree2.branch)
> +        tree.merge_from_branch(tree3.branch)
> +        output = StringIO()
> +        tree.lock_read()
> +        try:
> +            show_pending_merges(tree, output)
> +        finally:
> +            tree.unlock()
> +        # Even though 2b is merged by 3c also, it should only be displayed
> +        # the first time it shows u.

It's late (which is why this is only a partial review) but this is
either wrong or I'm missing something here. Did you mean to clone tree3
from tree2? If not, how is 2b merged into 3c?

Also, is it safe to simply step on the existing email config setting
like that? I half expected to see the old value being saved away and
restored on completion.

Sorry for the limited progress on this review today. I hope to complete
it tomorrow, at least to a 'code looks sane' level.

Ian C.