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

[Martin Packman]

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.

[Vincent Ladeuil (vila)]

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

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

=== 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 @@
 from __future__ import absolute_import
 
 import os
+import re
 
 from bzrlib.lazy_import import lazy_import
 lazy_import(globals(), """
@@ -82,6 +83,8 @@
 
 
 resolve_action_registry.register(
+    'auto', 'auto', 'Detect whether conflict has been resolved by user.')
+resolve_action_registry.register(
     'done', 'done', 'Marks the conflict as resolved.')
 resolve_action_registry.register(
     'take-this', 'take_this',
@@ -133,38 +136,26 @@
         else:
             tree, file_list = workingtree.WorkingTree.open_containing_paths(
                 file_list, directory)
-            if file_list is None:
-                if action is None:
-                    # FIXME: There is a special case here related to the option
-                    # handling that could be clearer and easier to discover by
-                    # providing an --auto action (bug #344013 and #383396) and
-                    # make it mandatory instead of implicit and active only
-                    # when no file_list is provided -- vila 091229
+            if action is None:
+                if file_list is None:
                     action = 'auto'
-            else:
-                if action is None:
+                else:
                     action = 'done'
-        if action == 'auto':
-            if file_list is None:
-                un_resolved, resolved = tree.auto_resolve()
-                if len(un_resolved) > 0:
-                    trace.note(ngettext('%d conflict auto-resolved.',
-                        '%d conflicts auto-resolved.', len(resolved)),
-                        len(resolved))
-                    trace.note(gettext('Remaining conflicts:'))
-                    for conflict in un_resolved:
-                        trace.note(unicode(conflict))
-                    return 1
-                else:
-                    trace.note(gettext('All conflicts resolved.'))
-                    return 0
+        before, after = resolve(tree, file_list, action=action)
+        # GZ 2012-07-27: Should unify UI below now that auto is less magical.
+        if action == 'auto' and file_list is None:
+            if after > 0:
+                trace.note(ngettext('%d conflict auto-resolved.',
+                    '%d conflicts auto-resolved.', before - after),
+                    before - after)
+                trace.note(gettext('Remaining conflicts:'))
+                for conflict in tree.conflicts():
+                    trace.note(unicode(conflict))
+                return 1
             else:
-                # FIXME: This can never occur but the block above needs some
-                # refactoring to transfer tree.auto_resolve() to
-                # conflict.auto(tree) --vila 091242
-                pass
+                trace.note(gettext('All conflicts resolved.'))
+                return 0
         else:
-            before, after = resolve(tree, file_list, action=action)
             trace.note(ngettext('{0} conflict resolved, {1} remaining',
                                 '{0} conflicts resolved, {1} remaining',
                                 before-after).format(before - after, after))
@@ -442,6 +433,9 @@
                 if e.errno != errno.ENOENT:
                     raise
 
+    def action_auto(self, tree):
+        raise NotImplementedError(self.action_auto)
+
     def action_done(self, tree):
         """Mark the conflict as solved once it has been handled."""
         # This method does nothing but simplifies the design of upper levels.
@@ -639,6 +633,8 @@
 
     rformat = '%(class)s(%(path)r, %(file_id)r)'
 
+    _conflict_re = re.compile('^(<{7}|={7}|>{7})')
+
     def associated_filenames(self):
         return [self.path + suffix for suffix in CONFLICT_SUFFIXES]
 
@@ -668,6 +664,18 @@
         tt.version_file(self.file_id, winner_tid)
         tt.apply()
 
+    def action_auto(self, tree):
+        # GZ 2012-07-27: Using NotImplementedError to signal that a conflict
+        #                can't be auto resolved does not seem ideal.
+        if tree.kind(self.file_id) != 'file':
+            raise NotImplementedError("Conflict is not a file")
+        conflict_markers_in_line = self._conflict_re.search
+        # GZ 2012-07-27: What if not tree.has_id(self.file_id) due to removal?
+        with tree.get_file(self.file_id) as f:
+            for line in f:
+                if conflict_markers_in_line(line):
+                    raise NotImplementedError("Conflict markers present")
+
     def action_take_this(self, tree):
         self._resolve_with_cleanups(tree, 'THIS')
 

=== 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 @@
     bzrdir,
     conflicts,
     errors,
+    symbol_versioning,
     transport,
     workingtree,
     workingtree_3,
@@ -438,6 +439,12 @@
 
 class TestAutoResolve(TestCaseWithTransport):
 
+    def _auto_resolve(self, tree):
+        """Call auto_resolve on tree expecting deprecation"""
+        return self.applyDeprecated(
+            symbol_versioning.deprecated_in((2, 6, 0)),
+            tree.auto_resolve,)
+
     def test_auto_resolve(self):
         base = self.make_branch_and_tree('base')
         self.build_tree_contents([('base/hello', 'Hello')])
@@ -453,24 +460,24 @@
         this.merge_from_branch(other.branch)
         self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
                          this.conflicts())
-        this.auto_resolve()
+        self._auto_resolve(this)
         self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
                          this.conflicts())
         self.build_tree_contents([('this/hello', '<<<<<<<')])
-        this.auto_resolve()
+        self._auto_resolve(this)
         self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
                          this.conflicts())
         self.build_tree_contents([('this/hello', '=======')])
-        this.auto_resolve()
+        self._auto_resolve(this)
         self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
                          this.conflicts())
         self.build_tree_contents([('this/hello', '\n>>>>>>>')])
-        remaining, resolved = this.auto_resolve()
+        remaining, resolved = self._auto_resolve(this)
         self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
                          this.conflicts())
         self.assertEqual([], resolved)
         self.build_tree_contents([('this/hello', 'hELLO wORLD')])
-        remaining, resolved = this.auto_resolve()
+        remaining, resolved = self._auto_resolve(this)
         self.assertEqual([], this.conflicts())
         self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
                          resolved)
@@ -482,7 +489,7 @@
         tree.add('hello', 'hello-id')
         file_conflict = conflicts.TextConflict('file', 'hello-id')
         tree.set_conflicts(conflicts.ConflictList([file_conflict]))
-        tree.auto_resolve()
+        self._auto_resolve(tree)
 
 
 class TestFindTrees(TestCaseWithTransport):

=== 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 @@
 import itertools
 import operator
 import stat
-import re
 
 from bzrlib import (
     branch,
@@ -74,7 +73,6 @@
 # is guaranteed to be registered.
 from bzrlib import (
     bzrdir,
-    symbol_versioning,
     )
 
 from bzrlib.decorators import needs_read_lock, needs_write_lock
@@ -95,6 +93,8 @@
 from bzrlib.trace import mutter, note
 from bzrlib.revision import CURRENT_REVISION
 from bzrlib.symbol_versioning import (
+    deprecated_in,
+    deprecated_method,
     deprecated_passed,
     DEPRECATED_PARAMETER,
     )
@@ -1698,6 +1698,7 @@
         """
         raise NotImplementedError(self._walkdirs)
 
+    @deprecated_method(deprecated_in((2, 6, 0)))
     @needs_tree_write_lock
     def auto_resolve(self):
         """Automatically resolve text conflicts according to contents.
@@ -1711,23 +1712,14 @@
         """
         un_resolved = _mod_conflicts.ConflictList()
         resolved = _mod_conflicts.ConflictList()
-        conflict_re = re.compile('^(<{7}|={7}|>{7})')
         for conflict in self.conflicts():
-            if (conflict.typestring != 'text conflict' or
-                self.kind(conflict.file_id) != 'file'):
+            try:
+                conflict.action_auto(self)
+            except NotImplementedError:
                 un_resolved.append(conflict)
-                continue
-            my_file = open(self.id2abspath(conflict.file_id), 'rb')
-            try:
-                for line in my_file:
-                    if conflict_re.search(line):
-                        un_resolved.append(conflict)
-                        break
-                else:
-                    resolved.append(conflict)
-            finally:
-                my_file.close()
-        resolved.remove_files(self)
+            else:
+                conflict.cleanup(self)
+                resolved.append(conflict)
         self.set_conflicts(un_resolved)
         return un_resolved, resolved