Re: [PATCH v1 1/2] git-p4: print stderr if P4 read_pipe operation fails

> larsxschneider@gmail.com writes:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> If read_pipe crashes then the caller can inspect the error and handle
>> it appropriately.
>> 
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>> git-p4.py | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/git-p4.py b/git-p4.py
>> index 073f87b..36a4bcb 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -134,11 +134,11 @@ def read_pipe(c, ignore_error=False):
>>         sys.stderr.write('Reading pipe: %s\n' % str(c))
>> 
>>     expand = isinstance(c,basestring)
>> -    p = subprocess.Popen(c, stdout=subprocess.PIPE, shell=expand)
>> +    p = subprocess.Popen(c, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=expand)
>>     pipe = p.stdout
>>     val = pipe.read()
>>     if p.wait() and not ignore_error:
>> -        die('Command failed: %s' % str(c))
>> +        die('Command failed: %s\nError: %s' % (str(c), p.stderr.read()))
> 
> I don't know enough about the callers of this helper function to
> tell offhand if that is an issue, but this looks unsafe depending on
> what the process on the other side of these pipes are doing.
> 
> If it attempts to spew a lot on its standard error stream first and
> then write some to its standard output, I would not be surprised it
> would get stuck waiting for us to read and drain its standard error
> before it can proceed to write to its standard output, and in the
> meantime we would be waiting for it to say something on its standard
> output, no?
>