Conversation
…hell
`run' builds its command as text and evals it, requoting every word that is not
a shell operator. That requoting escaped `"', backtick and backslash but not
`$', so a `$' inside a quoted word was expanded by the eval in the ZUnit shell
and the command saw an empty value:
run zsh -f -c 'typeset inner=INNER_VALUE; print -r -- "got=[${inner}]"'
got=[] # this fork
got=[INNER_VALUE] # zdharma/zunit v0.8.2
Upstream avoids it by never evaluating, running "${cmd[@]}" directly. The eval
here is deliberate and worth keeping: dont_quote is what lets `run' accept
redirections, `;', `|', `&&' and var=... assignments, and the leading newline is
what puts a line number in `run:1: command not found: ...'.
Add `$' to the escaped set at both requoting sites, so a `$' in a quoted word
survives the eval and reaches the command's own shell.
The failure was silent: the parameter emptied rather than erroring, so
assertions failed for reasons that looked unrelated to quoting, or passed
vacuously. z-shell/zd tests/snippets.zunit test 4 fails under this fork with
`no such file or directory: "/._zi/mirror-backend"' because ${object_path}
empties, while passing in CI, which installs upstream v0.8.2. With this change
that suite goes from 3/4 to 4/4 locally.
Both new tests in tests/run.zunit were observed failing against the unmodified
source and passing with the change. Error messages keep their line number and
the suite goes from 123 to 125 passing.
Closes #24
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.
Adds
$to the requoting escape set inrun. Two characters of source, plus two regression tests.The defect
runbuilds its command as text andevals it, requoting every word that is not a shell operator:${MATCH//(#b)([\"\`\\])/\\${match[1]}}", backtick and backslash are escaped.$is not, so a$inside a quoted word is expanded by theevalin the ZUnit shell and never reaches the command:run zsh -f -c 'typeset inner=INNER_VALUE; print -r -- "got=[${inner}]"'zdharma/zunitv0.8.2got=[INNER_VALUE]got=[]got=[INNER_VALUE]Upstream is unaffected because it never evals:
output=$("${cmd[@]}" 2>&1).Why not just adopt upstream's approach
The
evalis deliberate and earns its place.dont_quoteis what letsrunaccept redirections,;,|,&&andvar=...assignments, which upstream cannot handle, and the leading newline is what puts a line number intorun:1: command not found: .... Both behaviours have tests here. The narrow fix keeps them.Why it mattered
The failure is silent. The parameter empties rather than erroring, so assertions fail for reasons that look unrelated to quoting, or pass vacuously.
Found via
z-shell/zd:tests/snippets.zunittest 4 fails under this fork withno such file or directory: "/._zi/mirror-backend"because${object_path}empties, while the same test passes in that repo's CI, which installs upstream v0.8.2. With this change the suite goes from 3/4 to 4/4 locally.Verification
Both new tests in
tests/run.zunitwere run against the unmodified source and observed failing, then passing with the change:Test non-existent commandstill assertsrun:[0-9]+: command not found:, so the line-number behaviour the newline exists for is unchanged. Suite goes from 123 to 125 passing.Closes #24