-
Notifications
You must be signed in to change notification settings - Fork 443
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 dollar signs along with our quotes #217
base: master
Are you sure you want to change the base?
Conversation
Ugh. Just found out that this breaks too:
Need to escape this, too:
There's got to be a better generalized solution. |
@@ -154,6 +154,7 @@ def parse(args) | |||
|
|||
def self.escape_quotes(str) | |||
str.gsub(/'|"/) { |c| "\\#{c}" } | |||
.gsub(/\$/) { |c| '\\$' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would Shellwords.escape
be a good fit here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly. I am not a ruby guy, but it looks good. However, we are not just passing this to the shell, we are passing it to the shell and putting it in double quotes. I think if we used shellwords.escape we might also want to change all the calling instances to not be wrapped in double quotes. Not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we've had to remove it in the past because it's not implemented in Ruby 1.8. A large number of svn2git users don't otherwise manage Ruby installations, so they just install on whatever their system already has. We support at least the Ubuntu LTS releases (so, going back 5 years) and a couple versions of MacOS X.
I'd need to reevaluate since Ubuntu 10.04 has now fallen off the support path, but we might not be able to realistically use any 1.9+ specific methods yet.
I ran into the same problem as pudge and was going to post a similar pull request until I found pudge's. Is this pull request going to be merged? |
self.escape_quotes is used for putting everything in double quotes, but a $ in double quotes isn't happy, for example:
With this patch: