Merge lp:~abentley/bzr/fix-switch-branch into lp:bzr

Proposed by Aaron Bentley
Status: Merged
Approved by: John A Meinel
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~abentley/bzr/fix-switch-branch
Merge into: lp:bzr
Diff against target: 93 lines (+33/-4)
4 files modified
NEWS (+3/-0)
bzrlib/merge.py (+4/-1)
bzrlib/switch.py (+1/-2)
bzrlib/tests/test_switch.py (+25/-1)
To merge this branch: bzr merge lp:~abentley/bzr/fix-switch-branch
Reviewer Review Type Date Requested Status
John A Meinel Approve
Review via email: mp+24050@code.launchpad.net

Description of the change

Hi all,

This branch fixes a bug it took me a while to track down, because I initially
assume it was related to pipelines.

Basically, the issue is that Merge3Merger does not guarantee that
self.this_branch has a value, but ConfigurableFileMerger assumes it does.

Merger does provide this guarantee, but switch doesn't use Merger.

This manifested in switch as:
  File "/home/abentley/bzr/fix-switch-branch/bzrlib/merge.py", line 1313, in _do_merge_contents
    hook_status, lines = hook.merge_contents(params)
  File "/home/abentley/bzr/fix-switch-branch/bzrlib/merge.py", line 161, in merge_contents
    not self.filename_matches_config(params)):
  File "/home/abentley/bzr/fix-switch-branch/bzrlib/merge.py", line 133, in filename_matches_config
    config = self.merger.this_branch.get_config()
AttributeError: 'NoneType' object has no attribute 'get_config'

But of course, it is only triggered if a ConfigurableFileMerger is used, e.g.
when editing NEWS.

This branch fixes it at Merge3Merger, by ensuring that this_branch is set to
this_tree.branch if not supplied. It could also have been fixed at switch, but
I was concerned that this could lead to repeats of this problem.

To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :

There is an open bug on this, in fact there are like 3. It should be mentioned in NEWS. Otherwise:

review: Approve
Revision history for this message
Andrew Bennetts (spiv) wrote :

John A Meinel wrote:
> Review: Approve
> There is an open bug on this, in fact there are like 3. It should be mentioned in NEWS. Otherwise:

The bug is 559436, and it even has a fix I wrote that is still waiting for
review:

<https://code.edge.launchpad.net/~spiv/bzr/switch-crash-559436/+merge/23817>

Happily it's essentially the same as Aaron's fix, but I wonder why his got
reviewed already when mine from 3 days earlier is still waiting? Is it because
I targetted 2.1?

-Andrew.

Revision history for this message
John A Meinel (jameinel) wrote :

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

Andrew Bennetts wrote:
> John A Meinel wrote:
>> Review: Approve
>> There is an open bug on this, in fact there are like 3. It should be mentioned in NEWS. Otherwise:
>
> The bug is 559436, and it even has a fix I wrote that is still waiting for
> review:
>
> <https://code.edge.launchpad.net/~spiv/bzr/switch-crash-559436/+merge/23817>
>
> Happily it's essentially the same as Aaron's fix, but I wonder why his got
> reviewed already when mine from 3 days earlier is still waiting? Is it because
> I targetted 2.1?
>
> -Andrew.
>

For my part, it is because his e-mail showed up while I was willing to
review. Plus it has a test...

John
=:->

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

iEYEARECAAYFAkvWTdYACgkQJdeBCYSNAAMz+ACfQDdyJMctqtqSbbWmbqLsc47A
bRQAoLEbH1DrgPQWMd39+7XNfqMpcSw0
=exb9
-----END PGP SIGNATURE-----

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2010-04-24 09:08:27 +0000
3+++ NEWS 2010-04-26 23:42:26 +0000
4@@ -110,6 +110,9 @@
5 http://bugs.python.org/issue8396 .
6 (Parth Malwankar, #413406)
7
8+* ``bzr switch`` does not die if a ConfigurableFileMerger is used.
9+ (Aaron Bentley, #559436)
10+
11 * ``bzr update`` when a pending merge in the working tree has been merged
12 into the master branch will no longer claim that old commits have become
13 pending merges. (Robert Collins, #562079)
14
15=== modified file 'bzrlib/merge.py'
16--- bzrlib/merge.py 2010-04-16 07:56:51 +0000
17+++ bzrlib/merge.py 2010-04-26 23:42:26 +0000
18@@ -704,7 +704,8 @@
19 :param this_tree: The local tree in the merge operation
20 :param base_tree: The common tree in the merge operation
21 :param other_tree: The other tree to merge changes from
22- :param this_branch: The branch associated with this_tree
23+ :param this_branch: The branch associated with this_tree. Defaults to
24+ this_tree.branch if not supplied.
25 :param interesting_ids: The file_ids of files that should be
26 participate in the merge. May not be combined with
27 interesting_files.
28@@ -728,6 +729,8 @@
29 if interesting_files is not None and interesting_ids is not None:
30 raise ValueError(
31 'specify either interesting_ids or interesting_files')
32+ if this_branch is None:
33+ this_branch = this_tree.branch
34 self.interesting_ids = interesting_ids
35 self.interesting_files = interesting_files
36 self.this_tree = working_tree
37
38=== modified file 'bzrlib/switch.py'
39--- bzrlib/switch.py 2010-03-25 14:22:41 +0000
40+++ bzrlib/switch.py 2010-04-26 23:42:26 +0000
41@@ -17,8 +17,7 @@
42 # Original author: David Allouche
43
44 from bzrlib import errors, merge, revision
45-from bzrlib.branch import Branch, BranchFormat, BranchReferenceFormat
46-from bzrlib.bzrdir import BzrDir
47+from bzrlib.branch import Branch
48 from bzrlib.trace import note
49
50
51
52=== modified file 'bzrlib/tests/test_switch.py'
53--- bzrlib/tests/test_switch.py 2010-01-15 04:05:36 +0000
54+++ bzrlib/tests/test_switch.py 2010-04-26 23:42:26 +0000
55@@ -19,7 +19,13 @@
56
57 import os
58
59-from bzrlib import branch, errors, switch, tests
60+from bzrlib import (
61+ branch,
62+ errors,
63+ merge as _mod_merge,
64+ switch,
65+ tests,
66+)
67
68
69 class TestSwitch(tests.TestCaseWithTransport):
70@@ -129,6 +135,24 @@
71 switch.switch(checkout.bzrdir, tree2.branch)
72 self.assertEqual('custom-root-id', tree2.get_root_id())
73
74+ def test_switch_configurable_file_merger(self):
75+ class DummyMerger(_mod_merge.ConfigurableFileMerger):
76+ name_prefix = 'file'
77+
78+ _mod_merge.Merger.hooks.install_named_hook(
79+ 'merge_file_content', DummyMerger,
80+ 'test factory')
81+ foo = self.make_branch('foo')
82+ checkout = foo.create_checkout('checkout', lightweight=True)
83+ self.build_tree_contents([('checkout/file', 'a')])
84+ checkout.add('file')
85+ checkout.commit('a')
86+ bar = foo.bzrdir.sprout('bar').open_workingtree()
87+ self.build_tree_contents([('bar/file', 'b')])
88+ bar.commit('b')
89+ self.build_tree_contents([('checkout/file', 'c')])
90+ switch.switch(checkout.bzrdir, bar.branch)
91+
92
93 class TestSwitchHeavyweight(TestSwitch):
94