Re: [PATCH v2 1/2] git-p4: minor optimization in read_pip_lines

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 7, 2022, 17:28 UTC
Message-ID: <xmqqy1trfsqm.fsf@gitster.g>
In-Reply-To: <69d57d56-2b74-5ee0-a279-de5eb10df7bf@dunelm.org.uk>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 11 quoted lines

> Hi Daniel
>
> On 07/10/2022 15:38, Daniel Sonbolian via GitGitGadget wrote:
>> From: Daniel Sonbolian <dsal3389@gmail.com>
>> checking for an error condition before reading and/or decoding
>> lines from the pip stream to avoid unnecessary computation
>
> It would be helpful to say a little more about what the error is
> you're detecting why it is better to detect it earlier. Having looked
> at the changes I'm not really sure what these changes are trying to
> improve

Thanks for the comments.

Also, even though Documentation/SubmittingPatches does not mention in its [[describe-changes]] section, we do use the usual capitalization in the body of the commit log message (after the <area>: prefix on the commit title is the only exception).

I agree with everything you said in your review about the code. Unless what pipe.readlines() would read is so small that it fits in the OS pipe buffer, waiting for the subprocess to finish without reading its output is likely to deadlock.

Thanks.

Show 38 quoted lines

>> Signed-off-by: Daniel Sonbolian <dsal3389@gmail.com>
>> ---
>>   git-p4.py | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>> diff --git a/git-p4.py b/git-p4.py
>> index d26a980e5ac..097272a5543 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -399,11 +399,15 @@ def read_pipe_lines(c, raw=False, *k, **kw):
>>         p = subprocess.Popen(c, stdout=subprocess.PIPE, *k, **kw)
>>       pipe = p.stdout
>> +
>> +    if p.wait():
>> +        die('Command failed: {}'.format(' '.join(c)))
>
> I'm not a python programmer, but the documentation for Popen.wait()
> says that this will wait for the process to finish, so I think calling
> it before reading the lines below is an error.
>
>>       lines = pipe.readlines()
>> +    pipe.close()
>
> You're now ignoring the return value of pipe.close() - is that safe?
>
>> +
>>       if not raw:
>> -        lines = [decode_text_stream(line) for line in lines]
>> -    if pipe.close() or p.wait():
>> -        die('Command failed: {}'.format(' '.join(c)))
>> +        return [decode_text_stream(line) for line in lines]
>>       return lines
>
> I'm not really sure what you're tying to achieve here - what was wrong
> with the original code?
>
> Best Wishes
>
> Phillip