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.
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.
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).
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)
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.
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.
[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
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.
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. ✅
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.
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.
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.
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.
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 ...
return0;}
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:
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.
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.
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.
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.
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).
- 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
- 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
- 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)
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.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
New --incremental flag that queries the server before sending each file and skips files that already exist with matching size and mtime.
All 7 unit tests and 31 integration tests pass with zero warnings.
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+--incrementalclient_send.c:58-68→file.c:144When
use_sendfile && use_incrementalare both true and a file needs re-transfer (STATUS_NEXT), the code falls through tofile_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 redundantsend_str(path)atfile.c:145will be misinterpreted by the server, corrupting the protocol stream.Fix: create a
file_send_sendfile_no_path()variant (likefile_send_single_calls_no_path), or restructure to not callfile_send_sendfileafter a CHECK.2.
--chunk-synchronizationsilently ignores--incrementalclient_send.c:23-37When
use_chunk_serializationis true, the entire chunk is serialized as a single STATUS_CHUNK blob. Theuse_incrementalflag 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 --incrementalat startup, or add per-file CHECK logic inside the chunk path.3.
file_send_single_calls_no_pathmutates file data in-placefile.c:81-96When compression is enabled, this function compresses
file->dataand 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.
--incrementalrequires-M(metadata) to work, but this is not validatedclient_send.c:51Without
-M,chunk->items[i]->metadatais NULL, and mtime is sent as0. The server will compare mtime0against 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
--incrementalis used without-M, or automatically enable metadata when--incrementalis set.5.
STATUS_ERRORfrom server not handled explicitly in clientclient_send.c:53The 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_CHECKis a protocol change butPROTOCOL_VERSIONis not bumpedprotocol.h:9Adding 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. BumpingPROTOCOL_VERSIONwould give both sides a clean mismatch error.Design / Code Quality
7. Massive code duplication — STATUS_CHECK handler is copy-pasted
server.c:26-85vsmultiprocessing.c:133-180The entire STATUS_CHECK receive+decompress+write logic (~55 lines) is duplicated verbatim between
receive_files()andreceive_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-56vsclient_send.c:60-82The 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 likemaybe_send_incremental_check().9. Test doesn't verify files were actually skipped
test.pyThe test only checks
r2.returncode == 0and 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:
-Mis implied by--incremental(issue #4)PROTOCOL_VERSIONor 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)
Re-review of
3fc9b1aAll 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 + --incrementalrejected — clean error at startup.file_send_single_calls_no_pathandfile_send_single_callsnow use a localdata_to_sendpointer + separatecompressed_data. Originalfile->datais untouched. Retry is safe.-M—--incrementalimplicitly enables metadata preservation with a log message.STATUS_ERRORhandling — clear error message: "Server reported error for file".PROTOCOL_VERSIONbumped to1.1.0— old clients get a clean version mismatch error.Also improved
file_send_single_calls— removed deadexit(EXIT_FAILURE)code path, properdata_destroy(compressed_data)cleanup on all error paths.No new issues. LGTM.
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 pathWhen
receive_str()returns NULL in theSTATUS_CHECKhandler, the function returnsthrd_errorwithout notifying the client. Compare withserver.c:32which correctly sendsSTATUS_ERROR. The client will block forever waiting for a response.Fix: Add
send_status(file_descriptor, STATUS_ERROR);beforereturn thrd_error;.[2]
src/shared/multiprocessing.c:137-141— same issue on second error pathWhen
receive_n_data()fails for check_size/check_mtime, the function freescheck_pathand returnsthrd_errorwithout sendingSTATUS_ERROR. Same client hang as above.Fix: Add
send_status(file_descriptor, STATUS_ERROR);beforereturn thrd_error;.[3]
src/server/server.c:55/src/shared/multiprocessing.c:167— metadata_receive() failure silently ignoredmetadata_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-54vs86-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
1.0.0→1.1.0) with proper version checkuse_incrementalfile_send_single_callsrefactored — oldexit(EXIT_FAILURE)replaced with proper return/cleanup_no_pathvariants correctly avoid redundant path transmissionuse_metadata = truewhen--incrementalis usedSTATUS_ERRORst_mtimecomparison useslong longon both sides of the wireThe 2 warnings in
multiprocessing.c(missingSTATUS_ERRORsends) should be fixed before merge since they cause client hangs in the multithreaded server path.Re-review after commit
3fc9b1aAddressed from previous review
file_send_single_calls_no_path— now uses a localdata_to_sendpointer instead of mutatingfile->data. Properdata_destroy(compressed_data)on all error paths. ✅file_send_sendfile_no_path— new function added, correctly sends file via sendfile without path. ✅--incremental+-srejection — properly blocks incompatible flags. ✅-Mwith--incremental—use_metadataforced true when incremental is used. ✅STATUS_ERRORand unexpected status now logged inclient_send.c. ✅PROTOCOL_VERSIONbumped to1.1.0. ✅Still unfixed
src/shared/multiprocessing.c:133,137-141— TheSTATUS_CHECKhandler inreceive_threadstill returnsthrd_errorwithout sendingSTATUS_ERRORto the client on two error paths:receive_str()returns NULL (line ~131)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.cwhich consistently sendsSTATUS_ERRORbefore returning on error.Fix: Add
send_status(file_descriptor, STATUS_ERROR);before eachreturn thrd_error;in theSTATUS_CHECKhandler.Re-review after commit
9a26eb1Verdict: PASS — All remaining issues fixed
Fixes in this commit
multiprocessing.c:133—send_status(file_descriptor, STATUS_ERROR)now sent beforereturn thrd_errorwhenreceive_str()returns NULL ✅multiprocessing.c:137-141—send_status(file_descriptor, STATUS_ERROR)now sent beforereturn thrd_errorwhenreceive_n_data()fails ✅metadata_receive()signature — now takesint *okparameter to distinguish I/O errors from "no metadata present". All callers (server.c,file.c,multiprocessing.c) checkmeta_okand sendSTATUS_ERRORon failure ✅incremental_check()helper — extracted duplicated protocol exchange fromclient_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:
STATUS_ERRORsends — fixed ✅metadata_receive()silent failure — fixed withokparameter ✅Safe to merge.
9a26eb187etof629ebceb7Re-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_ERRORfixes, andmetadata_receive()signature change.Verdict: PASS — No new issues. Safe to merge.
2dcfad0321to38b59ad53cRe-review after commit
38b59adVerdict: 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. ReturnsFile*or NULL withskipped=truewhen file matches.file_save_to_disk(root_directory, file)— handles path construction,to_disk(), andfile_restore_metadata()in one call.server.crefactored:receive_chunk()andreceive_manifest()extracted as static helpersreceive_incremental_check()instead of inline logicgoto nextpattern used for skipped files (works, slightly unusual)multiprocessing.crefactored:receive_incremental_check()instead of inline 50-line blockwrite_threadnow callsfile_save_to_disk()instead of inline disk write logicmetadata.handsys/stat.hincludes (no longer needed directly)What looks good
server.candmultiprocessing.creceive_incremental_check()handles all error paths correctly:send_status(STATUS_ERROR)on every failurefile_save_to_disk()is a clean abstraction used by both server pathsskippedout-parameter pattern is clear and correctskipped/file==NULLstates correctlyMinor observations (non-blocking)
server.cusesgoto nextfor the skip case — works but could usecontinuewith a restructured loop. Style preference only.receive_incremental_check()sendsSTATUS_ERRORinternally on failure, so callers don't need to send it again. This is correct but worth documenting in the header.Safe to merge.
Remaining Duplication Scan
Found 4 duplicated patterns still in the branch:
1.
receive_chunk(server.c) vsreceive_chunk_enqueue(multiprocessing.c) — ~25 linesBoth do identical receive/decompress/deserialize:
Only the post-deserialize action differs:
file_save_to_diskloop vsqueue_enqueueloop.Fix: Extract
receive_and_deserialize_chunk(fd, config) → Chunk*to a shared helper infile.corchunk.c. Both callers use the returnedChunk*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 manualfreeloop beforearray_list_delete(thefreedestructor already handles this).Fix: Move
receive_manifest()to a shared location (e.g.,file.corprotocol.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 duplicatedsend_files()andsend_files_multithreaded()both have identical dry run blocks: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 duplicatedsend_files()andsend_chunks_multithreaded()both have identical SSH vs TCP connection logic:Fix: Extract to
static Client *connect_client(Config *config)and call from both functions.5. Manifest sending in
client_send.c— ~5 lines duplicatedBoth
send_files()andsend_chunks_multithreaded()have:Fix: Extract to
static void send_manifest(Client *client, ArrayList *manifest).Severity
All are style issues (code duplication), not functional bugs. The
receive_chunkduplication (#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.
6.
file_send_single_calls/file_send_sendfile+_no_pathvariants — PR #19 already fixed thisThis 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_pathparameter. This branch should be rebased onto main to pick up that change, or apply the same refactor independently.The
_no_pathvariants differ from the originals only in whethersend_str(file->path)is called. A singlesend_pathbool eliminates ~30 lines of duplicated code.7. Branch needs rebase onto main
This branch still contains
file_send_sendfile_no_pathandfile_send_single_calls_no_pathas separate functions. PR #19 (already merged to main) consolidated these into the main functions with abool send_pathparameter.The branch was rebased onto main after PRs #15/#16 merged but not after #19 merged. A rebase onto current main would:
_no_pathvariants (already handled by PR #19)send_pathbool refactor automaticallyAction needed: Rebase
incremental-synconto currentmainbefore merge.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_pathvariants (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_pathorsend_path.Correct fix: Apply the
send_pathbool refactor directly in this branch (same approach as PR #19), replacing the 4 functions with 2 functions that take abool send_pathparameter. This eliminates ~30 lines of duplicated code before the branch is merged.Re-review after commit
2979f43Addressed
receive_chunk_data()extracted tochunk.c. Bothserver.candmultiprocessing.cnow call it instead of duplicating the receive/decompress/deserialize logic ✅receive_manifest()extracted tofile.c. Bothserver.candmultiprocessing.cnow call it. The redundant manualfreeloop in multiprocessing.c is removed ✅Still open
Finding #6 —
_no_pathvariants (file_send_sendfile_no_path,file_send_single_calls_no_path) are still present as separate functions. Thesend_pathbool refactor from PR #19 is not applied. These should be consolidated: addbool send_pathtofile_send_sendfileandfile_send_single_calls, remove the_no_pathvariants, and update callers to passtrue/false.Findings #3, #4, #5 — Dry run code, client connection setup, and manifest sending duplication in
client_send.cremain (non-blocking).Re-review after commit
bd0c986Finding #6 resolved ✅
file_send_single_callsnow takesbool send_path— skipssend_str(file->path)when falsefile_send_sendfilenow takesbool send_path— same pattern_no_pathvariants removed fromfile.handfile.cclient_send.cpasstrue(normal) orfalse(incremental)All 7 findings from the duplication scan are now addressed or non-blocking:
receive_chunk_datashared helperreceive_manifestshared helper_no_path→send_pathboolSafe to merge.
bd0c986a59to8178e129008178e12900to46c650e9ae