Fix multiple write commands to same file - #532
Open
dspinellis wants to merge 1 commit into
Open
Conversation
Maintining different writers for the same path can fail due to buffering or underlying OS restrictions. In particular, this fails on Windows. Fix it through a map of canonicalized write paths.
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR fixes failures caused by creating multiple independent writers targeting the same output path (notably on Windows) by reusing writers keyed by a canonicalized path, and adds support/tests for the non-POSIX W command (write first line of pattern space).
Changes:
- Reuse
NamedWriterinstances via a thread-local map keyed by canonicalized output paths. - Update write APIs to optionally append a trailing newline based on input termination, and implement
Wruntime handling. - Expand test coverage for
w/Wbehaviors (sandbox rejection, multiple writes to same file, newline/no-newline cases).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/by-util/test_sed.rs | Adds regression + behavior tests for w/W, sandbox behavior, newline handling, and Windows multi-writer failure prevention. |
| src/sed/processor.rs | Updates w and s///w writing to respect newline-termination; adds W command execution logic. |
| src/sed/named_writer.rs | Introduces canonical-path writer reuse via HashMap, optional-newline write API, and new unit tests. |
| src/sed/compiler.rs | Enables W (non-POSIX) command compilation. |
| README.md | Documents the new W command behavior. |
Suppressed comments (1)
src/sed/named_writer.rs:136
- Iterating
HashMap::values()makesflush_all()traversal order non-deterministic. If multiple flushes could fail (e.g., due to OS-level I/O errors), which file’s error is returned first can vary run-to-run, making operational debugging and tests that assert specific failures less stable. If deterministic behavior matters here, consider iterating in a stable order (e.g., collect keys and sort, or store writers in an order-preserving structure for flushing).
/// Flush buffered content to all open files, returning descriptive errors.
pub fn flush_all() -> UResult<()> {
WRITERS.with(|writers| {
for writer in writers.borrow().values() {
writer.borrow_mut().flush()?;
}
Ok(())
})
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let canonical_path = canonicalize_output_path(&path, &location)?; | ||
|
|
||
| if let Some(writer) = WRITERS.with(|writers| writers.borrow().get(&canonical_path).cloned()) | ||
| { |
Comment on lines
+1704
to
+1716
| #[test] | ||
| fn sandbox_rejects_first_line_write_command() -> std::io::Result<()> { | ||
| let temp = NamedTempFile::new()?; | ||
| let cmd = format!("W {}", temp.path().display()); | ||
|
|
||
| new_ucmd!() | ||
| .args(&["--sandbox", &cmd, LINES1]) | ||
| .fails() | ||
| .stderr_contains("command not allowed with --sandbox"); | ||
|
|
||
| let mut actual = String::new(); | ||
| temp.reopen()?.read_to_string(&mut actual)?; | ||
| assert!(actual.is_empty()); |
| * An `F` command outputs the name of the file currently being processed. | ||
| * A `Q` command (optionally followed by an exit code) quits immediately. | ||
| * The `q` command can be optionally followed by an exit code. | ||
| * A `W` command writes to a file the pattern's first line. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## W-command #532 +/- ##
=============================================
+ Coverage 83.01% 83.12% +0.11%
=============================================
Files 13 13
Lines 7037 7083 +46
Branches 401 401
=============================================
+ Hits 5842 5888 +46
Misses 1192 1192
Partials 3 3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Maintaining different writers for the same path can fail due to buffering
or underlying OS restrictions. In particular, this fails on Windows.
Fix it through a map of canonicalized write paths.