Skip to content

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

Open
x15sr71 wants to merge 10 commits into
RsyncProject:masterfrom
x15sr71:fix/backup-empty-dir-delete
Open

delete: preserve empty dirs in --backup-dir when deleting#967
x15sr71 wants to merge 10 commits into
RsyncProject:masterfrom
x15sr71:fix/backup-empty-dir-delete

Conversation

@x15sr71

@x15sr71 x15sr71 commented Jun 5, 2026

Copy link
Copy Markdown

Description

When using --backup --backup-dir --delete, rsync was silently removing empty directories from the destination instead of preserving them in --backup-dir. Files in those directories were backed up correctly, but the empty directories themselves were not.

Root cause

delete_item() routed S_ISDIR directly to do_rmdir_at(), bypassing the make_backups/backup_dir logic that only existed in the non-directory else branch.

Fix

After delete_dir_contents() processes all children, delete_item() attempts rmdir() first — this is the atomic emptiness proof. If anything survived inside (e.g. a protect-filtered file), rmdir() fails and the directory is left untouched. If rmdir() succeeds, mkdir() recreates the directory in --backup-dir. EEXIST from mkdir() is expected when child file backups already created the target directory.
Also fixes rmdir() error handling on Solaris, which returns EEXIST (errno 17) instead of ENOTEMPTY for non-empty directories — both are POSIX-valid. The existing check now handles both.

Fixes #842.

Tests added in testsuite/backup-empty-dir_test.py:

  • Phase 1: basic --backup does not keep empty directories in --backup-dir #842 reproducer — empty dir appears in backup-dir
  • Phase 2: collision safety — existing backup-dir/sub/ from child file
    backups is not destroyed when parent dir is later backed up
  • Phase 3: nested empty directories (a/b/c)
  • Phase 4: no regression without --backup-dir — empty dir still deleted
  • Phase 5: --delete-delay variant — backup path reached for queued deletions

this demonstrates a subtle bug in PR 967
@tridge

tridge commented Jun 5, 2026

Copy link
Copy Markdown
Member

@x15sr71 thanks for the PR! There is a subtle bug with it in the handling of files that should not go into the backup dir. I've added another test to the PR that demonstrates the bug

@x15sr71

x15sr71 commented Jun 6, 2026

Copy link
Copy Markdown
Author

Thanks for the catch! Switched to rmdir-first, as if anything survived inside it naturally fails, then mkdir in backup-dir. All tests pass.

@x15sr71

x15sr71 commented Jun 6, 2026

Copy link
Copy Markdown
Author

Pushed a fix for the Solaris CI failure in the last CI run - Solaris returns EEXIST instead of ENOTEMPTY from rmdir() on non-empty directories, both are POSIX-valid. Added EEXIST to the existing check so it maps to DR_NOT_EMPTY correctly. Updated the PR description. Could you approve the workflows when you get a chance?

@x15sr71

x15sr71 commented Jun 14, 2026

Copy link
Copy Markdown
Author

@tridge The NetBSD failure is in daemon-access, the wo module was missing from the daemon listing, which is unrelated to this PR's delete/backup path. Both backup-empty-dir and backup-pinned-dir pass on NetBSD. Could you re-run it when you get a chance to see if it reproduces? Thanks!

@x15sr71

x15sr71 commented Jul 11, 2026

Copy link
Copy Markdown
Author

@tridge, pinging in case this got buried — the run results are no longer visible on the PR checks tab, so linking the only failed run from the last workflow run for reference: NetBSD run #394

Could you approve a workflow re-run when you get a chance? Thanks!

P.S. Happy to rebase and squash the merge commits if you'd like — the history would just be your test commit and mine.

Comment thread delete.c
Comment on lines 163 to +167
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))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removes fbuf before backup creation is known to succeed. If get_backup_name() returns NULL or do_mkdir_at() fails, the directory is gone but ok remains true and deletion reports success. ACCESSPERMS also loses the original directory mode. Please handle backup failure without reporting success and preserve the original attributes with failure and mode-preservation tests.

Comment on lines +4 to +5
# 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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still describes the superseded rename implementation. Please update it to describe the protected-directory regression the final test guards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

--backup does not keep empty directories in --backup-dir

3 participants