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

[x15sr71]

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

[tridge]

@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]

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]

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]

@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]

@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.

[steadytao]

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.

[steadytao]

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

[tridge]

Automated review — generated by OpenAI Codex. Findings were reproduced
locally but not exhaustively hand-verified.

The rmdir-first direction is sound (the successful rmdir() is the atomic
emptiness proof, so a non-empty or protected-survivor directory is left intact),
and the Solaris EEXIST handling is correct (POSIX permits EEXIST or
ENOTEMPTY for a non-empty rmdir()/unlinkat(AT_REMOVEDIR); the wrapper's
parent-open path has no other normal source of EEXIST). Nested a/b/c works —
deletion is deepest-first, so when c is removed the destination parents still
exist and get_backup_name()/copy_valid_path() create backup/a/b; no ENOENT
gap. The make_backups > 0 / DEL_FOR_BACKUP / backup_dir guards are correct,
and --delete-delay/--delete-after/--force all reach the same path.

Three issues should be fixed before this is release-ready; the first two are
data-loss paths.

1. Backup failure after rmdir() is treated as success

In delete_item()'s S_ISDIR branch, a successful rmdir() sets ok, but a
subsequent failure of get_backup_name() or do_mkdir_at() never clears it.
Reproduced:

• Unwritable --backup-dir: the empty directory is removed, no backup is
  created, a warning is printed, and rsync exits 0.
• Blocked intermediate backup component: get_backup_name() returns NULL
  after the directory was already removed; rsync exits 23 but the directory is
  already gone.

File contents cannot be lost (the directory was proven empty), but the directory
object and its metadata are lost despite --backup. This differs from the
non-directory path, where a make_backup() failure aborts the deletion. Suggest
creating the backup directory before the rmdir() (so a failure leaves the
source in place), or otherwise propagating the failure instead of deleting.

2. EEXIST from the backup mkdir() accepts a non-directory

The EEXIST/EISDIR tolerance does not check the existing object's type.
Reproduced: with a regular file already at backup-dir/empty, dst/empty/ is
deleted, the stale regular file remains, no directory backup is created, and
rsync exits 0. Tolerating an existing directory is required (children may have
been backed up during the same recursive deletion), but a regular file or symlink
must not be accepted — it needs type validation and DEL_FOR_BACKUP collision
handling (or failure propagation).

3. Directory attributes are not preserved

The leaf is recreated with ACCESSPERMS only; an -aAX probe changed an empty
directory from mode 0701 to 0775 and lost its mtime, xattrs, and ownership.
Nested parents fare only slightly better: copy_valid_path() transfers their
attributes initially, but creating child backup directories dirties their mtimes,
and the EEXIST path does not restore them, so nested directory mtimes are lost.
Since copy_valid_path() explicitly attempts to transfer directory settings,
this reads as a defect rather than an intentional placeholder-directory policy.

Tests

Both new tests pass on HEAD and are non-vacuous (basic case, child-collision
preservation, nested dirs, suffix-mode regression, --delete-delay). Missing
coverage: an existing regular file/symlink at the backup leaf; a backup
mkdir()/get_backup_name() failure and its exit code; and mode/owner/mtime/
xattr/ACL preservation. backup-pinned-dir's comments still describe the earlier
rename-based implementation and should be updated.

[dr-who]

I went through this one carefully — reproduced the bug, mutation-tested the new hunk against the tests that ship with it, and checked whether the existing backup machinery could do the job instead. Summary: the shape of the fix is right, but it is partial — the backed-up directory loses its mode and timestamps.

The bug is real, and this code is needed

Confirmed on master: with --backup --backup-dir --delete, dst/sub/file.txt lands in the backup dir but dst/sub/edir/ is silently dropped.

I also checked whether routing directories through the existing make_backup() would be the cleaner fix, since it already handles the backup-dir path creation and starts with link_or_rename() — and rename() moves a directory wholesale, metadata and all. It would be wrong here. A directory that still holds a protect-filtered survivor must stay put; rename() would move it and the protected file into the backup tree. The rmdir()-first approach in this PR is the correct emptiness proof, and I verified master and this PR both keep the protected file in place. So the structure is right.

The gap: metadata is not preserved

do_mkdir_at(buf, ACCESSPERMS) creates a fresh directory rather than preserving the one being deleted:

	mode (orig 0751)	mtime (orig 2019-03-04)
master	dir lost entirely	—
this PR	0755	today

For --backup, whose purpose is to keep what was there, that seems worth fixing. It is a small change — capture the stat before the rmdir() (which is what destroys it), then use it:

STRUCT_STAT dir_st;
int want_backup = make_backups > 0 && !(flags & DEL_FOR_BACKUP) && backup_dir;
int have_st = want_backup && do_lstat_at(fbuf, &dir_st) == 0;

what = "rmdir";
ok = do_rmdir_at(fbuf) == 0;
if (ok && want_backup) {
        char *buf = get_backup_name(fbuf);
        mode_t bmode = have_st ? dir_st.st_mode & CHMOD_BITS : ACCESSPERMS;
        if (buf && do_mkdir_at(buf, bmode) == 0) {
                if (have_st)
                        set_times(buf, &dir_st);
                ...

I tried that locally: mode 0751 and mtime 2019-03-04 both preserved, this PR's own backup-empty-dir and backup-pinned-dir tests still pass, full suite 109 passed / 0 failed.

Mutation testing of the new hunk

Eleven small edits to the added code, each rebuilt and run against backup-empty-dir, backup-pinned-dir, backup, backup-deep, delete, delete-deep. 4 killed, 7 survived. The interesting survivors:

• ACCESSPERMS → 0 (mode 000) and → 0700 both survive. Nothing asserts anything about the backed-up directory's mode or times — the tests only check that the path exists. That is the gap above, confirmed from the other direction.
• Reverting the Solaris || errno == EEXIST survives everything, which is expected — Linux rmdir() returns ENOTEMPTY, so no test on this CI can exercise it. It is an unverifiable change riding along with an unrelated fix; it may be worth splitting out so it can be justified on its own.
• Dropping EEXIST/EISDIR from the mkdir-failure filter survives, so the Phase-2 collision case isn't actually asserting that no spurious warning is emitted.

One I want to explicitly not overclaim: dropping the ok && guard also survived, but I could not construct a case where it changes behaviour — a directory holding a protected file returns DR_NOT_EMPTY from delete_dir_contents() and goto check_ret happens before this block is reached. So that guard looks defensive rather than load-bearing.

Suggestions

1. Preserve the directory's mode and times (patch above) — otherwise the "backup" is a new empty directory rather than the one that was deleted.
2. Add a mode/mtime assertion to the test; without one, the metadata behaviour is entirely unpinned (two mutants prove it).
3. Consider splitting the Solaris EEXIST change into its own commit, since nothing here can verify it.
4. Minor, and possibly deliberate: --backup without --backup-dir (suffix mode) still drops empty dirs, since the new block requires backup_dir. The file branch handles both (backup_dir || !is_backup_file(...)), so the asymmetry may be worth a note either way.