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

Only quote arguments during cmdify if needed #2563

Merged
merged 2 commits into from Jul 11, 2018

Conversation

Projects
None yet
2 participants
@uranusjr
Member

uranusjr commented Jul 11, 2018

Windows built-in commands don't always work well with quotes.

rem The path can be interpreted correctly, but not "/w".
dir "/w" "C:\"

rem Outputs "foo" "bar", with quotes!
echo "foo" "bar"

The solution (I think) is to be less aggressive. Only quote things if they need to be quoted (i.e. contains whitespaces).

uranusjr added some commits Jul 11, 2018

@techalchemy

This comment has been minimized.

Member

techalchemy commented Jul 11, 2018

knock this off, I have the canonical function already - I stole this from stackoverflow where they insisted that it was the correct way to handle this problem on windows

def escape_grouped_arguments(s):
    """Prepares a string for the shell (on Windows too!)

    Only for use on grouped arguments (passed as a string to Popen)
    """
    if s is None:
        return None

    # Additional escaping for windows paths
    if os.name == "nt":
        s = "{}".format(s.replace("\\", "\\\\"))
    return '"' + s.replace("'", "'\\''") + '"'
@techalchemy

This comment has been minimized.

Member

techalchemy commented Jul 11, 2018

(this wasn't that long ago and the insistence was that there is no better alternative in any part of any library that can handle this -- it seemed true then and AFAIK this is pretty close)

To summarize my concern / question more broadly though, since I haven't had a chance to really review and I don't have a problem replacing this on principle, just in practical terms -- what is the advantage to doing regex parsing here / is it really needed?

@uranusjr

This comment has been minimized.

Member

uranusjr commented Jul 11, 2018

@techalchemy The use case is a bit different here. That function is good if you already have a string, but here we have a list of arguments, i.e. ["dir", "/w", "C:\Program Files"]. The problem is to decide when to quote, not how (that is easy, and the two solutions are logically equivalent).

@techalchemy

This comment has been minimized.

Member

techalchemy commented Jul 11, 2018

[shellquote(arg) for arg in args if " " in arg] (if thats where you landed on this I agree)

@uranusjr

This comment has been minimized.

Member

uranusjr commented Jul 11, 2018

That won’t work either because it escapes single quotes, and Windows don’t want you to.

import shlex
import os
import re


def escape_grouped_arguments(s):
    """Prepares a string for the shell (on Windows too!)

    Only for use on grouped arguments (passed as a string to Popen)
    """
    if s is None:
        return None

    # Additional escaping for windows paths
    if os.name == "nt":
        s = "{}".format(s.replace("\\", "\\\\"))
    return '"' + s.replace("'", "'\\''") + '"'


def my_quote(s):
    return '"{0}"'.format(re.sub(r'(\\*)"', r'\1\1\\"', s))

s = "print('hello world')"
print('SO:   ', 'python', '-c', escape_grouped_arguments(s))
print('shlex:', 'python', '-c', shlex.quote(s))
print('mine: ', 'python', '-c', my_quote(s))
SO:    python -c "print('\''hello world'\'')"
shlex: python -c 'print('"'"'hello world'"'"')'
mine:  python -c "print('hello world')"

Only mine works correctly in cmd.exe.

@techalchemy

This comment has been minimized.

Member

techalchemy commented Jul 11, 2018

Damn. You should make a library out of this or something

@uranusjr uranusjr merged commit aec7881 into master Jul 11, 2018

2 checks passed

VSTS: pipenv-Python Package-CI 20180711.4 succeeded
Details
buildkite/pipenv Build #851 passed (8 minutes, 42 seconds)
Details

@uranusjr uranusjr deleted the cmd-quote-better branch Jul 11, 2018

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