From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS3215 2.6.0.0/16 X-Spam-Status: No, score=-3.8 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20]) by dcvr.yhbt.net (Postfix) with ESMTP id 913BD1F403 for ; Fri, 7 Oct 2022 17:28:42 +0000 (UTC) Authentication-Results: dcvr.yhbt.net; dkim=pass (1024-bit key; unprotected) header.d=pobox.com header.i=@pobox.com header.b="kRqpGMJI"; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229723AbiJGR2j (ORCPT ); Fri, 7 Oct 2022 13:28:39 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53078 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229709AbiJGR2h (ORCPT ); Fri, 7 Oct 2022 13:28:37 -0400 Received: from pb-smtp2.pobox.com (pb-smtp2.pobox.com [64.147.108.71]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 13018D73DE for ; Fri, 7 Oct 2022 10:28:35 -0700 (PDT) Received: from pb-smtp2.pobox.com (unknown [127.0.0.1]) by pb-smtp2.pobox.com (Postfix) with ESMTP id 411B114594D; Fri, 7 Oct 2022 13:28:35 -0400 (EDT) (envelope-from junio@pobox.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed; d=pobox.com; h=from:to:cc :subject:references:date:in-reply-to:message-id:mime-version :content-type; s=sasl; bh=Hh4CIF/Rev8CdEujFe/PKLIHZDphs/1xMZGCL7 dMnZw=; b=kRqpGMJI5r9oAubEujzu+ykCqBk2mWX4jfZeNAR3loa6t2dL008X5Y jrPt8OJUzcgnneyLkSXYuEva3qSKEzSHH0yDhyWfxjlQRmVmOyvNG0Vo7fcOkab+ W5oSa4TcvbiDUnlL0hkMaifT1Bp0qR5bEWNYvBESYDwWHYFyexbHU= Received: from pb-smtp2.nyi.icgroup.com (unknown [127.0.0.1]) by pb-smtp2.pobox.com (Postfix) with ESMTP id 394C714594C; Fri, 7 Oct 2022 13:28:35 -0400 (EDT) (envelope-from junio@pobox.com) Received: from pobox.com (unknown [34.83.5.33]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by pb-smtp2.pobox.com (Postfix) with ESMTPSA id 9F7F314594B; Fri, 7 Oct 2022 13:28:34 -0400 (EDT) (envelope-from junio@pobox.com) From: Junio C Hamano To: Phillip Wood Cc: Daniel Sonbolian via GitGitGadget , git@vger.kernel.org, Daniel Sonbolian Subject: Re: [PATCH v2 1/2] git-p4: minor optimization in read_pip_lines References: <69d57d56-2b74-5ee0-a279-de5eb10df7bf@dunelm.org.uk> Date: Fri, 07 Oct 2022 10:28:33 -0700 In-Reply-To: <69d57d56-2b74-5ee0-a279-de5eb10df7bf@dunelm.org.uk> (Phillip Wood's message of "Fri, 7 Oct 2022 16:17:33 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Pobox-Relay-ID: 789010EC-4665-11ED-9EF8-307A8E0A682E-77302942!pb-smtp2.pobox.com Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Phillip Wood writes: > Hi Daniel > > On 07/10/2022 15:38, Daniel Sonbolian via GitGitGadget wrote: >> From: Daniel Sonbolian >> 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 : 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 >> --- >> 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