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

[Aaron Bentley]

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.

[John A Meinel (jameinel)]

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

review: Approve

[Andrew Bennetts (spiv)]

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.

[John A Meinel (jameinel)]

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

=== modified file 'NEWS'
--- NEWS	2010-04-24 09:08:27 +0000
+++ NEWS	2010-04-26 23:42:26 +0000
@@ -110,6 +110,9 @@
   http://bugs.python.org/issue8396 .
   (Parth Malwankar, #413406)
 
+* ``bzr switch`` does not die if a ConfigurableFileMerger is used.
+  (Aaron Bentley, #559436)
+
 * ``bzr update`` when a pending merge in the working tree has been merged
   into the master branch will no longer claim that old commits have become
   pending merges. (Robert Collins, #562079)
 
=== modified file 'bzrlib/merge.py'
--- bzrlib/merge.py	2010-04-16 07:56:51 +0000
+++ bzrlib/merge.py	2010-04-26 23:42:26 +0000
@@ -704,7 +704,8 @@
         :param this_tree: The local tree in the merge operation
         :param base_tree: The common tree in the merge operation
         :param other_tree: The other tree to merge changes from
-        :param this_branch: The branch associated with this_tree
+        :param this_branch: The branch associated with this_tree.  Defaults to
+            this_tree.branch if not supplied.
         :param interesting_ids: The file_ids of files that should be
             participate in the merge.  May not be combined with
             interesting_files.
@@ -728,6 +729,8 @@
         if interesting_files is not None and interesting_ids is not None:
             raise ValueError(
                 'specify either interesting_ids or interesting_files')
+        if this_branch is None:
+            this_branch = this_tree.branch
         self.interesting_ids = interesting_ids
         self.interesting_files = interesting_files
         self.this_tree = working_tree
 
=== modified file 'bzrlib/switch.py'
--- bzrlib/switch.py	2010-03-25 14:22:41 +0000
+++ bzrlib/switch.py	2010-04-26 23:42:26 +0000
@@ -17,8 +17,7 @@
 # Original author: David Allouche
 
 from bzrlib import errors, merge, revision
-from bzrlib.branch import Branch, BranchFormat, BranchReferenceFormat
-from bzrlib.bzrdir import BzrDir
+from bzrlib.branch import Branch
 from bzrlib.trace import note
 
 
 
=== modified file 'bzrlib/tests/test_switch.py'
--- bzrlib/tests/test_switch.py	2010-01-15 04:05:36 +0000
+++ bzrlib/tests/test_switch.py	2010-04-26 23:42:26 +0000
@@ -19,7 +19,13 @@
 
 import os
 
-from bzrlib import branch, errors, switch, tests
+from bzrlib import (
+    branch,
+    errors,
+    merge as _mod_merge,
+    switch,
+    tests,
+)
 
 
 class TestSwitch(tests.TestCaseWithTransport):
@@ -129,6 +135,24 @@
         switch.switch(checkout.bzrdir, tree2.branch)
         self.assertEqual('custom-root-id', tree2.get_root_id())
 
+    def test_switch_configurable_file_merger(self):
+        class DummyMerger(_mod_merge.ConfigurableFileMerger):
+            name_prefix = 'file'
+
+        _mod_merge.Merger.hooks.install_named_hook(
+            'merge_file_content', DummyMerger,
+            'test factory')
+        foo = self.make_branch('foo')
+        checkout = foo.create_checkout('checkout', lightweight=True)
+        self.build_tree_contents([('checkout/file', 'a')])
+        checkout.add('file')
+        checkout.commit('a')
+        bar = foo.bzrdir.sprout('bar').open_workingtree()
+        self.build_tree_contents([('bar/file', 'b')])
+        bar.commit('b')
+        self.build_tree_contents([('checkout/file', 'c')])
+        switch.switch(checkout.bzrdir, bar.branch)
+
 
 class TestSwitchHeavyweight(TestSwitch):