[PATCH] D54807: llvm-git: More tweaks.

[James Y Knight via Phabricator via llvm-commits]

jyknight added a comment.

BTW, I've tested this on Linux in both python2 and python3, but not on windows.



================
Comment at: llvm/utils/git-svn/git-llvm:120-121
 
-    if force_binary_stdin and stdin:
-        stdin = stdin.encode('utf-8')
-
----------------
zturner wrote:
> Don't we still need this line?  When I wrote this patch, it was because after experimenting I found that `subprocess.Popen` would with `universal_newlines=False` unless `stdin` was a `bytes`.
Yes, that's correct. "universal_newlines" in python3 really means "text". If it's enabled, the input/output are str, otherwise they're bytes. And for binary data, we _want_ (and now provide) bytes input, so this line is unneeded and undesirable. (in python3.7, they in fact fixed the confusing name, and added a "text" keyword arg as an alias of the "universal_newlines" one.). 


================
Comment at: llvm/utils/git-svn/git-llvm:132-134
-    if not text:
-        stdout = stdout.decode('utf-8')
-        stderr = stderr.decode('utf-8')
----------------
zturner wrote:
> Don't we need these lines too?  Like before, I found that if `universal_newlines=False` on Python 3, then `stdout` and `stderr` would both be a `bytes` object, and subsequent lines of code where we do something like `stdout.strip('\r\n')` would fail because it would be trying to mix `bytes` and `str`.
We don't need them -- the purpose of this change is exactly to allow the use of bytes objects for binary I/O. Note the diff below to use rstrip(b'\r\n') instead of rstrip('\r\n') when !text.


https://reviews.llvm.org/D54807