Merge lp:~vila/bzr/split-diff-tests into lp:bzr

[Vincent Ladeuil]

Split some tests to be able to get finer grained failures

Some simple refactoring to get finer grained failures from tests.

I also noted that --forward-ed is not part of diff 3.3 and I'm wondering if
we really want to keep that option.

[Richard Wilbur (richard-wilbur)]

Thanks, Vincent, for the improved granularity and specificity of the tests. The use of scenarios is powerful!

Regarding '--forward-ed' not part of diff v3.3:
How does diff v3.3 treat a command line which includes the removed option '--forward-ed'? Does it quietly ignore it or abort with an error code?

In either case, since the user is explicitly specifying the option, it seems benign to continue to regard it as a valid format option--at least as long as we are still supporting diff v3.2.

The other question to consider is, "How long would it be useful to support the diff v3.2 behaviour?" Ubuntu 12.04 has diff v3.2 while the first Ubuntu Linux release which includes diff v3.3 is 13.10.

1. If diff v3.3 quietly ignores the removed option, then we might regard it as important to determine which version of GNU diff is installed in order to either warn the user that this option no longer specifies a format or substitute '-u'. Maybe both.
2. If diff v3.3 aborts with an error message, then this will likely be a self-correcting problem--the user who explicitly specified the option will learn it is no longer supported and cease specifying that option on next invocation.

Regardless of these considerations, I approve of this patch.

+1

review: Approve

[Vincent Ladeuil (vila)]

> Thanks, Vincent, for the improved granularity and specificity of the tests.
> The use of scenarios is powerful!
>
> Regarding '--forward-ed' not part of diff v3.3:
> How does diff v3.3 treat a command line which includes the removed option
> '--forward-ed'? Does it quietly ignore it or abort with an error code?

Good point ! The option appears to be still supported even if it's not mentioned in the man page nor the online help anymore.

>
> In either case, since the user is explicitly specifying the option, it seems
> benign to continue to regard it as a valid format option--at least as long as
> we are still supporting diff v3.2.

Agreed, I've removed the comment.

And for the future, I think you're right, once the option is not supported by diff anymore, our users will soon enough discover that the issue is with diff itself.

[Vincent Ladeuil (vila)]

sent to pqm by email

[Richard Wilbur (richard-wilbur)]

On Mon, Oct 6, 2014 at 6:35 AM, Vincent Ladeuil <email address hidden> wrote:
>> Regarding '--forward-ed' not part of diff v3.3:
>> How does diff v3.3 treat a command line which includes the removed option
>> '--forward-ed'? Does it quietly ignore it or abort with an error code?
>
> Good point ! The option appears to be still supported even if it's not mentioned in the man page nor the online help anymore.

I posted a documentation regression bug (#18655) on the bug-diffutils
mailing list. The thread is in their archives[*]. They said they
dropped documentation from shorter manuals because they thought no one
in their right mind would use the option but that it is still
documented in the full manual.

Reference:
[*] http://lists.gnu.org/archive/html/bug-diffutils/2014-10/index.html

[Vincent Ladeuil (vila)]

Wow ! Thanks for tracking this !

=== modified file 'bzrlib/diff.py'
--- bzrlib/diff.py	2014-09-16 13:42:23 +0000
+++ bzrlib/diff.py	2014-10-06 12:35:04 +0000
@@ -286,21 +286,19 @@
     finally:
         oldtmpf.close()                 # and delete
         newtmpf.close()
-        # Clean up. Warn in case the files couldn't be deleted
-        # (in case windows still holds the file open, but not
-        # if the files have already been deleted)
-        try:
-            os.remove(old_abspath)
-        except OSError, e:
-            if e.errno not in (errno.ENOENT,):
-                warning('Failed to delete temporary file: %s %s',
-                        old_abspath, e)
-        try:
-            os.remove(new_abspath)
-        except OSError, e:
-            if e.errno not in (errno.ENOENT,):
-                warning('Failed to delete temporary file: %s %s',
-                        new_abspath, e)
+
+        def cleanup(path):
+            # Warn in case the file couldn't be deleted (in case windows still
+            # holds the file open, but not if the files have already been
+            # deleted)
+            try:
+                os.remove(path)
+            except OSError, e:
+                if e.errno not in (errno.ENOENT,):
+                    warning('Failed to delete temporary file: %s %s', path, e)
+
+        cleanup(old_abspath)
+        cleanup(new_abspath)
 
 
 def get_trees_and_branches_to_diff_locked(
 
=== modified file 'bzrlib/tests/test_diff.py'
--- bzrlib/tests/test_diff.py	2014-09-16 13:42:23 +0000
+++ bzrlib/tests/test_diff.py	2014-10-06 12:35:04 +0000
@@ -17,7 +17,6 @@
 import os
 from cStringIO import StringIO
 import subprocess
-import sys
 import tempfile
 
 from bzrlib import (
@@ -32,12 +31,15 @@
     tests,
     transform,
     )
-from bzrlib.symbol_versioning import deprecated_in
-from bzrlib.tests import features, EncodingAdapter
-from bzrlib.tests.blackbox.test_diff import subst_dates
 from bzrlib.tests import (
     features,
-    )
+    EncodingAdapter,
+)
+from bzrlib.tests.blackbox.test_diff import subst_dates
+from bzrlib.tests.scenarios import load_tests_apply_scenarios
+
+
+load_tests = load_tests_apply_scenarios
 
 
 def udiff_lines(old, new, allow_binary=False):
@@ -63,6 +65,29 @@
     return lines
 
 
+class TestDiffOptions(tests.TestCase):
+
+    def test_unified_added(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']))
+
+
+class TestDiffOptionsScenarios(tests.TestCase):
+
+    scenarios = [(s, dict(style=s)) for s in diff.style_option_list]
+    style = None # Set by load_tests_apply_scenarios from scenarios
+
+    def test_unified_not_added(self):
+        # Verify that for all valid style options, '-u' is not
+        # appended to option list.
+        ret_opts = diff.default_style_unified(diff_opts=["%s" % (self.style,)])
+        self.assertEqual(["%s" % (self.style,)], ret_opts)
+
+
 class TestDiff(tests.TestCase):
 
     def test_add_nl(self):
@@ -143,19 +168,6 @@
                           '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()
@@ -1484,7 +1496,6 @@
     def test_encodable_filename(self):
         # Just checks file path for external diff tool.
         # We cannot change CPython's internal encoding used by os.exec*.
-        import sys
         diffobj = diff.DiffFromTool(['dummy', '@old_path', '@new_path'],
                                     None, None, None)
         for _, scenario in EncodingAdapter.encoding_scenarios:
@@ -1502,7 +1513,6 @@
             self.assert_(fullpath.startswith(diffobj._root + '/safe'))
 
     def test_unencodable_filename(self):
-        import sys
         diffobj = diff.DiffFromTool(['dummy', '@old_path', '@new_path'],
                                     None, None, None)
         for _, scenario in EncodingAdapter.encoding_scenarios: