Skip to content
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

BF: convert revision to str for call #52

Closed
wants to merge 1 commit into from

Conversation

matthew-brett
Copy link

Avoid error converting str / bytes in subprocess.call

Without this change, on Windows and Python 3.5, I get this from ghp-import _build/html:

(py35) C:\repos\la-eeg-uci [master]> ghp-import.exe ._build\html
Traceback (most recent call last):
File "C:\tmp\py35\Scripts\ghp-import-script.py", line 9, in
load_entry_point('ghp-import==0.4.2', 'console_scripts', 'ghp-import')()
File "c:\repos\ghp-import\ghp_import.py", line 194, in main
if not try_rebase(opts.remote, opts.branch):
File "c:\repos\ghp-import\ghp_import.py", line 78, in try_rebase
if sp.call(cmd) != 0:
File "C:\Python35\lib\subprocess.py", line 560, in call
with Popen(_popenargs, *_kwargs) as p:
File "C:\Python35\lib\subprocess.py", line 950, in init
restore_signals, start_new_session)
File "C:\Python35\lib\subprocess.py", line 1194, in _execute_child
args = list2cmdline(args)
File "C:\Python35\lib\subprocess.py", line 754, in list2cmdline
needquote = (" " in arg) or ("\t" in arg) or not arg
TypeError: a bytes-like object is required, not 'str'

Avoid error converting str / bytes in subprocess.call
@matthew-brett matthew-brett mentioned this pull request Mar 8, 2016
@@ -74,7 +74,8 @@ def try_rebase(remote, branch):
(rev, ignore) = p.communicate()
if p.wait() != 0:
return True
cmd = ['git', 'update-ref', 'refs/heads/%s' % branch, rev.strip()]
rev = rev.strip().decode('ascii')
Copy link
Contributor

@waylan waylan Apr 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why you didn't just call dec(rev.strip()) with the dec function defined earlier? That works for me. AFAICT L77 should just be:

cmd = ['git', 'update-ref', 'refs/heads/%s' % branch, dec(rev.strip())]

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm right in saying that rev should always be a commit hash, so ascii or utf-8 decoding (via dec) should give the same result. I guess the only advantage of ascii is that it will give an error when the result is other than a hash.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but dec also does a type check and avoids a call to decode is it is not needed, possibly avoiding an error in some situations. Interestingly, the proposed fix in #42 also does the type check, but doesn't decode to ASCII. Seems like my proposed fix or #42 with ASCII would be a better fix.

waylan added a commit to waylan/mkdocs that referenced this pull request Apr 21, 2016
@waylan
Copy link
Contributor

waylan commented Apr 21, 2016

This appears to be a duplicate of #42.

davisp added a commit that referenced this pull request Dec 16, 2016
The subprocess.call function requires all strings.

Thanks to @waylan for the fix. Thanks to @W1ld and @matthew-brett for
the bug reports.

Fixes #41, #42, #52
@davisp davisp closed this Dec 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants