From 210e22846c469f3d068e5c4efe91656d7eacb579 Mon Sep 17 00:00:00 2001 From: Chandragupt Singh Date: Wed, 3 Jun 2026 22:47:15 +0530 Subject: [PATCH 1/4] delete: preserve empty dirs in --backup-dir when deleting; add regression test --- delete.c | 22 +++++- testsuite/backup-empty-dir_test.py | 107 +++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 2 deletions(-) create mode 100644 testsuite/backup-empty-dir_test.py diff --git a/delete.c b/delete.c index 4a52122d3..ffda1b3f3 100644 --- a/delete.c +++ b/delete.c @@ -159,8 +159,26 @@ enum delret delete_item(char *fbuf, uint16 mode, uint16 flags) } if (S_ISDIR(mode)) { - what = "rmdir"; - ok = do_rmdir_at(fbuf) == 0; + ok = 0; + if (make_backups > 0 && !(flags & DEL_FOR_BACKUP) && backup_dir) { + char *buf = get_backup_name(fbuf); + if (buf && do_rename_at(fbuf, buf) == 0) { + if (INFO_GTE(BACKUP, 1)) + rprintf(FINFO, "backed up %s to %s\n", fbuf, buf); + what = "rename"; + ok = 1; + } else if (buf && errno != EEXIST && errno != ENOTEMPTY + && errno != EISDIR) { + /* Rename failed for an unexpected reason (e.g. EXDEV). + * Warn and fall through to rmdir. */ + rprintf(FWARNING, "skipping backup of directory %s: %s\n", + fbuf, strerror(errno)); + } + } + if (!ok) { + what = "rmdir"; + ok = do_rmdir_at(fbuf) == 0; + } } else { if (make_backups > 0 && !(flags & DEL_FOR_BACKUP) && (backup_dir || !is_backup_file(fbuf))) { what = "make_backup"; diff --git a/testsuite/backup-empty-dir_test.py b/testsuite/backup-empty-dir_test.py new file mode 100644 index 000000000..5833c46f4 --- /dev/null +++ b/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') From b9ddeb51ca4f424154e9f3d72a128dbe788c6af0 Mon Sep 17 00:00:00 2001 From: Andrew Tridgell Date: Sat, 6 Jun 2026 06:35:06 +1000 Subject: [PATCH 2/4] testsuite: check that only the right files go into backup this demonstrates a subtle bug in PR 967 --- testsuite/backup-pinned-dir_test.py | 62 +++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 testsuite/backup-pinned-dir_test.py diff --git a/testsuite/backup-pinned-dir_test.py b/testsuite/backup-pinned-dir_test.py new file mode 100644 index 000000000..fc59fb5af --- /dev/null +++ b/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)') From fa8b6fe0aad248a30e21909f7677393219b48c7a Mon Sep 17 00:00:00 2001 From: Chandragupt Singh Date: Sat, 6 Jun 2026 20:39:15 +0530 Subject: [PATCH 3/4] delete: use rmdir+mkdir instead of rename to safely back up empty dirs --- delete.c | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/delete.c b/delete.c index ffda1b3f3..b5361a7f9 100644 --- a/delete.c +++ b/delete.c @@ -159,26 +159,17 @@ enum delret delete_item(char *fbuf, uint16 mode, uint16 flags) } if (S_ISDIR(mode)) { - ok = 0; - if (make_backups > 0 && !(flags & DEL_FOR_BACKUP) && backup_dir) { + 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_rename_at(fbuf, buf) == 0) { + if (buf && do_mkdir_at(buf, ACCESSPERMS) == 0) { if (INFO_GTE(BACKUP, 1)) rprintf(FINFO, "backed up %s to %s\n", fbuf, buf); - what = "rename"; - ok = 1; - } else if (buf && errno != EEXIST && errno != ENOTEMPTY - && errno != EISDIR) { - /* Rename failed for an unexpected reason (e.g. EXDEV). - * Warn and fall through to rmdir. */ - rprintf(FWARNING, "skipping backup of directory %s: %s\n", - fbuf, strerror(errno)); + } else if (buf && errno != EEXIST && errno != EISDIR) { + rsyserr(FWARNING, errno, "backup mkdir %s failed", buf); } } - if (!ok) { - what = "rmdir"; - ok = do_rmdir_at(fbuf) == 0; - } } else { if (make_backups > 0 && !(flags & DEL_FOR_BACKUP) && (backup_dir || !is_backup_file(fbuf))) { what = "make_backup"; From 851a24d993ea155cda6e70fdb1aa1ac8d57f05b8 Mon Sep 17 00:00:00 2001 From: Chandragupt Singh Date: Sun, 7 Jun 2026 02:49:52 +0530 Subject: [PATCH 4/4] delete: treat EEXIST like ENOTEMPTY for rmdir on Solaris --- delete.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/delete.c b/delete.c index b5361a7f9..9a42293ef 100644 --- a/delete.c +++ b/delete.c @@ -203,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;