From 084c0ac06513196d6524add5ce4021b156d06f19 Mon Sep 17 00:00:00 2001
From: David Wilson <dw@botanicus.net>
Date: Mon, 20 Aug 2018 13:24:09 +0100
Subject: [PATCH] ansible: avoid roundtrip in copy action due to
 fixup_perms2().

On top of existing temporary files work, this reduces the number of
roundtrips required for "copy" and "template" actions from 6 to 3.
---
 ansible_mitogen/mixins.py                     |   7 +-
 tests/ansible/integration/action/all.yml      |   5 +-
 .../integration/action/fixup_perms2__copy.yml | 104 ++++++++++++++++++
 3 files changed, 113 insertions(+), 3 deletions(-)
 create mode 100644 tests/ansible/integration/action/fixup_perms2__copy.yml

diff --git a/ansible_mitogen/mixins.py b/ansible_mitogen/mixins.py
index 7d8f5518..7be5444f 100644
--- a/ansible_mitogen/mixins.py
+++ b/ansible_mitogen/mixins.py
@@ -216,6 +216,11 @@ class ActionModuleMixin(ansible.plugins.action.ActionBase):
         self._connection.put_data(remote_path, data)
         return remote_path
 
+    #: Actions listed here cause :func:`_fixup_perms2` to avoid a needless
+    #: roundtrip, as they modify file modes separately afterwards. This is due
+    #: to the method prototype having a default of `execute=True`.
+    FIXUP_PERMS_RED_HERRING = set(['copy'])
+
     def _fixup_perms2(self, remote_paths, remote_user=None, execute=True):
         """
         Mitogen always executes ActionBase helper methods in the context of the
@@ -224,7 +229,7 @@ class ActionModuleMixin(ansible.plugins.action.ActionBase):
         """
         LOG.debug('_fixup_perms2(%r, remote_user=%r, execute=%r)',
                   remote_paths, remote_user, execute)
-        if execute:
+        if execute and self._load_name not in self.FIXUP_PERMS_RED_HERRING:
             return self._remote_chmod(remote_paths, mode='u+x')
         return self.COMMAND_RESULT.copy()
 
diff --git a/tests/ansible/integration/action/all.yml b/tests/ansible/integration/action/all.yml
index ebbff26a..22904c23 100644
--- a/tests/ansible/integration/action/all.yml
+++ b/tests/ansible/integration/action/all.yml
@@ -1,5 +1,6 @@
-- import_playbook: remote_file_exists.yml
-- import_playbook: remote_expand_user.yml
+- import_playbook: fixup_perms2__copy.yml
 - import_playbook: low_level_execute_command.yml
 - import_playbook: make_tmp_path.yml
+- import_playbook: remote_expand_user.yml
+- import_playbook: remote_file_exists.yml
 - import_playbook: transfer_data.yml
diff --git a/tests/ansible/integration/action/fixup_perms2__copy.yml b/tests/ansible/integration/action/fixup_perms2__copy.yml
new file mode 100644
index 00000000..17bfbed9
--- /dev/null
+++ b/tests/ansible/integration/action/fixup_perms2__copy.yml
@@ -0,0 +1,104 @@
+# Verify action plugins still set file modes correctly even though
+# fixup_perms2() avoids setting execute bit despite being asked to.
+
+- name: integration/action/fixup_perms2__copy.yml
+  hosts: test-targets
+  any_errors_fatal: true
+  tasks:
+    - name: Get default remote file mode
+      shell: python -c 'import os; print("%04o" % (int("0666", 8) & ~os.umask(0)))'
+      register: py_umask
+
+    - name: Set default file mode
+      set_fact:
+        mode: "{{py_umask.stdout}}"
+
+    #
+    # copy module (no mode).
+    #
+
+    - name: "Copy files (no mode)"
+      copy:
+        content: ""
+        dest: /tmp/copy-no-mode
+
+    - stat: path=/tmp/copy-no-mode
+      register: out
+    - assert:
+        that:
+        -  out.stat.mode == mode
+
+    #
+    # copy module (explicit mode).
+    #
+
+    - name: "Copy files from content: arg"
+      copy:
+        content: ""
+        mode: 0400
+        dest: /tmp/copy-with-mode
+
+    - stat: path=/tmp/copy-with-mode
+      register: out
+    - assert:
+        that:
+        -  out.stat.mode == "0400"
+
+    #
+    # copy module (existing disk files, no mode).
+    #
+
+    - file:
+        path: /tmp/weird-mode
+        state: absent
+
+    - name: Create local test file.
+      connection: local
+      copy:
+        content: "weird mode"
+        dest: "/tmp/weird-mode"
+        mode: "1462"
+
+    - copy:
+        src: "/tmp/weird-mode"
+        dest: "/tmp/weird-mode"
+
+    - stat:
+        path: "/tmp/weird-mode"
+      register: out
+    - assert:
+        that:
+        - out.stat.mode == mode
+
+    #
+    # copy module (existing disk files, preserve mode).
+    #
+
+    - copy:
+        src: "/tmp/weird-mode"
+        dest: "/tmp/weird-mode"
+        mode: preserve
+
+    - stat:
+        path: "/tmp/weird-mode"
+      register: out
+    - assert:
+        that:
+        - out.stat.mode == "1462"
+
+    #
+    # copy module (existing disk files, explicit mode).
+    #
+
+    - copy:
+        src: "/tmp/weird-mode"
+        dest: "/tmp/weird-mode"
+        mode: "1461"
+
+    - stat:
+        path: "/tmp/weird-mode"
+      register: out
+
+    - assert:
+        that:
+        - out.stat.mode == "1461"