Skip to content

Allow escaping "$" in command arguments - #505

Open
rkennedy wants to merge 4 commits into
magefile:masterfrom
rkennedy:bugfix/env-expand
Open

rkennedy wants to merge 4 commits into
magefile:masterfrom
rkennedy:bugfix/env-expand

Conversation

@rkennedy

@rkennedy rkennedy commented May 1, 2024 •

Copy link
Copy Markdown

The sh.Exec family of functions used to use os.Expand to expand environment variables in command arguments, but that function offers no escape syntax — no way to avoid expanding dollar signs that the caller really wants to include on the command line. This commit implements a new, mostly compatible Expand function to allow escaping the environment-variable syntax, plus an accompanying Escape function to producing escaped strings.

This technically breaks backward compatibility; any existing code that has used the sequences "\\" or "\$" in its command arguments will need to be updated. Although the compatibility argument prevents os.Expand from changing, documentation for sh.Exec hasn't actually stated that it follows all the same rules as os.Expand, so it's possible to view this change as merely a bugfix, formalizing a new syntax that had not previously been established.

Fixes #434

@natefinch natefinch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling literal dollar signs in command arguments. I applied this patch to current master (0953947) to review it. I found two compatibility regressions that need addressing before merging; the inline comments include reproductions. The Escape round-trip worked in the cases I exercised, but replacing os.Expand also changes behavior for existing arguments unrelated to dollar escaping.

The updated patch also needs a small lint cleanup: use strings.ReplaceAll, fix the spelling of preceding, and use t.Setenv in the precedence test.

Comment thread sh/cmd.go Resolved
Comment thread sh/cmd.go Resolved
The `sh.Exec` family of functions used to use `os.Expand` to expand
environment variables in command arguments, but that function offers no
escape syntax -- no way to avoid expanding dollar signs that the caller
really wants to include on the command line. This commit implements a
new, mostly compatible `Expand` function to allow escaping the
environment-variable syntax, plus an accompanying `Escape` function to
produce escaped strings.

This technically breaks backward compatibility: Any existing code that
has used the sequence "\\" or "\$" in its command arguments will need to
be updated. Although Go's compatibility promise prevents `os.Expand`
from changing, documentation for `sh.Exec` hasn't actually stated that
it follows all the same rules as `os.Expand`, so it's possible to view
this change as merely a bugfix, formalizing a new syntax that had not
previously been established.

Fixes magefile#434
They had previously been removed, on the grounds that they were reserved
for future use, but now they'll just be passed back verbatim -- no
longer reserved.
This is "new" with respect to the Go versions supported at the time the
surrounding code was written.
The stdlib function accepts different syntax depending on whether braces
are used, and now our `Expand` function will behave likewise.

Unit tests that involve new backslash-escape syntax have hard-coded
expected results, but all other tests of `sh.Expand` defer to
`os.Expand` as their oracle.
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.

Exec args always expanded

2 participants