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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Oct 7, 2022, 15:17 UTC
Message-ID: <69d57d56-2b74-5ee0-a279-de5eb10df7bf@dunelm.org.uk>
In-Reply-To: <dd81a2cadec3e3f131d7b573bf110d4b6cc8f40d.1665153486.git.gitgitgadget@gmail.com>

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

Show 16 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?

Show 7 quoted lines

> +
>       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