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")