opendev/git-review
Code Issues Proposed changes

Fix output printing with python3

Browse Source 
I'm not sure what print_safe_encoding is trying to do
(Ie834931a549175471af029a6ec4d5794543d8c92).

  print(str.encode(sys.stdout.encoding, 'replace'))

means that print() ends up getting a bytes-object encoded in the
output encoding.  Thus print() is then going to output this as raw
bytes... not only is unicode going to come out as "\x..\x.." etc, but
even newlines get squished and you end up with stuff like

  $ /tmp/gitreview3/bin/git-review
  b'remote: \rremote: Processing changes: updated: 1, refs: 1\rremote: Processing changes: updated: 1,

I think this problem stems from not ensuring that incoming data
(output of git) is encoded as a string.  By setting
universal_newlines=True in run_command_status() we automatically
ensure that any incoming data is a unicode str, not bytes.

The final section, with the message about rhbz#1058167, appears to be
related to running git under an alternative locale where it's output
is translated.  I1bf1124f0b09d6658a7b0703e3b9e74ed80f4eea changed
things to run the commands under C locale, so there shouldn't be
unicode in here I wouldn't think.  But, by ensuring it's just a
string, we can print() it safely anyway.

Change-Id: I596dcba317ecfbaf437cb72d9850580ed6765fce
This commit is contained in:
Ian Wienand 2017-11-15 11:43:34 +11:00
parent f918bf76d2
commit bbea22f773
1 changed files with 19 additions and 36 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

55
git_review/cmd.py
View File

@ -68,13 +68,6 @@ _rewrites = None
_rewrites_push = None _rewrites_push = None
def print_safe_encoding(str):
if sys.stdout.encoding is None:
print(str)
else:
print(str.encode(sys.stdout.encoding, 'replace'))
class colors(object): class colors(object):
yellow = '\033[33m' yellow = '\033[33m'
green = '\033[92m' green = '\033[92m'
@ -154,9 +147,8 @@ def run_command_status(*argv, **kwargs):
stdin=subprocess.PIPE if stdin else None, stdin=subprocess.PIPE if stdin else None,
stdout=subprocess.PIPE, stdout=subprocess.PIPE,
stderr=subprocess.STDOUT, stderr=subprocess.STDOUT,
env=newenv) env=newenv, universal_newlines=True)
(out, nothing) = p.communicate(stdin) (out, nothing) = p.communicate(stdin)
out = out.decode('utf-8', 'replace')
return (p.returncode, out.strip()) return (p.returncode, out.strip())
@ -266,7 +258,7 @@ def run_custom_script(action):
raise CustomScriptException(status, output, [path], {}) raise CustomScriptException(status, output, [path], {})
elif output and VERBOSE: elif output and VERBOSE:
print("script %s output is:" % (path)) print("script %s output is:" % (path))
print_safe_encoding(output) print(output)
def git_config_get_value(section, option, default=None, as_bool=False): def git_config_get_value(section, option, default=None, as_bool=False):
@ -659,7 +651,7 @@ def query_reviews_over_ssh(remote_url, project=None, change=None,
"gerrit", "query", "gerrit", "query",
"--format=JSON %s" % query) "--format=JSON %s" % query)
if VERBOSE: if VERBOSE:
print_safe_encoding(output) print(output)
changes = [] changes = []
try: try:
@ -671,7 +663,7 @@ def query_reviews_over_ssh(remote_url, project=None, change=None,
changes.append(data) changes.append(data)
except Exception: except Exception:
if VERBOSE: if VERBOSE:
print_safe_encoding(output) print(output)
except Exception as err: except Exception as err:
raise parse_exc(err) raise parse_exc(err)
return changes return changes
@ -738,11 +730,11 @@ def update_remote(remote):
cmd = "git remote update %s" % remote cmd = "git remote update %s" % remote
(status, output) = run_command_status(cmd) (status, output) = run_command_status(cmd)
if VERBOSE: if VERBOSE:
print_safe_encoding(output) print(output)
if status != 0: if status != 0:
print("Problem running '%s'" % cmd) print("Problem running '%s'" % cmd)
if not VERBOSE: if not VERBOSE:
print_safe_encoding(output) print(output)
return False return False
return True return True
@ -850,7 +842,7 @@ def rebase_changes(branch, remote, interactive=True):
if status != 0: if status != 0:
print("Errors running %s" % cmd) print("Errors running %s" % cmd)
if interactive: if interactive:
print_safe_encoding(output) print(output)
return False return False
_orig_head = output _orig_head = output
@ -871,14 +863,14 @@ def rebase_changes(branch, remote, interactive=True):
if status != 0: if status != 0:
print("Errors running %s" % cmd) print("Errors running %s" % cmd)
if interactive: if interactive:
print_safe_encoding(output) print(output)
print("It is likely that your change has a merge conflict. " printwrap("It is likely that your change has a merge conflict. "
"You may resolve it in the working tree now as " "You may resolve it in the working tree now as "
"described above and then run 'git review' again, or " "described above and then run 'git review' again, or "
"if you do not want to resolve it yet (note that the " "if you do not want to resolve it yet (note that the "
"change can not merge until the conflict is resolved) " "change can not merge until the conflict is resolved) "
"you may run 'git rebase --abort' then 'git review -R' " "you may run 'git rebase --abort' then 'git review -R' "
"to upload the change without rebasing.") "to upload the change without rebasing.")
return False return False
return True return True
@ -892,7 +884,7 @@ def undo_rebase():
(status, output) = run_command_status(cmd) (status, output) = run_command_status(cmd)
if status != 0: if status != 0:
print("Errors running %s" % cmd) print("Errors running %s" % cmd)
print_safe_encoding(output) print(output)
return False return False
return True return True
@ -919,7 +911,7 @@ def assert_one_change(remote, branch, yes, have_hook):
(status, output) = run_command_status(cmd) (status, output) = run_command_status(cmd)
if status != 0: if status != 0:
print("Had trouble running %s" % cmd) print("Had trouble running %s" % cmd)
print_safe_encoding(output) print(output)
sys.exit(1) sys.exit(1)
filtered = filter(None, output.split("\n")) filtered = filter(None, output.split("\n"))
output_lines = sum(1 for s in filtered) output_lines = sum(1 for s in filtered)
@ -1655,7 +1647,7 @@ def _main():
print("\t%s\n" % cmd) print("\t%s\n" % cmd)
else: else:
(status, output) = run_command_status(cmd) (status, output) = run_command_status(cmd)
print_safe_encoding(output) print(output)
if options.finish and not options.dry and status == 0: if options.finish and not options.dry and status == 0:
finish_branch(branch) finish_branch(branch)
@ -1670,16 +1662,7 @@ def main():
try: try:
_main() _main()
except GitReviewException as e: except GitReviewException as e:
# If one does unguarded print(e) here, in certain locales the implicit print(e)
# str(e) blows up with familiar "UnicodeEncodeError ... ordinal not in
# range(128)". See rhbz#1058167.
try:
u = unicode(e)
except NameError:
# Python 3, we're home free.
print(e)
else:
print(u.encode('utf-8'))
sys.exit(getattr(e, 'EXIT_CODE', -1)) sys.exit(getattr(e, 'EXIT_CODE', -1))