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
=== modified file 'bzrlib/conflicts.py'
--- bzrlib/conflicts.py 2012-01-16 14:45:48 +0000
+++ bzrlib/conflicts.py 2012-07-27 17:50:26 +0000
@@ -20,6 +20,7 @@
20from __future__ import absolute_import20from __future__ import absolute_import
2121
22import os22import os
23import re
2324
24from bzrlib.lazy_import import lazy_import25from bzrlib.lazy_import import lazy_import
25lazy_import(globals(), """26lazy_import(globals(), """
@@ -82,6 +83,8 @@
8283
8384
84resolve_action_registry.register(85resolve_action_registry.register(
86 'auto', 'auto', 'Detect whether conflict has been resolved by user.')
87resolve_action_registry.register(
85 'done', 'done', 'Marks the conflict as resolved.')88 'done', 'done', 'Marks the conflict as resolved.')
86resolve_action_registry.register(89resolve_action_registry.register(
87 'take-this', 'take_this',90 'take-this', 'take_this',
@@ -133,38 +136,26 @@
133 else:136 else:
134 tree, file_list = workingtree.WorkingTree.open_containing_paths(137 tree, file_list = workingtree.WorkingTree.open_containing_paths(
135 file_list, directory)138 file_list, directory)
136 if file_list is None:139 if action is None:
137 if action is None:140 if file_list is None:
138 # FIXME: There is a special case here related to the option
139 # handling that could be clearer and easier to discover by
140 # providing an --auto action (bug #344013 and #383396) and
141 # make it mandatory instead of implicit and active only
142 # when no file_list is provided -- vila 091229
143 action = 'auto'141 action = 'auto'
144 else:142 else:
145 if action is None:
146 action = 'done'143 action = 'done'
147 if action == 'auto':144 before, after = resolve(tree, file_list, action=action)
148 if file_list is None:145 # GZ 2012-07-27: Should unify UI below now that auto is less magical.
149 un_resolved, resolved = tree.auto_resolve()146 if action == 'auto' and file_list is None:
150 if len(un_resolved) > 0:147 if after > 0:
151 trace.note(ngettext('%d conflict auto-resolved.',148 trace.note(ngettext('%d conflict auto-resolved.',
152 '%d conflicts auto-resolved.', len(resolved)),149 '%d conflicts auto-resolved.', before - after),
153 len(resolved))150 before - after)
154 trace.note(gettext('Remaining conflicts:'))151 trace.note(gettext('Remaining conflicts:'))
155 for conflict in un_resolved:152 for conflict in tree.conflicts():
156 trace.note(unicode(conflict))153 trace.note(unicode(conflict))
157 return 1154 return 1
158 else:
159 trace.note(gettext('All conflicts resolved.'))
160 return 0
161 else:155 else:
162 # FIXME: This can never occur but the block above needs some156 trace.note(gettext('All conflicts resolved.'))
163 # refactoring to transfer tree.auto_resolve() to157 return 0
164 # conflict.auto(tree) --vila 091242
165 pass
166 else:158 else:
167 before, after = resolve(tree, file_list, action=action)
168 trace.note(ngettext('{0} conflict resolved, {1} remaining',159 trace.note(ngettext('{0} conflict resolved, {1} remaining',
169 '{0} conflicts resolved, {1} remaining',160 '{0} conflicts resolved, {1} remaining',
170 before-after).format(before - after, after))161 before-after).format(before - after, after))
@@ -442,6 +433,9 @@
442 if e.errno != errno.ENOENT:433 if e.errno != errno.ENOENT:
443 raise434 raise
444435
436 def action_auto(self, tree):
437 raise NotImplementedError(self.action_auto)
438
445 def action_done(self, tree):439 def action_done(self, tree):
446 """Mark the conflict as solved once it has been handled."""440 """Mark the conflict as solved once it has been handled."""
447 # This method does nothing but simplifies the design of upper levels.441 # This method does nothing but simplifies the design of upper levels.
@@ -639,6 +633,8 @@
639633
640 rformat = '%(class)s(%(path)r, %(file_id)r)'634 rformat = '%(class)s(%(path)r, %(file_id)r)'
641635
636 _conflict_re = re.compile('^(<{7}|={7}|>{7})')
637
642 def associated_filenames(self):638 def associated_filenames(self):
643 return [self.path + suffix for suffix in CONFLICT_SUFFIXES]639 return [self.path + suffix for suffix in CONFLICT_SUFFIXES]
644640
@@ -668,6 +664,18 @@
668 tt.version_file(self.file_id, winner_tid)664 tt.version_file(self.file_id, winner_tid)
669 tt.apply()665 tt.apply()
670666
667 def action_auto(self, tree):
668 # GZ 2012-07-27: Using NotImplementedError to signal that a conflict
669 # can't be auto resolved does not seem ideal.
670 if tree.kind(self.file_id) != 'file':
671 raise NotImplementedError("Conflict is not a file")
672 conflict_markers_in_line = self._conflict_re.search
673 # GZ 2012-07-27: What if not tree.has_id(self.file_id) due to removal?
674 with tree.get_file(self.file_id) as f:
675 for line in f:
676 if conflict_markers_in_line(line):
677 raise NotImplementedError("Conflict markers present")
678
671 def action_take_this(self, tree):679 def action_take_this(self, tree):
672 self._resolve_with_cleanups(tree, 'THIS')680 self._resolve_with_cleanups(tree, 'THIS')
673681
674682
=== modified file 'bzrlib/tests/test_workingtree.py'
--- bzrlib/tests/test_workingtree.py 2012-07-19 15:44:55 +0000
+++ bzrlib/tests/test_workingtree.py 2012-07-27 17:50:26 +0000
@@ -19,6 +19,7 @@
19 bzrdir,19 bzrdir,
20 conflicts,20 conflicts,
21 errors,21 errors,
22 symbol_versioning,
22 transport,23 transport,
23 workingtree,24 workingtree,
24 workingtree_3,25 workingtree_3,
@@ -438,6 +439,12 @@
438439
439class TestAutoResolve(TestCaseWithTransport):440class TestAutoResolve(TestCaseWithTransport):
440441
442 def _auto_resolve(self, tree):
443 """Call auto_resolve on tree expecting deprecation"""
444 return self.applyDeprecated(
445 symbol_versioning.deprecated_in((2, 6, 0)),
446 tree.auto_resolve,)
447
441 def test_auto_resolve(self):448 def test_auto_resolve(self):
442 base = self.make_branch_and_tree('base')449 base = self.make_branch_and_tree('base')
443 self.build_tree_contents([('base/hello', 'Hello')])450 self.build_tree_contents([('base/hello', 'Hello')])
@@ -453,24 +460,24 @@
453 this.merge_from_branch(other.branch)460 this.merge_from_branch(other.branch)
454 self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],461 self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
455 this.conflicts())462 this.conflicts())
456 this.auto_resolve()463 self._auto_resolve(this)
457 self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],464 self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
458 this.conflicts())465 this.conflicts())
459 self.build_tree_contents([('this/hello', '<<<<<<<')])466 self.build_tree_contents([('this/hello', '<<<<<<<')])
460 this.auto_resolve()467 self._auto_resolve(this)
461 self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],468 self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
462 this.conflicts())469 this.conflicts())
463 self.build_tree_contents([('this/hello', '=======')])470 self.build_tree_contents([('this/hello', '=======')])
464 this.auto_resolve()471 self._auto_resolve(this)
465 self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],472 self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
466 this.conflicts())473 this.conflicts())
467 self.build_tree_contents([('this/hello', '\n>>>>>>>')])474 self.build_tree_contents([('this/hello', '\n>>>>>>>')])
468 remaining, resolved = this.auto_resolve()475 remaining, resolved = self._auto_resolve(this)
469 self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],476 self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
470 this.conflicts())477 this.conflicts())
471 self.assertEqual([], resolved)478 self.assertEqual([], resolved)
472 self.build_tree_contents([('this/hello', 'hELLO wORLD')])479 self.build_tree_contents([('this/hello', 'hELLO wORLD')])
473 remaining, resolved = this.auto_resolve()480 remaining, resolved = self._auto_resolve(this)
474 self.assertEqual([], this.conflicts())481 self.assertEqual([], this.conflicts())
475 self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],482 self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
476 resolved)483 resolved)
@@ -482,7 +489,7 @@
482 tree.add('hello', 'hello-id')489 tree.add('hello', 'hello-id')
483 file_conflict = conflicts.TextConflict('file', 'hello-id')490 file_conflict = conflicts.TextConflict('file', 'hello-id')
484 tree.set_conflicts(conflicts.ConflictList([file_conflict]))491 tree.set_conflicts(conflicts.ConflictList([file_conflict]))
485 tree.auto_resolve()492 self._auto_resolve(tree)
486493
487494
488class TestFindTrees(TestCaseWithTransport):495class TestFindTrees(TestCaseWithTransport):
489496
=== modified file 'bzrlib/workingtree.py'
--- bzrlib/workingtree.py 2012-07-18 20:06:21 +0000
+++ bzrlib/workingtree.py 2012-07-27 17:50:26 +0000
@@ -43,7 +43,6 @@
43import itertools43import itertools
44import operator44import operator
45import stat45import stat
46import re
4746
48from bzrlib import (47from bzrlib import (
49 branch,48 branch,
@@ -74,7 +73,6 @@
74# is guaranteed to be registered.73# is guaranteed to be registered.
75from bzrlib import (74from bzrlib import (
76 bzrdir,75 bzrdir,
77 symbol_versioning,
78 )76 )
7977
80from bzrlib.decorators import needs_read_lock, needs_write_lock78from bzrlib.decorators import needs_read_lock, needs_write_lock
@@ -95,6 +93,8 @@
95from bzrlib.trace import mutter, note93from bzrlib.trace import mutter, note
96from bzrlib.revision import CURRENT_REVISION94from bzrlib.revision import CURRENT_REVISION
97from bzrlib.symbol_versioning import (95from bzrlib.symbol_versioning import (
96 deprecated_in,
97 deprecated_method,
98 deprecated_passed,98 deprecated_passed,
99 DEPRECATED_PARAMETER,99 DEPRECATED_PARAMETER,
100 )100 )
@@ -1698,6 +1698,7 @@
1698 """1698 """
1699 raise NotImplementedError(self._walkdirs)1699 raise NotImplementedError(self._walkdirs)
17001700
1701 @deprecated_method(deprecated_in((2, 6, 0)))
1701 @needs_tree_write_lock1702 @needs_tree_write_lock
1702 def auto_resolve(self):1703 def auto_resolve(self):
1703 """Automatically resolve text conflicts according to contents.1704 """Automatically resolve text conflicts according to contents.
@@ -1711,23 +1712,14 @@
1711 """1712 """
1712 un_resolved = _mod_conflicts.ConflictList()1713 un_resolved = _mod_conflicts.ConflictList()
1713 resolved = _mod_conflicts.ConflictList()1714 resolved = _mod_conflicts.ConflictList()
1714 conflict_re = re.compile('^(<{7}|={7}|>{7})')
1715 for conflict in self.conflicts():1715 for conflict in self.conflicts():
1716 if (conflict.typestring != 'text conflict' or1716 try:
1717 self.kind(conflict.file_id) != 'file'):1717 conflict.action_auto(self)
1718 except NotImplementedError:
1718 un_resolved.append(conflict)1719 un_resolved.append(conflict)
1719 continue1720 else:
1720 my_file = open(self.id2abspath(conflict.file_id), 'rb')1721 conflict.cleanup(self)
1721 try:1722 resolved.append(conflict)
1722 for line in my_file:
1723 if conflict_re.search(line):
1724 un_resolved.append(conflict)
1725 break
1726 else:
1727 resolved.append(conflict)
1728 finally:
1729 my_file.close()
1730 resolved.remove_files(self)
1731 self.set_conflicts(un_resolved)1723 self.set_conflicts(un_resolved)
1732 return un_resolved, resolved1724 return un_resolved, resolved
17331725