Fix git cl upload deadlock.

Fix git cl upload deadlock.

If the stderr buffer used in git diff became full
before stdout was completely consumed, git cl upload
would deadlock.

This also sets GIT_PAGER to be perfectly sure no pager
is triggered by the git commands run. I haven't
seen that as a problem but it's a reasonable (and
common) precaution.

BUG=558726
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=298090

[Daniel Bratell, 2015-12-22 13:58:02]

tandrii, can you please take a look?

[Daniel Bratell, 2015-12-22 14:02:51]

bratell@opera.com changed reviewers:
+ tandrii@chromium.org

[Daniel Bratell, 2015-12-22 14:02:51]

tandrii, can you please take a look? (this time actually adding tandrii to the
reviewer list)

[tandrii(chromium), 2015-12-22 14:49:37]

LGTM % since it's better than what it used to be.

https://codereview.chromium.org/1544813002/diff/20001/third_party/upload.py
File third_party/upload.py (right):

https://codereview.chromium.org/1544813002/diff/20001/third_party/upload.py#n...
third_party/upload.py:867: # without the potential for deadlocks. We will hope
for the best
indeed, hard. we have a subprocess2 module for this purpose:

https://chromium.googlesource.com/chromium/tools/depot_tools/+/master/subproc...
, but after looking into it, i don't see immediate replacement.

[tandrii(chromium), 2015-12-22 14:49:49]

and thanks for your help!

[Daniel Bratell, 2015-12-22 15:09:02]

The CQ bit was checked by bratell@opera.com

[commit-bot: I haz the power, 2015-12-22 15:09:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1544813002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1544813002/20001

[commit-bot: I haz the power, 2015-12-22 15:10:46]

Description was changed from

==========
Fix git cl upload deadlock.

If the stderr buffer used in git diff became full
before stdout was completely consumed, git cl upload
would deadlock.

This also sets GIT_PAGER to be perfectly sure no pager
is triggered by the git commands run. I haven't
seen that as a problem but it's a reasonable (and
common) precaution.

BUG=558726
==========

to

==========
Fix git cl upload deadlock.

If the stderr buffer used in git diff became full
before stdout was completely consumed, git cl upload
would deadlock.

This also sets GIT_PAGER to be perfectly sure no pager
is triggered by the git commands run. I haven't
seen that as a problem but it's a reasonable (and
common) precaution.

BUG=558726

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=298090
==========

[commit-bot: I haz the power, 2015-12-22 15:10:46]

Committed patchset #2 (id:20001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=298090

[Daniel Bratell, 2015-12-22 15:11:28]

On 2015/12/22 14:49:37, tandrii(chromium) wrote:
> , but after looking into it, i don't see immediate replacement.

Seems the options are:

1. communicate() which return (output, erroutput) in a batch.
2. Merging the streams (so that you only have one piped stream).
3. Ignoring one of the streams (sending to /dev/null)
4. Writing really tricky code.

communicate() probably solves this problem 99% of the time. I was lucky to have
run into this issue once or twice before.

[Dirk Pranke, 2015-12-23 03:55:27]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2015-12-23 03:55:28]

+ some rietveld/upload hackers ...

I don't actually see any callers that set print_output=True. Can we just delete
those code paths and simplify things? 

(It looks like this code is also cloned into gcl.py, where we might want to
clean up some things as well).

[Daniel Bratell, 2015-12-23 07:48:02]

On 2015/12/23 03:55:28, Dirk Pranke wrote:
> + some rietveld/upload hackers ...
> 
> I don't actually see any callers that set print_output=True. Can we just
delete
> those code paths and simplify things? 
> 
> (It looks like this code is also cloned into gcl.py, where we might want to
> clean up some things as well).

gcl.py is fine because it only uses one pipe (stderr is redirected to stdout
which uses the pipe). 

I've uploaded a cleanup patch for upload.py:
https://codereview.chromium.org/1545913002