Merge lp:~richard-wilbur/bzr/update-diff-format-parser into lp:bzr

[Richard Wilbur]

Split diff format option parser into a separate function, update to include all format options for GNU diff v3.2, and test parser. Fixes lp:1370435

Split diff format option parser into a separate function for testability, update to include all format options for GNU diff v3.2, and test parser.

This is needed because we default to 'unified' diff format if no other format is specified, but the parser didn't recognize several valid format specifiers. If we add '-u' (unified diff format) to a set of options already containing a valid format specifier, diff rightfully complains.

Also fixed a diagnostic message, an exception handler, some spelling and latest copyright dates.

Fixes lp:1370435

[Richard Wilbur (richard-wilbur)]

The changes are relatively trivial.

Can't reproduce the bug with these changes.

Passes the test suite.

+1

review: Approve

[Richard Wilbur (richard-wilbur)]

sent to pqm by email

=== modified file 'bzrlib/diff.py'
--- bzrlib/diff.py	2012-07-24 16:56:07 +0000
+++ bzrlib/diff.py	2014-09-16 17:36:26 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005-2011 Canonical Ltd.
+# Copyright (C) 2005-2014 Canonical Ltd.
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -119,7 +119,7 @@
 
 
 def _spawn_external_diff(diffcmd, capture_errors=True):
-    """Spawn the externall diff process, and return the child handle.
+    """Spawn the external diff process, and return the child handle.
 
     :param diffcmd: The command list to spawn
     :param capture_errors: Capture stderr as well as setting LANG=C
@@ -154,6 +154,40 @@
 
     return pipe
 
+# diff style options as of GNU diff v3.2
+style_option_list = ['-c', '-C', '--context',
+                     '-e', '--ed',
+                     '-f', '--forward-ed',
+                     '-q', '--brief',
+                     '--normal',
+                     '-n', '--rcs',
+                     '-u', '-U', '--unified',
+                     '-y', '--side-by-side',
+                     '-D', '--ifdef']
+
+def default_style_unified(diff_opts):
+    """Default to unified diff style if alternative not specified in diff_opts.
+
+        diff only allows one style to be specified; they don't override.
+        Note that some of these take optargs, and the optargs can be
+        directly appended to the options.
+        This is only an approximate parser; it doesn't properly understand
+        the grammar.
+
+    :param diff_opts: List of options for external (GNU) diff.
+    :return: List of options with default style=='unified'.
+    """
+    for s in style_option_list:
+        for j in diff_opts:
+            if j.startswith(s):
+                break
+        else:
+            continue
+        break
+    else:
+        diff_opts.append('-u')
+    return diff_opts
+
 
 def external_diff(old_filename, oldlines, new_filename, newlines, to_file,
                   diff_opts):
@@ -195,26 +229,7 @@
                    '--binary',
                   ]
 
-        # diff only allows one style to be specified; they don't override.
-        # note that some of these take optargs, and the optargs can be
-        # directly appended to the options.
-        # this is only an approximate parser; it doesn't properly understand
-        # the grammar.
-        for s in ['-c', '-u', '-C', '-U',
-                  '-e', '--ed',
-                  '-q', '--brief',
-                  '--normal',
-                  '-n', '--rcs',
-                  '-y', '--side-by-side',
-                  '-D', '--ifdef']:
-            for j in diff_opts:
-                if j.startswith(s):
-                    break
-            else:
-                continue
-            break
-        else:
-            diffcmd.append('-u')
+        diff_opts = default_style_unified(diff_opts)
 
         if diff_opts:
             diffcmd.extend(diff_opts)
@@ -265,7 +280,7 @@
                 msg = 'exit code %d' % rc
 
             raise errors.BzrError('external diff failed with %s; command: %r'
-                                  % (rc, diffcmd))
+                                  % (msg, diffcmd))
 
 
     finally:
@@ -282,7 +297,7 @@
                         old_abspath, e)
         try:
             os.remove(new_abspath)
-        except OSError:
+        except OSError, e:
             if e.errno not in (errno.ENOENT,):
                 warning('Failed to delete temporary file: %s %s',
                         new_abspath, e)

=== modified file 'bzrlib/tests/test_diff.py'
--- bzrlib/tests/test_diff.py	2012-07-12 15:59:35 +0000
+++ bzrlib/tests/test_diff.py	2014-09-16 17:36:26 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005-2011 Canonical Ltd
+# Copyright (C) 2005-2014 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -143,6 +143,19 @@
                           'old', ['boo\n'], 'new', ['goo\n'],
                           StringIO(), diff_opts=['-u'])
 
+    def test_default_style_unified(self):
+        """Check for default style '-u' only if no other style specified
+        in 'diff-options'.
+        """
+        # Verify that style defaults to unified, id est '-u' appended
+        # to option list, in the absence of an alternative style.
+        self.assertEqual(['-a', '-u'], diff.default_style_unified(["-a"]))
+        # Verify that for all valid style options, '-u' is not
+        # appended to option list.
+        for s in diff.style_option_list:
+            ret_opts = diff.default_style_unified(diff_opts=["%s" % (s,)])
+            self.assertEqual(["%s" % (s,)], ret_opts)
+
     def test_internal_diff_default(self):
         # Default internal diff encoding is utf8
         output = StringIO()
@@ -1391,7 +1404,7 @@
         diff_obj._execute('old', 'new')
         self.assertEqual(output.getvalue().rstrip(), 'old new')
 
-    def test_excute_missing(self):
+    def test_execute_missing(self):
         diff_obj = diff.DiffFromTool(['a-tool-which-is-unlikely-to-exist'],
                                      None, None, None)
         self.addCleanup(diff_obj.finish)