From 946aa934cc8bc615484ed7dddca1eb40cd29edf7 Mon Sep 17 00:00:00 2001 From: TapTap Date: Thu, 17 Sep 2026 01:00:55 +0200 Subject: [PATCH] fix(delete): scope -R per-directory delete walk to the transferred prefix The -R prefix marker installed in synced_dirs was discarded when finalizing the per-directory delete sender (--delete-during/--delete-delay), so the up-front root plan was the receive root '.', whose keep list only held the first prefix component. The receiver then deleted destination content outside the transferred prefix (e.g. unrelated/keep.txt), a data-loss bug; rsync keeps it. Confine the walk to the -R prefix: send that prefix's plan as the root plan, only transmit plans at or below it, and never emit the receive root plan for a scoped run. Add a differential test covering both --delete-during and --delete-delay. --- src/client/client_send.c | 25 ++++++- src/shared/delete_plan.c | 31 +++++++-- src/shared/delete_plan.h | 11 ++- tests/integration/test_parity_blockers.py | 82 +++++++++++++++++++++++ 4 files changed, 139 insertions(+), 10 deletions(-) create mode 100644 tests/integration/test_parity_blockers.py diff --git a/src/client/client_send.c b/src/client/client_send.c index 49dd445..7a0046c 100644 --- a/src/client/client_send.c +++ b/src/client/client_send.c @@ -454,6 +454,21 @@ static char* delete_scope_root_marker(const Config* config) { return str_dup("."); } +/* The -R destination prefix that confines a per-directory delete walk, or NULL + * when the whole receive root is in scope. The marker was installed into + * `synced_dirs` by delete_scope_root_marker(); for a plain recursive transfer + * it is "." (whole root) and for --files-from the list is not a single prefix. */ +static const char* delete_plan_walk_root(const Config* config, const ArrayList* synced_dirs) { + if (!config || config->files_from_set != NULL || !config->relative || !config->send_directory) + return NULL; + if (!synced_dirs || synced_dirs->size != 1) + return NULL; + const char* marker = (const char*)synced_dirs->items[0]; + if (marker[0] == '\0' || strcmp(marker, ".") == 0) + return NULL; + return marker; +} + /* True when some --files-from entry is an ancestor-or-equal directory of * `rel` (an empty entry -- the whole tree "." -- counts as the root). */ static bool file_list_ancestor_listed(const FileListSet* set, const char* rel) { @@ -2898,7 +2913,9 @@ int send_files(Config* config) { bool prescan_ok = scan_paths_only(config, &prepared.options, NULL, plan_sender, &had_scan_io); bool plans_ok = false; if (prescan_ok) { - delete_plan_sender_finalize(plan_sender, config->files_from_set ? synced_dirs : NULL); + const char* walk_root = delete_plan_walk_root(config, synced_dirs); + const ArrayList* scope = config->files_from_set ? synced_dirs : (walk_root ? synced_dirs : NULL); + delete_plan_sender_finalize(plan_sender, scope, walk_root); delete_plan_sender_set_config(plan_sender, excluded, size_skipped, missing_args); if (had_scan_io && delete_plan_sender_empty(plan_sender)) { log_message(LOG_LEVEL_ERROR, @@ -3261,8 +3278,10 @@ int send_files_multithreaded(Config** config_ptr) { context->delete_plans, &context->scan_had_io_error); prepared_scanner_destroy(&prepared); if (per_dir && prebuilt) { - delete_plan_sender_finalize(context->delete_plans, - config->files_from_set ? context->synced_dirs : NULL); + const char* walk_root = delete_plan_walk_root(config, context->synced_dirs); + const ArrayList* scope = + config->files_from_set ? context->synced_dirs : (walk_root ? context->synced_dirs : NULL); + delete_plan_sender_finalize(context->delete_plans, scope, walk_root); delete_plan_sender_set_config(context->delete_plans, context->excluded_paths, context->size_skipped_paths, context->missing_args); } diff --git a/src/shared/delete_plan.c b/src/shared/delete_plan.c index 23e7a2b..fbab423 100644 --- a/src/shared/delete_plan.c +++ b/src/shared/delete_plan.c @@ -42,6 +42,10 @@ struct DeletePlanSender { bool config_sent; bool all_synced; const ArrayList* synced_dirs; + /* Owned by the caller's synced_dirs list; non-NULL only for a general -R + transfer, where it is the destination prefix the delete walk is confined + to. NULL means the whole receive root (or a --files-from scope). */ + const char* walk_root; const ArrayList* protected_prefixes; const ArrayList* size_skipped; const ArrayList* missing_args; @@ -260,11 +264,13 @@ bool delete_plan_sender_add(DeletePlanSender* sender, const char* path, bool is_ return ok; } -void delete_plan_sender_finalize(DeletePlanSender* sender, const ArrayList* synced_dirs) { +void delete_plan_sender_finalize(DeletePlanSender* sender, const ArrayList* synced_dirs, + const char* walk_root) { if (!sender) return; sender->synced_dirs = synced_dirs; - sender->all_synced = synced_dirs == NULL; + sender->all_synced = synced_dirs == NULL && walk_root == NULL; + sender->walk_root = walk_root; } bool delete_plan_sender_empty(const DeletePlanSender* sender) { @@ -280,9 +286,20 @@ void delete_plan_sender_set_config(DeletePlanSender* sender, const ArrayList* pr sender->missing_args = missing_args; } +/* True when `dir` is `root` itself or a descendant of it (path-component + * aware, so "foo" does not match "foobar"). */ +static bool path_at_or_under(const char* dir, const char* root) { + if (!dir || !root) + return false; + size_t n = strlen(root); + return strncmp(dir, root, n) == 0 && (dir[n] == '\0' || dir[n] == '/'); +} + static bool plan_is_allowed(const DeletePlanSender* sender, const char* dir) { if (sender->all_synced) return true; + if (sender->walk_root) + return path_at_or_under(dir, sender->walk_root); return list_contains_str(sender->synced_dirs, dir); } @@ -329,9 +346,10 @@ static int send_prefix_plan(int fd, DeletePlanSender* sender, const char* dir) { int delete_plan_send_root(int fd, DeletePlanSender* sender) { if (!sender) return -1; - if (!plan_ensure(sender, ".")) + const char* root = sender->walk_root ? sender->walk_root : "."; + if (!plan_ensure(sender, root)) return -1; - return send_prefix_plan(fd, sender, "."); + return send_prefix_plan(fd, sender, root); } int delete_plan_send_for_path(int fd, DeletePlanSender* sender, const char* path, bool is_dir) { @@ -340,7 +358,10 @@ int delete_plan_send_for_path(int fd, DeletePlanSender* sender, const char* path char* clean = plan_clean_path(path); if (!clean) return -1; - int rc = send_prefix_plan(fd, sender, "."); + /* The walk root (the -R prefix, or ".") is sent up front by + delete_plan_send_root(); never emit the receive-root plan for a scoped -R + run, whose "." keep list would delete the prefix's siblings. */ + int rc = sender->walk_root ? 0 : send_prefix_plan(fd, sender, "."); if (rc == 0 && *clean != '\0') { size_t len = strlen(clean); size_t end = len; diff --git a/src/shared/delete_plan.h b/src/shared/delete_plan.h index 8abede3..bcc6d16 100644 --- a/src/shared/delete_plan.h +++ b/src/shared/delete_plan.h @@ -34,8 +34,15 @@ void delete_plan_sender_destroy(DeletePlanSender* sender); bool delete_plan_sender_add(DeletePlanSender* sender, const char* path, bool is_dir); /* Drop plans for directories outside `synced_dirs` (the --files-from * synchronization scope; pass NULL when a full recursive transfer synchronized - * every directory). The receive root is the "." sentinel. */ -void delete_plan_sender_finalize(DeletePlanSender* sender, const ArrayList* synced_dirs); + * every directory). The receive root is the "." sentinel. + * + * `walk_root` scopes a general -R transfer: when non-NULL it is the + * reconstructed destination prefix the run actually transferred, and only the + * plan for that prefix (and directories below it) is ever transmitted, so the + * prefix's parent-directory siblings are never walked. Pass NULL for a plain + * recursive transfer and for --files-from. */ +void delete_plan_sender_finalize(DeletePlanSender* sender, const ArrayList* synced_dirs, + const char* walk_root); /* True when no transmitted entry was recorded (an ambiguous empty scan). */ bool delete_plan_sender_empty(const DeletePlanSender* sender); /* Attach the global config sections advertised on the first plan frame. */ diff --git a/tests/integration/test_parity_blockers.py b/tests/integration/test_parity_blockers.py new file mode 100644 index 0000000..a85f1d0 --- /dev/null +++ b/tests/integration/test_parity_blockers.py @@ -0,0 +1,82 @@ +"""Differential/regression coverage for the parity-completion review blockers. + +Each test pins a fix against real ``rsync 3.4.1`` where a deterministic +comparison exists; the differential tests skip cleanly when rsync is absent. +""" +import os +import shutil +import subprocess +import sys + +import pytest + +sys.path.insert(0, os.path.dirname(__file__)) +from common import ( # noqa: E402 + TEST_DATA_DIR, + ServerManager, + clean_dir, + get_dest_received_dir, + run_client, +) + +RSYNC = shutil.which("rsync") +requires_rsync = pytest.mark.skipif(RSYNC is None, reason="rsync 3.4.1 not installed") + + +def _write(path, content): + os.makedirs(os.path.dirname(path), exist_ok=True) + with open(path, "wb") as fh: + fh.write(content) + + +def _tree(root): + """Sorted relative paths of every entry below root (files and dirs).""" + out = [] + for dirpath, dirs, files in os.walk(root): + for name in dirs: + out.append(os.path.relpath(os.path.join(dirpath, name), root)) + for name in files: + out.append(os.path.relpath(os.path.join(dirpath, name), root)) + return sorted(out) + + +def _rsync(args): + env = dict(os.environ, LC_ALL="C") + return subprocess.run([RSYNC] + args, capture_output=True, text=True, env=env, timeout=120) + + +class TestRelativePerDirDeleteScope: + """Blocker #1: -R --delete-during/--delete-delay must not delete destination + content outside the transferred prefix (rsync keeps sibling directories).""" + + @requires_rsync + @pytest.mark.ci + @pytest.mark.parametrize("timing", ["--delete-during", "--delete-delay"]) + def test_prefix_scoped_delete_matches_rsync(self, timing): + source = os.path.join(TEST_DATA_DIR, "delblk_src") + clean_dir(source) + _write(os.path.join(source, "foo", "a.txt"), b"payload\n") + spec = source + "/./foo" + + def seed(root): + clean_dir(root) + _write(os.path.join(root, "foo", "extra.txt"), b"stale\n") + _write(os.path.join(root, "unrelated", "keep.txt"), b"keep\n") + + rdst = os.path.join(TEST_DATA_DIR, "delblk_rdst") + dest = os.path.join(TEST_DATA_DIR, "delblk_dst") + seed(rdst) + seed(dest) + r = _rsync(["-aR", timing, spec, rdst + "/"]) + assert r.returncode == 0, r.stderr + with ServerManager() as server: + server.start(extra_args=["--allow-delete"]) + result, _ = run_client(spec, dest, flags=["-a", "-R", timing], port=server.port) + assert result.returncode == 0, (result.stderr or result.stdout)[:300] + # The prefix's parent-directory sibling survives on both sides. + assert os.path.isfile(os.path.join(dest, "unrelated", "keep.txt")) + assert os.path.isfile(os.path.join(rdst, "unrelated", "keep.txt")) + # The in-scope extra is removed on both sides. + assert not os.path.exists(os.path.join(dest, "foo", "extra.txt")) + assert not os.path.exists(os.path.join(rdst, "foo", "extra.txt")) + assert _tree(dest) == _tree(rdst)