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

[mgutt]

rsync deletes empty directories instead of moving them to the --backup-dir if --backup is used.

Version

root@atum:/mnt/disk1/test# rsync -V
rsync  version 3.4.1  protocol version 32

Test

# mkdir /tmp/src
# mkdir /tmp/dst
# mkdir /tmp/dst/sub
# mkdir /tmp/dst/sub/empty_dir
# touch /tmp/dst/sub/file.txt
# mkdir /tmp/backup_dir
# find /tmp/dst
/tmp/dst
/tmp/dst/sub
/tmp/dst/sub/file.txt
/tmp/dst/sub/empty_dir
# rsync -r --itemize-changes --delete --backup --backup-dir="/tmp/backup_dir/" /tmp/src/ /tmp/dst
*deleting   sub/empty_dir/
*deleting   sub/file.txt
*deleting   sub/

Result:

# find /tmp/backup_dir/
/tmp/backup_dir/
/tmp/backup_dir/sub
/tmp/backup_dir/sub/file.txt

Expected result:

# find /tmp/backup_dir/
/tmp/backup_dir/
/tmp/backup_dir/sub
/tmp/backup_dir/sub/empty_dir
/tmp/backup_dir/sub/file.txt

Research

It seems this bug is already known since 2014:
https://bugzilla.samba.org/show_bug.cgi?id=10724#c3

I wonder if that commit was trying to fix another bug that exists in at least 3.1.0, and possibly other versions. When an empty directory is deleted from source but exists in target, and the --delete option is enabled, the empty directory is not placed in --backup-dir. Going to see if I can figure out what exactly the commit changed.

But it seems nobody fixed this or opened an issue for that.

[x15sr71]

I traced the issue to delete_item() in delete.c where directories are routed directly to do_rmdir() without backup logic. I believe a minimal fix is to mirror the file backup check for S_ISDIR, attempting a rename into --backup-dir before deletion. I can put together a patch and tests if this approach sounds acceptable.

[steadytao]

I looked through the current delete/backup path and the behaviour matches the code as delete_item() routes directories directly to do_rmdir_at() while the make_backup() path is only used for non-directories.

One caution for a fix, simply calling make_backup() for every directory delete looks risky. With --backup-dir, child files can already have created backup_dir/sub/...; later trying to back up the parent directory sub/ itself would collide with that existing backup directory. If the collision handling deletes the backup target first it could remove the child backups that were just preserved.

So the safe shape probably needs to be narrower, e.g. backing up a deleted directory only when the backup destination for that directory does not already exist, or otherwise preserving existing backup-dir contents and just removing the now-empty source directory.

Happy to work on this if that direction sounds right to either of you @mgutt / @x15sr71

[x15sr71]

@steadytao your collision analysis was spot on - the fix handles exactly that case by falling through to rmdir() when the backup destination already exists from child file backups. Opened a PR at #967 that incorporates that approach. Would appreciate your review if you have time.