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

[John Arbash Meinel]

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

Ian Clatworthy wrote:
| 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.

good catch.

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

Absolutely.

|
|> +        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?

It is, though you didn't include the redundant version. This is the original,
the other one is the redundant one.

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

Can do. Callers should have been locking anyway, but we were probably using more
"convenient" apis. "get_graph()" doesn't lock the repository, and requires that
it is already locked. (versus get_revision_graph, etc)


|
|> +    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?

You're right. It should have been cloned from tree2.

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

$HOME is set to a custom directory as part of all TestCaseInTempDir.setUp()
functions, so that you can write whatever you want to your config files and have
it preserved. Just as the os.environ variables are also sanitized at the start
of each test (though they are automatically restored). for global config, it
gets restored because you start a new test, which gets a different $HOME.

So it is safe to do. Though if you prefer, I could be passing 'committer=XXX' to
every tree.commit().

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

Thanks for the review so far.

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

iEYEARECAAYFAkgY7/gACgkQJdeBCYSNAAM44QCggujlV+YELE20h/DbpeyeF96x
CmYAnA0dbQbhiWdVMjf6wMNLPAKbkZuv
=84tP
-----END PGP SIGNATURE-----