Bazaar

Merge lp:~vila/bzr/per-file-merge-hook into lp:bzr

  1. per-file-merge-hook
  2. Merge into bzr.dev
Proposed by Vincent Ladeuil
Status: Merged
Approved by: John A Meinel
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~vila/bzr/per-file-merge-hook
Merge into: lp:bzr
Prerequisite: lp:~spiv/bzr/per-file-merge-hook-491711
Diff against target: 262 lines (+115/-19)
8 files modified
bzrlib/decorators.py (+2/-1)
bzrlib/merge.py (+2/-2)
bzrlib/plugins/news_merge/README (+1/-3)
bzrlib/plugins/news_merge/__init__.py (+28/-5)
bzrlib/plugins/news_merge/news_merge.py (+0/-1)
bzrlib/plugins/news_merge/tests/__init__.py (+23/-0)
bzrlib/plugins/news_merge/tests/test_news_merge.py (+51/-0)
bzrlib/tests/per_merger.py (+8/-7)
To merge this branch: bzr merge lp:~vila/bzr/per-file-merge-hook
Reviewer Review Type Date Requested Status
bzr-core Pending
Review via email: mp+17754@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Vincent Ladeuil (vila) wrote :

Since spiv is on leave, I've finished his lp:~spiv/bzr/per-file-merge-hook-491711
merge proposal based on:
- the reviews,
- some discussion on IRC,
- some more cleanup.

The core modification is about caching the config data to avoid re-reading
the config files for each merged file.

* bzrlib/tests/per_merger.py:
Fix line too long and spurious spaces.

* bzrlib/plugins/news_merge/tests/test_news_merge.py:
(TestFilenameMatchesConfig): Ensure that the params get updated.

* bzrlib/plugins/news_merge/__init__.py:
(filename_matches_config): Save the relevant config variable in
the hook params.
(install_hook): Wrap the hook installation so we can reuse it for
tests.

* bzrlib/plugins/news_merge/README:
Update the instructions by pointing to the plugin help.

* bzrlib/merge.py:
(MergeHookParams): Delete spurious spaces.

* bzrlib/decorators.py:
(use_pretty_decorators): Mention that we get clearance to copy
launchpad code here (since canonical has copyrights on both code
bases).

As it's far too much to be considered simple tweaks, I'd like a review on the diff
before (if approved :) landing both.

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

looks reasonable to me

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'bzrlib/decorators.py'
--- bzrlib/decorators.py 2010-01-20 16:12:23 +0000
+++ bzrlib/decorators.py 2010-01-20 16:12:25 +0000
@@ -256,7 +256,8 @@
256256
257257
258# This implementation of cachedproperty is copied from Launchpad's258# This implementation of cachedproperty is copied from Launchpad's
259# canonical.launchpad.cachedproperty module.259# canonical.launchpad.cachedproperty module (with permission from flacoste)
260# -- spiv & vila 100120
260def cachedproperty(attrname_or_fn):261def cachedproperty(attrname_or_fn):
261 """A decorator for methods that makes them properties with their return262 """A decorator for methods that makes them properties with their return
262 value cached.263 value cached.
263264
=== modified file 'bzrlib/merge.py'
--- bzrlib/merge.py 2010-01-20 16:12:23 +0000
+++ bzrlib/merge.py 2010-01-20 16:12:25 +0000
@@ -89,11 +89,11 @@
89 self.this_kind = this_kind89 self.this_kind = this_kind
90 self.other_kind = other_kind90 self.other_kind = other_kind
91 self.winner = winner91 self.winner = winner
92 92
93 def is_file_merge(self):93 def is_file_merge(self):
94 """True if this_kind and other_kind are both 'file'."""94 """True if this_kind and other_kind are both 'file'."""
95 return self.this_kind == 'file' and self.other_kind == 'file'95 return self.this_kind == 'file' and self.other_kind == 'file'
96 96
97 @decorators.cachedproperty97 @decorators.cachedproperty
98 def base_lines(self):98 def base_lines(self):
99 """The lines of the 'base' version of the file."""99 """The lines of the 'base' version of the file."""
100100
=== modified file 'bzrlib/plugins/news_merge/README'
--- bzrlib/plugins/news_merge/README 2010-01-20 16:12:23 +0000
+++ bzrlib/plugins/news_merge/README 2010-01-20 16:12:25 +0000
@@ -1,8 +1,6 @@
1A plugin for merging bzr's NEWS file.1A plugin for merging bzr's NEWS file.
22
3Install as usual (e.g. symlink this directory into ~/.bazaar/plugins), and any3This plugin is activated via configuration variables, see 'bzr help news_merge'.
4file with a path of NEWS that is in bzr's NEWS format will be merged with this
5hook.
64
7This hook can resolve conflicts where both sides add entries at the same place.5This hook can resolve conflicts where both sides add entries at the same place.
8If it encounters a more difficult conflict it gives up and bzr will fallback to6If it encounters a more difficult conflict it gives up and bzr will fallback to
97
=== modified file 'bzrlib/plugins/news_merge/__init__.py'
--- bzrlib/plugins/news_merge/__init__.py 2010-01-20 16:12:23 +0000
+++ bzrlib/plugins/news_merge/__init__.py 2010-01-20 16:12:25 +0000
@@ -32,6 +32,9 @@
32 how to resolve that, so bzr will fallback to the default line-based merge.32 how to resolve that, so bzr will fallback to the default line-based merge.
33"""33"""
3434
35# Since we are a built-in plugin we share the bzrlib version
36from bzrlib import version_info
37
35# Put most of the code in a separate module that we lazy-import to keep the38# Put most of the code in a separate module that we lazy-import to keep the
36# overhead of this plugin as minimal as possible.39# overhead of this plugin as minimal as possible.
37from bzrlib.lazy_import import lazy_import40from bzrlib.lazy_import import lazy_import
@@ -59,15 +62,35 @@
5962
6063
61def filename_matches_config(params):64def filename_matches_config(params):
62 config = params.merger.this_branch.get_config()65 affected_files = getattr(params, '_news_merge_affected_files', None)
63 affected_files = config.get_user_option_as_list('news_merge_files')66 if affected_files is None:
67 config = params.merger.this_tree.branch.get_config()
68 # Until bzr provides a better policy for caching the config, we just
69 # add the part we're interested in to the params to avoid reading the
70 # config files repeatedly (bazaar.conf, location.conf, branch.conf).
71 affected_files = config.get_user_option_as_list('news_merge_files')
72 if affected_files is None:
73 # If nothing was specified in the config, we have nothing to do,
74 # but we use None in the params to start the caching.
75 affected_files = []
76 params._news_merge_affected_files = affected_files
64 if affected_files:77 if affected_files:
65 filename = params.merger.this_tree.id2path(params.file_id)78 filename = params.merger.this_tree.id2path(params.file_id)
66 if filename in affected_files:79 if filename in affected_files:
67 return True80 return True
68 return False81 return False
6982
7083def install_hook():
71Merger.hooks.install_named_hook(84 Merger.hooks.install_named_hook(
72 'merge_file_content', news_merge_hook, 'NEWS file merge')85 'merge_file_content', news_merge_hook, 'NEWS file merge')
86install_hook()
87
88
89def load_tests(basic_tests, module, loader):
90 testmod_names = [
91 'tests',
92 ]
93 basic_tests.addTest(loader.loadTestsFromModuleNames(
94 ["%s.%s" % (__name__, tmn) for tmn in testmod_names]))
95 return basic_tests
7396
7497
=== modified file 'bzrlib/plugins/news_merge/news_merge.py'
--- bzrlib/plugins/news_merge/news_merge.py 2010-01-20 16:12:23 +0000
+++ bzrlib/plugins/news_merge/news_merge.py 2010-01-20 16:12:25 +0000
@@ -85,4 +85,3 @@
8585
86def sort_key(s):86def sort_key(s):
87 return s.replace('`', '').lower()87 return s.replace('`', '').lower()
88
8988
=== added directory 'bzrlib/plugins/news_merge/tests'
=== added file 'bzrlib/plugins/news_merge/tests/__init__.py'
--- bzrlib/plugins/news_merge/tests/__init__.py 1970-01-01 00:00:00 +0000
+++ bzrlib/plugins/news_merge/tests/__init__.py 2010-01-20 16:12:25 +0000
@@ -0,0 +1,23 @@
1# Copyright (C) 2010 by Canonical Ltd
2#
3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by
5# the Free Software Foundation; either version 2 of the License, or
6# (at your option) any later version.
7#
8# This program is distributed in the hope that it will be useful,
9# but WITHOUT ANY WARRANTY; without even the implied warranty of
10# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
11# GNU General Public License for more details.
12#
13# You should have received a copy of the GNU General Public License
14# along with this program; if not, write to the Free Software
15# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
16
17def load_tests(basic_tests, module, loader):
18 testmod_names = [
19 'test_news_merge',
20 ]
21 basic_tests.addTest(loader.loadTestsFromModuleNames(
22 ["%s.%s" % (__name__, tmn) for tmn in testmod_names]))
23 return basic_tests
024
=== added file 'bzrlib/plugins/news_merge/tests/test_news_merge.py'
--- bzrlib/plugins/news_merge/tests/test_news_merge.py 1970-01-01 00:00:00 +0000
+++ bzrlib/plugins/news_merge/tests/test_news_merge.py 2010-01-20 16:12:25 +0000
@@ -0,0 +1,51 @@
1# Copyright (C) 2010 by Canonical Ltd
2#
3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by
5# the Free Software Foundation; either version 2 of the License, or
6# (at your option) any later version.
7#
8# This program is distributed in the hope that it will be useful,
9# but WITHOUT ANY WARRANTY; without even the implied warranty of
10# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
11# GNU General Public License for more details.
12#
13# You should have received a copy of the GNU General Public License
14# along with this program; if not, write to the Free Software
15# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
16
17# FIXME: This is totally incomplete but I'm only the patch pilot :-)
18# -- vila 100120
19
20from bzrlib import (
21 option,
22 tests,
23 )
24from bzrlib.plugins import news_merge
25from bzrlib.tests import test_merge_core
26
27
28class TestFilenameMatchesConfig(tests.TestCaseWithTransport):
29
30 def test_affected_files_cached(self):
31 """Ensures that the config variable is cached"""
32 news_merge.install_hook()
33 self.affected_files = None
34 orig = news_merge.filename_matches_config
35 def wrapper(params):
36 ret = orig(params)
37 # Capture the value set by the hook
38 self.affected_files = params._news_merge_affected_files
39 return ret
40 def restore():
41 news_merge.filename_matches_config = orig
42 self.addCleanup(restore)
43 news_merge.filename_matches_config = wrapper
44
45 builder = test_merge_core.MergeBuilder(self.test_base_dir)
46 self.addCleanup(builder.cleanup)
47 builder.add_file('NEWS', builder.tree_root, 'name1', 'text1', True)
48 builder.change_contents('NEWS', other='text4', this='text3')
49 conflicts = builder.merge()
50 # The hook should set the variable
51 self.assertIsNot(None, self.affected_files)
052
=== modified file 'bzrlib/tests/per_merger.py'
--- bzrlib/tests/per_merger.py 2010-01-20 16:12:23 +0000
+++ bzrlib/tests/per_merger.py 2010-01-20 16:12:25 +0000
@@ -142,7 +142,8 @@
142 this_tree = branch.bzrdir.create_workingtree()142 this_tree = branch.bzrdir.create_workingtree()
143 this_tree.lock_write()143 this_tree.lock_write()
144 self.addCleanup(this_tree.unlock)144 self.addCleanup(this_tree.unlock)
145 other_tree = this_tree.bzrdir.sprout('other', 'OTHER-id').open_workingtree()145 other_tree = this_tree.bzrdir.sprout('other',
146 'OTHER-id').open_workingtree()
146 self.do_merge(this_tree, other_tree)147 self.do_merge(this_tree, other_tree)
147 if self.merge_type is _mod_merge.LCAMerger:148 if self.merge_type is _mod_merge.LCAMerger:
148 self.expectFailure("lca merge doesn't track deleted lines",149 self.expectFailure("lca merge doesn't track deleted lines",
@@ -174,7 +175,7 @@
174 deletiondir = transform._deletiondir175 deletiondir = transform._deletiondir
175 transform.finalize()176 transform.finalize()
176 return (limbodir, deletiondir)177 return (limbodir, deletiondir)
177 178
178 def test_merge_with_existing_limbo(self):179 def test_merge_with_existing_limbo(self):
179 wt = self.make_branch_and_tree('this')180 wt = self.make_branch_and_tree('this')
180 (limbodir, deletiondir) = self.get_limbodir_deletiondir(wt)181 (limbodir, deletiondir) = self.get_limbodir_deletiondir(wt)
@@ -196,7 +197,7 @@
196 def setUp(self):197 def setUp(self):
197 TestCaseWithTransport.setUp(self)198 TestCaseWithTransport.setUp(self)
198 self.hook_log = []199 self.hook_log = []
199 200
200 def install_hook_noop(self):201 def install_hook_noop(self):
201 def hook_na(merge_params):202 def hook_na(merge_params):
202 # This hook unconditionally does nothing.203 # This hook unconditionally does nothing.
@@ -213,7 +214,7 @@
213 return 'not_applicable', None214 return 'not_applicable', None
214 _mod_merge.Merger.hooks.install_named_hook(215 _mod_merge.Merger.hooks.install_named_hook(
215 'merge_file_content', hook_success, 'test hook (success)')216 'merge_file_content', hook_success, 'test hook (success)')
216 217
217 def install_hook_conflict(self):218 def install_hook_conflict(self):
218 def hook_conflict(merge_params):219 def hook_conflict(merge_params):
219 self.hook_log.append(('conflict',))220 self.hook_log.append(('conflict',))
@@ -222,7 +223,7 @@
222 return 'not_applicable', None223 return 'not_applicable', None
223 _mod_merge.Merger.hooks.install_named_hook(224 _mod_merge.Merger.hooks.install_named_hook(
224 'merge_file_content', hook_conflict, 'test hook (delete)')225 'merge_file_content', hook_conflict, 'test hook (delete)')
225 226
226 def install_hook_delete(self):227 def install_hook_delete(self):
227 def hook_delete(merge_params):228 def hook_delete(merge_params):
228 self.hook_log.append(('delete',))229 self.hook_log.append(('delete',))
@@ -231,7 +232,7 @@
231 return 'not_applicable', None232 return 'not_applicable', None
232 _mod_merge.Merger.hooks.install_named_hook(233 _mod_merge.Merger.hooks.install_named_hook(
233 'merge_file_content', hook_delete, 'test hook (delete)')234 'merge_file_content', hook_delete, 'test hook (delete)')
234 235
235 def install_hook_log_lines(self):236 def install_hook_log_lines(self):
236 """Install a hook that saves the get_lines for the this, base and other237 """Install a hook that saves the get_lines for the this, base and other
237 versions of the file.238 versions of the file.
@@ -255,7 +256,7 @@
255 def create_file_needing_contents_merge(self, builder, file_id):256 def create_file_needing_contents_merge(self, builder, file_id):
256 builder.add_file(file_id, builder.tree_root, "name1", "text1", True)257 builder.add_file(file_id, builder.tree_root, "name1", "text1", True)
257 builder.change_contents(file_id, other="text4", this="text3")258 builder.change_contents(file_id, other="text4", this="text3")
258 259
259 def test_change_vs_change(self):260 def test_change_vs_change(self):
260 """Hook is used for (changed, changed)"""261 """Hook is used for (changed, changed)"""
261 self.install_hook_success()262 self.install_hook_success()