delete: preserve empty dirs in --backup-dir when deleting

delete.c

@@ -161,6 +161,15 @@ enum delret delete_item(char *fbuf, uint16 mode, uint16 flags)
 	if (S_ISDIR(mode)) {
 		what = "rmdir";
 		ok = do_rmdir_at(fbuf) == 0;
+		if (ok && make_backups > 0 && !(flags & DEL_FOR_BACKUP) && backup_dir) {
+			char *buf = get_backup_name(fbuf);
+			if (buf && do_mkdir_at(buf, ACCESSPERMS) == 0) {
+				if (INFO_GTE(BACKUP, 1))
+					rprintf(FINFO, "backed up %s to %s\n", fbuf, buf);
+			} else if (buf && errno != EEXIST && errno != EISDIR) {
+				rsyserr(FWARNING, errno, "backup mkdir %s failed", buf);
+			}
+		}
 	} else {
 		if (make_backups > 0 && !(flags & DEL_FOR_BACKUP) && (backup_dir || !is_backup_file(fbuf))) {
 			what = "make_backup";
@@ -194,7 +203,7 @@ enum delret delete_item(char *fbuf, uint16 mode, uint16 flags)
 		}
 		ret = DR_SUCCESS;
 	} else {
-		if (S_ISDIR(mode) && errno == ENOTEMPTY) {
+		if (S_ISDIR(mode) && (errno == ENOTEMPTY || errno == EEXIST)) {
 			rprintf(FINFO, "cannot delete non-empty directory: %s\n",
 				fbuf);
 			ret = DR_NOT_EMPTY;

testsuite/backup-empty-dir_test.py

@@ -0,0 +1,107 @@
+#!/usr/bin/env python3
+# Regression test for issue #842:
+#   rsync --backup --backup-dir --delete must preserve empty directories
+#   in --backup-dir rather than silently removing them.
+
+import re
+import subprocess
+
+from rsyncfns import (
+    FROMDIR, TODIR, TMPDIR,
+    assert_exists, assert_not_exists,
+    makepath, rmtree,
+    run_rsync, rsync_argv, test_fail,
+)
+
+bakdir = TMPDIR / 'bak'
+
+
+# ---------------------------------------------------------------------------
+# Phase 1: basic reproducer from issue #842.
+# src/ is empty; dst/sub/empty_dir/ and dst/sub/file.txt get deleted.
+# Both must land in backup-dir, not be silently discarded.
+# ---------------------------------------------------------------------------
+makepath(FROMDIR, TODIR / 'sub' / 'empty_dir', bakdir)
+(TODIR / 'sub' / 'file.txt').write_text('data\n')
+
+proc = subprocess.run(
+    rsync_argv('-r', '--delete', '--backup', f'--backup-dir={bakdir}',
+               '--info=backup', f'{FROMDIR}/', f'{TODIR}/'),
+    capture_output=True, text=True,
+)
+if proc.returncode != 0:
+    test_fail(f'rsync failed (phase 1): {proc.stderr}')
+output = proc.stdout + proc.stderr
+
+assert_exists(bakdir / 'sub' / 'file.txt',     label='phase1: file.txt in backup-dir')
+assert_exists(bakdir / 'sub' / 'empty_dir',    label='phase1: empty_dir in backup-dir')
+if not (bakdir / 'sub' / 'empty_dir').is_dir():
+    test_fail('phase1: backup-dir/sub/empty_dir must be a directory')
+assert_not_exists(TODIR / 'sub', label='phase1: dst/sub removed after sync')
+
+if not re.search(r'backed up sub/empty_dir to ', output, re.MULTILINE):
+    test_fail('phase1: no "backed up sub/empty_dir" log line in rsync output')
+
+
+# ---------------------------------------------------------------------------
+# Phase 2: collision safety (steadytao concern, issue #842).
+# File backups create backup_dir/sub/ first. The subsequent attempt to back
+# up sub/ itself must not wipe those already-preserved child files.
+# ---------------------------------------------------------------------------
+rmtree(FROMDIR); rmtree(TODIR); rmtree(bakdir)
+makepath(FROMDIR, TODIR / 'sub' / 'empty_dir', bakdir)
+(TODIR / 'sub' / 'file1.txt').write_text('file1\n')
+(TODIR / 'sub' / 'file2.txt').write_text('file2\n')
+
+run_rsync('-r', '--delete', '--backup', f'--backup-dir={bakdir}',
+          f'{FROMDIR}/', f'{TODIR}/')
+
+assert_exists(bakdir / 'sub' / 'file1.txt',  label='phase2: file1.txt survives')
+assert_exists(bakdir / 'sub' / 'file2.txt',  label='phase2: file2.txt survives')
+assert_exists(bakdir / 'sub' / 'empty_dir',  label='phase2: empty_dir in backup-dir')
+assert_not_exists(TODIR / 'sub', label='phase2: dst/sub removed after sync')
+
+
+# ---------------------------------------------------------------------------
+# Phase 3: nested empty directories (a/b/c).
+# ---------------------------------------------------------------------------
+rmtree(FROMDIR); rmtree(TODIR); rmtree(bakdir)
+makepath(FROMDIR, TODIR / 'a' / 'b' / 'c', bakdir)
+
+run_rsync('-r', '--delete', '--backup', f'--backup-dir={bakdir}',
+          f'{FROMDIR}/', f'{TODIR}/')
+
+assert_exists(bakdir / 'a' / 'b' / 'c', label='phase3: a/b/c in backup-dir')
+if not (bakdir / 'a' / 'b' / 'c').is_dir():
+    test_fail('phase3: backup-dir/a/b/c must be a directory')
+assert_not_exists(TODIR / 'a', label='phase3: dst/a removed after sync')
+
+
+# ---------------------------------------------------------------------------
+# Phase 4: no regression without --backup-dir.
+# Empty dir must still be deleted; no empty_dir~ artefact must appear.
+# ---------------------------------------------------------------------------
+rmtree(FROMDIR); rmtree(TODIR)
+makepath(FROMDIR, TODIR / 'empty_dir')
+
+run_rsync('-r', '--delete', '--backup', f'{FROMDIR}/', f'{TODIR}/')
+
+assert_not_exists(TODIR / 'empty_dir',  label='phase4: empty_dir removed without --backup-dir')
+assert_not_exists(TODIR / 'empty_dir~', label='phase4: no empty_dir~ created')
+
+
+# ---------------------------------------------------------------------------
+# Phase 5: --delete-delay variant.
+# Deletions are queued and executed after the transfer; the backup path
+# in delete_item() must still be reached for empty directories.
+# ---------------------------------------------------------------------------
+rmtree(FROMDIR); rmtree(TODIR); rmtree(bakdir)
+makepath(FROMDIR, TODIR / 'sub' / 'empty_dir', bakdir)
+(TODIR / 'sub' / 'file.txt').write_text('data\n')
+
+run_rsync('-r', '--delete-delay', '--backup', f'--backup-dir={bakdir}',
+          f'{FROMDIR}/', f'{TODIR}/')
+
+assert_exists(bakdir / 'sub' / 'empty_dir', label='phase5: empty_dir backed up with --delete-delay')
+assert_exists(bakdir / 'sub' / 'file.txt',  label='phase5: file.txt backed up with --delete-delay')
+assert_not_exists(TODIR / 'sub', label='phase5: dst/sub removed after sync')

testsuite/backup-pinned-dir_test.py

@@ -0,0 +1,62 @@
+#!/usr/bin/env python3
+# Regression test for the bug in PR #967.
+#
+# PR #967 makes delete_item() rename a directory into --backup-dir before
+# falling back to rmdir, so empty dirs are preserved in the backup tree
+# (issue #842).  The rename branch fires whenever DEL_DIR_IS_EMPTY is set,
+# but that flag is NOT proof of emptiness: delete_dir_contents() sets it on
+# a child even when the child's own content deletion returned DR_NOT_EMPTY
+# (a grandchild survived).  do_rmdir_at() was safe there -- it fails with
+# ENOTEMPTY -- but do_rename_at() happily moves the whole non-empty subtree
+# into the backup-dir.
+#
+# A "protect" filter (P) pins a file against deletion, leaving its parent
+# non-empty.  rsync must leave that file where it is.  With the PR bug, the
+# pinned parent gets renamed wholesale into --backup-dir, relocating data
+# rsync deliberately chose not to delete.
+#
+# Correct behaviour (master): the protected file stays in the destination.
+# Buggy behaviour (PR #967): the protected file is moved into --backup-dir.
+
+from rsyncfns import (
+    FROMDIR, TODIR, TMPDIR,
+    assert_exists, assert_not_exists,
+    makepath, rmtree, run_rsync, test_fail,
+)
+
+bakdir = TMPDIR / 'bak'
+
+# The pinned file must be the ONLY thing in its nested directory, and that
+# directory must have no sibling whose own backup would pre-create the
+# backup-dir counterpart -- otherwise the rename collides (EEXIST/ENOTEMPTY)
+# and harmlessly falls back to rmdir, masking the bug.  'sibling.txt' lives
+# one level up so it creates bak/sub/ but not bak/sub/inner/.
+rmtree(FROMDIR); rmtree(TODIR); rmtree(bakdir)
+makepath(FROMDIR, TODIR / 'sub' / 'inner', bakdir)
+(TODIR / 'sub' / 'inner' / 'keep.txt').write_text('keepme\n')
+(TODIR / 'sub' / 'sibling.txt').write_text('sib\n')
+
+# src is empty: everything under dst/ is extraneous and would be deleted,
+# except sub/inner/keep.txt which the protect filter pins in place.
+run_rsync('-r', '--delete', '--backup', f'--backup-dir={bakdir}',
+          '--filter=P sub/inner/keep.txt',
+          f'{FROMDIR}/', f'{TODIR}/')
+
+# The protected file must remain exactly where it was.
+assert_exists(TODIR / 'sub' / 'inner' / 'keep.txt',
+              label='protected keep.txt must stay in the destination')
+
+# ...and must NOT have been relocated into the backup-dir.
+assert_not_exists(bakdir / 'sub' / 'inner' / 'keep.txt',
+                  label='protected keep.txt must NOT be moved into backup-dir')
+
+# Its pinned parent directory must also remain in place.
+assert_exists(TODIR / 'sub' / 'inner',
+              label='pinned dir sub/inner must stay in the destination')
+
+# The non-pinned sibling should still be backed up normally.
+assert_exists(bakdir / 'sub' / 'sibling.txt',
+              label='non-pinned sibling.txt backed up as usual')
+
+if not (TODIR / 'sub' / 'inner' / 'keep.txt').is_file():
+    test_fail('protected file was relocated by the backup-dir rename (PR #967 bug)')