From 1b67f5ffcfef1ac3dc38c27751cd4e534dd8dbc9 Mon Sep 17 00:00:00 2001 From: TapTap Date: Sun, 6 Sep 2026 12:49:49 +0200 Subject: [PATCH] docs: clarify %b semantics, temper thread-safety claim, harden %M scan; add -m coverage Address c-review nits on the itemize/output feature: - RSYNC_COMPAT.md: state that %b is the source length (always == %l) because no wire-byte counter exists; keep Summary equal to the matrix (recounted: 54 implemented / 84 not-implemented, 147 rows total - four rows flipped). - change_list.h/.c: document bytes_sent == size; note itemize/out-format lines never interleave with each other but may interleave with legacy log messages sharing the stream; mark the %M stat() path best-effort. - change_render_format scan in format_uses_mtime now mirrors the tokenizer (skips '%%' and unknown '%X' pairs) so a literal '%%M' no longer triggers the stat() fallback. - Integration tests: --list-only under -m; a changed file on a second --incremental run emits exactly one '>f' line while unchanged files print nothing; --log-file + --log-file-format under -m. --- RSYNC_COMPAT.md | 4 +- src/client/change_list.c | 22 ++++++++-- src/client/change_list.h | 22 ++++++---- tests/integration/test_features.py | 67 ++++++++++++++++++++++++++++++ 4 files changed, 101 insertions(+), 14 deletions(-) diff --git a/RSYNC_COMPAT.md b/RSYNC_COMPAT.md index 81e9cc2..2adbadd 100644 --- a/RSYNC_COMPAT.md +++ b/RSYNC_COMPAT.md @@ -41,9 +41,9 @@ This document maps rsync's full feature set to FastSync's current implementation | `-i`, `--itemize-changes` | Per-file change summary | ✅ Implemented | Prints rsync-style `>f+++++++++` lines to stdout only for files actually sent (also under `-m`); unchanged files print nothing, matching single-`-i` behavior | | `--progress` | Show progress | ✅ Implemented | Progress callback in sender | | `-P` | Same as --partial --progress | ⚠️ Partial | Parses and enables progress, but interrupted files are not retained for resumable transfers | -| `--out-format=FORMAT` | Custom output format | ✅ Implemented | Per-transfer template on stdout; tokens `%f` `%n` `%l` `%b` `%M` `%%`; unknown escapes preserved | +| `--out-format=FORMAT` | Custom output format | ✅ Implemented | Per-transfer template on stdout; tokens `%f` `%n` `%l` `%b` `%M` `%%` (`%b` is the source length, always `== %l`; post-compression/delta wire bytes are not counted); unknown escapes preserved | | `--log-file=FILE` | Log to file | ✅ Implemented | `log_file` config field | -| `--log-file-format=FMT` | Log format | ✅ Implemented | Requires `--log-file`; writes one template line per transferred file using the same token set as `--out-format` | +| `--log-file-format=FMT` | Log format | ✅ Implemented | Requires `--log-file`; writes one template line per transferred file using the same token set as `--out-format` (including `%b` `==` source length) | | `--8-bit-output`, `-8` | Leave high-bit chars unescaped | ✅ Implemented | Applies to displayed paths and protocol debug output | | `--list-only` | List files instead of copying | ✅ Implemented | `ls -l`-style listing of files that would be transferred; scans the source only, contacts no server, writes nothing; also works with `-n` | diff --git a/src/client/change_list.c b/src/client/change_list.c index 51b1687..2467035 100644 --- a/src/client/change_list.c +++ b/src/client/change_list.c @@ -266,9 +266,20 @@ void change_emit(const Config* config, const ChangeEvent* event) { static bool format_uses_mtime(const char* format) { if (format == NULL) return false; - for (const char* p = format; *p != '\0'; p++) { - if (p[0] == '%' && p[1] == 'M') + /* Mirror change_render_format's tokenizer: "%%" is a literal percent (so + * "%%M" does NOT expand %M) and unknown "%X" escapes consume both chars. + * This keeps the optional stat() fallback below in step with the renderer. */ + for (const char* p = format; *p != '\0';) { + if (*p != '%') { + p++; + continue; + } + char token = p[1]; + if (token == '\0') + break; + if (token == 'M') return true; + p += 2; } return false; } @@ -282,12 +293,15 @@ void change_emit_file_sent(const Config* config, const File* file) { event.decision = CHANGE_SENT; event.is_directory = false; event.size = file->data != NULL ? file->data->size : 0; - /* FastSync does not currently count post-compression/delta wire bytes, so - * the reported value is the source length that had to be delivered. */ + /* FastSync has no wire-byte counter yet, so %b reports the source length + * that had to be delivered (always equal to %l); the actual bytes written + * to the socket (compressed/delta) are not measured. */ event.bytes_sent = event.size; if (file->metadata != NULL) { event.mtime_sec = file->metadata->mtime_sec; } else if (format_uses_mtime(config->out_format) || format_uses_mtime(config->log_file_format)) { + /* Best-effort fallback for %M when no metadata was captured (no -M): the + * path is stat()ed just to fill the field, and any failure leaves 0. */ struct stat st; if (file->path != NULL && stat(file->path, &st) == 0) event.mtime_sec = st.st_mtime; diff --git a/src/client/change_list.h b/src/client/change_list.h index b3088b8..499bcaf 100644 --- a/src/client/change_list.h +++ b/src/client/change_list.h @@ -14,8 +14,10 @@ * FastSync is a push-style tool: the client sends files from the source tree * to a server that writes them under the destination root. Events are * emitted by whichever code path decides a file's fate (the single-threaded - * send loop and the `-m` sender thread both call the same per-file sender, so - * only that one thread ever reports events - no cross-thread printing races). + * send loop and the `-m` sender thread both call the same per-file sender), so + * all change events are emitted by exactly one thread and itemize/out-format + * lines never interleave with each other. They may still interleave with + * legacy log messages (log.c) that share the same stdout/log-file stream. */ typedef enum { @@ -27,9 +29,12 @@ typedef struct { const char* path; /* full source path */ ChangeDecision decision; bool is_directory; - unsigned long long size; /* source file length in bytes */ - unsigned long long bytes_sent; /* payload bytes sent (best effort) */ - time_t mtime_sec; /* 0 when unknown */ + unsigned long long size; /* source file length in bytes */ + /* The number of bytes reported for a sent file. FastSync has no wire-byte + * counter, so this is always the source length (== size / %l); actual + * post-compression/delta bytes on the wire are not counted. */ + unsigned long long bytes_sent; + time_t mtime_sec; /* 0 when unknown */ } ChangeEvent; /* True when any output mode is active and per-file events matter. */ @@ -46,9 +51,10 @@ bool change_list_enabled(const Config* config); char* change_render_itemize(const ChangeEvent* event); /* Expand an --out-format/--log-file-format template. Tokens: - * %f full source path %b bytes sent (== %l for a whole file) - * %n leaf (base) name %M mtime in whole seconds since the epoch - * %l file length in bytes %% a literal percent sign + * %f full source path %b "bytes sent" == the source length (%l); + * %n leaf (base) name actual post-compression/delta wire bytes + * %l file length in bytes are not counted + * %M mtime in whole seconds %% a literal percent sign * Unknown %X sequences are preserved verbatim. Caller frees the result. */ char* change_render_format(const char* format, const ChangeEvent* event); diff --git a/tests/integration/test_features.py b/tests/integration/test_features.py index bfb51b7..800ecbc 100644 --- a/tests/integration/test_features.py +++ b/tests/integration/test_features.py @@ -1051,6 +1051,15 @@ class TestListOnly: received = get_dest_received_dir(DEST_DIR, SOURCE_DIR) assert not os.path.exists(received) + def test_list_only_multithreaded(self): + clean_dir(DEST_DIR) + result, _ = run_client(SOURCE_DIR, DEST_DIR, flags=["--list-only", "-m"]) + assert result.returncode == 0, f"list-only -m failed: {result.stderr[:200]}" + for full_path in _source_files(): + assert full_path in result.stdout, f"list-only -m omitted {full_path}" + received = get_dest_received_dir(DEST_DIR, SOURCE_DIR) + assert not os.path.exists(received), "list-only -m wrote to the destination" + class TestItemizeChanges: """-i/--itemize-changes prints rsync-style lines only for files sent.""" @@ -1091,6 +1100,38 @@ class TestItemizeChanges: result, _ = run_client(SOURCE_DIR, DEST_DIR, flags=["-i", "--dry-run"]) assert result.returncode == 0, f"dry-run -i failed: {result.stderr[:200]}" + def test_changed_file_on_second_incremental_run_prints_exactly_one_line(self, shared_server): + """A changed file itemizes exactly once on an incremental rerun while + unchanged files print nothing (no double emission).""" + source = os.path.join(TEST_DATA_DIR, "itemize_change_src") + dest = os.path.join(TEST_DATA_DIR, "itemize_change_dst") + clean_dir(source) + clean_dir(dest) + changed = os.path.join(source, "changed.txt") + untouched = os.path.join(source, "untouched.txt") + with open(changed, "wb") as fh: + fh.write(b"original\n") + with open(untouched, "wb") as fh: + fh.write(b"stable\n") + + result, _ = run_client(source, dest, flags=["-M"], port=shared_server.port) + assert result.returncode == 0, f"seed sync failed: {result.stderr[:200]}" + + with open(changed, "wb") as fh: + fh.write(b"edited payload\n") + + result, _ = run_client(source, dest, + flags=["-M", "-i", "--incremental"], + port=shared_server.port) + assert result.returncode == 0, f"incremental itemize failed: {result.stderr[:200]}" + itemized = [line for line in result.stdout.splitlines() if line.startswith(">f")] + assert itemized == [">f+++++++++ " + changed], ( + f"expected exactly one itemize line for {changed}, got {itemized}" + ) + received = get_dest_received_dir(dest, source) + assert _read_file(os.path.join(received, "changed.txt")) == b"edited payload\n" + assert _read_file(os.path.join(received, "untouched.txt")) == b"stable\n" + class TestOutFormat: """--out-format prints a line per transferred file using the template.""" @@ -1135,3 +1176,29 @@ class TestLogFileFormat: for line in expected: assert line in content, f"log file missing {line!r}" + def test_log_file_format_multithreaded_writes_transferred_files(self, shared_server): + source = os.path.join(TEST_DATA_DIR, "itemize_log_mt_src") + dest = os.path.join(TEST_DATA_DIR, "itemize_log_mt_dst") + clean_dir(source) + clean_dir(dest) + files = {"a.txt": b"alpha\n", "b.txt": b"beta\n"} + for rel, data in files.items(): + with open(os.path.join(source, rel), "wb") as fh: + fh.write(data) + log_path = os.path.join(TEST_DATA_DIR, "itemize_mt.log") + if os.path.exists(log_path): + os.unlink(log_path) + result, _ = run_client( + source, + dest, + flags=["--log-file", log_path, "--log-file-format=%f %l", "-m"], + port=shared_server.port, + ) + assert result.returncode == 0, f"log-file -m sync failed: {result.stderr[:200]}" + assert os.path.exists(log_path), "--log-file created no log" + with open(log_path, encoding="utf-8", errors="replace") as fh: + content = fh.read() + expected = {f"{os.path.join(source, rel)} {len(data)}" for rel, data in files.items()} + for line in expected: + assert line in content, f"log file (-m) missing {line!r}" +