diff --git a/delete.c b/delete.c index 4a52122d3..9a42293ef 100644 --- a/delete.c +++ b/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; 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') 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)')