Merge branch 'copy-diff-take2' of git://github.com/stoned/ansible into better_diff

Conflicts:
	library/file

lib/ansible/callbacks.py

@@ -217,9 +217,8 @@ class DefaultRunnerCallbacks(object):
     def on_async_failed(self, host, res, jid):
         call_callback_module('runner_on_async_failed', host, res, jid)
 
-    def on_file_diff(self, host, before_string, after_string):
-        if before_string and after_string:
-            call_callback_module('runner_on_file_diff', before_string, after_string)
+    def on_file_diff(self, host, diff):
+        call_callback_module('runner_on_file_diff', diff)
 
 ########################################################################
 
@@ -285,11 +284,10 @@ class CliRunnerCallbacks(DefaultRunnerCallbacks):
         if self.options.tree:
             utils.write_tree_file(self.options.tree, host, utils.jsonify(result2,format=True))
     
-    def on_file_diff(self, host, before_string, after_string):
-        if before_string and after_string:
-            if self.options.diff:
-                print utils.get_diff(before_string, after_string)
-            super(CliRunnerCallbacks, self).on_file_diff(host, before_string, after_string)
+    def on_file_diff(self, host, diff):
+        if self.options.diff:
+            print utils.get_diff(diff)
+        super(CliRunnerCallbacks, self).on_file_diff(host, diff)
 
 ########################################################################
 
@@ -422,10 +420,9 @@ class PlaybookRunnerCallbacks(DefaultRunnerCallbacks):
         print stringc(msg, 'red')
         super(PlaybookRunnerCallbacks, self).on_async_failed(host,res,jid)
 
-    def on_file_diff(self, host, before_string, after_string):
-        if before_string and after_string:
-            print utils.get_diff(before_string, after_string)
-            super(PlaybookRunnerCallbacks, self).on_file_diff(host, before_string, after_string)
+    def on_file_diff(self, host, diff):
+        print utils.get_diff(diff)
+        super(PlaybookRunnerCallbacks, self).on_file_diff(host, diff)
 
 ########################################################################
 

lib/ansible/module_common.py

@@ -458,6 +458,7 @@ class AnsibleModule(object):
                 kwargs['state'] = 'file'
             if HAVE_SELINUX and self.selinux_enabled():
                 kwargs['secontext'] = ':'.join(self.selinux_context(path))
+            kwargs['size'] = st[stat.ST_SIZE]
         else:
             kwargs['state'] = 'absent'
         return kwargs

lib/ansible/runner/__init__.py

@@ -506,7 +506,7 @@ class Runner(object):
                 self.callbacks.on_failed(host, data, ignore_errors)
             else:
                 if self.diff:
-                    self.callbacks.on_file_diff(conn.host, result.before_diff_value, result.after_diff_value)
+                    self.callbacks.on_file_diff(conn.host, result.diff)
                 self.callbacks.on_ok(host, data)
         return result
 

lib/ansible/runner/action_plugins/copy.py

@@ -20,6 +20,8 @@ import os
 from ansible import utils
 from ansible import errors
 from ansible.runner.return_data import ReturnData
+import base64
+import stat
 
 class ActionModule(object):
 
@@ -74,10 +76,13 @@ class ActionModule(object):
         exec_rc = None
         if local_md5 != remote_md5:
 
+            if self.runner.diff:
+                diff = self._get_diff_data(conn, tmp, inject, dest, source)
+            else:
+                diff = {}
+
             if self.runner.check:
-                # TODO: if the filesize is small, include a nice pretty-printed diff by 
-                # calling a (new) diff callback
-                return ReturnData(conn=conn, result=dict(changed=True))
+                return ReturnData(conn=conn, result=dict(changed=True), diff=diff)
 
             # transfer the file to a remote tmp location
             tmp_src = tmp + os.path.basename(source)
@@ -100,3 +105,40 @@ class ActionModule(object):
                 module_args = "%s CHECKMODE=True" % module_args
             return self.runner._execute_module(conn, tmp, 'file', module_args, inject=inject)
 
+    def _get_diff_data(self, conn, tmp, inject, destination, source):
+        peek_result = self.runner._execute_module(conn, tmp, 'file', "path=%s diff_peek=1" % destination, inject=inject, persist_files=True)
+
+        if not peek_result.is_successful():
+            return {}
+
+        diff = {}
+        if peek_result.result['state'] == 'absent':
+            diff['before'] = ''
+        elif peek_result.result['appears_binary']:
+            diff['dst_binary'] = 1
+        elif peek_result.result['size'] > utils.MAX_FILE_SIZE_FOR_DIFF:
+            diff['dst_larger'] = utils.MAX_FILE_SIZE_FOR_DIFF
+        else:
+            dest_result = self.runner._execute_module(conn, tmp, 'slurp', "path=%s" % destination, inject=inject, persist_files=True)
+            if 'content' in dest_result.result:
+                dest_contents = dest_result.result['content']
+                if dest_result.result['encoding'] == 'base64':
+                    dest_contents = base64.b64decode(dest_contents)
+                else:
+                    raise Exception("unknown encoding, failed: %s" % dest_result.result)
+                diff['before_header'] = destination
+                diff['before'] = dest_contents
+
+        src = open(source)
+        src_contents = src.read(8192)
+        st = os.stat(source)
+        if src_contents.find("\x00") != -1:
+            diff['src_binary'] = 1
+        elif st[stat.ST_SIZE] > utils.MAX_FILE_SIZE_FOR_DIFF:
+            diff['src_larger'] = utils.MAX_FILE_SIZE_FOR_DIFF
+        else:
+            src.seek(0)
+            diff['after_header'] = source
+            diff['after'] = src.read()
+
+        return diff

lib/ansible/runner/action_plugins/template.py

@@ -82,7 +82,7 @@ class ActionModule(object):
             # template is different from the remote value
 
             # if showing diffs, we need to get the remote value
-            dest_contents = None
+            dest_contents = ''
 
             if self.runner.diff:
                 # using persist_files to keep the temp directory around to avoid needing to grab another
@@ -93,8 +93,6 @@ class ActionModule(object):
                         dest_contents = base64.b64decode(dest_contents)
                     else:
                         raise Exception("unknown encoding, failed: %s" % dest_result.result)
-                else:
-                    dest_result = ''
  
             xfered = self.runner._transfer_str(conn, tmp, 'source', resultant)
 
@@ -106,11 +104,10 @@ class ActionModule(object):
             module_args = "%s src=%s dest=%s" % (module_args, xfered, dest)
 
             if self.runner.check:
-                return ReturnData(conn=conn, comm_ok=True, result=dict(changed=True), before_diff_value=dest_contents, after_diff_value=resultant)
+                return ReturnData(conn=conn, comm_ok=True, result=dict(changed=True), diff=dict(before_header=dest, after_header=source, before=dest_contents, after=resultant))
             else:
                 res = self.runner._execute_module(conn, tmp, 'copy', module_args, inject=inject)
-                res.before_diff_value = dest_contents
-                res.after_diff_value = resultant
+                res.diff = dict(before=dest_contents, after=resultant)
                 return res
         else:
             return self.runner._execute_module(conn, tmp, 'file', module_args, inject=inject)

lib/ansible/runner/return_data.py

@@ -20,10 +20,10 @@ from ansible import utils
 class ReturnData(object):
     ''' internal return class for runner execute methods, not part of public API signature '''
 
-    __slots__ = [ 'result', 'comm_ok', 'host', 'before_diff_value', 'after_diff_value' ]
+    __slots__ = [ 'result', 'comm_ok', 'host', 'diff' ]
 
     def __init__(self, conn=None, host=None, result=None, 
-        comm_ok=True, before_diff_value=None, after_diff_value=None):
+        comm_ok=True, diff=dict()):
 
         # which host is this ReturnData about?
         if conn is not None:
@@ -41,8 +41,7 @@ class ReturnData(object):
 
         # if these values are set and used with --diff we can show
         # changes made to particular files
-        self.before_diff_value = before_diff_value
-        self.after_diff_value = after_diff_value
+        self.diff = diff
 
         if type(self.result) in [ str, unicode ]:
             self.result = utils.parse_json(self.result)

lib/ansible/utils/__init__.py

@@ -39,6 +39,8 @@ import warnings
 
 VERBOSITY=0
 
+MAX_FILE_SIZE_FOR_DIFF=1*1024*1024
+
 try:
     import json
 except ImportError:
@@ -620,15 +622,35 @@ def make_sudo_cmd(sudo_user, executable, cmd):
         prompt, sudo_user, executable or '$SHELL', pipes.quote(cmd))
     return ('/bin/sh -c ' + pipes.quote(sudocmd), prompt)
 
-def get_diff(before, after):
+def get_diff(diff):
     # called by --diff usage in playbook and runner via callbacks
     # include names in diffs 'before' and 'after' and do diff -U 10
 
     try:
         with warnings.catch_warnings():
             warnings.simplefilter('ignore')
-            differ = difflib.unified_diff(before.splitlines(True), after.splitlines(True), 'before', 'after', '', '', 10)
-            return "".join(list(differ))
+            ret = []
+            if 'dst_binary' in diff:
+                ret.append("diff skipped: destination file appears to be binary\n")
+            if 'src_binary' in diff:
+                ret.append("diff skipped: source file appears to be binary\n")
+            if 'dst_larger' in diff:
+                ret.append("diff skipped: destination file size is greater than %d\n" % diff['dst_larger'])
+            if 'src_larger' in diff:
+                ret.append("diff skipped: source file size is greater than %d\n" % diff['src_larger'])
+            if 'before' in diff and 'after' in diff:
+                if 'before_header' in diff:
+                    before_header = "before: %s" % diff['before_header']
+                else:
+                    before_header = 'before'
+                if 'after_header' in diff:
+                    after_header = "after: %s" % diff['after_header']
+                else:
+                    after_header = 'after'
+                differ = difflib.unified_diff(diff['before'].splitlines(True), diff['after'].splitlines(True), before_header, after_header, '', '', 10)
+                for line in list(differ):
+                    ret.append(line)
+            return "".join(ret)
     except UnicodeDecodeError:
         return ">> the files are different, but the diff library cannot compare unicode strings"
 

library/file

@@ -141,6 +141,7 @@ def main():
             state = dict(choices=['file','directory','link','absent'], default='file'),
             path  = dict(aliases=['dest', 'name'], required=True),
             recurse  = dict(default='no', type='bool')
+            diff_peek = dict(default=None)
         ),
         add_file_common_args=True,
         supports_check_mode=True
@@ -150,6 +151,19 @@ def main():
     state  = params['state']
     params['path'] = path = os.path.expanduser(params['path'])
 
+    # short-circuit for diff_peek
+    if params.get('diff_peek', None) is not None:
+        appears_binary = False
+        try:
+            f = open(path)
+            b = f.read(8192)
+            f.close()
+            if b.find("\x00") != -1:
+                appears_binary = True
+        except:
+            pass
+        module.exit_json(path=path, changed=False, appears_binary=appears_binary)
+
     # source is both the source of a symlink or an informational passing of the src for a template module
     # or copy module, even if this module never uses it, it is needed to key off some things