Hi Daniel
On 07/10/2022 15:38, Daniel Sonbolian via GitGitGadget wrote:
From: Daniel Sonbolian <dsal3389@xxxxxxxxx>
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 <dsal3389@xxxxxxxxx>
---
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
