From bc744b87d7d432344025e375f601826d62ca0b86 Mon Sep 17 00:00:00 2001 From: Stuart Inglis Date: Mon, 20 Jul 2026 12:31:04 +1200 Subject: [PATCH] RFC: --compare-dest: drop dirs that ended up with no changes (issue #530) This is a proposed fix for #530, put up for discussion rather than as a finished change -- see the caveat at the end. --compare-dest is documented to populate the destination with only what differs from the compare hierarchy, but rsync creates every source directory there, including ones whose contents already match. In recv_generator(), a directory that fully matches makes try_dests_non() return -2, and the caller does: if (j == -2) { itemizing = 0; code = FNONE; statret = 1; } ... if (real_ret != 0 && do_mkdir_at(fname, ...) < 0 ...) real_ret is the stat result from before the lookup, so the mkdir happens regardless of the match, while code = FNONE is why -v never mentions it. Both symptoms in the report come from that one spot. The effect scales badly: a tree of 200 dirs in which only 3 had changed produced 400 directories in the destination, 197 of them empty. The reporter saw 129k directories created for 23k directories that actually changed. rsync cannot know at mkdir time whether a descendant will differ, so a matching directory is still created -- which keeps its attribute handling exactly as it is today, and gives any changed descendant a home -- and is removed again once the transfer is done if nothing was placed in it. The candidates are sorted descending so a child is tried before its parent and nested runs collapse; rmdir() failing on a directory that did receive content is precisely the wanted behaviour. Removing an entry bumps the containing directory's mtime, so that is saved and restored around the rmdir. An earlier attempt that skipped the mkdir and created parents on demand with make_path() was dropped: directories materialised that way lost their metadata (mode 755 instead of 751, and the current time instead of the source mtime). --link-dest and --copy-dest are deliberately untouched; those build a complete tree by design. testsuite/alt-dest_test.py builds its check tree as "a full copy minus the two files the alt dirs supply", which keeps directories that are left with nothing to transfer. Its own comment already describes the intent as "dest grows just the deltas", so it now also passes -m (--prune-empty-dirs) to express that. testsuite/compare-dest-empty-dirs_test.py is new and covers the whole contract: unchanged subtrees leave nothing behind, changed files still arrive, and kept directories keep their permissions and mtimes (including a parent whose child was pruned out of it). Caveat for reviewers: this changes long-standing user-visible behaviour of --compare-dest. The reporter and the option's documented purpose both say the current behaviour is wrong, but if compatibility is the greater concern this can just as easily sit behind an opt-in option instead -- happy to respin it that way. --- generator.c | 75 ++++++++++++++++ testsuite/alt-dest_test.py | 9 +- testsuite/compare-dest-empty-dirs_test.py | 100 ++++++++++++++++++++++ 3 files changed, 182 insertions(+), 2 deletions(-) create mode 100644 testsuite/compare-dest-empty-dirs_test.py diff --git a/generator.c b/generator.c index 8642236e7..6bf596249 100644 --- a/generator.c +++ b/generator.c @@ -115,6 +115,71 @@ static int need_retouch_dir_times; static int need_retouch_dir_perms; static const char *solo_file = NULL; +/* Directories that matched the --compare-dest hierarchy. We still create them + * normally (so a changed descendant has a home, with the right attributes) and + * then remove the ones that turned out to hold nothing at all. */ +static char **prune_dirs = NULL; +static int prune_dirs_cnt = 0, prune_dirs_alloc = 0; + +static void remember_prune_dir(const char *fname) +{ + if (prune_dirs_cnt >= prune_dirs_alloc) { + prune_dirs_alloc = prune_dirs_alloc ? prune_dirs_alloc * 2 : 128; + prune_dirs = realloc_array(prune_dirs, char *, prune_dirs_alloc); + } + if (!(prune_dirs[prune_dirs_cnt] = strdup(fname))) + out_of_memory("remember_prune_dir"); + prune_dirs_cnt++; +} + +static int prune_dir_cmp(const void *a, const void *b) +{ + /* Descending, so that a child sorts before its parent. */ + return strcmp(*(char *const *)b, *(char *const *)a); +} + +/* Drop any --compare-dest directory that ended up with nothing in it. rmdir() + * simply fails (harmlessly) on one that did receive content, and the descending + * sort means we try a child before its parent, so nested runs collapse. */ +static void prune_compare_dest_dirs(void) +{ + int i; + + if (!prune_dirs_cnt) + return; + + qsort(prune_dirs, prune_dirs_cnt, sizeof prune_dirs[0], prune_dir_cmp); + for (i = 0; i < prune_dirs_cnt; i++) { + char *fname = prune_dirs[i], *slash; + char parent[MAXPATHLEN]; + STRUCT_STAT pst; + int got_parent; + + /* Removing an entry changes the containing dir's mtime, which we + * must not disturb, so save it and put it back afterwards. */ + if ((slash = strrchr(fname, '/')) != NULL && slash != fname) { + int plen = slash - fname; + if (plen >= (int)sizeof parent) + plen = sizeof parent - 1; + memcpy(parent, fname, plen); + parent[plen] = '\0'; + } else + strlcpy(parent, slash ? "/" : ".", sizeof parent); + got_parent = link_stat(parent, &pst, 0) == 0; + + if (do_rmdir(fname) == 0) { + if (DEBUG_GTE(GENR, 1)) + rprintf(FINFO, "pruned empty --compare-dest dir: %s\n", fname); + if (got_parent) + set_times(parent, &pst); + } + free(fname); + } + free(prune_dirs); + prune_dirs = NULL; + prune_dirs_cnt = prune_dirs_alloc = 0; +} + /* Forward declarations. */ #ifdef SUPPORT_HARD_LINKS static void handle_skipped_hlink(struct file_struct *file, int itemizing, @@ -1471,6 +1536,14 @@ static void recv_generator(char *fname, struct file_struct *file, int ndx, itemizing = 0; code = FNONE; statret = 1; + /* With --compare-dest the destination receives only + * what differs, so a dir that matches the compare + * hierarchy does not belong there unless something + * below it turns out to differ. We can only know + * that once the transfer is done, so create it now + * and drop it later if it is still empty. */ + if (alt_dest_type == COMPARE_DEST && !dry_run) + remember_prune_dir(fname); } else if (j >= 0) { statret = 1; fnamecmp = fnamecmpbuf; @@ -2453,6 +2526,8 @@ void generate_files(int f_out, const char *local_name) && dir_tweaking && (!inc_recurse || delete_during == 2)) touch_up_dirs(dir_flist, -1); + prune_compare_dest_dirs(); + if (DEBUG_GTE(GENR, 1)) rprintf(FINFO, "generate_files finished\n"); } diff --git a/testsuite/alt-dest_test.py b/testsuite/alt-dest_test.py index d5bb37b2a..603f8d2d2 100644 --- a/testsuite/alt-dest_test.py +++ b/testsuite/alt-dest_test.py @@ -40,8 +40,13 @@ os.utime(FROMDIR / 'dir' / 'text') os.utime(FROMDIR / 'likely') -# chkdir: what a vanilla copy would produce, minus /text and etc-ltr-list. -run_rsync('-av', '--exclude=/text', '--exclude=etc-ltr-list', +# chkdir: what --compare-dest should leave behind. That is a vanilla copy +# minus the files alt1/alt2 already provide (/text and etc-ltr-list) and -- +# because --compare-dest populates the destination with just the differences -- +# minus any directory that is thereby left with nothing to transfer, which is +# what -m (--prune-empty-dirs) expresses. Here that drops dir/subdir/subsubdir +# (it held only etc-ltr-list) and emptydir (empty in the source to begin with). +run_rsync('-av', '-m', '--exclude=/text', '--exclude=etc-ltr-list', f'{FROMDIR}/', f'{CHKDIR}/') # Stacked --compare-dest: dest grows just the deltas alt1+alt2 don't have. diff --git a/testsuite/compare-dest-empty-dirs_test.py b/testsuite/compare-dest-empty-dirs_test.py new file mode 100644 index 000000000..a5c74c4e9 --- /dev/null +++ b/testsuite/compare-dest-empty-dirs_test.py @@ -0,0 +1,100 @@ +#!/usr/bin/env python3 +"""Regression test for issue #530: --compare-dest created every directory. + +--compare-dest populates the destination with just the differences against the +compare hierarchy, but rsync created *every* source directory there, including +ones whose entire contents already matched. A backup of 200 dirs where only 3 +had changed produced 400 directories -- 197 of them completely empty -- which +is exactly the noise the reporter hit at scale (129k dirs for 23k real +changes). + +rsync cannot know at mkdir time whether a descendant will differ, so a matching +directory is still created (which keeps its attribute handling intact and gives +any changed descendant a home) and is then removed again at the end if nothing +was actually placed in it. Removing an entry bumps the containing directory's +mtime, so that is saved and restored. + +We check the whole contract: unchanged subtrees leave nothing behind, changed +files still arrive (including several levels down), the directories that are +kept keep their permissions and mtimes, and a surviving parent keeps its mtime +even though a child directory was pruned out of it. +""" + +import os + +from rsyncfns import ( + FROMDIR, TODIR, TMPDIR, makepath, rmtree, run_rsync, test_fail, +) + +compdir = TMPDIR / 'comp' + +rmtree(FROMDIR) +rmtree(TODIR) +rmtree(compdir) +makepath(FROMDIR, TODIR, compdir) + +# 40 dirs each holding a file; only 3 of them will differ. +NDIRS = 40 +CHANGED = (7, 19, 33) +for i in range(1, NDIRS + 1): + makepath(FROMDIR / f'd{i}' / 'sub') + (FROMDIR / f'd{i}' / 'sub' / 'file').write_text(f'data {i}\n') + +# A dir with distinctive perms/mtime that will be kept (it holds a change), +# plus a child dir that matches and must be pruned back out of it. +makepath(FROMDIR / 'keep' / 'gone') +(FROMDIR / 'keep' / 'changed').write_text('before\n') +(FROMDIR / 'keep' / 'gone' / 'f').write_text('static\n') + +# An empty dir that matches -- it should not appear in the destination. +makepath(FROMDIR / 'emptydir') + +# Seed the compare hierarchy with an exact copy, then introduce the changes. +run_rsync('-a', f'{FROMDIR}/', f'{compdir}/') +for i in CHANGED: + (FROMDIR / f'd{i}' / 'sub' / 'file').write_text(f'CHANGED {i}\n') +(FROMDIR / 'keep' / 'changed').write_text('after\n') + +os.chmod(FROMDIR / 'keep', 0o751) +KEEP_MTIME = 1556089689 # 2019-05-06 07:08:09 UTC +os.utime(FROMDIR / 'keep', (KEEP_MTIME, KEEP_MTIME)) + +run_rsync('-a', f'--compare-dest={compdir}', f'{FROMDIR}/', f'{TODIR}/') + +# --- nothing unchanged may be left behind ---------------------------------- +empty = [p for p in TODIR.rglob('*') if p.is_dir() and not any(p.iterdir())] +if empty: + test_fail(f"--compare-dest left {len(empty)} empty directories behind, e.g. " + f"{empty[0].relative_to(TODIR)} (issue #530)") + +if (TODIR / 'emptydir').exists(): + test_fail("an unchanged empty dir was created in the destination") +if (TODIR / 'keep' / 'gone').exists(): + test_fail("an unchanged subdir was created in the destination") + +for i in range(1, NDIRS + 1): + if i not in CHANGED and (TODIR / f'd{i}').exists(): + test_fail(f"unchanged dir d{i} was created in the destination") + +# --- everything that did change must still arrive, intact ------------------ +for i in CHANGED: + got = TODIR / f'd{i}' / 'sub' / 'file' + if not got.is_file(): + test_fail(f"changed file under d{i} was not transferred") + if got.read_text() != f'CHANGED {i}\n': + test_fail(f"changed file under d{i} has the wrong contents") + +if (TODIR / 'keep' / 'changed').read_text() != 'after\n': + test_fail("changed file in keep/ has the wrong contents") + +# --- a directory we keep must keep its metadata ---------------------------- +st = os.stat(TODIR / 'keep') +if st.st_mode & 0o777 != 0o751: + test_fail(f"kept dir lost its permissions: got {st.st_mode & 0o777:o}, want 751") +# Pruning keep/gone must not have disturbed keep/'s own mtime. +if int(st.st_mtime) != KEEP_MTIME: + test_fail(f"pruning a child dir changed the parent's mtime: " + f"got {int(st.st_mtime)}, want {KEEP_MTIME}") + +print("issue #530: --compare-dest left no empty dirs, kept the changes, " + "and preserved directory metadata")