feat!: split godo and shell placeholder namespaces, quote substituted values - #2
Merged
Merged
Conversation
… values
Bodies are shell space: godo claims only ${godo:…} there and passes every
other ${…} to the host shell untouched. A matcher capture stays declared
${NAME} in the key and written ${NAME} in @deps — neither ever meets a
shell — and is consumed ${godo:argv[NAME]} in the body. Collision between
catalog syntax and shell syntax is no longer resolved; it cannot occur.
Values godo substitutes are now shell-quoted: one argument in is one
argument out, whatever it contains. ${godo:…:raw} opts a single
placeholder back into verbatim interpolation for globs and deliberate
shell constructs.
Also: @deps forms a DAG, so a shared dependency runs once rather than
once per path; @deps entries resolve to tokens instead of to a line that
is re-split, so a capture holding a space no longer fragments the
invocation; Dialect.Match takes a single Script, dropping the dead loop
every implementation carried.
The Windows quoting path is untested — CI runs Linux only and the runner
tests skip on Windows. cmd.exe expands %VAR% before a command sees its
arguments, which no command-line quoting suppresses.
BREAKING CHANGE: ${NAME} in a script body must become ${godo:argv[NAME]}.
A body left unedited does not error: the shell receives the braces and
expands them to nothing. Scripts relying on an argument carrying a glob
or a shell operator need ${godo:…:raw}.
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.
Summary
Two changes to how a
godo.yamlreaches the shell, plus two smaller fixes foundon the way.
Namespace split. godo now claims only
${godo:…}inside a script body.Everything else —
${HOME},$$, any shell construct — is passed throughuntouched. Captures are still declared
${NAME}in the matcher key andwritten
${NAME}in@deps, because neither of those ever meets a shell; theyare consumed
${godo:argv[NAME]}in the body.@deps${NAME}${godo:…}This removes the
$${…}escape and the "unknown capture" failure for bare${…}: both existed only to rescue host environment variables from being readas captures, and there is nothing left to rescue them from.
Quoting. Substituted values are shell-quoted, so an argument arrives as one
argument whatever it contains. Ordinary tokens (flags, paths) stay unquoted so
previews stay readable —
godo test -v ./...still previews asgo test -v ./....${godo:…:raw}opts out per placeholder.Also fixed
@depsforms a DAG: a shared dependency runs once, at its first(deepest-first) position, instead of once per path. Keyed on the expanded
invocation, so the same script reached with different captures still runs once
per capture set.
@depsentries resolve to tokens instead of to a line that is then re-split,so a capture holding a space no longer fragments the invocation.
Dialect.Matchtakes a singleScript. The engine already walked the catalogin definition order and passed one script; the slice parameter made both
implementations carry a loop over a one-element slice.
Before / after
Measured against the pre-change binary:
godo greet 'a; touch PWNED'echo ${HOME}in a bodyunknown capture/home/you@depsleaf mid leaf rootleaf mid root@depsBreaking
${NAME}in a body must become${godo:argv[NAME]}.This does not fail loudly. A body left unedited expands to nothing in the
shell rather than erroring. There is no automated migration check — a load-time
lint (matcher key declares
${MODULE}, body mentions it bare) would catch itand is not in this PR.
Scripts relying on an argument carrying a glob or a shell operator need
${godo:…:raw}.Not covered
The Windows quoting path is untested. CI runs
ubuntu-latestonly and thethree
execshellrunner tests skip on Windows, soquoteWindowsdoes notexecute anywhere — while release builds ship
windows/amd64. This predates thePR, but the PR adds code to that blind spot.
cmd.exealso expands%VAR%before a command sees its arguments, and nocommand-line quoting suppresses it, so a value containing
%is not safe onWindows. Documented in
docs/contract.md.Follow-ups, not in this PR:
QuoteFor(goos, s)to make the Windows pathtestable from Linux,
windows-latestin the CI matrix, and the choice betweencmd.exeand PowerShell.Checklist
./godo cipasses locally