copy module - fix copying directories recursively with remote_src=True (#76997)

* copy module - fix copying directories containing modified subdirs with remote_src=True. Previously, the first changed subdir would prevent recursively checking for changes for in subdirs at the same level.

* Fix reporting changed for copying empty directories with remote_src=True. If a directory is created on the remote but nothing else, changed is True.

changelogs/fragments/76997-fix-copy-subdirs-with-remote-src.yml

@@ -0,0 +1,4 @@
+bugfixes:
+  - copy remote_src=true - fix copying subdirs recursively when the dest exists and the src and dest have
+    multiple common subdirectories in a common directory (https://github.com/ansible/ansible/issues/74536).
+  - copy remote_src=true - fix reporting changed for copying empty directories.

lib/ansible/modules/copy.py

@@ -518,7 +518,7 @@ def copy_common_dirs(src, dest, module):
             changed = True
 
         # recurse into subdirectory
-        changed = changed or copy_common_dirs(os.path.join(src, item), os.path.join(dest, item), module)
+        changed = copy_common_dirs(os.path.join(src, item), os.path.join(dest, item), module) or changed
     return changed
 
 
@@ -616,6 +616,7 @@ def main():
                 module.fail_json(**e.results)
 
             os.makedirs(b_dirname)
+            changed = True
             directory_args = module.load_file_common_arguments(module.params)
             directory_mode = module.params["directory_mode"]
             if directory_mode is not None:
@@ -744,8 +745,6 @@ def main():
             except (IOError, OSError):
                 module.fail_json(msg="failed to copy: %s to %s" % (src, dest), traceback=traceback.format_exc())
         changed = True
-    else:
-        changed = False
 
     # If neither have checksums, both src and dest are directories.
     if checksum_src is None and checksum_dest is None:
@@ -793,13 +792,12 @@ def main():
                 b_dest = to_bytes(os.path.join(b_dest, b_basename), errors='surrogate_or_strict')
                 if not module.check_mode and not os.path.exists(b_dest):
                     os.makedirs(b_dest)
+                    changed = True
                     b_src = to_bytes(os.path.join(module.params['src'], ""), errors='surrogate_or_strict')
                     diff_files_changed = copy_diff_files(b_src, b_dest, module)
                     left_only_changed = copy_left_only(b_src, b_dest, module)
                     common_dirs_changed = copy_common_dirs(b_src, b_dest, module)
                     owner_group_changed = chown_recursive(b_dest, module)
-                    if diff_files_changed or left_only_changed or common_dirs_changed or owner_group_changed:
-                        changed = True
                 if module.check_mode and not os.path.exists(b_dest):
                     changed = True
 

test/integration/targets/copy/tasks/tests.yml

@@ -419,6 +419,80 @@
        - "stat_results2.stat.checksum == ('foo.txt\n'|hash('sha1'))"
        - "stat_results2.stat.mode == '0547'"
 
+#
+# test copying an empty dir to a dest dir with remote_src=True
+#
+
+- name: create empty test dir
+  file:
+    path: '{{ remote_dir }}/testcase_empty_dir'
+    state: directory
+
+- name: test copying an empty dir to a dir that does not exist (dest ends with slash)
+  copy:
+    src: '{{ remote_dir }}/testcase_empty_dir/'
+    remote_src: yes
+    dest: '{{ remote_dir }}/testcase_empty_dir_dest/'
+  register: copy_result
+
+- name: get stat of newly created dir
+  stat:
+    path: '{{ remote_dir }}/testcase_empty_dir_dest'
+  register: stat_result
+
+- assert:
+    that:
+      - copy_result.changed
+      - stat_result.stat.exists
+      - stat_result.stat.isdir
+
+- name: test no change is made running the task twice
+  copy:
+    src: '{{ remote_dir }}/testcase_empty_dir/'
+    remote_src: yes
+    dest: '{{ remote_dir }}/testcase_empty_dir_dest/'
+  register: copy_result
+  failed_when: copy_result is changed
+
+- name: remove to test dest with no trailing slash
+  file:
+    path: '{{ remote_dir }}/testcase_empty_dir_dest/'
+    state: absent
+
+- name: test copying an empty dir to a dir that does not exist (both src/dest have no trailing slash)
+  copy:
+    src: '{{ remote_dir }}/testcase_empty_dir'
+    remote_src: yes
+    dest: '{{ remote_dir }}/testcase_empty_dir_dest'
+  register: copy_result
+
+- name: get stat of newly created dir
+  stat:
+    path: '{{ remote_dir }}/testcase_empty_dir_dest'
+  register: stat_result
+
+- assert:
+    that:
+      - copy_result.changed
+      - stat_result.stat.exists
+      - stat_result.stat.isdir
+
+- name: test no change is made running the task twice
+  copy:
+    src: '{{ remote_dir }}/testcase_empty_dir/'
+    remote_src: yes
+    dest: '{{ remote_dir }}/testcase_empty_dir_dest/'
+  register: copy_result
+  failed_when: copy_result is changed
+
+- name: clean up src and dest
+  file:
+    path: "{{ item }}"
+    state: absent
+  loop:
+    - '{{ remote_dir }}/testcase_empty_dir'
+    - '{{ remote_dir }}/testcase_empty_dir_dest'
+
 #
 # test recursive copy local_follow=False, no trailing slash
 #
@@ -2284,3 +2358,81 @@
     that:
     - fail_copy_directory_with_enc_file is failed
     - fail_copy_directory_with_enc_file.msg == 'A vault password or secret must be specified to decrypt {{role_path}}/files-different/vault/vault-file'
+
+#
+# Test for issue 74536: recursively copy all nested directories with remote_src=yes and src='dir/' when dest exists
+#
+- vars:
+    src: '{{ remote_dir }}/testcase_74536'
+  block:
+    - name: create source dir with 3 nested subdirs
+      file:
+        path: '{{ src }}/a/b1/c1'
+        state: directory
+
+    - name: copy the source dir with a trailing slash
+      copy:
+        src: '{{ src }}/'
+        remote_src: yes
+        dest: '{{ src }}_dest/'
+      register: copy_result
+      failed_when: copy_result is not changed
+
+    - name: remove the source dir to recreate with different subdirs
+      file:
+        path: '{{ src }}'
+        state: absent
+
+    - name: recreate source dir
+      file:
+        path: "{{ item }}"
+        state: directory
+      loop:
+        - '{{ src }}/a/b1/c2'
+        - '{{ src }}/a/b2/c3'
+
+    - name: copy the source dir containing new subdirs into the existing dest dir
+      copy:
+        src: '{{ src }}/'
+        remote_src: yes
+        dest: '{{ src }}_dest/'
+      register: copy_result
+
+    - name: stat each directory that should exist
+      stat:
+        path: '{{ item }}'
+      register: stat_result
+      loop:
+        - '{{ src }}_dest'
+        - '{{ src }}_dest/a'
+        - '{{ src }}_dest/a/b1'
+        - '{{ src }}_dest/a/b2'
+        - '{{ src }}_dest/a/b1/c1'
+        - '{{ src }}_dest/a/b1/c2'
+        - '{{ src }}_dest/a/b2/c3'
+
+    - debug: msg="{{ stat_result }}"
+
+    - assert:
+        that:
+          - copy_result is changed
+          # all paths exist
+          - stat_result.results | map(attribute='stat') | map(attribute='exists') | unique == [true]
+          # all paths are dirs
+          - stat_result.results | map(attribute='stat') | map(attribute='isdir') | unique == [true]
+
+    - name: copy the src again to verify no changes will be made
+      copy:
+        src: '{{ src }}/'
+        remote_src: yes
+        dest: '{{ src }}_dest/'
+      register: copy_result
+      failed_when: copy_result is changed
+
+    - name: clean up src and dest
+      file:
+        path: '{{ item }}'
+        state: absent
+      loop:
+        - '{{ src }}'
+        - '{{ src }}_dest'