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

Change single quotes to double quotes in get_author_info_from_short_sha to fix error in Windows #238

Merged
merged 8 commits into from May 14, 2018

Conversation

Projects
None yet
7 participants
@andresdelfino
Copy link
Contributor

andresdelfino commented Apr 21, 2018

cherry_picker --continue breaks on Windows.

This happens because the process started from function get_author_info_from_short_sha is executed this way:

C:\WINDOWS\system32\cmd.exe /c "git log -1 --format='%aN <%ae>' shortsha"

The single quotes don't escape the angle brackets, so cmd tries to read the non-existent file "%ae", producing a "The system cannot find the file specified" error (this can be observed in stderr after editing subprocess.py)

Using double quotes fixes this.

Environment to reproduce the issue:

Windows 10 x64
Python 3.6.5 x64
Git for Windows 2.16.2 64-bit (running a bash-only configuration)

Traceback of the error:

šŸ šŸ’ ā›
Traceback (most recent call last):
  File "c:\program files\python36\lib\runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "c:\program files\python36\lib\runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "C:\Program Files\Python36\Scripts\cherry_picker.exe\__main__.py", line 9, in <module>
  File "c:\program files\python36\lib\site-packages\click\core.py", line 722, in __call__
    return self.main(*args, **kwargs)
  File "c:\program files\python36\lib\site-packages\click\core.py", line 697, in main
    rv = self.invoke(ctx)
  File "c:\program files\python36\lib\site-packages\click\core.py", line 895, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "c:\program files\python36\lib\site-packages\click\core.py", line 535, in invoke
    return callback(*args, **kwargs)
  File "c:\program files\python36\lib\site-packages\cherry_picker\cherry_picker.py", line 402, in cherry_pick_cli
    cherry_picker.continue_cherry_pick()
  File "c:\program files\python36\lib\site-packages\cherry_picker\cherry_picker.py", line 328, in continue_cherry_pick
    co_author_info = f"Co-authored-by: {get_author_info_from_short_sha(short_sha)}"
  File "c:\program files\python36\lib\site-packages\cherry_picker\cherry_picker.py", line 441, in get_author_info_from_short_sha
    output = subprocess.check_output(cmd, shell=True, stderr=subprocess.STDOUT)
  File "c:\program files\python36\lib\subprocess.py", line 336, in check_output
    **kwargs).stdout
  File "c:\program files\python36\lib\subprocess.py", line 418, in run
    output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command 'git log -1 --format='%aN <%ae>' b81ca28' returned non-zero exit status 1.
@terryjreedy
Copy link
Member

terryjreedy left a comment

If the CI test tests more than 'module compiles', it should be run on Windows (Appveyor) as well as Linux (Travis).

@@ -437,7 +437,7 @@ def get_full_sha_from_short(short_sha):


def get_author_info_from_short_sha(short_sha):
cmd = f"git log -1 --format='%aN <%ae>' {short_sha}"
cmd = f'git log -1 --format="%aN <%ae>" {short_sha}'

This comment has been minimized.

@terryjreedy

terryjreedy Apr 22, 2018

Member

I have had cherry_picker -- continue work for me, but I am using the original version, which does not have this code ;-). I know that double quotes are required for Windows commands. I have the impression that single quotes are required for Linux. This is the wrong fix. The subprocess doc says

"args is required for all calls and should be a string, or a sequence of program arguments. Providing a sequence of arguments is generally preferred, as it allows the module to take care of any required escaping and quoting of arguments (e.g. to permit spaces in file names). If passing a single string, either shell must be True (see below) or else the string must simply name the program to be executed without specifying any arguments."

When neither of the last two two conditions is true, the idea is to let subprocess handle the OS-specific escaping and quoting. (At least on Windows, a string properly formatted for Windows does work, and I know people use them.) It seems that args should be a list. I am not sure what constitutes a 'program argument', in in particular, how one is 'supposed to' handle '--argname=value', but maybe this works on both Linux and *nix.

`cmd = ['git', 'log', '-1', '--format=', '%aN <%ae>', f'{short_sha}']

This comment has been minimized.

@ericvsmith

ericvsmith Apr 22, 2018

Member

I agree with Terry, although I haven't tested this particular case on Windows.

If using a list for cmd, it should be:
cmd = ['git', 'log', '-1', '--format=%aN <%ae>', f'{short_sha}']

Since the shell wouldn't split --argname=value, it should remain a single item in the list.

This comment has been minimized.

@andresdelfino

andresdelfino Apr 22, 2018

Contributor

I followed @ericvsmith suggestion and I can confirm it works on Windows.

@andresdelfino

This comment has been minimized.

Copy link
Contributor

andresdelfino commented Apr 22, 2018

Should I change all cmd occurrences of strings to lists?

@andresdelfino

This comment has been minimized.

Copy link
Contributor

andresdelfino commented Apr 22, 2018

Plus, I think we can get rid of "shell=True", right?

@ericvsmith

This comment has been minimized.

Copy link
Member

ericvsmith commented Apr 22, 2018

I think changing all cmd's to lists, and on those getting rid of shell=True would be a good improvement.

@andresdelfino

This comment has been minimized.

Copy link
Contributor

andresdelfino commented Apr 24, 2018

All calls now work with lists and without shell.

I have moved a few calls to separate variables to make the code easier to grep.

@serhiy-storchaka
Copy link
Member

serhiy-storchaka left a comment

Just a style nit: use single quotes for consistency with the rest of the code (and this is a preferable style in Python). Double quotes was used because command line strings contained single quotes around arguments.

The rest LGTM.


def run_cmd(self, cmd, shell=False):
def run_cmd(self, cmd):
if self.dry_run:

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka Apr 24, 2018

Member

Just for the case add:

assert not isinstance(cmd, str)

This comment has been minimized.

@andresdelfino

andresdelfino Apr 24, 2018

Contributor

I think it's a great idea to add an assertion here. Perhaps we should go with positive, instead of the negative? Let me know if you think otherwise and I'll change it.

This comment has been minimized.

@andresdelfino

andresdelfino Apr 24, 2018

Contributor

Nevermind, I followed your suggestion in the end because testing that cmd is a sequence but it isn't a string would bloat a simple line with no real benefit.

@andresdelfino

This comment has been minimized.

Copy link
Contributor

andresdelfino commented Apr 26, 2018

@terryjreedy do the changes I made address your comments?

@ericvsmith

This comment has been minimized.

Copy link
Member

ericvsmith commented Apr 26, 2018

I'd like @Mariatta to give her opinion.

@brettcannon

This comment has been minimized.

Copy link
Member

brettcannon commented Apr 27, 2018

FYI Mariatta is taking time off from open source until I think June, so she won't get to this for a while.

@Mariatta Mariatta merged commit 0099339 into python:master May 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Mariatta

This comment has been minimized.

Copy link
Member

Mariatta commented May 14, 2018

Thanks @andresdelfino šŸŒ®
Sorry for the delay in reviewing this. I'll get this published to PyPI soon.

@andresdelfino andresdelfino deleted the andresdelfino:fix-windows-errors branch May 14, 2018

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