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.

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

Escape shell command arguments. #87

Merged
merged 1 commit into from May 5, 2013

Conversation

Projects
None yet
2 participants
@matsuu
Contributor

matsuu commented May 5, 2013

No description provided.

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy May 5, 2013

Owner

Your patch doesn't work well. So I will not merge it.

For example,
matsuu@82f9a80#L6L132

This cron check command results in error.

# crontab -u root -l | grep -- \\\\\\*\\ \\\\\\*\\ \\\\\\*\\ \\\\\\*\\ \\\\\\*\\ /usr/local/bin/batch.sh
zsh: no matches found: \\\*\

Also error occurs with bash.

# crontab -u root -l | grep -- \\\\\\*\\ \\\\\\*\\ \\\\\\*\\ \\\\\\*\\ \\\\\\*\\ /usr/local/bin/batch.sh
grep: Trailing backslash

Ruby's shellwords escapes the characters that are not needed to be escaped, so I will not use shellwords in serverspec.

Owner

mizzy commented May 5, 2013

Your patch doesn't work well. So I will not merge it.

For example,
matsuu@82f9a80#L6L132

This cron check command results in error.

# crontab -u root -l | grep -- \\\\\\*\\ \\\\\\*\\ \\\\\\*\\ \\\\\\*\\ \\\\\\*\\ /usr/local/bin/batch.sh
zsh: no matches found: \\\*\

Also error occurs with bash.

# crontab -u root -l | grep -- \\\\\\*\\ \\\\\\*\\ \\\\\\*\\ \\\\\\*\\ \\\\\\*\\ /usr/local/bin/batch.sh
grep: Trailing backslash

Ruby's shellwords escapes the characters that are not needed to be escaped, so I will not use shellwords in serverspec.

@mizzy mizzy closed this May 5, 2013

@mizzy mizzy reopened this May 5, 2013

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy May 5, 2013

Owner

I've tried your patch in other environment and it works well.So I reopen this issue and will check deeply.

Owner

mizzy commented May 5, 2013

I've tried your patch in other environment and it works well.So I reopen this issue and will check deeply.

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy May 5, 2013

Owner

Ah, I see.

crontab -u root -l | grep -- \\\\\\*\\ \\\\\\*\\ \\\\\\*\\ \\\\\\*\\ \\\\\\*\\ /usr/local/bin/batch.sh

Above runs as below actually.

crontab -u root -l | grep -- \\\*\ \\\*\ \\\*\ \\\*\ \\\*\ /usr/local/bin/batch.sh
``

So it works well.

Sorry for my misunderstanding.
Owner

mizzy commented May 5, 2013

Ah, I see.

crontab -u root -l | grep -- \\\\\\*\\ \\\\\\*\\ \\\\\\*\\ \\\\\\*\\ \\\\\\*\\ /usr/local/bin/batch.sh

Above runs as below actually.

crontab -u root -l | grep -- \\\*\ \\\*\ \\\*\ \\\*\ \\\*\ /usr/local/bin/batch.sh
``

So it works well.

Sorry for my misunderstanding.

mizzy added a commit that referenced this pull request May 5, 2013

Merge pull request #87 from matsuu/escape-shell-2
Escape shell command arguments.

@mizzy mizzy merged commit ff15773 into mizzy:master May 5, 2013

1 check passed

default The Travis build passed
Details
@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy May 5, 2013

Owner

It looks work well. Thanks!

Owner

mizzy commented May 5, 2013

It looks work well. Thanks!

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