edit: review-driven fixes for --in-place, bounds checks, and silent no-ops#3786
Merged
jqnatividad merged 7 commits intomasterfrom Apr 29, 2026
Merged
edit: review-driven fixes for --in-place, bounds checks, and silent no-ops#3786jqnatividad merged 7 commits intomasterfrom
jqnatividad merged 7 commits intomasterfrom
Conversation
…o-ops - Remove vestigial extension() guard so --in-place works on extensionless files - Reject --in-place with stdin or missing input instead of mis-renaming "-" - checked_sub on headers.len() avoids underflow when column == "_" on empty headers - Numeric column index out of range now errors instead of silently no-op'ing - Track row match; emit "Row N not found." instead of silently rewriting unchanged - Only allocate NamedTempFile when --in-place is set - Build target row via single ByteRecord instead of per-field write loop - Add tests for extensionless in-place, stdin rejection, and out-of-range row/column Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 16 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
- stdout/--output mode now warns and passes input through unchanged when the row is out of range (exit 0); --in-place still errors so the .bak rename is skipped - Refuse to overwrite a pre-existing .bak file in --in-place mode - Use checked_add on row+1 to avoid usize::MAX overflow - Document new --in-place .bak collision behavior and stdout warn-vs-error semantics in USAGE - Add tests for unknown column name, --in-place out-of-range row, stdout out-of-range row pass-through, and pre-existing .bak collision Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Replace TOCTOU exists-check with hard_link reservation so a concurrent process can't clobber an existing .bak between the check and the rename - Reword stdout warning to "input passed through unchanged" since --output does write a file (just unmodified relative to input) - Test stdout pass-through now also asserts the stderr warning is emitted Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Replace hard_link reservation with create_new on the .bak path: works on FAT32/exFAT/SMB filesystems where hard_link is unsupported, while still closing the TOCTOU window - Reject symlinks for --in-place up front rather than silently resolving through them and leaving a hard-linked backup pointing at the target - Place the tempfile in the input's parent directory and persist it via rename so input_path is never missing during the swap (no gap window) - Add unix-only test that --in-place rejects symlinks and leaves both the symlink target and any .bak paths untouched - Document symlink rejection in USAGE Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Roll back the .bak rename if tempfile.persist fails so the user isn't left with a missing input_path; surface backup location in the error - Correct the comment to acknowledge a small window (two renames) rather than claim "no gap" - Narrow USAGE wording from "Symlinks" to "Symbolic links" and note that other Windows reparse points (junction points) are not detected Coverage for the new create_new path is already exercised end-to-end by edit_in_place + edit_in_place_existing_bak_errors (both AlreadyExists and success branches). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Branch the persist-failure error message on the actual rollback result
("original restored from <bak>" vs "rollback also failed ...; original
remains at <bak>") so the user gets a deterministic statement about
where their data is, instead of an ambiguous parenthetical
- Add an inline rationale for accepting the small two-rename window
(copy-then-persist would double disk I/O on every successful edit) so
future readers don't try to "fix" it again
- Note inline that the persist-failure rollback branch is not directly
covered by automated tests because reliably forcing the failure
requires platform-specific filesystem manipulation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Inline citation of std::fs::rename's documented cross-platform replace behavior so future reviewers don't flag the create_new placeholder + rename pattern as broken on Windows. The only refused case is directory-on-directory, which doesn't apply to our regular-file placeholder. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--in-placesilently dropping edits on extensionless files (vestigialextension().is_some()guard left over from thewith_extension→with_added_extensionrefactor).--in-placewith stdin (-) or missing input rather than callingstd::fs::rename("-", "-.bak").headers.len() - 1withchecked_sub(1)socolumn == "_"on empty headers returns "Invalid column selected." instead of panicking.NamedTempFilewhen--in-placeis actually set; build the target row via a singleByteRecordinstead of a per-field write loop.Test plan
cargo test test_edit -F all_features(10/10 pass, including 4 new tests for extensionless in-place, stdin rejection, out-of-range row, out-of-range column index)cargo build --locked --bin qsv -F all_featurescleancargo clippy --bin qsv -F all_featuresclean🤖 Generated with Claude Code