On 09 Sep 2015, at 18:00, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> larsxschneider@xxxxxxxxx writes:
>
>> From: Lars Schneider <larsxschneider@xxxxxxxxx>
>>
>> If read_pipe crashes then the caller can inspect the error and handle
>> it appropriately.
>>
>> Signed-off-by: Lars Schneider <larsxschneider@xxxxxxxxx>
>> ---
>> 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?
>
You are right. I will use the “communicate” function here as recommended in the Python docs:
https://docs.python.org/2/library/subprocess.html#subprocess.Popen.wait
Thanks!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html