Merge lp:~gz/bzr/resolve_auto_refactor into lp:bzr

Proposed by Martin Packman on 2012-07-27
Status: Needs review
Proposed branch: lp:~gz/bzr/resolve_auto_refactor
Merge into: lp:bzr
Diff against target: 243 lines (+58/-51)
3 files modified
bzrlib/conflicts.py (+36/-28)
bzrlib/tests/test_workingtree.py (+13/-6)
bzrlib/workingtree.py (+9/-17)
To merge this branch: bzr merge lp:~gz/bzr/resolve_auto_refactor
Reviewer Review Type Date Requested Status
bzr-core 2012-07-27 Pending
Review via email: mp+117051@code.launchpad.net

Description of the Change

Creates a new kind of conflict resolution action named 'auto', refactoring code from `WorkingTree.auto_resolve` and deprecating that method. There should be no behaviour change.

I'd like to write some specific tests for this action type, but can't really see a way in to bt.test_conflicts to do that.

Is there documentation other than in conflict-types.txt that this branch should update?

A followup branch will make `bzr resolve FILE` default to --auto rather than --done.

May also be a good idea to unify the --all switch and deprecate it.

I'm not wild about using NotImplementedError as the only mechanism of signalling a conflict as not having been resolved. Perhaps another exception type should be added for this?

The UI can benefit from some cleanup later, notably `bzr resolve` gives a return code if there are pending conflicts, and there are slightly different spellings of the results.

Having the pending conflicts returned by the auto_resolve method is actually a little useful. In general, conficts.resolve just returning the counts is fine, but to report the details having the conflict objects is handy.

To post a comment you must log in.
Vincent Ladeuil (vila) wrote :

> I'd like to write some specific tests for this action type, but can't really see a way in to bt.test_conflicts to do that.

Indeed, IIRC, the tests were added for --take-[this|other]. For them there is no user action.

You need a slight variation where some action is performed by the user after 'merge' but before 'resolve'. In the general case, you want to test for robustness there as really the user may do weird stuff (like deleting some file you expect).

> Is there documentation other than in conflict-types.txt that this branch should update?

I can't remember one.

> Perhaps another exception type should be added for this?

No objection your honor :)

> May also be a good idea to unify the --all switch and deprecate it.

I don't like '--all' (far too many people have used it with disastrous results), so I'll approve its deprecation :)

Now, you may want to get feedback from the list about whether anybody finds it useful *and* used it in the last years. I'm pretty sure it was introduced as a way to get out of weird situations without having to use 'bzr revert'.

> The UI can benefit from some cleanup later, notably `bzr resolve` gives a return code if there are pending conflicts, and there are slightly different spellings of the results.

Yup, look at the annotations, search the mps, the GUI came as an afterthought and was never really designed properly.

> to report the details having the conflict objects is handy.

Leave that to the conflicts objects if you really need to, no need to move them around (especially because the ConflictList object isn't really necessary and makes manipulating the Conflict objects harder than needed).

lp:~gz/bzr/resolve_auto_refactor updated on 2012-07-27
6546. By Martin Packman on 2012-07-27

Use tree.get_file rather than touching filesystem directly to auto resolve

6547. By Martin Packman on 2012-07-27

Note issue over looking for conflict markers in a removed file

Unmerged revisions

6547. By Martin Packman on 2012-07-27

Note issue over looking for conflict markers in a removed file

6546. By Martin Packman on 2012-07-27

Use tree.get_file rather than touching filesystem directly to auto resolve

6545. By Martin Packman on 2012-07-27

Deprecate WorkingTree.auto_resolve

6544. By Martin Packman on 2012-07-27

Refactor auto_resolve tree method into auto action on conflicts

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/conflicts.py'
2--- bzrlib/conflicts.py 2012-01-16 14:45:48 +0000
3+++ bzrlib/conflicts.py 2012-07-27 17:50:26 +0000
4@@ -20,6 +20,7 @@
5 from __future__ import absolute_import
6
7 import os
8+import re
9
10 from bzrlib.lazy_import import lazy_import
11 lazy_import(globals(), """
12@@ -82,6 +83,8 @@
13
14
15 resolve_action_registry.register(
16+ 'auto', 'auto', 'Detect whether conflict has been resolved by user.')
17+resolve_action_registry.register(
18 'done', 'done', 'Marks the conflict as resolved.')
19 resolve_action_registry.register(
20 'take-this', 'take_this',
21@@ -133,38 +136,26 @@
22 else:
23 tree, file_list = workingtree.WorkingTree.open_containing_paths(
24 file_list, directory)
25- if file_list is None:
26- if action is None:
27- # FIXME: There is a special case here related to the option
28- # handling that could be clearer and easier to discover by
29- # providing an --auto action (bug #344013 and #383396) and
30- # make it mandatory instead of implicit and active only
31- # when no file_list is provided -- vila 091229
32+ if action is None:
33+ if file_list is None:
34 action = 'auto'
35- else:
36- if action is None:
37+ else:
38 action = 'done'
39- if action == 'auto':
40- if file_list is None:
41- un_resolved, resolved = tree.auto_resolve()
42- if len(un_resolved) > 0:
43- trace.note(ngettext('%d conflict auto-resolved.',
44- '%d conflicts auto-resolved.', len(resolved)),
45- len(resolved))
46- trace.note(gettext('Remaining conflicts:'))
47- for conflict in un_resolved:
48- trace.note(unicode(conflict))
49- return 1
50- else:
51- trace.note(gettext('All conflicts resolved.'))
52- return 0
53+ before, after = resolve(tree, file_list, action=action)
54+ # GZ 2012-07-27: Should unify UI below now that auto is less magical.
55+ if action == 'auto' and file_list is None:
56+ if after > 0:
57+ trace.note(ngettext('%d conflict auto-resolved.',
58+ '%d conflicts auto-resolved.', before - after),
59+ before - after)
60+ trace.note(gettext('Remaining conflicts:'))
61+ for conflict in tree.conflicts():
62+ trace.note(unicode(conflict))
63+ return 1
64 else:
65- # FIXME: This can never occur but the block above needs some
66- # refactoring to transfer tree.auto_resolve() to
67- # conflict.auto(tree) --vila 091242
68- pass
69+ trace.note(gettext('All conflicts resolved.'))
70+ return 0
71 else:
72- before, after = resolve(tree, file_list, action=action)
73 trace.note(ngettext('{0} conflict resolved, {1} remaining',
74 '{0} conflicts resolved, {1} remaining',
75 before-after).format(before - after, after))
76@@ -442,6 +433,9 @@
77 if e.errno != errno.ENOENT:
78 raise
79
80+ def action_auto(self, tree):
81+ raise NotImplementedError(self.action_auto)
82+
83 def action_done(self, tree):
84 """Mark the conflict as solved once it has been handled."""
85 # This method does nothing but simplifies the design of upper levels.
86@@ -639,6 +633,8 @@
87
88 rformat = '%(class)s(%(path)r, %(file_id)r)'
89
90+ _conflict_re = re.compile('^(<{7}|={7}|>{7})')
91+
92 def associated_filenames(self):
93 return [self.path + suffix for suffix in CONFLICT_SUFFIXES]
94
95@@ -668,6 +664,18 @@
96 tt.version_file(self.file_id, winner_tid)
97 tt.apply()
98
99+ def action_auto(self, tree):
100+ # GZ 2012-07-27: Using NotImplementedError to signal that a conflict
101+ # can't be auto resolved does not seem ideal.
102+ if tree.kind(self.file_id) != 'file':
103+ raise NotImplementedError("Conflict is not a file")
104+ conflict_markers_in_line = self._conflict_re.search
105+ # GZ 2012-07-27: What if not tree.has_id(self.file_id) due to removal?
106+ with tree.get_file(self.file_id) as f:
107+ for line in f:
108+ if conflict_markers_in_line(line):
109+ raise NotImplementedError("Conflict markers present")
110+
111 def action_take_this(self, tree):
112 self._resolve_with_cleanups(tree, 'THIS')
113
114
115=== modified file 'bzrlib/tests/test_workingtree.py'
116--- bzrlib/tests/test_workingtree.py 2012-07-19 15:44:55 +0000
117+++ bzrlib/tests/test_workingtree.py 2012-07-27 17:50:26 +0000
118@@ -19,6 +19,7 @@
119 bzrdir,
120 conflicts,
121 errors,
122+ symbol_versioning,
123 transport,
124 workingtree,
125 workingtree_3,
126@@ -438,6 +439,12 @@
127
128 class TestAutoResolve(TestCaseWithTransport):
129
130+ def _auto_resolve(self, tree):
131+ """Call auto_resolve on tree expecting deprecation"""
132+ return self.applyDeprecated(
133+ symbol_versioning.deprecated_in((2, 6, 0)),
134+ tree.auto_resolve,)
135+
136 def test_auto_resolve(self):
137 base = self.make_branch_and_tree('base')
138 self.build_tree_contents([('base/hello', 'Hello')])
139@@ -453,24 +460,24 @@
140 this.merge_from_branch(other.branch)
141 self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
142 this.conflicts())
143- this.auto_resolve()
144+ self._auto_resolve(this)
145 self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
146 this.conflicts())
147 self.build_tree_contents([('this/hello', '<<<<<<<')])
148- this.auto_resolve()
149+ self._auto_resolve(this)
150 self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
151 this.conflicts())
152 self.build_tree_contents([('this/hello', '=======')])
153- this.auto_resolve()
154+ self._auto_resolve(this)
155 self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
156 this.conflicts())
157 self.build_tree_contents([('this/hello', '\n>>>>>>>')])
158- remaining, resolved = this.auto_resolve()
159+ remaining, resolved = self._auto_resolve(this)
160 self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
161 this.conflicts())
162 self.assertEqual([], resolved)
163 self.build_tree_contents([('this/hello', 'hELLO wORLD')])
164- remaining, resolved = this.auto_resolve()
165+ remaining, resolved = self._auto_resolve(this)
166 self.assertEqual([], this.conflicts())
167 self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
168 resolved)
169@@ -482,7 +489,7 @@
170 tree.add('hello', 'hello-id')
171 file_conflict = conflicts.TextConflict('file', 'hello-id')
172 tree.set_conflicts(conflicts.ConflictList([file_conflict]))
173- tree.auto_resolve()
174+ self._auto_resolve(tree)
175
176
177 class TestFindTrees(TestCaseWithTransport):
178
179=== modified file 'bzrlib/workingtree.py'
180--- bzrlib/workingtree.py 2012-07-18 20:06:21 +0000
181+++ bzrlib/workingtree.py 2012-07-27 17:50:26 +0000
182@@ -43,7 +43,6 @@
183 import itertools
184 import operator
185 import stat
186-import re
187
188 from bzrlib import (
189 branch,
190@@ -74,7 +73,6 @@
191 # is guaranteed to be registered.
192 from bzrlib import (
193 bzrdir,
194- symbol_versioning,
195 )
196
197 from bzrlib.decorators import needs_read_lock, needs_write_lock
198@@ -95,6 +93,8 @@
199 from bzrlib.trace import mutter, note
200 from bzrlib.revision import CURRENT_REVISION
201 from bzrlib.symbol_versioning import (
202+ deprecated_in,
203+ deprecated_method,
204 deprecated_passed,
205 DEPRECATED_PARAMETER,
206 )
207@@ -1698,6 +1698,7 @@
208 """
209 raise NotImplementedError(self._walkdirs)
210
211+ @deprecated_method(deprecated_in((2, 6, 0)))
212 @needs_tree_write_lock
213 def auto_resolve(self):
214 """Automatically resolve text conflicts according to contents.
215@@ -1711,23 +1712,14 @@
216 """
217 un_resolved = _mod_conflicts.ConflictList()
218 resolved = _mod_conflicts.ConflictList()
219- conflict_re = re.compile('^(<{7}|={7}|>{7})')
220 for conflict in self.conflicts():
221- if (conflict.typestring != 'text conflict' or
222- self.kind(conflict.file_id) != 'file'):
223+ try:
224+ conflict.action_auto(self)
225+ except NotImplementedError:
226 un_resolved.append(conflict)
227- continue
228- my_file = open(self.id2abspath(conflict.file_id), 'rb')
229- try:
230- for line in my_file:
231- if conflict_re.search(line):
232- un_resolved.append(conflict)
233- break
234- else:
235- resolved.append(conflict)
236- finally:
237- my_file.close()
238- resolved.remove_files(self)
239+ else:
240+ conflict.cleanup(self)
241+ resolved.append(conflict)
242 self.set_conflicts(un_resolved)
243 return un_resolved, resolved
244