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

Commit Message

Daniel Sonbolian Oct. 7, 2022, 2:38 p.m. UTC

From: Daniel Sonbolian <[e-mail]>

checking for an error condition before reading and/or decoding
lines from the pip stream to avoid unnecessary computation

Signed-off-by: Daniel Sonbolian <[e-mail]>
---
 git-p4.py | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Phillip Wood Oct. 7, 2022, 3:17 p.m. UTC | #1

Hi Daniel

On 07/10/2022 15:38, Daniel Sonbolian via GitGitGadget wrote:
> From: Daniel Sonbolian <[e-mail]>
> 
> 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

> Signed-off-by: Daniel Sonbolian <[e-mail]>
> ---
>   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

Junio C Hamano Oct. 7, 2022, 5:28 p.m. UTC | #2

Phillip Wood <[e-mail]> writes:

> Hi Daniel
>
> On 07/10/2022 15:38, Daniel Sonbolian via GitGitGadget wrote:
>> From: Daniel Sonbolian <[e-mail]>
>> 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.

>> Signed-off-by: Daniel Sonbolian <[e-mail]>
>> ---
>>   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

Patch

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)))
+
     lines = pipe.readlines()
+    pipe.close()
+
     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