Fix sparse literal byte accounting#2146
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| state.accumulate(leading); | ||
|
|
||
| if leading == chunk.len() { | ||
| return Ok(chunk.len()); | ||
| return Ok(0); | ||
| } | ||
|
|
||
| let trailing = trailing_zero_run(&chunk[leading..]); | ||
| let data_start = leading; | ||
| let data_end = chunk.len() - trailing; | ||
|
|
||
| if data_end > data_start { | ||
| let literal_written = if data_end > data_start { | ||
| state.flush(writer, destination)?; | ||
| writer | ||
| .write_all(&chunk[data_start..data_end]) | ||
| .map_err(|error| LocalCopyError::io("copy file", destination.to_path_buf(), error))?; | ||
| } | ||
| data_end - data_start | ||
| } else { | ||
| 0 | ||
| }; | ||
|
|
||
| state.replace(trailing); | ||
|
|
||
| Ok(chunk.len()) | ||
| Ok(literal_written) |
There was a problem hiding this comment.
Changing write_sparse_chunk (crates/engine/src/local_copy/executor/file/sparse.rs) to return only the number of literal bytes written means it now returns 0 whenever a chunk is entirely zeros. However, file::copy::transfer still computes wrote_data = outcome.literal_bytes() > 0 || append_offset > 0 (around line 386) and uses that flag to mark checksum changes and, when --times is disabled, to decide whether to touch the destination mtime. As a result, overwriting an existing file in sparse mode with data that is all zeros will now leave wrote_data == false, so the change set shows no checksum change and the modification time is left untouched even though the file contents were replaced. This silently skips itemized notifications and mtime updates for a real data change. wrote_data needs to be recomputed to account for logical bytes (e.g. compare the logical length or track “any chunk processed”) instead of relying solely on literal_bytes().
Useful? React with 👍 / 👎.
Upstream rsync (options.c:2382) rejects the combination `--append --whole-file` with the message `--append cannot be used with --whole-file` and exits with RERR_SYNTAX (1). oc-rsync was silently accepting both flags together. Add the conflict check to both validation layers that gate config construction: - `ClientConfigBuilder::validate` (core) - the primary path used by the CLI; surfaces a `ConfigConflict` whose `Display` matches upstream wording. - `LocalCopyOptionsBuilder::validate` (engine) - keeps the engine-level builder in sync so programmatic embedders also reject the combination. Only an explicit `--whole-file` (`Some(true)`) conflicts; the default (`None`) and `--no-whole-file` (`Some(false)`) remain accepted so existing call sites that set the tri-state from auto-detection are unaffected. Tests cover the engine builder, the core config builder (including the exact upstream error string), and an end-to-end CLI integration test that drives `cli::run` and asserts the RERR_SYNTAX exit code plus the diagnostic mentions both flags. Refs: #2145, #2146, #2147
Upstream rsync (options.c:2382) rejects the combination `--append --whole-file` with the message `--append cannot be used with --whole-file` and exits with RERR_SYNTAX (1). oc-rsync was silently accepting both flags together. Add the conflict check to both validation layers that gate config construction: - `ClientConfigBuilder::validate` (core) - the primary path used by the CLI; surfaces a `ConfigConflict` whose `Display` matches upstream wording. - `LocalCopyOptionsBuilder::validate` (engine) - keeps the engine-level builder in sync so programmatic embedders also reject the combination. Only an explicit `--whole-file` (`Some(true)`) conflicts; the default (`None`) and `--no-whole-file` (`Some(false)`) remain accepted so existing call sites that set the tri-state from auto-detection are unaffected. Tests cover the engine builder, the core config builder (including the exact upstream error string), and an end-to-end CLI integration test that drives `cli::run` and asserts the RERR_SYNTAX exit code plus the diagnostic mentions both flags. Refs: #2145, #2146, #2147
Upstream rsync (options.c:2382) rejects the combination `--append --whole-file` with the message `--append cannot be used with --whole-file` and exits with RERR_SYNTAX (1). oc-rsync was silently accepting both flags together. Add the conflict check to both validation layers that gate config construction: - `ClientConfigBuilder::validate` (core) - the primary path used by the CLI; surfaces a `ConfigConflict` whose `Display` matches upstream wording. - `LocalCopyOptionsBuilder::validate` (engine) - keeps the engine-level builder in sync so programmatic embedders also reject the combination. Only an explicit `--whole-file` (`Some(true)`) conflicts; the default (`None`) and `--no-whole-file` (`Some(false)`) remain accepted so existing call sites that set the tri-state from auto-detection are unaffected. Tests cover the engine builder, the core config builder (including the exact upstream error string), and an end-to-end CLI integration test that drives `cli::run` and asserts the RERR_SYNTAX exit code plus the diagnostic mentions both flags. Refs: #2145, #2146, #2147
1 participant
Summary
Testing
Codex Task