Re: [PATCH v2] pager: fix crash when pager program doesn't exist

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