From: Jeff King <peff@peff.net>
Date: Sat, 20 Nov 2021 03:38:14 GMT
Subject: Re: [PATCH] pager: fix crash when pager program doesn't exist
Message-ID: <YZhtplvcVkY0Yzmt@coredump.intra.peff.net>
In-Reply-To: <20211120030621.2xbcz5usaqkh2ald@cyberdelia>

On Sat, Nov 20, 2021 at 12:06:21AM -0300, Enzo Matsumiya wrote:

> On 11/19, Enzo Matsumiya wrote:
> > > When pager setup succeeds, the second run is a noop, because isatty(1)
> > > is no longer true. But for the case you're interested in, the first one
> > > fails, so we do try again. And I can reproduce your problem with:
> > 
> > No, it's not a noop such as that it's clear that things are different on
> > the second call.
> 
> Here I meant that setup_pager() is effectivelly called from 2 different places:
> 
> First backtrace:
> setup_pager()
> commit_pager_choice()
> run_builtin()
> handle_builtin()
> run_argv()
> cmd_main()
> 
> Second backtrace:
> setup_pager()
> cmd_log_init_finish()
> cmd_log_init()
> cmd_show()
> run_builtin()
> handle_builtin()
> run_argv()
> cmd_main()
> 
> Also, isatty(1) is not false in neither of the calls. Otherwise I
> wouldn't hit this bug (pager would be NULL and setup_pager() a noop as
> you said).

Right, I mean in the "normal" case that the pager actually starts, the
second call hits isatty(1), then git_pager() returns NULL, and we return
from setup_pager() immediately.

It is only in the broken-pager case that the bug you found is triggered
(which is probably why nobody has really noticed it).

-Peff