-
-
Notifications
You must be signed in to change notification settings - Fork 59
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
Conversation
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.
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}' |
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 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}']
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 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.
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 followed @ericvsmith suggestion and I can confirm it works on Windows.
Should I change all cmd occurrences of strings to lists? |
Plus, I think we can get rid of "shell=True", right? |
I think changing all cmd's to lists, and on those getting rid of |
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. |
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.
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: |
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.
Just for the case add:
assert not isinstance(cmd, str)
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 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.
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.
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.
@terryjreedy do the changes I made address your comments? |
I'd like @Mariatta to give her opinion. |
FYI Mariatta is taking time off from open source until I think June, so she won't get to this for a while. |
Thanks @andresdelfino 🌮 |
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: