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: AS53758 23.128.96.0/24 X-Spam-Status: No, score=-3.8 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_PASS, SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by dcvr.yhbt.net (Postfix) with ESMTP id 86D031F953 for ; Tue, 30 Nov 2021 21:18:09 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1343966AbhK3VVZ (ORCPT ); Tue, 30 Nov 2021 16:21:25 -0500 Received: from cloud.peff.net ([104.130.231.41]:40538 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1344079AbhK3VVK (ORCPT ); Tue, 30 Nov 2021 16:21:10 -0500 Received: (qmail 14119 invoked by uid 109); 30 Nov 2021 21:17:49 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 30 Nov 2021 21:17:49 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 30461 invoked by uid 111); 30 Nov 2021 21:17:50 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Tue, 30 Nov 2021 16:17:50 -0500 Authentication-Results: peff.net; auth=none Date: Tue, 30 Nov 2021 16:17:48 -0500 From: Jeff King To: =?utf-8?B?w4Z2YXIgQXJuZmrDtnLDsA==?= Bjarmason Cc: Junio C Hamano , git@vger.kernel.org, Enzo Matsumiya , Eric Sunshine , Emily Shaffer Subject: Re: ab/run-command + em/missing-pager (was: What's cooking in git.git (Nov 2021, #07; Mon, 29)) Message-ID: References: <211130.86k0gpcpy2.gmgdl@evledraar.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <211130.86k0gpcpy2.gmgdl@evledraar.gmail.com> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Tue, Nov 30, 2021 at 09:54:33PM +0100, Ævar Arnfjörð Bjarmason wrote: > > * em/missing-pager (2021-11-24) 1 commit > > - pager: fix crash when pager program doesn't exist > > As noted in [2] I'm happy to get this more isolated fix first. I'd > missed that there was a re-submission[3] until now (since In-Reply-To > wasn't maintained). > > The code change in [3] isn't needed anymore when combined with my > ab/run-command. > > Depending on how you're planning to advance these perhaps you'd like to > revert that as it's merged with ab/run-command, or I can re-roll > ab/run-command on top of it if you'd like. > > We could also just leave that now-redundant child_process_init() in > pager.c, but having something that'll amount to cargo-culting to get > around a bug in dead code doesn't seem ideal. It would be nice to have > the API use reflect "argv" and "env" being gone. IMHO that extra init is still doing the right thing, because it means the struct is in the same known state in every run of setup_pager(). Fixing the argv/env thing stops the immediate bug, but there are other potential ones. E.g., starting the command will overwrite the in/out/err parameters; we're OK in this instance because we unconditionally overwrite them. So I think reusing a child_process struct without reinitializing it is always a bit iffy, though it _usually_ works in practice. We should model good use of the API by initializing on each use. -Peff