Skip to content

fix: make m re-enable multi-line and stop s disabling it - #349

Open
vlasky wants to merge 1 commit into
chmln:masterfrom
vlasky:fix/multi-line-flag-handling
Open

fix: make m re-enable multi-line and stop s disabling it#349
vlasky wants to merge 1 commit into
chmln:masterfrom
vlasky:fix/multi-line-flag-handling

Conversation

@vlasky

@vlasky vlasky commented Jul 30, 2026

Copy link
Copy Markdown

Two silent bugs in the -f flag loop: wrong result, exit status 0, no diagnostic. They are one fix because the second is caused by a workaround for the first. Offered as the separate issues #348 mentioned; there is no existing issue for either, so the report is inline.

1. e is irreversible

'm' is a no-op, on the reasoning that multi-line is already the default:

'm' => {},
'e' => { regex.multi_line(false); },

So -f em reads as "off, then on" but gives off, and nothing later in the flag string can undo an e. c and i are last-one-wins; m and e are not. On printf 'aa\nbb\n' replacing ^\w+ with M, under -A:

Invocation Result Expected
-f e M\nbb M\nbb, as documented
-f em M\nbb M\nM

2. s disables multi-line as an undocumented side effect

's' => {
    if !flags.contains('m') {
        regex.multi_line(false);
    }
    regex.dot_matches_new_line(true);
},

s is documented as "make . match newlines", but it also stops ^ and $ matching at line boundaries:

Invocation ^\w+ under -A Expected
-f s M\nbb M\nM
-f sm M\nM M\nM

The flags.contains('m') guard is a lookahead into the rest of the flag string, so -f s and -f sm differ in a way neither --help nor the man page describes. It also makes plain s a silent alias for es: there is no way to ask for dot-matches-newline while keeping per-line anchoring.

Provenance

Both arrived via commits doing something else, and neither diff made the interaction visible. s was added correctly in 1a622ce (2019-07-24, dot_matches_new_line only). Then 87491b4 (2020-01-25, "Let ^$ match lines by default") made m a no-op, which regressed -f s. Two months later 1532f80 (2020-03-29, "Misc cleanup") added the guard, which looks like a patch for exactly that regression: with m a no-op there was no other way to keep -f s behaving as it had. Fixing m properly removes the reason the guard existed.

Fix

Make m a real setter, which restores last-one-wins and makes the guard redundant, so the 's' arm collapses to what it was in 1a622ce:

'm' => { regex.multi_line(true); },
'e' => { regex.multi_line(false); },
's' => { regex.dot_matches_new_line(true); },

-f es can now express both effects at once, which was previously impossible.

This is a behaviour change

Flagging it explicitly rather than calling it a pure bug fix. I swept all 156 subsets and permutations of cimes up to length 3 against four probes (^ anchor under -A, $ anchor under -A, dot-matches-newline, case), before and after.

Exactly 38 change. A predicate written independently of the diff, "s with neither m nor e" or "e followed later by m", matches those 38 with no mismatches in either direction. The dot-matches-newline and case probes are identical across all 156, so nothing outside multi-line behaviour moved.

Anyone relying on -f s implying -f es gets different output. Both old behaviours were undocumented and contradicted the help text, so this reads as a fix rather than a break, but it is your call.

Tests

No test currently exercises s, m or e at all, which is why neither bug was caught. Adds five unit tests and two CLI tests; two of the five fail without this change, one per bug. The rest are regression guards: . still does not match a newline without s, -f me still ends up off, multi-line is still the default, and -f es expresses both effects.

Full suite passes. cargo clippy --all-targets --all-features and cargo fmt --check -p sd are clean.

Relationship to my other open PRs

This is the fourth and last I plan to open. All four are branched off master and none depends on another, so they can be merged in any order. I tested all six pairwise merges to see where the friction actually is:

Pair Result
this <-> #346 clean
this <-> #348 conflict, sd/src/replacer/mod.rs + tests
this <-> #347 conflict, sd-cli/tests/cli.rs
#348 <-> #346 conflict, sd/src/replacer/mod.rs
#348 <-> #347 conflict, sd-cli/tests/cli.rs
#346 <-> #347 conflict, README.md

Every one of those is textual, not semantic: two branches inserting tests at the same anchor, or editing adjacent arms of the same match. I resolved this <-> #348 locally and the full suite passes with both applied.

Worth knowing why this PR merges cleanly with #346 while #348 does not: #346 moves the flag loop into build_regex without changing any of its arms, and this PR only edits arms, so the change travels with the loop. #348 adds code before the builder is constructed, which is exactly the region #346 relocates.

Suggested order, if it helps

Entirely your call, and none of this is blocking. My suggestion:

  1. fix: make -f w compose with other flags and bind to the whole pattern #348 (-f w composition), a pure bug fix with no behaviour change outside the broken flag, and the worst of the bugs: -f w 'foo|bar' currently replaces inside words, the opposite of what it advertises.
  2. This one, also a bug fix, but with the 38-of-156 behaviour change above, so it deserves more thought than fix: make -f w compose with other flags and bind to the whole pattern #348.
  3. feat: reject replacement references to undefined capture groups #346 (reject undefined capture references), so the build_regex refactor lands after the two fixes and neither fix needs rebasing into it. I will rebase this one onto whatever is in master by then.
  4. feat: add --must-match to exit non-zero when nothing matched #347 (--must-match), a purely additive new flag that conflicts only in README.md and test files, so it is the easiest to slot in whenever.

The reasoning is fixes before features, the fix with no behaviour change before the one with, and the refactor after the fixes rather than before. If you would rather take #346 first, that also works and I will rebase the other three; it just means two rebases instead of one. Happy to do any of these rebases, or to combine PRs, on request.

Notes

  • m's help text now reads "multi-line matching (the default; use to override an earlier e)", with gen/sd.1 regenerated via cargo xtask gen. The s line already said "make . match newlines" and only now became true, so it is unchanged.
  • Still unfixed and left alone, since it is a breaking change wanting its own error type rather than a bug fix: unknown flags are silently ignored, so sd -f z 'abc' 'X' succeeds with exit 0 and a typo'd flag is indistinguishable from a correct one. Happy to open an issue or a PR if you want it.

Two independent problems in the flag loop, both silent: wrong result,
exit status 0, no diagnostic.

`m` was a no-op, on the reasoning that multi-line is already the
default. But that leaves `e` irreversible: `-f em` reads as "off, then
on" and gives off, with no way to undo an `e` later in the flag string.
Making `m` a real setter restores last-one-wins, which is how `c` and
`i` already behave.

`s` disabled multi-line as an undocumented side effect, so `-f s` also
stopped `^`/`$` matching at line boundaries. It is documented as
affecting `.` only. The `if !flags.contains('m')` guard around it was a
lookahead into the rest of the flag string, which made `-f s` and
`-f sm` differ in a way neither the help text nor the man page
describes, and made plain `s` a silent alias for `es` -- there was no
way to ask for dot-matches-newline while keeping per-line anchoring.
With `m` now a real setter the guard is unnecessary, so it is gone.

Characterised by sweeping all 156 subsets and permutations of `cimes`
up to length 3 against four probes (`^` anchor, `$` anchor,
dot-matches-newline, case). Exactly 38 change, and all 38 are predicted
by "`s` without `m` or `e`" or "`e` followed later by `m`". Nothing
else moves, and the dot-matches-newline and case probes are untouched
throughout.

Adds five unit tests and two CLI tests; two of the unit tests fail
without this change, one per problem. The rest are regression guards,
including that `.` still does not match a newline without `s` and that
`-f es` can now express both effects at once.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant