Merge lp:~jelmer/brz/change-editor-consistent into lp:brz

[Jelmer Vernooij]

Use standard syntax for the ``change_editor`` configuration option.

Use standard syntax for the ``change_editor`` configuration option.

Use {old_path} and {new_path} rather than @old_path and @new_path.

[Martin Packman (gz)]

Makes sense, thanks!

review: Approve

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

Merging failed
https://ci.breezy-vcs.org/job/brz/job/brz-land/433/

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

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

=== modified file 'breezy/builtins.py'
--- breezy/builtins.py	2019-06-29 09:30:39 +0000
+++ breezy/builtins.py	2019-07-07 17:32:39 +0000
@@ -6571,10 +6571,10 @@
     editor program to decide what the file remaining in the working copy
     should look like.  To do this, add the configuration option
 
-        change_editor = PROGRAM @new_path @old_path
+        change_editor = PROGRAM {new_path} {old_path}
 
-    where @new_path is replaced with the path of the new version of the
-    file and @old_path is replaced with the path of the old version of
+    where {new_path} is replaced with the path of the new version of the
+    file and {old_path} is replaced with the path of the old version of
     the file.  The PROGRAM should save the new file with the desired
     contents of the file in the working tree.
 
 
=== modified file 'breezy/config.py'
--- breezy/config.py	2019-06-22 11:51:56 +0000
+++ breezy/config.py	2019-07-07 17:32:39 +0000
@@ -91,6 +91,7 @@
 
 from breezy import (
     atomicfile,
+    cmdline,
     controldir,
     debug,
     directory_service,
@@ -328,6 +329,11 @@
         cmd = self._get_change_editor()
         if cmd is None:
             return None
+        cmd = cmd.replace('@old_path', '{old_path}')
+        cmd = cmd.replace('@new_path', '{new_path}')
+        cmd = cmdline.split(cmd)
+        if '{old_path}' not in cmd:
+            cmd.extend(['{old_path}', '{new_path}'])
         return diff.DiffFromTool.from_string(cmd, old_tree, new_tree,
                                              sys.stdout)
 
@@ -831,7 +837,7 @@
         return POLICY_NONE
 
     def _get_change_editor(self):
-        return self.get_user_option('change_editor')
+        return self.get_user_option('change_editor', expand=False)
 
     def _get_signature_checking(self):
         """See Config._get_signature_checking."""
 
=== modified file 'breezy/diff.py'
--- breezy/diff.py	2019-06-15 17:53:40 +0000
+++ breezy/diff.py	2019-07-07 17:32:39 +0000
@@ -19,7 +19,6 @@
 import difflib
 import os
 import re
-import string
 import sys
 
 from .lazy_import import lazy_import
@@ -30,7 +29,6 @@
 import tempfile
 
 from breezy import (
-    cmdline,
     controldir,
     errors,
     osutils,
@@ -54,12 +52,6 @@
 DEFAULT_CONTEXT_AMOUNT = 3
 
 
-class AtTemplate(string.Template):
-    """Templating class that uses @ instead of $."""
-
-    delimiter = '@'
-
-
 # TODO: Rather than building a changeset object, we should probably
 # invoke callbacks on an object.  That object can either accumulate a
 # list, write them out directly, etc etc.
@@ -790,11 +782,8 @@
         self._root = osutils.mkdtemp(prefix='brz-diff-')
 
     @classmethod
-    def from_string(klass, command_string, old_tree, new_tree, to_file,
+    def from_string(klass, command_template, old_tree, new_tree, to_file,
                     path_encoding='utf-8'):
-        command_template = cmdline.split(command_string)
-        if '@' not in command_string:
-            command_template.extend(['@old_path', '@new_path'])
         return klass(command_template, old_tree, new_tree, to_file,
                      path_encoding)
 
@@ -810,7 +799,7 @@
 
     def _get_command(self, old_path, new_path):
         my_map = {'old_path': old_path, 'new_path': new_path}
-        command = [AtTemplate(t).substitute(my_map) for t in
+        command = [t.format(**my_map) for t in
                    self.command_template]
         if sys.platform == 'win32':  # Popen doesn't accept unicode on win32
             command_encoded = []
 
=== modified file 'breezy/tests/test_config.py'
--- breezy/tests/test_config.py	2019-06-16 01:03:51 +0000
+++ breezy/tests/test_config.py	2019-07-07 17:32:39 +0000
@@ -191,7 +191,7 @@
 [DEFAULT]
 email=Erik B\u00e5gfors <[e-mail]>
 editor=vim
-change_editor=vimdiff -of @new_path @old_path
+change_editor=vimdiff -of {new_path} {old_path}
 gpg_signing_key=DD4D5088
 log_format=short
 validate_signatures_in_log=true
@@ -395,6 +395,7 @@
         super(InstrumentedConfig, self).__init__()
         self._calls = []
         self._signatures = config.CHECK_NEVER
+        self._change_editor = 'vimdiff -fo {new_path} {old_path}'
 
     def _get_user_id(self):
         self._calls.append('_get_user_id')
@@ -406,7 +407,7 @@
 
     def _get_change_editor(self):
         self._calls.append('_get_change_editor')
-        return 'vimdiff -fo @new_path @old_path'
+        return self._change_editor
 
 
 bool_config = b"""[DEFAULT]
@@ -514,7 +515,28 @@
         change_editor = my_config.get_change_editor('old_tree', 'new_tree')
         self.assertEqual(['_get_change_editor'], my_config._calls)
         self.assertIs(diff.DiffFromTool, change_editor.__class__)
-        self.assertEqual(['vimdiff', '-fo', '@new_path', '@old_path'],
+        self.assertEqual(['vimdiff', '-fo', '{new_path}', '{old_path}'],
+                         change_editor.command_template)
+
+    def test_get_change_editor_implicit_args(self):
+        # If there are no substitution variables, then assume the
+        # old and new path are the last arguments.
+        my_config = InstrumentedConfig()
+        my_config._change_editor = 'vimdiff -o'
+        change_editor = my_config.get_change_editor('old_tree', 'new_tree')
+        self.assertEqual(['_get_change_editor'], my_config._calls)
+        self.assertIs(diff.DiffFromTool, change_editor.__class__)
+        self.assertEqual(['vimdiff', '-o', '{old_path}', '{new_path}'],
+                         change_editor.command_template)
+
+    def test_get_change_editor_old_style(self):
+        # Test the old style format for the change_editor setting.
+        my_config = InstrumentedConfig()
+        my_config._change_editor = 'vimdiff -o @old_path @new_path'
+        change_editor = my_config.get_change_editor('old_tree', 'new_tree')
+        self.assertEqual(['_get_change_editor'], my_config._calls)
+        self.assertIs(diff.DiffFromTool, change_editor.__class__)
+        self.assertEqual(['vimdiff', '-o', '{old_path}', '{new_path}'],
                          change_editor.command_template)
 
 
@@ -1101,7 +1123,7 @@
         my_config = self._get_sample_config()
         change_editor = my_config.get_change_editor('old', 'new')
         self.assertIs(diff.DiffFromTool, change_editor.__class__)
-        self.assertEqual('vimdiff -of @new_path @old_path',
+        self.assertEqual('vimdiff -of {new_path} {old_path}',
                          ' '.join(change_editor.command_template))
 
     def test_get_no_change_editor(self):
 
=== modified file 'breezy/tests/test_diff.py'
--- breezy/tests/test_diff.py	2019-06-17 23:08:34 +0000
+++ breezy/tests/test_diff.py	2019-07-07 17:32:39 +0000
@@ -867,26 +867,28 @@
 class TestDiffFromTool(tests.TestCaseWithTransport):
 
     def test_from_string(self):
-        diff_obj = diff.DiffFromTool.from_string('diff', None, None, None)
+        diff_obj = diff.DiffFromTool.from_string(
+            ['diff', '{old_path}', '{new_path}'],
+            None, None, None)
         self.addCleanup(diff_obj.finish)
-        self.assertEqual(['diff', '@old_path', '@new_path'],
+        self.assertEqual(['diff', '{old_path}', '{new_path}'],
                          diff_obj.command_template)
 
     def test_from_string_u5(self):
-        diff_obj = diff.DiffFromTool.from_string('diff "-u 5"',
-                                                 None, None, None)
+        diff_obj = diff.DiffFromTool.from_string(
+            ['diff', "-u 5", '{old_path}', '{new_path}'], None, None, None)
         self.addCleanup(diff_obj.finish)
-        self.assertEqual(['diff', '-u 5', '@old_path', '@new_path'],
+        self.assertEqual(['diff', '-u 5', '{old_path}', '{new_path}'],
                          diff_obj.command_template)
         self.assertEqual(['diff', '-u 5', 'old-path', 'new-path'],
                          diff_obj._get_command('old-path', 'new-path'))
 
     def test_from_string_path_with_backslashes(self):
         self.requireFeature(features.backslashdir_feature)
-        tool = 'C:\\Tools\\Diff.exe'
+        tool = ['C:\\Tools\\Diff.exe', '{old_path}', '{new_path}']
         diff_obj = diff.DiffFromTool.from_string(tool, None, None, None)
         self.addCleanup(diff_obj.finish)
-        self.assertEqual(['C:\\Tools\\Diff.exe', '@old_path', '@new_path'],
+        self.assertEqual(['C:\\Tools\\Diff.exe', '{old_path}', '{new_path}'],
                          diff_obj.command_template)
         self.assertEqual(['C:\\Tools\\Diff.exe', 'old-path', 'new-path'],
                          diff_obj._get_command('old-path', 'new-path'))
@@ -894,7 +896,7 @@
     def test_execute(self):
         output = BytesIO()
         diff_obj = diff.DiffFromTool([sys.executable, '-c',
-                                      'print("@old_path @new_path")'],
+                                      'print("{old_path} {new_path}")'],
                                      None, None, output)
         self.addCleanup(diff_obj.finish)
         diff_obj._execute('old', 'new')
@@ -922,7 +924,7 @@
         basis_tree.lock_read()
         self.addCleanup(basis_tree.unlock)
         diff_obj = diff.DiffFromTool([sys.executable, '-c',
-                                      'print "@old_path @new_path"'],
+                                      'print "{old_path} {new_path}"'],
                                      basis_tree, tree, output)
         diff_obj._prepare_files('file', 'file', file_id=b'file-id')
         # The old content should be readonly
@@ -958,7 +960,7 @@
         tree.lock_read()
         self.addCleanup(tree.unlock)
         diff_obj = diff.DiffFromTool([sys.executable, '-c',
-                                      'print "@old_path @new_path"'],
+                                      'print "{old_path} {new_path}"'],
                                      old_tree, tree, output)
         self.addCleanup(diff_obj.finish)
         self.assertContainsRe(diff_obj._root, 'brz-diff-[^/]*')
@@ -980,7 +982,7 @@
     def test_encodable_filename(self):
         # Just checks file path for external diff tool.
         # We cannot change CPython's internal encoding used by os.exec*.
-        diffobj = diff.DiffFromTool(['dummy', '@old_path', '@new_path'],
+        diffobj = diff.DiffFromTool(['dummy', '{old_path}', '{new_path}'],
                                     None, None, None)
         for _, scenario in EncodingAdapter.encoding_scenarios:
             encoding = scenario['encoding']
@@ -995,7 +997,7 @@
             self.assertTrue(fullpath.startswith(diffobj._root + '/safe'))
 
     def test_unencodable_filename(self):
-        diffobj = diff.DiffFromTool(['dummy', '@old_path', '@new_path'],
+        diffobj = diff.DiffFromTool(['dummy', '{old_path}', '{new_path}'],
                                     None, None, None)
         for _, scenario in EncodingAdapter.encoding_scenarios:
             encoding = scenario['encoding']
 
=== modified file 'doc/en/release-notes/brz-3.1.txt'
--- doc/en/release-notes/brz-3.1.txt	2019-07-07 16:54:07 +0000
+++ doc/en/release-notes/brz-3.1.txt	2019-07-07 17:32:39 +0000
@@ -54,6 +54,12 @@
 * Automatically upgrade to branch format 8 when setting branch references.
   (Jelmer Vernooij)
 
+* The substitution variables for the ``change_editor`` configuration
+  option are now "{old_path}" and "{new_path}" rather than "@old_path" and
+  "@new_path". The former is more consistent with the way substitutions
+  work in other configuration options. The old syntax is still supported.
+  (Jelmer Vernooij, #708718)
+
 * The ``brz inventory`` command now accepts a ``--include-root``
   argument to show the tree root. (Jelmer Vernooij)