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

Phillip Wood [off-list ref] writes:

Hi Daniel

On 07/10/2022 15:38, Daniel Sonbolian via GitGitGadget wrote:

quoted

From: Daniel Sonbolian <redacted>
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.

quoted

Signed-off-by: Daniel Sonbolian <redacted>
---
  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.

quoted

      lines = pipe.readlines()
+    pipe.close()

You're now ignoring the return value of pipe.close() - is that safe?

quoted

+
      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