sed: take the -i suffix only when attached, as GNU sed does - #583
Conversation
|
GNU sed testsuite comparison: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #583 +/- ##
==========================================
+ Coverage 83.67% 83.97% +0.30%
==========================================
Files 14 14
Lines 7067 7201 +134
Branches 413 417 +4
==========================================
+ Hits 5913 6047 +134
Misses 1149 1149
Partials 5 5
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:
|
Merging this PR will degrade performance by 2.07%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | access_log_subst |
2.2 s | 2.4 s | -5.98% |
| ⚡ | no_op_short |
2.4 s | 2.4 s | +2% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing DePasqualeOrg:fix-in-place-detached-suffix (f6c9934) with main (dfae00d)
bb946b1 to
f6c9934
Compare
|
GNU sed testsuite comparison: |
| let mut args = args.into_iter(); | ||
| // The program name. | ||
| let mut out: Vec<OsString> = args.next().into_iter().collect(); | ||
| for arg in args.by_ref() { |
There was a problem hiding this comment.
a detached value isn't skipped here: sed -f -ifoo.sed file would become -f --in-place=foo.sed
GNU reads -ifoo.sed as the script file
edge case, but could you please skip the next arg after -e/-f/-l and add a test? thanks
There was a problem hiding this comment.
Done. This also required letting -e, -f and -l accept values starting with -, as GNU sed does.
| } | ||
|
|
||
| #[test] | ||
| fn test_in_place_suffix_forms() { |
There was a problem hiding this comment.
these unit tests mostly repeat the tests in tests/by-util/test_sed.rs
do we need both? i'd keep the gnu_in_place_args rewrite cases and the integration tests
| .args(&[arg, "s/world/universe/p", path.to_str().unwrap()]) | ||
| .succeeds(); | ||
|
|
||
| let expected = if arg == "-ni.bak" { |
There was a problem hiding this comment.
please put the expected output in the array with the arg instead of this if
|
Thanks for your PR |
Claude Code and Codex flagged this gap during my work with this project. The changes here have gone through extensive automated review. I'm happy to make changes as needed or close this PR and maintain it as a private patch if it doesn't meet your standards.
-itook the next argument as its backup suffix, so insed -i SCRIPT FILE..., the usual GNU form, the first file name became the script. This usually fails, but when the name happens to be a valid command, sed applies it to the remaining files and exits with 0:As in GNU sed, the suffix must now be attached (
-i.bak,--in-place=.bak). The in-place option now setsrequire_equals, and an attached-iSUFFIXis rewritten to--in-place=SUFFIXbefore clap parses, including inside clusters such as-ni.bak. The values of other options are left alone, and-e,-fand-lnow accept values that begin with-, as in GNU sed, sosed -f -ifoo.sed filereads its script from-ifoo.sed. This drops BSD's detached-i ''and-i .bak, which GNU sed reads as a script or file name, anddocs/src/extensions.mdnow says so. The GNU testsuite'sinplace-holdandstdin-progtests now pass.This builds on #433 by @MukundaKatta, which was left unfinished, and adds handling of clusters and
--.Fixes #165, fixes #229, fixes #425, fixes #549.