Re: [PATCH] pager: die when paging to non-existing command

From: Jeff King <peff@peff.net>
Date: Jun 21, 2024, 06:51 UTC
Message-ID: <20240621065127.GC2105230@coredump.intra.peff.net>
In-Reply-To: <xmqqplsbqm2l.fsf@gitster.g>

On Thu, Jun 20, 2024 at 03:35:46PM -0700, Junio C Hamano wrote:

Show 22 quoted lines

> Johannes Sixt <j6t@kdbg.org> writes:
> 
> > Am 20.06.24 um 21:04 schrieb Junio C Hamano:
> >> Just in case there is a reason why we should instead silently return
> >> on MinGW, I'll Cc the author of bfdd9ffd, though.
> >
> > I don't think there is a reason. IIRC, originally on Windows, failing to
> > start a pager would still let Git operate normally, just without paged
> > output. I might have regarded this as better than to fail the operation.
> 
> The "better keep going than to fail" is what Rubén finds worse, so
> both sides are quite understandable.
> 
> It is unlikely that real-world users are taking advantage of the
> fact.  If they do not want their invocation of Git command paged,
> "GIT_PAGER=cat git foo" is just as easy as "GIT_PAGER=no git foo",
> and if it was done by mistake to configure a non-working pager
> (e.g., configure core.pager to the program xyzzy and then
> uninstalling xyzzy without realizing you still have users), fixing
> it would be a one-time operation either way (you update core.pager
> or you reinstall xyzzy), so I would say that it is better to make
> the failure more stand out.

The compelling thing to me is that just about every other failure mode of the pager will result in a SIGPIPE, so the "be nice with a non-working pager" trick really only applies to the very narrow case of execve() failing.

I did assume that a bogus option like:

  # oops, there is no -l option!
  GIT_PAGER='less -l' git log

would be a plausible such misconfiguration, but to my surprise "less" prints "hey, there is no -l option" and then pages anyway. How helpful. :)

But something like:

  # oops, there is no -X option!
  GIT_PAGER='cat -X' git log

yields just:

  cat: invalid option -- 'X'
  Try 'cat --help' for more information.

with no other output. It's a little confusing if you don't realize that "cat" is the pager. We obviously don't want to complain about SIGPIPE, because it's common for the user to simply exit the pager without reading all of the possible data. It might be nice if we printed some message when the pager exits non-zero, but I'd worry there might be false positives, depending on the behavior of various pagers.

-Peff