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 E2E001F9F4 for ; Mon, 22 Nov 2021 17:41:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240078AbhKVRoY (ORCPT ); Mon, 22 Nov 2021 12:44:24 -0500 Received: from cloud.peff.net ([104.130.231.41]:36436 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232316AbhKVRoX (ORCPT ); Mon, 22 Nov 2021 12:44:23 -0500 Received: (qmail 21799 invoked by uid 109); 22 Nov 2021 17:41:16 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Mon, 22 Nov 2021 17:41:16 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 21778 invoked by uid 111); 22 Nov 2021 17:41:16 -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; Mon, 22 Nov 2021 12:41:16 -0500 Authentication-Results: peff.net; auth=none Date: Mon, 22 Nov 2021 12:41:15 -0500 From: Jeff King To: =?utf-8?B?w4Z2YXIgQXJuZmrDtnLDsA==?= Bjarmason Cc: Enzo Matsumiya , Junio C Hamano , git@vger.kernel.org Subject: Re: [PATCH v2] pager: fix crash when pager program doesn't exist Message-ID: References: <20211120194048.12125-1-ematsumiya@suse.de> <20211122153119.h2t2ti3lkiycd7pb@cyberdelia> <211122.86a6hwyx1b.gmgdl@evledraar.gmail.com> <20211122164635.6zrqjqow4xa7idnn@cyberdelia> <211122.861r38yuun.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: <211122.861r38yuun.gmgdl@evledraar.gmail.com> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Mon, Nov 22, 2021 at 06:10:47PM +0100, Ævar Arnfjörð Bjarmason wrote: > I think a better approach is to just die early and not fallback if the > user's pager setting is broken. I.e. let's not second-guess their > explicit configuration, trust it, and if it doesn't work die() or > error(). > > We do fallback on emitting to stdout, so perhaps there's a reason to do > more exhaustive fallbacks here, I'm not really sure about the above. I suspect this fallback-to-stdout does not kick in very much in practice. It only kicks in when start_command() fails, which means either: - some OS-level error with fork, pipe(), etc - execve() failed to find the program (we detect this even over fork() with a magic protocol between the parent and child) The second case is the more interesting one for a fallback, but it only works if we aren't running the pager through a shell. And we do set use_shell, so it kicks in at all only because of our "skip the shell" optimization when the command looks simple. So this falls back: $ GIT_PAGER=does-not-exist git log -1 --pretty=format:foo error: cannot run does-not-exist: No such file or directory foo but this does not: $ GIT_PAGER='does-not-exist --arg' git log -1 --pretty=format:foo does-not-exist --arg: 1: does-not-exist: not found In the non-fallback case we just send all output (including stderr!) to a dead pipe (and probably die with SIGPIPE, though it's racy). So we may be better to just die() immediately when pager setup fails. That naturally fixes this bug, too. ;) Of course if we are going to do more exhaustive fallback, then it is going in the opposite direction. But I tend to agree that we should stick to what the user told us to do, and fail if it's not possible. -Peff