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
