Merge lp:~jelmer/brz/resolve-auto-refactor into lp:brz/3.0

[Jelmer Vernooij]

Create a new kind of conflict resolution action named 'auto'.

Create 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.

(from lp:~gz/bzr/resolve_auto_refactor)

[Martin Packman (gz)]

Thanks, some wally should probably have landed this long ago.

review: Approve

[The Breezy Bot (the-breezy-bot)]

Running landing tests failed
https://ci.breezy-vcs.org/job/brz-3.0/job/brz-3.0-land/46/

=== modified file 'breezy/conflicts.py'
--- breezy/conflicts.py	2018-11-11 04:08:32 +0000
+++ breezy/conflicts.py	2019-06-16 16:47:08 +0000
@@ -20,6 +20,7 @@
 from __future__ import absolute_import
 
 import os
+import re
 
 from .lazy_import import lazy_import
 lazy_import(globals(), """
@@ -84,6 +85,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',
@@ -137,38 +140,27 @@
         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(text_type(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(text_type(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))
@@ -453,6 +445,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.
@@ -651,6 +646,8 @@
 
     rformat = '%(class)s(%(path)r, %(file_id)r)'
 
+    _conflict_re = re.compile(b'^(<{7}|={7}|>{7})')
+
     def associated_filenames(self):
         return [self.path + suffix for suffix in CONFLICT_SUFFIXES]
 
@@ -681,6 +678,22 @@
         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.
+        try:
+            kind = tree.kind(self.path)
+        except errors.NoSuchFile:
+            return
+        if kind != '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.path) 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 'breezy/tests/test_workingtree.py'
--- breezy/tests/test_workingtree.py	2018-11-12 01:41:38 +0000
+++ breezy/tests/test_workingtree.py	2019-06-16 16:47:08 +0000
@@ -18,6 +18,7 @@
 from .. import (
     conflicts,
     errors,
+    symbol_versioning,
     transport,
     workingtree,
     )
@@ -405,6 +406,12 @@
 
 class TestAutoResolve(TestCaseWithTransport):
 
+    def _auto_resolve(self, tree):
+        """Call auto_resolve on tree expecting deprecation"""
+        return self.applyDeprecated(
+            symbol_versioning.deprecated_in((3, 0, 1)),
+            tree.auto_resolve,)
+
     def test_auto_resolve(self):
         base = self.make_branch_and_tree('base')
         self.build_tree_contents([('base/hello', b'Hello')])
@@ -420,24 +427,24 @@
         this.merge_from_branch(other.branch)
         self.assertEqual([conflicts.TextConflict('hello', b'hello_id')],
                          this.conflicts())
-        this.auto_resolve()
-        self.assertEqual([conflicts.TextConflict('hello', b'hello_id')],
-                         this.conflicts())
-        self.build_tree_contents([('this/hello', b'<<<<<<<')])
-        this.auto_resolve()
-        self.assertEqual([conflicts.TextConflict('hello', b'hello_id')],
-                         this.conflicts())
-        self.build_tree_contents([('this/hello', b'=======')])
-        this.auto_resolve()
-        self.assertEqual([conflicts.TextConflict('hello', b'hello_id')],
-                         this.conflicts())
-        self.build_tree_contents([('this/hello', b'\n>>>>>>>')])
-        remaining, resolved = this.auto_resolve()
+        self._auto_resolve(this)
+        self.assertEqual([conflicts.TextConflict('hello', b'hello_id')],
+                         this.conflicts())
+        self.build_tree_contents([('this/hello', '<<<<<<<')])
+        self._auto_resolve(this)
+        self.assertEqual([conflicts.TextConflict('hello', b'hello_id')],
+                         this.conflicts())
+        self.build_tree_contents([('this/hello', '=======')])
+        self._auto_resolve(this)
+        self.assertEqual([conflicts.TextConflict('hello', b'hello_id')],
+                         this.conflicts())
+        self.build_tree_contents([('this/hello', '\n>>>>>>>')])
+        remaining, resolved = self._auto_resolve(this)
         self.assertEqual([conflicts.TextConflict('hello', b'hello_id')],
                          this.conflicts())
         self.assertEqual([], resolved)
         self.build_tree_contents([('this/hello', b'hELLO wORLD')])
-        remaining, resolved = this.auto_resolve()
+        remaining, resolved = self._auto_resolve(this)
         self.assertEqual([], this.conflicts())
         self.assertEqual([conflicts.TextConflict('hello', b'hello_id')],
                          resolved)
@@ -447,9 +454,23 @@
         tree = self.make_branch_and_tree('tree')
         self.build_tree(['tree/hello/'])
         tree.add('hello', b'hello-id')
-        file_conflict = conflicts.TextConflict('file', b'hello-id')
-        tree.set_conflicts(conflicts.ConflictList([file_conflict]))
-        tree.auto_resolve()
+        file_conflict = conflicts.TextConflict('hello', b'hello-id')
+        tree.set_conflicts(conflicts.ConflictList([file_conflict]))
+        remaining, resolved = self._auto_resolve(tree)
+        self.assertEqual(
+            remaining,
+            conflicts.ConflictList([conflicts.TextConflict(u'hello', 'hello-id')]))
+        self.assertEqual(resolved, [])
+
+    def test_auto_resolve_missing(self):
+        tree = self.make_branch_and_tree('tree')
+        file_conflict = conflicts.TextConflict('hello', b'hello-id')
+        tree.set_conflicts(conflicts.ConflictList([file_conflict]))
+        remaining, resolved = self._auto_resolve(tree)
+        self.assertEqual(remaining, [])
+        self.assertEqual(
+            resolved,
+            conflicts.ConflictList([conflicts.TextConflict(u'hello', 'hello-id')]))
 
 
 class TestFindTrees(TestCaseWithTransport):

=== modified file 'breezy/workingtree.py'
--- breezy/workingtree.py	2019-02-14 22:18:59 +0000
+++ breezy/workingtree.py	2019-06-16 16:47:08 +0000
@@ -31,7 +31,6 @@
 
 import errno
 import os
-import re
 import sys
 
 import breezy
@@ -62,6 +61,7 @@
     )
 from .i18n import gettext
 from . import mutabletree
+from .symbol_versioning import deprecated_method, deprecated_in
 from .trace import mutter, note
 
 
@@ -1271,6 +1271,7 @@
         """
         raise NotImplementedError(self.walkdirs)
 
+    @deprecated_method(deprecated_in((3, 0, 1)))
     def auto_resolve(self):
         """Automatically resolve text conflicts according to contents.
 
@@ -1284,21 +1285,14 @@
         with self.lock_tree_write():
             un_resolved = _mod_conflicts.ConflictList()
             resolved = _mod_conflicts.ConflictList()
-            conflict_re = re.compile(b'^(<{7}|={7}|>{7})')
             for conflict in self.conflicts():
-                path = self.id2path(conflict.file_id)
-                if (conflict.typestring != 'text conflict' or
-                        self.kind(path) != 'file'):
+                try:
+                    conflict.action_auto(self)
+                except NotImplementedError:
                     un_resolved.append(conflict)
-                    continue
-                with open(self.abspath(path), 'rb') as my_file:
-                    for line in my_file:
-                        if conflict_re.search(line):
-                            un_resolved.append(conflict)
-                            break
-                    else:
-                        resolved.append(conflict)
-            resolved.remove_files(self)
+                else:
+                    conflict.cleanup(self)
+                    resolved.append(conflict)
             self.set_conflicts(un_resolved)
             return un_resolved, resolved
 

=== modified file 'doc/en/release-notes/brz-3.0.txt'
--- doc/en/release-notes/brz-3.0.txt	2019-06-15 18:57:07 +0000
+++ doc/en/release-notes/brz-3.0.txt	2019-06-16 16:47:08 +0000
@@ -45,6 +45,9 @@
 * Print full upgrade command to run when complaining about lack of
   support for tags. (Jelmer Vernooij, #163908)
 
+* Refactor auto conflict handling and add a ``--auto`` flag
+  to ``bzr resolve``. (Martin Packman, #688506)
+
 Documentation
 *************