Add Shell method for running commands via the system shell - #245
Add Shell method for running commands via the system shell#245Dhanalakshmi-D04 wants to merge 3 commits into
Conversation
510d46a to
f844d44
Compare
Adds a new Shell method (and package-level Shell function) that runs a command line through the native OS shell (sh -c on Unix, cmd /C on Windows), rather than parsing arguments manually like Exec does. This allows correct variable expansion when used with WithEnv, fixing the issue described in bitfield#239, while leaving Exec unchanged for backward compatibility.
Good spotting—that's out of date. I'll update the contributing guide. |
|
Is it possible to implement this in terms of |
|
Good point on the duplication — that makes sense, especially with the context support in #244 that Shell would otherwise miss out on. I'll refactor so Shell builds on Exec's command-setup logic instead of duplicating it, and update the PR shortly. |
|
Done — extracted the shared command-running logic (stdin/stdout/stderr, WithEnv, start/wait, error handling) into an unexported runCmd helper used by both Exec and Shell. Shell now also uses exec.CommandContext, so it picks up context support from #244 automatically. Verified with go build, go vet, and the full test suite on both Linux and Windows — all clean. Pushed as a separate commit so the diff is easy to follow alongside the original. |
Extracts the shared 'configure and run a command' logic (stdin/stdout/ stderr wiring, WithEnv, start/wait, error handling) into a new unexported runCmd helper, used by both Exec and Shell. This avoids duplicating that logic between the two methods and means Shell now picks up context support (added in bitfield#244) automatically, as suggested in review.
Adds a new
Shellmethod (and package-levelShellfunction) that runsa command line through the operating system's native shell, rather than
manually parsing arguments the way
Execdoes. This fixes theWithEnv/variable-expansion bug described in #239, while leavingExecitself unchanged for backward compatibility, per the discussion on the
issue.
--Implementation
sh -cunmodified,so
$VAR-style expansion, quoting, and other shell syntax are handledby the real shell.
cmd /C, following thedirection suggested in the issue ("give the command line to CMD.exe
so that it's processed in the same way that it would be if the user
had typed it into a terminal"). Note that Windows uses
%VAR%syntaxrather than
$VAR, since that's native tocmd.exe.shell_unix.goand
shell_windows.go, using the same//go:buildpattern alreadyused for
script_unix_test.go/script_windows_test.go.--Testing
a test confirming
WithEnvvariables are now correctly expanded(
TestShell_ExpandsEnvironmentVariablesSetViaWithEnv), directlyverifying the fix for the original bug.
pass, including the Windows equivalent using
%VAR%syntax.go test ./...) passes on both platformswith no regressions.
--Docs
Shell(package function and method).I couldn't find a
doMethodsOnPipestress-test helper mentioned inCONTRIBUTING.md anywhere in the current codebase — let me know if it's
been renamed or removed, and I'll update accordingly.
Happy to adjust naming, structure, or behavior based on feedback.