Skip to content

Use shell parser to correctly parse exec commands with quotes #445

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jul 29, 2025
Merged

Use shell parser to correctly parse exec commands with quotes #445

merged 1 commit into from
Jul 29, 2025

Conversation

MatthewCane
Copy link
Contributor

@MatthewCane MatthewCane commented Jul 3, 2025 •
edited
Loading

This fixes #443.

Using the sh shell parser fixes the issue where quotes were not being properly split meaning commands like sh -c 'echo foo' would always fail. Previously it would be parsed as:

["sh", "-c", "'echo", "foo'"]

whereas it should be parsed as:

["sh", "-c", "'echo foo'"]

This also means that pipes can be used correctly as pipes are not supported by the os/exec module without using a subshell, see:

Unlike the "system" library call from C and other languages, the os/exec package intentionally does not invoke the system shell and does not expand any glob patterns or handle other expansions, pipelines, or redirections typically done by shells.

@MatthewCane MatthewCane changed the title Use shlex to correctly parse exec command with proper quoting #443 Use shlex to correctly parse exec commands with quotes #443 Jul 3, 2025
@MatthewCane
Copy link
Contributor Author

MatthewCane commented Jul 3, 2025 •
edited
Loading

To the maintainers: I am on MacOS and having some trouble running the test suite. It builds and behaves correctly as far as I can tell, however I might need someone who has the environment set up to run the test suite. I am more than happy to fix any issues come up.

Edit: I got my test setup working, the Testcontainer setup was looking at the default Docker socket, but I am using Colima so I had to change that and it worked

@MatthewCane MatthewCane mentioned this pull request Jul 3, 2025
Closed
@MatthewCane MatthewCane force-pushed the feature/parse-cmd-correctly branch from 8112212 to 324cb85 Compare July 3, 2025 19:23
@MatthewCane MatthewCane changed the title Use shlex to correctly parse exec commands with quotes #443 Fix/Use shlex to correctly parse exec commands with quotes #443 Jul 4, 2025
@mortymacs
Copy link
Member

Hi @MatthewCane ,

Thanks for the PR! The only issue we have is that the library you're using is outdated (last commit was about 5 years ago), and we need something more current. If you don't mind, I'll update the PR.

Cheers,
Mort

@MatthewCane
Copy link
Contributor Author

You are right @mortymacs, it is pretty old. I did see a Google package that is even older, and a few others, but your knowledge of the Go ecosystem is probably a lot better than mine so you are more than welcome to update the PR with a better maintained library.

@MatthewCane MatthewCane force-pushed the feature/parse-cmd-correctly branch from 324cb85 to 2ba9996 Compare July 22, 2025 13:59
@MatthewCane MatthewCane changed the title Fix/Use shlex to correctly parse exec commands with quotes #443 Use shell parser to correctly parse exec commands with quotes #443 Jul 22, 2025
@MatthewCane MatthewCane changed the title Use shell parser to correctly parse exec commands with quotes #443 Use shell parser to correctly parse exec commands with quotes Jul 22, 2025
@MatthewCane MatthewCane force-pushed the feature/parse-cmd-correctly branch from 2ba9996 to 6aa4afc Compare July 22, 2025 14:04
@MatthewCane MatthewCane force-pushed the feature/parse-cmd-correctly branch from 6aa4afc to 8d5fb06 Compare July 22, 2025 14:05
@MatthewCane
Copy link
Contributor Author

@mortymacs I have found a replacement library, https://github.com/mvdan/sh, that is well maintained and does what we need. Let me know if this is okay!

@mortymacs
Copy link
Member

@MatthewCane awesome! thanks you very much! I'm going to add a few tests and then we'll merge it soon 👍

@coveralls
Copy link

Pull Request Test Coverage Report for Build 16446650440

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 5 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.09%) to 55.775%
Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/cmd/exec.go 0 5 0.0%
Totals Coverage Status
Change from base Build 16291965470: -0.09%
Covered Lines: 1357
Relevant Lines: 2433
💛 - Coveralls

@mortymacs
Copy link
Member

I'm gonna merge the PR and I'll add tests in a separate PR.

@mortymacs mortymacs merged commit ff9b085 into wait4x:main Jul 29, 2025
6 of 7 checks passed
@mortymacs
Copy link
Member

Thanks @MatthewCane !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mortymacs mortymacs mortymacs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: Exec function is not working as expected
3 participants
@MatthewCane @mortymacs @coveralls