Improve --diff output when files lack trailing newlines

The behavior now matches GNU diff.

Fixes #14094.

Example of output before this change:

    TASK [healthchecks.io : hourly healthchecks.io ping] ***************************
    changed: [ranka]
    --- before: /etc/cron.hourly/mg-healthchecks-dot-io
    +++ after: /tmp/tmpOTvXTw
    @@ -1,2 +1,2 @@
     #!/bin/sh
    -curl -sS https://hchk.io/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx > /dev/null+curl -sS https://hchk.io/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx > /dev/null

after this change:

    TASK [healthchecks.io : hourly healthchecks.io ping] ***************************
    changed: [ranka]
    --- before: /etc/cron.hourly/mg-healthchecks-dot-io
    +++ after: /tmp/tmpOTvXTw
    @@ -1,2 +1,2 @@
     #!/bin/sh
    -curl -sS https://hchk.io/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx > /dev/null
    \ No newline at end of file
    +curl -sS https://hchk.io/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx > /dev/null

The added unit tests contain more examples.

This commit also takes care to avoid "no newline at EOF" warnings when
no_log is in effect, and also when modules return dicts rather than
strings.  (It also removes trailing whitespace from using json
serialization when diffing dicts, because I hate trailing whitespace in
Python source files, even if they're test files.)

lib/ansible/plugins/action/__init__.py

@@ -864,7 +864,7 @@ class ActionBase(with_metaclass(ABCMeta, object)):
             if 'before' in diff:
                 diff["before"] = ""
             if 'after' in diff:
-                diff["after"] = " [[ Diff output has been hidden because 'no_log: true' was specified for this result ]]"
+                diff["after"] = " [[ Diff output has been hidden because 'no_log: true' was specified for this result ]]\n"
 
         return diff
 

lib/ansible/plugins/callback/__init__.py

@@ -19,8 +19,9 @@
 from __future__ import (absolute_import, division, print_function)
 __metaclass__ = type
 
-import json
 import difflib
+import json
+import sys
 import warnings
 from copy import deepcopy
 
@@ -124,7 +125,7 @@ class CallbackBase:
                         # format complex structures into 'files'
                         for x in ['before', 'after']:
                             if isinstance(diff[x], dict):
-                                diff[x] = json.dumps(diff[x], sort_keys=True, indent=4)
+                                diff[x] = json.dumps(diff[x], sort_keys=True, indent=4, separators=(',', ': ')) + '\n'
                         if 'before_header' in diff:
                             before_header = "before: %s" % diff['before_header']
                         else:
@@ -133,15 +134,29 @@ class CallbackBase:
                             after_header = "after: %s" % diff['after_header']
                         else:
                             after_header = 'after'
-                        differ = difflib.unified_diff(to_text(diff['before']).splitlines(True),
+                        before_lines = to_text(diff['before']).splitlines(True)
-                                                      to_text(diff['after']).splitlines(True),
+                        after_lines = to_text(diff['after']).splitlines(True)
+                        if before_lines and not before_lines[-1].endswith('\n'):
+                            before_lines[-1] += '\n\\ No newline at end of file\n'
+                        if after_lines and not after_lines[-1].endswith('\n'):
+                            after_lines[-1] += '\n\\ No newline at end of file\n'
+                        differ = difflib.unified_diff(before_lines,
+                                                      after_lines,
                                                       fromfile=before_header,
                                                       tofile=after_header,
                                                       fromfiledate='',
                                                       tofiledate='',
                                                       n=C.DIFF_CONTEXT)
+                        difflines = list(differ)
+                        if len(difflines) >= 3 and sys.version_info[:2] == (2, 6):
+                            # difflib in Python 2.6 adds trailing spaces after
+                            # filenames in the -- before/++ after headers.
+                            difflines[0] = difflines[0].replace(' \n', '\n')
+                            difflines[1] = difflines[1].replace(' \n', '\n')
+                            # it also treats empty files differently
+                            difflines[2] = difflines[2].replace('-1,0', '-0,0').replace('+1,0', '+0,0')
                         has_diff = False
-                        for line in differ:
+                        for line in difflines:
                             has_diff = True
                             if line.startswith('+'):
                                 line = stringc(line, C.COLOR_DIFF_ADD)

test/units/plugins/callback/test_callback.py

@@ -19,6 +19,8 @@
 from __future__ import (absolute_import, division, print_function)
 __metaclass__ = type
 
+import re
+import textwrap
 import types
 
 from ansible.compat.tests import unittest
@@ -134,9 +136,15 @@ class TestCallbackDumpResults(unittest.TestCase):
 #       that try except orig appeared in 61d01f549f2143fd9adfa4ffae42f09d24649c26
 #       in 2013 so maybe a < py2.6 issue
 class TestCallbackDiff(unittest.TestCase):
+
+    def setUp(self):
+        self.cb = CallbackBase()
+
+    def _strip_color(self, s):
+        return re.sub('\033\\[[^m]*m', '', s)
+
     def test_difflist(self):
         # TODO: split into smaller tests?
-        cb = CallbackBase()
         difflist = [{'before': ['preface\nThe Before String\npostscript'],
                      'after': ['preface\nThe After String\npostscript'],
                      'before_header': 'just before',
@@ -153,13 +161,158 @@ class TestCallbackDiff(unittest.TestCase):
                     {'before_header': 'just before'},
                     {'after_header': 'just after'}]
 
-        res = cb._get_diff(difflist)
+        res = self.cb._get_diff(difflist)
 
         self.assertIn('Before String', res)
         self.assertIn('After String', res)
         self.assertIn('just before', res)
         self.assertIn('just after', res)
 
+    def test_simple_diff(self):
+        self.assertMultiLineEqual(
+            self._strip_color(self.cb._get_diff({
+                'before_header': 'somefile.txt',
+                'after_header': 'generated from template somefile.j2',
+                'before': 'one\ntwo\nthree\n',
+                'after': 'one\nthree\nfour\n',
+            })),
+            textwrap.dedent('''\
+                --- before: somefile.txt
+                +++ after: generated from template somefile.j2
+                @@ -1,3 +1,3 @@
+                 one
+                -two
+                 three
+                +four
+
+            '''))
+
+    def test_new_file(self):
+        self.assertMultiLineEqual(
+            self._strip_color(self.cb._get_diff({
+                'before_header': 'somefile.txt',
+                'after_header': 'generated from template somefile.j2',
+                'before': '',
+                'after': 'one\ntwo\nthree\n',
+            })),
+            textwrap.dedent('''\
+                --- before: somefile.txt
+                +++ after: generated from template somefile.j2
+                @@ -0,0 +1,3 @@
+                +one
+                +two
+                +three
+
+            '''))
+
+    def test_clear_file(self):
+        self.assertMultiLineEqual(
+            self._strip_color(self.cb._get_diff({
+                'before_header': 'somefile.txt',
+                'after_header': 'generated from template somefile.j2',
+                'before': 'one\ntwo\nthree\n',
+                'after': '',
+            })),
+            textwrap.dedent('''\
+                --- before: somefile.txt
+                +++ after: generated from template somefile.j2
+                @@ -1,3 +0,0 @@
+                -one
+                -two
+                -three
+
+            '''))
+
+    def test_no_trailing_newline_before(self):
+        self.assertMultiLineEqual(
+            self._strip_color(self.cb._get_diff({
+                'before_header': 'somefile.txt',
+                'after_header': 'generated from template somefile.j2',
+                'before': 'one\ntwo\nthree',
+                'after': 'one\ntwo\nthree\n',
+            })),
+            textwrap.dedent('''\
+                --- before: somefile.txt
+                +++ after: generated from template somefile.j2
+                @@ -1,3 +1,3 @@
+                 one
+                 two
+                -three
+                \\ No newline at end of file
+                +three
+
+            '''))
+
+    def test_no_trailing_newline_after(self):
+        self.assertMultiLineEqual(
+            self._strip_color(self.cb._get_diff({
+                'before_header': 'somefile.txt',
+                'after_header': 'generated from template somefile.j2',
+                'before': 'one\ntwo\nthree\n',
+                'after': 'one\ntwo\nthree',
+            })),
+            textwrap.dedent('''\
+                --- before: somefile.txt
+                +++ after: generated from template somefile.j2
+                @@ -1,3 +1,3 @@
+                 one
+                 two
+                -three
+                +three
+                \\ No newline at end of file
+
+            '''))
+
+    def test_no_trailing_newline_both(self):
+        self.assertMultiLineEqual(
+            self.cb._get_diff({
+                'before_header': 'somefile.txt',
+                'after_header': 'generated from template somefile.j2',
+                'before': 'one\ntwo\nthree',
+                'after': 'one\ntwo\nthree',
+            }),
+            '')
+
+    def test_no_trailing_newline_both_with_some_changes(self):
+        self.assertMultiLineEqual(
+            self._strip_color(self.cb._get_diff({
+                'before_header': 'somefile.txt',
+                'after_header': 'generated from template somefile.j2',
+                'before': 'one\ntwo\nthree',
+                'after': 'one\nfive\nthree',
+            })),
+            textwrap.dedent('''\
+                --- before: somefile.txt
+                +++ after: generated from template somefile.j2
+                @@ -1,3 +1,3 @@
+                 one
+                -two
+                +five
+                 three
+                \\ No newline at end of file
+
+            '''))
+
+    def test_diff_dicts(self):
+        self.assertMultiLineEqual(
+            self._strip_color(self.cb._get_diff({
+                'before': dict(one=1, two=2, three=3),
+                'after': dict(one=1, three=3, four=4),
+            })),
+            textwrap.dedent('''\
+                --- before
+                +++ after
+                @@ -1,5 +1,5 @@
+                 {
+                +    "four": 4,
+                     "one": 1,
+                -    "three": 3,
+                -    "two": 2
+                +    "three": 3
+                 }
+
+            '''))
+
 
 class TestCallbackOnMethods(unittest.TestCase):
     def _find_on_methods(self, callback):