Escape percentage sign in env variables

[Fodoj]

Before:

ArgumentError: invalid value for Integer(): "/usr/bin/env git fetch origin" if env variable has percentage sign in it

Now:

works fine.

I guess inside within method this issue will still remain if someone will do in: "/usr/i_love_cra%zy_folders", but I assumed that it's rather rare usecase and no-one should have % inside folder names.

[leehambley]

This topic has been raised before about what escaping, if any to do for environment variables, as there's no way to do it safely and correctly it was decided that we wouldn't implement a half measure.

[Fodoj]

@leehambley but there is a half measure already: .gsub(/"/, '\"')

[leehambley]

Indeed, and one special case concession was already the wrong way to fix it. If you can find a way to reliably escape everything that is required under posix and bash's superset of special tokens.

• http://www.gnu.org/software/bash/manual/bashref.html#Escape-Character

gives a good reference, but there is no correct implementation in Ruby that we have found.

[Sinjo]

Was about to submit my own PR for a similar issue, but noticed this one and thought it would be better to comment here.

In command.rb, sprintf is used pretty extensively. In a few cases (e.g. the with method, environment_string is interpolated into a string which is then used in sprintf. If any environment variable contains a '%' symbol, an error ("malformed format string") will be raised as a result.

I've written a small change which fixes the with method, and would be happy to extend it to other affected methods.

My thought on why that escaping is okay is that it's not trying to protect the user from the realities of shell interpolation, rather it's making sure valid strings are passed to sprintf wherever it's used.

Let me know what you think!

[leehambley]

My thought on why that escaping is okay is that it's not trying to protect the user from the realities of shell interpolation, rather it's making sure valid strings are passed to sprintf wherever it's used.

I'd tend to agree, but there are no absolutes, and environment variables containing % are not valid in most shells. I'll look at this more thoroughly when I have time ,thanks @Sinjo

[Sinjo]

That's absolutely fair.

Do you think there's an argument for failing early by default if any environment variable contains a '%' character in that case, with an option for people to say "I know which shell I'm using, and it'll be okay"?

I realise that would be a breaking change, so needs more careful planning, and would take longer to make its way to a release, but it feels like it would be nice to fail early and explicitly where possible.

Edit: For my own knowledge, I'm curious which shells have issues with '%'. Was there a discussion somewhere I can read? The behaviour I've seen in bash is what I would have expected:

$ FOO="asdf%asdf"
$ echo $FOO
asdf%asdf

[leehambley]

The only internet based citation I can find is http://stackoverflow.com/a/2821183/119669 - But it's highly possible to misread, even the two quoted paragraphs form the open standards document referenced. I'd be fine, as I said, let me review the change a bit more thoroughly when I have change.

[Sinjo]

Thanks! Didn't intend to pressure/rush the change at all. Just always keen to improve my knowledge on things. :)

[leehambley]

All fine mate, by the way, please update the changelog, etc so I can merge this -

[leehambley]

Closing, as we have #280 now