fix: make m re-enable multi-line and stop s disabling it - #349
Open
vlasky wants to merge 1 commit into
Open
Conversation
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.
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.
Two silent bugs in the
-fflag 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.
eis irreversible'm'is a no-op, on the reasoning that multi-line is already the default:So
-f emreads as "off, then on" but gives off, and nothing later in the flag string can undo ane.candiare last-one-wins;mandeare not. Onprintf 'aa\nbb\n'replacing^\w+withM, under-A:-f eM\nbbM\nbb, as documented-f emM\nbbM\nM2.
sdisables multi-line as an undocumented side effectsis documented as "make.match newlines", but it also stops^and$matching at line boundaries:^\w+under-A-f sM\nbbM\nM-f smM\nMM\nMThe
flags.contains('m')guard is a lookahead into the rest of the flag string, so-f sand-f smdiffer in a way neither--helpnor the man page describes. It also makes plainsa silent alias fores: 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.
swas added correctly in 1a622ce (2019-07-24,dot_matches_new_lineonly). Then 87491b4 (2020-01-25, "Let^$match lines by default") madema 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: withma no-op there was no other way to keep-f sbehaving as it had. Fixingmproperly removes the reason the guard existed.Fix
Make
ma real setter, which restores last-one-wins and makes the guard redundant, so the's'arm collapses to what it was in 1a622ce:-f escan 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
cimesup 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, "
swith neithermnore" or "efollowed later bym", 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 simplying-f esgets 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,moreat 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 withouts,-f mestill ends up off, multi-line is still the default, and-f esexpresses both effects.Full suite passes.
cargo clippy --all-targets --all-featuresandcargo fmt --check -p sdare clean.Relationship to my other open PRs
This is the fourth and last I plan to open. All four are branched off
masterand 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:sd/src/replacer/mod.rs+ testssd-cli/tests/cli.rssd/src/replacer/mod.rssd-cli/tests/cli.rsREADME.mdEvery 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_regexwithout 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:
-f wcompose with other flags and bind to the whole pattern #348 (-f wcomposition), 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.-f wcompose with other flags and bind to the whole pattern #348.build_regexrefactor lands after the two fixes and neither fix needs rebasing into it. I will rebase this one onto whatever is inmasterby then.--must-match), a purely additive new flag that conflicts only inREADME.mdand 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 earliere)", withgen/sd.1regenerated viacargo xtask gen. Thesline already said "make.match newlines" and only now became true, so it is unchanged.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.