param-op: one substitution-spec splitter, not two (#684) - #751
Merged
Conversation
`${v/p/r}` and `${arr[@]/p/r}` take the same `pattern/replacement` spec, so
they must split it the same way. The per-element dispatch carried its own copy
of that split, and the copy had drifted: its no-separator branch skipped the
`\/` -> `/` canonicalization.
v=a/b/c; arr=(a/b/c)
${v//\/} -> abc
${arr[@]//\/} -> a/b/c the slashes survived
The element path now calls lush_param_op_split_substitution_spec -- the
function the scalar path already used, exported from param_op.c for the
purpose -- and the duplicate is deleted. One spec, one splitter; they cannot
drift again. The code even carried a comment naming this divergence and #684,
saying the fold belonged in its own commit. This is that commit.
This is the consolidation the expansion arc exists for: param_op.c is the
shared core both the legacy expander and the word CST call, so the fix lands
once for both routes rather than being copied to a second site.
Two PRE-EXISTING defects surfaced while verifying and are filed rather than
folded in, both byte-identical against a build of the parent commit:
- #749: a per-element operator collapses `"${arr[@]}"` to ONE field where
bash and zsh give N. The element VALUES are right and this fix corrected
them (`<a/b x/y>` became `<ab xy>`); the field boundary is lost separately,
in the dispatch's result handoff, and affects `#`/`%`/`^^`/`//` alike.
- #750: the REPLACEMENT half is never unescaped, so `${v//X/\/}` yields
`a\/b` where bash and zsh give `a/b`. Consistent across both paths -- they
now agree, they simply agree on the wrong replacement.
tests/integration/test_vector_substitution_spec.c: 17 checks. Twelve compare
the scalar and element results against EACH OTHER as well as a literal, so a
future change that breaks both the same way still fails; the rest cover the
vector, joined and positional forms. 4 fail against the parent build.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Closes #684.
The defect
${v/p/r}and${arr[@]/p/r}take the samepattern/replacementspec, so they must split it the same way. The per-element dispatch carried its own copy of that split, and the copy had drifted — its no-separator branch skipped the\/→/canonicalization:The element path now calls
lush_param_op_split_substitution_spec— the function the scalar path already used, exported fromparam_op.c— and the duplicate is deleted. One spec, one splitter.The code carried a comment naming this exact divergence and #684, noting the fold "belongs in its own commit". This is that commit. It is also the consolidation the expansion arc exists for:
param_op.cis the shared core that both the legacy expander and the word CST call, so the fix lands once for both routes.Two pre-existing defects found while verifying (filed, not folded in)
Both byte-identical against a build of the parent commit:
"${arr[@]}"to one field where bash and zsh give N. The element values are correct and this fix corrected them (<a/b x/y>→<ab xy>); the field boundary is lost separately and affects#,%,^^,//alike.cp "${files[@]#./}" destpasses one argument instead of N.${v//X/\/}yieldsa\/bwhere bash and zsh givea/b. Consistent on both paths: they now agree, they simply agree on the wrong replacement.Verification
tests/integration/test_vector_substitution_spec.c— 17 checks. Twelve compare the scalar and element results against each other as well as against a literal, so a future change that breaks both in the same way still fails. The rest cover the vector, joined and positional forms.Field count is deliberately not asserted here — that contract belongs to #749, and asserting today's collapsed behavior would lock in the defect.