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
1=== modified file 'bzrlib/decorators.py'
2--- bzrlib/decorators.py 2010-01-20 16:12:23 +0000
3+++ bzrlib/decorators.py 2010-01-20 16:12:25 +0000
4@@ -256,7 +256,8 @@
5
6
7 # This implementation of cachedproperty is copied from Launchpad's
8-# canonical.launchpad.cachedproperty module.
9+# canonical.launchpad.cachedproperty module (with permission from flacoste)
10+# -- spiv & vila 100120
11 def cachedproperty(attrname_or_fn):
12 """A decorator for methods that makes them properties with their return
13 value cached.
14
15=== modified file 'bzrlib/merge.py'
16--- bzrlib/merge.py 2010-01-20 16:12:23 +0000
17+++ bzrlib/merge.py 2010-01-20 16:12:25 +0000
18@@ -89,11 +89,11 @@
19 self.this_kind = this_kind
20 self.other_kind = other_kind
21 self.winner = winner
22-
23+
24 def is_file_merge(self):
25 """True if this_kind and other_kind are both 'file'."""
26 return self.this_kind == 'file' and self.other_kind == 'file'
27-
28+
29 @decorators.cachedproperty
30 def base_lines(self):
31 """The lines of the 'base' version of the file."""
32
33=== modified file 'bzrlib/plugins/news_merge/README'
34--- bzrlib/plugins/news_merge/README 2010-01-20 16:12:23 +0000
35+++ bzrlib/plugins/news_merge/README 2010-01-20 16:12:25 +0000
36@@ -1,8 +1,6 @@
37 A plugin for merging bzr's NEWS file.
38
39-Install as usual (e.g. symlink this directory into ~/.bazaar/plugins), and any
40-file with a path of NEWS that is in bzr's NEWS format will be merged with this
41-hook.
42+This plugin is activated via configuration variables, see 'bzr help news_merge'.
43
44 This hook can resolve conflicts where both sides add entries at the same place.
45 If it encounters a more difficult conflict it gives up and bzr will fallback to
46
47=== modified file 'bzrlib/plugins/news_merge/__init__.py'
48--- bzrlib/plugins/news_merge/__init__.py 2010-01-20 16:12:23 +0000
49+++ bzrlib/plugins/news_merge/__init__.py 2010-01-20 16:12:25 +0000
50@@ -32,6 +32,9 @@
51 how to resolve that, so bzr will fallback to the default line-based merge.
52 """
53
54+# Since we are a built-in plugin we share the bzrlib version
55+from bzrlib import version_info
56+
57 # Put most of the code in a separate module that we lazy-import to keep the
58 # overhead of this plugin as minimal as possible.
59 from bzrlib.lazy_import import lazy_import
60@@ -59,15 +62,35 @@
61
62
63 def filename_matches_config(params):
64- config = params.merger.this_branch.get_config()
65- affected_files = config.get_user_option_as_list('news_merge_files')
66+ affected_files = getattr(params, '_news_merge_affected_files', None)
67+ if affected_files is None:
68+ config = params.merger.this_tree.branch.get_config()
69+ # Until bzr provides a better policy for caching the config, we just
70+ # add the part we're interested in to the params to avoid reading the
71+ # config files repeatedly (bazaar.conf, location.conf, branch.conf).
72+ affected_files = config.get_user_option_as_list('news_merge_files')
73+ if affected_files is None:
74+ # If nothing was specified in the config, we have nothing to do,
75+ # but we use None in the params to start the caching.
76+ affected_files = []
77+ params._news_merge_affected_files = affected_files
78 if affected_files:
79 filename = params.merger.this_tree.id2path(params.file_id)
80 if filename in affected_files:
81 return True
82 return False
83
84-
85-Merger.hooks.install_named_hook(
86- 'merge_file_content', news_merge_hook, 'NEWS file merge')
87+def install_hook():
88+ Merger.hooks.install_named_hook(
89+ 'merge_file_content', news_merge_hook, 'NEWS file merge')
90+install_hook()
91+
92+
93+def load_tests(basic_tests, module, loader):
94+ testmod_names = [
95+ 'tests',
96+ ]
97+ basic_tests.addTest(loader.loadTestsFromModuleNames(
98+ ["%s.%s" % (__name__, tmn) for tmn in testmod_names]))
99+ return basic_tests
100
101
102=== modified file 'bzrlib/plugins/news_merge/news_merge.py'
103--- bzrlib/plugins/news_merge/news_merge.py 2010-01-20 16:12:23 +0000
104+++ bzrlib/plugins/news_merge/news_merge.py 2010-01-20 16:12:25 +0000
105@@ -85,4 +85,3 @@
106
107 def sort_key(s):
108 return s.replace('`', '').lower()
109-
110
111=== added directory 'bzrlib/plugins/news_merge/tests'
112=== added file 'bzrlib/plugins/news_merge/tests/__init__.py'
113--- bzrlib/plugins/news_merge/tests/__init__.py 1970-01-01 00:00:00 +0000
114+++ bzrlib/plugins/news_merge/tests/__init__.py 2010-01-20 16:12:25 +0000
115@@ -0,0 +1,23 @@
116+# Copyright (C) 2010 by Canonical Ltd
117+#
118+# This program is free software; you can redistribute it and/or modify
119+# it under the terms of the GNU General Public License as published by
120+# the Free Software Foundation; either version 2 of the License, or
121+# (at your option) any later version.
122+#
123+# This program is distributed in the hope that it will be useful,
124+# but WITHOUT ANY WARRANTY; without even the implied warranty of
125+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
126+# GNU General Public License for more details.
127+#
128+# You should have received a copy of the GNU General Public License
129+# along with this program; if not, write to the Free Software
130+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
131+
132+def load_tests(basic_tests, module, loader):
133+ testmod_names = [
134+ 'test_news_merge',
135+ ]
136+ basic_tests.addTest(loader.loadTestsFromModuleNames(
137+ ["%s.%s" % (__name__, tmn) for tmn in testmod_names]))
138+ return basic_tests
139
140=== added file 'bzrlib/plugins/news_merge/tests/test_news_merge.py'
141--- bzrlib/plugins/news_merge/tests/test_news_merge.py 1970-01-01 00:00:00 +0000
142+++ bzrlib/plugins/news_merge/tests/test_news_merge.py 2010-01-20 16:12:25 +0000
143@@ -0,0 +1,51 @@
144+# Copyright (C) 2010 by Canonical Ltd
145+#
146+# This program is free software; you can redistribute it and/or modify
147+# it under the terms of the GNU General Public License as published by
148+# the Free Software Foundation; either version 2 of the License, or
149+# (at your option) any later version.
150+#
151+# This program is distributed in the hope that it will be useful,
152+# but WITHOUT ANY WARRANTY; without even the implied warranty of
153+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
154+# GNU General Public License for more details.
155+#
156+# You should have received a copy of the GNU General Public License
157+# along with this program; if not, write to the Free Software
158+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
159+
160+# FIXME: This is totally incomplete but I'm only the patch pilot :-)
161+# -- vila 100120
162+
163+from bzrlib import (
164+ option,
165+ tests,
166+ )
167+from bzrlib.plugins import news_merge
168+from bzrlib.tests import test_merge_core
169+
170+
171+class TestFilenameMatchesConfig(tests.TestCaseWithTransport):
172+
173+ def test_affected_files_cached(self):
174+ """Ensures that the config variable is cached"""
175+ news_merge.install_hook()
176+ self.affected_files = None
177+ orig = news_merge.filename_matches_config
178+ def wrapper(params):
179+ ret = orig(params)
180+ # Capture the value set by the hook
181+ self.affected_files = params._news_merge_affected_files
182+ return ret
183+ def restore():
184+ news_merge.filename_matches_config = orig
185+ self.addCleanup(restore)
186+ news_merge.filename_matches_config = wrapper
187+
188+ builder = test_merge_core.MergeBuilder(self.test_base_dir)
189+ self.addCleanup(builder.cleanup)
190+ builder.add_file('NEWS', builder.tree_root, 'name1', 'text1', True)
191+ builder.change_contents('NEWS', other='text4', this='text3')
192+ conflicts = builder.merge()
193+ # The hook should set the variable
194+ self.assertIsNot(None, self.affected_files)
195
196=== modified file 'bzrlib/tests/per_merger.py'
197--- bzrlib/tests/per_merger.py 2010-01-20 16:12:23 +0000
198+++ bzrlib/tests/per_merger.py 2010-01-20 16:12:25 +0000
199@@ -142,7 +142,8 @@
200 this_tree = branch.bzrdir.create_workingtree()
201 this_tree.lock_write()
202 self.addCleanup(this_tree.unlock)
203- other_tree = this_tree.bzrdir.sprout('other', 'OTHER-id').open_workingtree()
204+ other_tree = this_tree.bzrdir.sprout('other',
205+ 'OTHER-id').open_workingtree()
206 self.do_merge(this_tree, other_tree)
207 if self.merge_type is _mod_merge.LCAMerger:
208 self.expectFailure("lca merge doesn't track deleted lines",
209@@ -174,7 +175,7 @@
210 deletiondir = transform._deletiondir
211 transform.finalize()
212 return (limbodir, deletiondir)
213-
214+
215 def test_merge_with_existing_limbo(self):
216 wt = self.make_branch_and_tree('this')
217 (limbodir, deletiondir) = self.get_limbodir_deletiondir(wt)
218@@ -196,7 +197,7 @@
219 def setUp(self):
220 TestCaseWithTransport.setUp(self)
221 self.hook_log = []
222-
223+
224 def install_hook_noop(self):
225 def hook_na(merge_params):
226 # This hook unconditionally does nothing.
227@@ -213,7 +214,7 @@
228 return 'not_applicable', None
229 _mod_merge.Merger.hooks.install_named_hook(
230 'merge_file_content', hook_success, 'test hook (success)')
231-
232+
233 def install_hook_conflict(self):
234 def hook_conflict(merge_params):
235 self.hook_log.append(('conflict',))
236@@ -222,7 +223,7 @@
237 return 'not_applicable', None
238 _mod_merge.Merger.hooks.install_named_hook(
239 'merge_file_content', hook_conflict, 'test hook (delete)')
240-
241+
242 def install_hook_delete(self):
243 def hook_delete(merge_params):
244 self.hook_log.append(('delete',))
245@@ -231,7 +232,7 @@
246 return 'not_applicable', None
247 _mod_merge.Merger.hooks.install_named_hook(
248 'merge_file_content', hook_delete, 'test hook (delete)')
249-
250+
251 def install_hook_log_lines(self):
252 """Install a hook that saves the get_lines for the this, base and other
253 versions of the file.
254@@ -255,7 +256,7 @@
255 def create_file_needing_contents_merge(self, builder, file_id):
256 builder.add_file(file_id, builder.tree_root, "name1", "text1", True)
257 builder.change_contents(file_id, other="text4", this="text3")
258-
259+
260 def test_change_vs_change(self):
261 """Hook is used for (changed, changed)"""
262 self.install_hook_success()