RFC / proposed fix for #530: --compare-dest leaves behind directories with no changes

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

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.

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