Incremental sync: --incremental to skip unchanged files #14

2026-07-16T16:28:57+02:00

TapTap commented

2026-07-16 16:28:57 +02:00

New --incremental flag that queries the server before sending each file and skips files that already exist with matching size and mtime.

New STATUS_CHECK protocol status (value 6)
Client sends path + size + mtime; server replies OK (skip) or NEXT (send)
config.h/c: use_incremental field sent/received over wire
file.c/h: file_send_single_calls_no_path helper for incremental path
client_cli.c: --incremental flag
client_send.c: per-file check before send in both single and multi-threaded paths
server.c: STATUS_CHECK handling in receive_files()
multiprocessing.c: STATUS_CHECK handling in receive_thread()
test.py: incremental sync test case (31 tests total)

All 7 unit tests and 31 integration tests pass with zero warnings.

New --incremental flag that queries the server before sending each file and skips files that already exist with matching size and mtime. - New STATUS_CHECK protocol status (value 6) - Client sends path + size + mtime; server replies OK (skip) or NEXT (send) - config.h/c: use_incremental field sent/received over wire - file.c/h: file_send_single_calls_no_path helper for incremental path - client_cli.c: --incremental flag - client_send.c: per-file check before send in both single and multi-threaded paths - server.c: STATUS_CHECK handling in receive_files() - multiprocessing.c: STATUS_CHECK handling in receive_thread() - test.py: incremental sync test case (31 tests total) All 7 unit tests and 31 integration tests pass with zero warnings.

TapTap commented

2026-07-16 16:49:40 +02:00

Code Review: Incremental sync

Overall the approach is solid — per-file SIZE+mtime check before transfer is the right design. Below are the issues I found.

BUGS

1. Protocol mismatch: --sendfile + --incremental
client_send.c:58-68 → file.c:144

When use_sendfile && use_incremental are both true and a file needs re-transfer (STATUS_NEXT), the code falls through to file_send_sendfile(), which sends: path + metadata + file_size + file_data. But the server's STATUS_CHECK handler expects only: metadata + data (no path — it already has it from the CHECK message). The redundant send_str(path) at file.c:145 will be misinterpreted by the server, corrupting the protocol stream.

Fix: create a file_send_sendfile_no_path() variant (like file_send_single_calls_no_path), or restructure to not call file_send_sendfile after a CHECK.

2. --chunk-synchronization silently ignores --incremental
client_send.c:23-37

When use_chunk_serialization is true, the entire chunk is serialized as a single STATUS_CHUNK blob. The use_incremental flag is never checked in this branch. Users get no warning that their incremental sync is being silently ignored.

Fix: either validate and reject --chunk-synchronization --incremental at startup, or add per-file CHECK logic inside the chunk path.

3. file_send_single_calls_no_path mutates file data in-place
file.c:81-96

When compression is enabled, this function compresses file->data and replaces it. This destroys the original uncompressed data. If the transfer fails and needs retry, the file data is now compressed — it will be compressed again on retry, producing double-compressed garbage.

Fix: either allocate a new Data for the compressed result without destroying the original, or reload the file data before retry.

Missing Validation / Error Handling

4. --incremental requires -M (metadata) to work, but this is not validated
client_send.c:51

Without -M, chunk->items[i]->metadata is NULL, and mtime is sent as 0. The server will compare mtime 0 against the real file's mtime, which will never match — every file gets re-transferred, silently defeating the purpose of --incremental.

Fix: either print a warning when --incremental is used without -M, or automatically enable metadata when --incremental is set.

5. STATUS_ERROR from server not handled explicitly in client
client_send.c:53

The client checks if (s == STATUS_OK) continue; if (s != STATUS_NEXT) return -1;. If the server sends STATUS_ERROR (e.g., disk write failure), the client treats it as an unknown status and returns -1 with no error message. Should log a clear message like "Server reported error for file".

6. STATUS_CHECK is a protocol change but PROTOCOL_VERSION is not bumped
protocol.h:9

Adding a new status value (6) changes the wire protocol. An old server receiving STATUS_CHECK will see an unknown status, fail the while loop, and error with "Did not receive FINISHED Status" — a confusing message that doesn't explain the real issue. Bumping PROTOCOL_VERSION would give both sides a clean mismatch error.

Design / Code Quality

7. Massive code duplication — STATUS_CHECK handler is copy-pasted
server.c:26-85 vs multiprocessing.c:133-180

The entire STATUS_CHECK receive+decompress+write logic (~55 lines) is duplicated verbatim between receive_files() and receive_thread(). This is the most error-prone part of the protocol. Should be extracted into a shared helper function.

8. Incremental check logic duplicated across sendfile/non-sendfile branches
client_send.c:39-56 vs client_send.c:60-82

The STATUS_CHECK → path → size → mtime → receive response → maybe send file pattern is copy-pasted in both the sendfile and non-sendfile branches of send_chunk(). Should be a shared function like maybe_send_incremental_check().

9. Test doesn't verify files were actually skipped
test.py

The test only checks r2.returncode == 0 and measures duration. It doesn't verify that no bytes were transferred on the second sync. A more robust test would check that the second sync completes significantly faster (or check transfer size from output).

Summary

Must-fix before merging:

Fix sendfile + incremental protocol mismatch (bug #1)
Handle or reject chunk-synchronization + incremental (bug #2)
Validate that -M is implied by --incremental (issue #4)
Bump PROTOCOL_VERSION or document the incompatibility (issue #6)

Nice-to-have:
5. Extract duplicated STATUS_CHECK handler into shared function (issue #7)
6. Strengthen the test to verify skip behavior (issue #9)

## Code Review: Incremental sync Overall the approach is solid — per-file SIZE+mtime check before transfer is the right design. Below are the issues I found. --- ### BUGS **1. Protocol mismatch: `--sendfile` + `--incremental`** `client_send.c:58-68` → `file.c:144` When `use_sendfile && use_incremental` are both true and a file needs re-transfer (STATUS_NEXT), the code falls through to `file_send_sendfile()`, which sends: `path + metadata + file_size + file_data`. But the server's STATUS_CHECK handler expects only: `metadata + data` (no path — it already has it from the CHECK message). The redundant `send_str(path)` at `file.c:145` will be misinterpreted by the server, corrupting the protocol stream. Fix: create a `file_send_sendfile_no_path()` variant (like `file_send_single_calls_no_path`), or restructure to not call `file_send_sendfile` after a CHECK. **2. `--chunk-synchronization` silently ignores `--incremental`** `client_send.c:23-37` When `use_chunk_serialization` is true, the entire chunk is serialized as a single STATUS_CHUNK blob. The `use_incremental` flag is never checked in this branch. Users get no warning that their incremental sync is being silently ignored. Fix: either validate and reject `--chunk-synchronization --incremental` at startup, or add per-file CHECK logic inside the chunk path. **3. `file_send_single_calls_no_path` mutates file data in-place** `file.c:81-96` When compression is enabled, this function compresses `file->data` and replaces it. This destroys the original uncompressed data. If the transfer fails and needs retry, the file data is now compressed — it will be compressed again on retry, producing double-compressed garbage. Fix: either allocate a new Data for the compressed result without destroying the original, or reload the file data before retry. --- ### Missing Validation / Error Handling **4. `--incremental` requires `-M` (metadata) to work, but this is not validated** `client_send.c:51` Without `-M`, `chunk->items[i]->metadata` is NULL, and mtime is sent as `0`. The server will compare mtime `0` against the real file's mtime, which will never match — every file gets re-transferred, silently defeating the purpose of `--incremental`. Fix: either print a warning when `--incremental` is used without `-M`, or automatically enable metadata when `--incremental` is set. **5. `STATUS_ERROR` from server not handled explicitly in client** `client_send.c:53` The client checks `if (s == STATUS_OK) continue; if (s != STATUS_NEXT) return -1;`. If the server sends STATUS_ERROR (e.g., disk write failure), the client treats it as an unknown status and returns -1 with no error message. Should log a clear message like `"Server reported error for file"`. **6. `STATUS_CHECK` is a protocol change but `PROTOCOL_VERSION` is not bumped** `protocol.h:9` Adding a new status value (6) changes the wire protocol. An old server receiving STATUS_CHECK will see an unknown status, fail the while loop, and error with `"Did not receive FINISHED Status"` — a confusing message that doesn't explain the real issue. Bumping `PROTOCOL_VERSION` would give both sides a clean mismatch error. --- ### Design / Code Quality **7. Massive code duplication — STATUS_CHECK handler is copy-pasted** `server.c:26-85` vs `multiprocessing.c:133-180` The entire STATUS_CHECK receive+decompress+write logic (~55 lines) is duplicated verbatim between `receive_files()` and `receive_thread()`. This is the most error-prone part of the protocol. Should be extracted into a shared helper function. **8. Incremental check logic duplicated across sendfile/non-sendfile branches** `client_send.c:39-56` vs `client_send.c:60-82` The STATUS_CHECK → path → size → mtime → receive response → maybe send file pattern is copy-pasted in both the sendfile and non-sendfile branches of `send_chunk()`. Should be a shared function like `maybe_send_incremental_check()`. **9. Test doesn't verify files were actually skipped** `test.py` The test only checks `r2.returncode == 0` and measures duration. It doesn't verify that no bytes were transferred on the second sync. A more robust test would check that the second sync completes significantly faster (or check transfer size from output). --- ### Summary Must-fix before merging: 1. Fix sendfile + incremental protocol mismatch (bug #1) 2. Handle or reject chunk-synchronization + incremental (bug #2) 3. Validate that `-M` is implied by `--incremental` (issue #4) 4. Bump `PROTOCOL_VERSION` or document the incompatibility (issue #6) Nice-to-have: 5. Extract duplicated STATUS_CHECK handler into shared function (issue #7) 6. Strengthen the test to verify skip behavior (issue #9)

TapTap commented

2026-07-16 17:10:07 +02:00

Re-review of `3fc9b1a`

All six issues from the previous review are fixed:

file_send_sendfile_no_path — new function sends metadata + size + data (no path). Matches server's STATUS_CHECK handler protocol.
-s + --incremental rejected — clean error at startup.
No data mutation — both file_send_single_calls_no_path and file_send_single_calls now use a local data_to_send pointer + separate compressed_data. Original file->data is untouched. Retry is safe.
Auto-enables -M — --incremental implicitly enables metadata preservation with a log message.
STATUS_ERROR handling — clear error message: "Server reported error for file".
PROTOCOL_VERSION bumped to 1.1.0 — old clients get a clean version mismatch error.

Also improved file_send_single_calls — removed dead exit(EXIT_FAILURE) code path, proper data_destroy(compressed_data) cleanup on all error paths.

No new issues. LGTM.

## Re-review of 3fc9b1a All six issues from the previous review are fixed: 1. **`file_send_sendfile_no_path`** — new function sends metadata + size + data (no path). Matches server's STATUS_CHECK handler protocol. 2. **`-s + --incremental` rejected** — clean error at startup. 3. **No data mutation** — both `file_send_single_calls_no_path` and `file_send_single_calls` now use a local `data_to_send` pointer + separate `compressed_data`. Original `file->data` is untouched. Retry is safe. 4. **Auto-enables `-M`** — `--incremental` implicitly enables metadata preservation with a log message. 5. **`STATUS_ERROR` handling** — clear error message: "Server reported error for file". 6. **`PROTOCOL_VERSION` bumped to `1.1.0`** — old clients get a clean version mismatch error. Also improved `file_send_single_calls` — removed dead `exit(EXIT_FAILURE)` code path, proper `data_destroy(compressed_data)` cleanup on all error paths. No new issues. LGTM.

TapTap commented

2026-07-16 17:20:52 +02:00

PR Review Summary

Verdict: PASS — No critical issues

Files reviewed: 10
Issues found: 3 (0 critical, 2 warning, 1 style)

Warnings

[1] src/shared/multiprocessing.c:133 — missing STATUS_ERROR on error path
When receive_str() returns NULL in the STATUS_CHECK handler, the function returns thrd_error without notifying the client. Compare with server.c:32 which correctly sends STATUS_ERROR. The client will block forever waiting for a response.
Fix: Add send_status(file_descriptor, STATUS_ERROR); before return thrd_error;.

[2] src/shared/multiprocessing.c:137-141 — same issue on second error path
When receive_n_data() fails for check_size/check_mtime, the function frees check_path and returns thrd_error without sending STATUS_ERROR. Same client hang as above.
Fix: Add send_status(file_descriptor, STATUS_ERROR); before return thrd_error;.

[3] src/server/server.c:55 / src/shared/multiprocessing.c:167 — metadata_receive() failure silently ignored
metadata_receive() returns NULL both when metadata is legitimately absent and on I/O error. If the socket read fails mid-metadata, the error is swallowed. Consider treating metadata I/O errors as fatal.

Style

src/client/client_send.c:43-54 vs 86-97 — The incremental protocol exchange is duplicated verbatim in both sendfile and non-sendfile branches (~25 lines). Consider extracting to a helper function.

What Looks Good

Protocol version bump (1.0.0 → 1.1.0) with proper version check
Config serialization order matches between send/receive for use_incremental
file_send_single_calls refactored — old exit(EXIT_FAILURE) replaced with proper return/cleanup
_no_path variants correctly avoid redundant path transmission
CLI guard forces use_metadata = true when --incremental is used
Error paths in single-threaded server.c consistently send STATUS_ERROR
st_mtime comparison uses long long on both sides of the wire

The 2 warnings in multiprocessing.c (missing STATUS_ERROR sends) should be fixed before merge since they cause client hangs in the multithreaded server path.

## PR Review Summary **Verdict: PASS** — No critical issues **Files reviewed:** 10 **Issues found:** 3 (0 critical, 2 warning, 1 style) --- ### Warnings **[1] `src/shared/multiprocessing.c:133` — missing STATUS_ERROR on error path** When `receive_str()` returns NULL in the `STATUS_CHECK` handler, the function returns `thrd_error` without notifying the client. Compare with `server.c:32` which correctly sends `STATUS_ERROR`. The client will block forever waiting for a response. **Fix:** Add `send_status(file_descriptor, STATUS_ERROR);` before `return thrd_error;`. **[2] `src/shared/multiprocessing.c:137-141` — same issue on second error path** When `receive_n_data()` fails for check_size/check_mtime, the function frees `check_path` and returns `thrd_error` without sending `STATUS_ERROR`. Same client hang as above. **Fix:** Add `send_status(file_descriptor, STATUS_ERROR);` before `return thrd_error;`. **[3] `src/server/server.c:55` / `src/shared/multiprocessing.c:167` — metadata_receive() failure silently ignored** `metadata_receive()` returns NULL both when metadata is legitimately absent and on I/O error. If the socket read fails mid-metadata, the error is swallowed. Consider treating metadata I/O errors as fatal. ### Style - **`src/client/client_send.c:43-54` vs `86-97`** — The incremental protocol exchange is duplicated verbatim in both sendfile and non-sendfile branches (~25 lines). Consider extracting to a helper function. --- ### What Looks Good - Protocol version bump (`1.0.0` → `1.1.0`) with proper version check - Config serialization order matches between send/receive for `use_incremental` - `file_send_single_calls` refactored — old `exit(EXIT_FAILURE)` replaced with proper return/cleanup - `_no_path` variants correctly avoid redundant path transmission - CLI guard forces `use_metadata = true` when `--incremental` is used - Error paths in single-threaded server.c consistently send `STATUS_ERROR` - `st_mtime` comparison uses `long long` on both sides of the wire The 2 warnings in `multiprocessing.c` (missing `STATUS_ERROR` sends) should be fixed before merge since they cause client hangs in the multithreaded server path.

TapTap commented

2026-07-16 17:33:52 +02:00

Re-review after commit `3fc9b1a`

Addressed from previous review

file_send_single_calls_no_path — now uses a local data_to_send pointer instead of mutating file->data. Proper data_destroy(compressed_data) on all error paths. ✅
file_send_sendfile_no_path — new function added, correctly sends file via sendfile without path. ✅
--incremental + -s rejection — properly blocks incompatible flags. ✅
Auto -M with --incremental — use_metadata forced true when incremental is used. ✅
Better error messages — STATUS_ERROR and unexpected status now logged in client_send.c. ✅
PROTOCOL_VERSION bumped to 1.1.0. ✅

Still unfixed

src/shared/multiprocessing.c:133,137-141 — The STATUS_CHECK handler in receive_thread still returns thrd_error without sending STATUS_ERROR to the client on two error paths:

When receive_str() returns NULL (line ~131)
When receive_n_data() fails (lines ~134-135)

In both cases the client is left waiting for a status response and will hang. Compare with server.c which consistently sends STATUS_ERROR before returning on error.

Fix: Add send_status(file_descriptor, STATUS_ERROR); before each return thrd_error; in the STATUS_CHECK handler.

## Re-review after commit `3fc9b1a` ### Addressed from previous review - **`file_send_single_calls_no_path`** — now uses a local `data_to_send` pointer instead of mutating `file->data`. Proper `data_destroy(compressed_data)` on all error paths. ✅ - **`file_send_sendfile_no_path`** — new function added, correctly sends file via sendfile without path. ✅ - **`--incremental` + `-s` rejection** — properly blocks incompatible flags. ✅ - **Auto `-M` with `--incremental`** — `use_metadata` forced true when incremental is used. ✅ - **Better error messages** — `STATUS_ERROR` and unexpected status now logged in `client_send.c`. ✅ - **`PROTOCOL_VERSION` bumped** to `1.1.0`. ✅ ### Still unfixed **`src/shared/multiprocessing.c:133,137-141`** — The `STATUS_CHECK` handler in `receive_thread` still returns `thrd_error` without sending `STATUS_ERROR` to the client on two error paths: 1. When `receive_str()` returns NULL (line ~131) 2. When `receive_n_data()` fails (lines ~134-135) In both cases the client is left waiting for a status response and will hang. Compare with `server.c` which consistently sends `STATUS_ERROR` before returning on error. **Fix:** Add `send_status(file_descriptor, STATUS_ERROR);` before each `return thrd_error;` in the `STATUS_CHECK` handler.

TapTap commented

2026-07-17 10:08:28 +02:00

Re-review after commit `9a26eb1`

Verdict: PASS — All remaining issues fixed

Fixes in this commit

multiprocessing.c:133 — send_status(file_descriptor, STATUS_ERROR) now sent before return thrd_error when receive_str() returns NULL ✅
multiprocessing.c:137-141 — send_status(file_descriptor, STATUS_ERROR) now sent before return thrd_error when receive_n_data() fails ✅
metadata_receive() signature — now takes int *ok parameter to distinguish I/O errors from "no metadata present". All callers (server.c, file.c, multiprocessing.c) check meta_ok and send STATUS_ERROR on failure ✅
incremental_check() helper — extracted duplicated protocol exchange from client_send.c. Both sendfile and non-sendfile branches now call the same function ✅

Review items status

All 3 warnings from previous reviews are now resolved:

Missing STATUS_ERROR sends — fixed ✅
metadata_receive() silent failure — fixed with ok parameter ✅
Duplicated incremental protocol code — extracted to helper ✅

Safe to merge.

## Re-review after commit `9a26eb1` **Verdict: PASS** — All remaining issues fixed ### Fixes in this commit - **`multiprocessing.c:133`** — `send_status(file_descriptor, STATUS_ERROR)` now sent before `return thrd_error` when `receive_str()` returns NULL ✅ - **`multiprocessing.c:137-141`** — `send_status(file_descriptor, STATUS_ERROR)` now sent before `return thrd_error` when `receive_n_data()` fails ✅ - **`metadata_receive()` signature** — now takes `int *ok` parameter to distinguish I/O errors from "no metadata present". All callers (`server.c`, `file.c`, `multiprocessing.c`) check `meta_ok` and send `STATUS_ERROR` on failure ✅ - **`incremental_check()` helper** — extracted duplicated protocol exchange from `client_send.c`. Both sendfile and non-sendfile branches now call the same function ✅ ### Review items status All 3 warnings from previous reviews are now resolved: 1. Missing `STATUS_ERROR` sends — fixed ✅ 2. `metadata_receive()` silent failure — fixed with `ok` parameter ✅ 3. Duplicated incremental protocol code — extracted to helper ✅ Safe to merge.

TapTap force-pushed incremental-sync from 9a26eb187e to f629ebceb7

2026-07-17 10:51:53 +02:00

Compare

TapTap commented

2026-07-17 10:57:38 +02:00

Re-review after rebase

Branch rebased onto merged main (PRs #15 and #16 now in base). Content unchanged from previous PASS review — same incremental_check() helper, STATUS_ERROR fixes, and metadata_receive() signature change.

Verdict: PASS — No new issues. Safe to merge.

## Re-review after rebase Branch rebased onto merged main (PRs #15 and #16 now in base). Content unchanged from previous PASS review — same `incremental_check()` helper, `STATUS_ERROR` fixes, and `metadata_receive()` signature change. **Verdict: PASS** — No new issues. Safe to merge.

TapTap referenced this pull request

2026-07-18 16:26:59 +02:00

refactor: merge sendfile and single_calls _no_path variants with send_path bool #19

TapTap force-pushed incremental-sync from 2dcfad0321 to 38b59ad53c

2026-07-18 16:47:56 +02:00

Compare

TapTap commented

2026-07-18 16:49:32 +02:00

Re-review after commit `38b59ad`

Verdict: PASS — Clean deduplication refactor

New changes

Shared helpers extracted to file.c:

receive_incremental_check(fd, config, &skipped) — handles the full STATUS_CHECK protocol: stat comparison, metadata receive, data receive, decompression. Returns File* or NULL with skipped=true when file matches.
file_save_to_disk(root_directory, file) — handles path construction, to_disk(), and file_restore_metadata() in one call.

server.c refactored:

receive_chunk() and receive_manifest() extracted as static helpers
STATUS_CHECK handler now calls receive_incremental_check() instead of inline logic
goto next pattern used for skipped files (works, slightly unusual)

multiprocessing.c refactored:

STATUS_CHECK handler now calls receive_incremental_check() instead of inline 50-line block
write_thread now calls file_save_to_disk() instead of inline disk write logic
Removed metadata.h and sys/stat.h includes (no longer needed directly)

What looks good

Eliminates ~100 lines of duplicated receive logic between server.c and multiprocessing.c
receive_incremental_check() handles all error paths correctly: send_status(STATUS_ERROR) on every failure
file_save_to_disk() is a clean abstraction used by both server paths
skipped out-parameter pattern is clear and correct
Both callers handle the skipped/file==NULL states correctly

Minor observations (non-blocking)

server.c uses goto next for the skip case — works but could use continue with a restructured loop. Style preference only.
receive_incremental_check() sends STATUS_ERROR internally on failure, so callers don't need to send it again. This is correct but worth documenting in the header.

Safe to merge.

## Re-review after commit `38b59ad` **Verdict: PASS** — Clean deduplication refactor ### New changes **Shared helpers extracted to `file.c`:** - `receive_incremental_check(fd, config, &skipped)` — handles the full STATUS_CHECK protocol: stat comparison, metadata receive, data receive, decompression. Returns `File*` or NULL with `skipped=true` when file matches. - `file_save_to_disk(root_directory, file)` — handles path construction, `to_disk()`, and `file_restore_metadata()` in one call. **`server.c` refactored:** - `receive_chunk()` and `receive_manifest()` extracted as static helpers - STATUS_CHECK handler now calls `receive_incremental_check()` instead of inline logic - `goto next` pattern used for skipped files (works, slightly unusual) **`multiprocessing.c` refactored:** - STATUS_CHECK handler now calls `receive_incremental_check()` instead of inline 50-line block - `write_thread` now calls `file_save_to_disk()` instead of inline disk write logic - Removed `metadata.h` and `sys/stat.h` includes (no longer needed directly) ### What looks good - Eliminates ~100 lines of duplicated receive logic between `server.c` and `multiprocessing.c` - `receive_incremental_check()` handles all error paths correctly: `send_status(STATUS_ERROR)` on every failure - `file_save_to_disk()` is a clean abstraction used by both server paths - `skipped` out-parameter pattern is clear and correct - Both callers handle the `skipped`/`file==NULL` states correctly ### Minor observations (non-blocking) - `server.c` uses `goto next` for the skip case — works but could use `continue` with a restructured loop. Style preference only. - `receive_incremental_check()` sends `STATUS_ERROR` internally on failure, so callers don't need to send it again. This is correct but worth documenting in the header. Safe to merge.

TapTap commented

2026-07-18 16:51:59 +02:00

Remaining Duplication Scan

Found 4 duplicated patterns still in the branch:

1. `receive_chunk` (server.c) vs `receive_chunk_enqueue` (multiprocessing.c) — ~25 lines

Both do identical receive/decompress/deserialize:

Data *chunk_data = receive_data(fd);
if (config->use_compression) {
  data_to_process = data_decompress(chunk_data);
  data_destroy(chunk_data);
  if (data_to_process == NULL) { ... }
}
Chunk *chunk = chunk_deserialize(data_to_process, config->use_metadata);
data_destroy(data_to_process);

Only the post-deserialize action differs: file_save_to_disk loop vs queue_enqueue loop.

Fix: Extract receive_and_deserialize_chunk(fd, config) → Chunk* to a shared helper in file.c or chunk.c. Both callers use the returned Chunk* for their specific action.

2. Manifest handling — `receive_manifest` (server.c) vs inline (multiprocessing.c)

Server has a clean receive_manifest() helper. Multiprocessing has ~15 lines of inline code doing the same thing, plus a redundant manual free loop before array_list_delete (the free destructor already handles this).

Fix: Move receive_manifest() to a shared location (e.g., file.c or protocol.c). The multiprocessing version also has a minor bug: fprintf(stderr, "Deleting files not in manifest...\n") is missing (present in server.c but not multiprocessing.c).

3. Dry run code in `client_send.c` — ~20 lines duplicated

send_files() and send_files_multithreaded() both have identical dry run blocks:

if (config->dry_run) {
  DirectoryScanner *scanner = directory_scanner_create(...);
  // ... same 15 lines ...
  return 0;
}

Fix: Extract to static int run_dry_run(Config *config) and call from both functions.

4. Client connection setup in `client_send.c` — ~10 lines duplicated

send_files() and send_chunks_multithreaded() both have identical SSH vs TCP connection logic:

if (config->transport == TRANSPORT_SSH) {
  if (config->use_sendfile) { error; return; }
  client = client_connect_ssh(...);
} else {
  client = client_create();
  if (!client || !client_connect(...)) { error; return; }
}

Fix: Extract to static Client *connect_client(Config *config) and call from both functions.

5. Manifest sending in `client_send.c` — ~5 lines duplicated

Both send_files() and send_chunks_multithreaded() have:

if (config->use_delete) {
  send_status(client->file_descriptor, STATUS_MANIFEST);
  send_int(client->file_descriptor, manifest->size);
  for (int i = 0; i < manifest->size; i++)
    send_str(client->file_descriptor, (char *)manifest->items[i]);
}

Fix: Extract to static void send_manifest(Client *client, ArrayList *manifest).

Severity

All are style issues (code duplication), not functional bugs. The receive_chunk duplication (#1) is the most significant (~25 lines). The manifest handling (#2) also has a missing log message in multiprocessing.c.

Non-blocking for merge, but worth addressing in a follow-up refactor.

## Remaining Duplication Scan Found 4 duplicated patterns still in the branch: --- ### 1. `receive_chunk` (server.c) vs `receive_chunk_enqueue` (multiprocessing.c) — ~25 lines Both do identical receive/decompress/deserialize: ```c Data *chunk_data = receive_data(fd); if (config->use_compression) { data_to_process = data_decompress(chunk_data); data_destroy(chunk_data); if (data_to_process == NULL) { ... } } Chunk *chunk = chunk_deserialize(data_to_process, config->use_metadata); data_destroy(data_to_process); ``` Only the post-deserialize action differs: `file_save_to_disk` loop vs `queue_enqueue` loop. **Fix:** Extract `receive_and_deserialize_chunk(fd, config) → Chunk*` to a shared helper in `file.c` or `chunk.c`. Both callers use the returned `Chunk*` for their specific action. --- ### 2. Manifest handling — `receive_manifest` (server.c) vs inline (multiprocessing.c) Server has a clean `receive_manifest()` helper. Multiprocessing has ~15 lines of inline code doing the same thing, plus a redundant manual `free` loop before `array_list_delete` (the `free` destructor already handles this). **Fix:** Move `receive_manifest()` to a shared location (e.g., `file.c` or `protocol.c`). The multiprocessing version also has a minor bug: `fprintf(stderr, "Deleting files not in manifest...\n")` is missing (present in server.c but not multiprocessing.c). --- ### 3. Dry run code in `client_send.c` — ~20 lines duplicated `send_files()` and `send_files_multithreaded()` both have identical dry run blocks: ```c if (config->dry_run) { DirectoryScanner *scanner = directory_scanner_create(...); // ... same 15 lines ... return 0; } ``` **Fix:** Extract to `static int run_dry_run(Config *config)` and call from both functions. --- ### 4. Client connection setup in `client_send.c` — ~10 lines duplicated `send_files()` and `send_chunks_multithreaded()` both have identical SSH vs TCP connection logic: ```c if (config->transport == TRANSPORT_SSH) { if (config->use_sendfile) { error; return; } client = client_connect_ssh(...); } else { client = client_create(); if (!client || !client_connect(...)) { error; return; } } ``` **Fix:** Extract to `static Client *connect_client(Config *config)` and call from both functions. --- ### 5. Manifest sending in `client_send.c` — ~5 lines duplicated Both `send_files()` and `send_chunks_multithreaded()` have: ```c if (config->use_delete) { send_status(client->file_descriptor, STATUS_MANIFEST); send_int(client->file_descriptor, manifest->size); for (int i = 0; i < manifest->size; i++) send_str(client->file_descriptor, (char *)manifest->items[i]); } ``` **Fix:** Extract to `static void send_manifest(Client *client, ArrayList *manifest)`. --- ### Severity All are **style** issues (code duplication), not functional bugs. The `receive_chunk` duplication (#1) is the most significant (~25 lines). The manifest handling (#2) also has a missing log message in multiprocessing.c. Non-blocking for merge, but worth addressing in a follow-up refactor.

TapTap commented

2026-07-18 16:54:42 +02:00

6. `file_send_single_calls` / `file_send_sendfile` + `_no_path` variants — PR #19 already fixed this

This branch still has 4 separate functions:

file_send_single_calls(file, fd, use_metadata, compression_level)
file_send_single_calls_no_path(file, fd, use_metadata, compression_level)
file_send_sendfile(file, fd, use_metadata)
file_send_sendfile_no_path(file, fd, use_metadata)

PR #19 (already merged) consolidated these into 2 functions with a bool send_path parameter. This branch should be rebased onto main to pick up that change, or apply the same refactor independently.

The _no_path variants differ from the originals only in whether send_str(file->path) is called. A single send_path bool eliminates ~30 lines of duplicated code.

### 6. `file_send_single_calls` / `file_send_sendfile` + `_no_path` variants — PR #19 already fixed this This branch still has 4 separate functions: - `file_send_single_calls(file, fd, use_metadata, compression_level)` - `file_send_single_calls_no_path(file, fd, use_metadata, compression_level)` - `file_send_sendfile(file, fd, use_metadata)` - `file_send_sendfile_no_path(file, fd, use_metadata)` PR #19 (already merged) consolidated these into 2 functions with a `bool send_path` parameter. This branch should be rebased onto main to pick up that change, or apply the same refactor independently. The `_no_path` variants differ from the originals only in whether `send_str(file->path)` is called. A single `send_path` bool eliminates ~30 lines of duplicated code.

TapTap commented

2026-07-18 16:57:05 +02:00

7. Branch needs rebase onto main

This branch still contains file_send_sendfile_no_path and file_send_single_calls_no_path as separate functions. PR #19 (already merged to main) consolidated these into the main functions with a bool send_path parameter.

The branch was rebased onto main after PRs #15/#16 merged but not after #19 merged. A rebase onto current main would:

Remove the _no_path variants (already handled by PR #19)
Pick up the send_path bool refactor automatically
Resolve any remaining conflicts with the merged code

Action needed: Rebase incremental-sync onto current main before merge.

### 7. Branch needs rebase onto main This branch still contains `file_send_sendfile_no_path` and `file_send_single_calls_no_path` as separate functions. PR #19 (already merged to main) consolidated these into the main functions with a `bool send_path` parameter. The branch was rebased onto main after PRs #15/#16 merged but **not** after #19 merged. A rebase onto current main would: 1. Remove the `_no_path` variants (already handled by PR #19) 2. Pick up the `send_path` bool refactor automatically 3. Resolve any remaining conflicts with the merged code **Action needed:** Rebase `incremental-sync` onto current `main` before merge.

TapTap commented

2026-07-18 17:01:48 +02:00

Correction to finding #7

I was wrong — PR #19 (refactor/merge-path-variants) was not actually merged to main. The commit only exists on the feature branch.

So the _no_path variants (file_send_sendfile_no_path, file_send_single_calls_no_path) exist only on this branch because they were introduced as part of the incremental sync feature. Main still has the original API without _no_path or send_path.

Correct fix: Apply the send_path bool refactor directly in this branch (same approach as PR #19), replacing the 4 functions with 2 functions that take a bool send_path parameter. This eliminates ~30 lines of duplicated code before the branch is merged.

### Correction to finding #7 I was wrong — PR #19 (`refactor/merge-path-variants`) was **not actually merged to main**. The commit only exists on the feature branch. So the `_no_path` variants (`file_send_sendfile_no_path`, `file_send_single_calls_no_path`) exist only on this branch because they were introduced as part of the incremental sync feature. Main still has the original API without `_no_path` or `send_path`. **Correct fix:** Apply the `send_path` bool refactor directly in this branch (same approach as PR #19), replacing the 4 functions with 2 functions that take a `bool send_path` parameter. This eliminates ~30 lines of duplicated code before the branch is merged.

TapTap commented

2026-07-18 17:03:26 +02:00

Re-review after commit `2979f43`

Addressed

Finding #1 — receive_chunk_data() extracted to chunk.c. Both server.c and multiprocessing.c now call it instead of duplicating the receive/decompress/deserialize logic ✅
Finding #2 — receive_manifest() extracted to file.c. Both server.c and multiprocessing.c now call it. The redundant manual free loop in multiprocessing.c is removed ✅

Still open

Finding #6 — _no_path variants (file_send_sendfile_no_path, file_send_single_calls_no_path) are still present as separate functions. The send_path bool refactor from PR #19 is not applied. These should be consolidated: add bool send_path to file_send_sendfile and file_send_single_calls, remove the _no_path variants, and update callers to pass true/false.
Findings #3, #4, #5 — Dry run code, client connection setup, and manifest sending duplication in client_send.c remain (non-blocking).

## Re-review after commit `2979f43` ### Addressed - **Finding #1** — `receive_chunk_data()` extracted to `chunk.c`. Both `server.c` and `multiprocessing.c` now call it instead of duplicating the receive/decompress/deserialize logic ✅ - **Finding #2** — `receive_manifest()` extracted to `file.c`. Both `server.c` and `multiprocessing.c` now call it. The redundant manual `free` loop in multiprocessing.c is removed ✅ ### Still open - **Finding #6** — `_no_path` variants (`file_send_sendfile_no_path`, `file_send_single_calls_no_path`) are still present as separate functions. The `send_path` bool refactor from PR #19 is not applied. These should be consolidated: add `bool send_path` to `file_send_sendfile` and `file_send_single_calls`, remove the `_no_path` variants, and update callers to pass `true`/`false`. - **Findings #3, #4, #5** — Dry run code, client connection setup, and manifest sending duplication in `client_send.c` remain (non-blocking).

TapTap commented

2026-07-18 17:07:14 +02:00

Re-review after commit `bd0c986`

Finding #6 resolved ✅

file_send_single_calls now takes bool send_path — skips send_str(file->path) when false
file_send_sendfile now takes bool send_path — same pattern
_no_path variants removed from file.h and file.c
Callers in client_send.c pass true (normal) or false (incremental)

All 7 findings from the duplication scan are now addressed or non-blocking:

✅ receive_chunk_data shared helper
✅ receive_manifest shared helper
Non-blocking (dry run duplication)
Non-blocking (client connection duplication)
Non-blocking (manifest sending duplication)
✅ _no_path → send_path bool
N/A (rebase note corrected)

Safe to merge.

## Re-review after commit `bd0c986` **Finding #6 resolved** ✅ - `file_send_single_calls` now takes `bool send_path` — skips `send_str(file->path)` when false - `file_send_sendfile` now takes `bool send_path` — same pattern - `_no_path` variants removed from `file.h` and `file.c` - Callers in `client_send.c` pass `true` (normal) or `false` (incremental) All 7 findings from the duplication scan are now addressed or non-blocking: 1. ✅ `receive_chunk_data` shared helper 2. ✅ `receive_manifest` shared helper 3. Non-blocking (dry run duplication) 4. Non-blocking (client connection duplication) 5. Non-blocking (manifest sending duplication) 6. ✅ `_no_path` → `send_path` bool 7. N/A (rebase note corrected) Safe to merge.

TapTap added 5 commits 2026-07-18 17:20:19 +02:00

Incremental sync: --incremental flag to skip unchanged files 1905369c9f

- New STATUS_CHECK protocol status (value 6)
- Client sends path + size + mtime; server replies OK (skip) or NEXT (send)
- config.h/c: use_incremental field sent/received over wire
- file.c/h: file_send_single_calls_no_path helper for incremental path
- client_cli.c: --incremental flag
- client_send.c: per-file check before send in both single and chunk paths
- server.c: STATUS_CHECK handling in receive_files()
- multiprocessing.c: STATUS_CHECK handling in receive_thread()
- test.py: incremental sync test case

Fix incremental-sync review: sendfile_no_path, reject -s+incremental, no data mutation, auto -M, STATUS_ERROR handling, PROTOCOL_VERSION bump f1af88a25c

fix: address review #69 - STATUS_ERROR sends, metadata_receive error handling, extract incremental_check helper 03d6ba0da5

refactor: deduplicate server receive logic, extract shared helpers 5ed12f2bf0

- Extract file_save_to_disk() helper (replaces 4x duplicated disk-save boilerplate)
- Extract receive_incremental_check() shared helper (deduplicates STATUS_CHECK
  handling between server.c single-threaded and multiprocessing.c multi-threaded paths)
- Break receive_files() into receive_chunk() and receive_manifest() sub-handlers
- Add incremental sync test case to test.py
- Rebase onto main

refactor: extract receive_chunk_data and receive_manifest shared helpers 7876721906

- receive_chunk_data(fd, config) -> Chunk*: shared receive/decompress/deserialize
  (eliminates ~25 lines of duplication between server.c and multiprocessing.c)
- receive_manifest(fd, config, next_status): moved from server.c to file.c,
  fixes missing 'Deleting files not in manifest...' log in multiprocessing.c
- Removes redundant manual free loop in multiprocessing.c manifest handling
  (array_list_create(free) destructor already handles this)
- server.c and multiprocessing.c: remove local chunk/manifest helpers,
  remove compression.h include (no longer needed)

TapTap force-pushed incremental-sync from bd0c986a59 to 8178e12900

2026-07-18 17:20:19 +02:00

Compare

TapTap added 1 commit 2026-07-18 17:27:34 +02:00

refactor: consolidate _no_path file send variants

CI / build-and-test (push) Successful in 26s

Details

CI / build-and-test (pull_request) Successful in 26s

Details

46c650e9ae

Add bool send_path parameter to file_send_single_calls and
file_send_sendfile, remove the _no_path variants. Callers in
client_send.c pass true (send path) or false (skip path) based
on whether an incremental check already transmitted the path.

TapTap force-pushed incremental-sync from 8178e12900 to 46c650e9ae

2026-07-18 17:27:34 +02:00

Compare

TapTap merged commit 25383f893b into main

2026-07-18 17:36:26 +02:00

TapTap deleted branch incremental-sync

2026-07-18 17:36:31 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TapTap/FastSync#14

Incremental sync: --incremental to skip unchanged files #14

Code Review: Incremental sync

BUGS

Missing Validation / Error Handling

Design / Code Quality

Summary

Re-review of 3fc9b1a

PR Review Summary

Warnings

Style

What Looks Good

Re-review after commit 3fc9b1a

Addressed from previous review

Still unfixed

Re-review after commit 9a26eb1

Fixes in this commit

Review items status

Re-review after rebase

Re-review after commit 38b59ad

New changes

What looks good

Minor observations (non-blocking)

Remaining Duplication Scan

1. receive_chunk (server.c) vs receive_chunk_enqueue (multiprocessing.c) — ~25 lines

2. Manifest handling — receive_manifest (server.c) vs inline (multiprocessing.c)

3. Dry run code in client_send.c — ~20 lines duplicated

4. Client connection setup in client_send.c — ~10 lines duplicated

5. Manifest sending in client_send.c — ~5 lines duplicated

Severity

6. file_send_single_calls / file_send_sendfile + _no_path variants — PR #19 already fixed this

7. Branch needs rebase onto main

Correction to finding #7

Re-review after commit 2979f43

Addressed

Still open

Re-review after commit bd0c986

Re-review of `3fc9b1a`

Re-review after commit `3fc9b1a`

Re-review after commit `9a26eb1`

Re-review after commit `38b59ad`

1. `receive_chunk` (server.c) vs `receive_chunk_enqueue` (multiprocessing.c) — ~25 lines

2. Manifest handling — `receive_manifest` (server.c) vs inline (multiprocessing.c)

3. Dry run code in `client_send.c` — ~20 lines duplicated

4. Client connection setup in `client_send.c` — ~10 lines duplicated

5. Manifest sending in `client_send.c` — ~5 lines duplicated

6. `file_send_single_calls` / `file_send_sendfile` + `_no_path` variants — PR #19 already fixed this

Re-review after commit `2979f43`

Re-review after commit `bd0c986`