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

["Rubén Justo" <rjusto@gmail.com>]

On Fri, Jun 21, 2024 at 02:40:20AM -0400, Jeff King wrote:

> > When trying to execute a non-existent program from GIT_PAGER, we display
> > an error.  However, we also send the complete text to the terminal
> > and return a successful exit code.  This can be confusing for the user
> > and the displayed error could easily become obscured by a lengthy
> > text.
> > 
> > For example, here the error message would be very far above after
> > sending 50 MB of text:
> > 
> >     $ GIT_PAGER=non-existent t/test-terminal.perl git log | wc -c
> >     error: cannot run non-existent: No such file or directory
> >     50314363
> > 
> > Let's make the error clear by aborting the process and return an error
> > so that the user can easily correct their mistake.
> > 
> > This will be the result of the change:
> > 
> >     $ GIT_PAGER=non-existent t/test-terminal.perl git log | wc -c
> >     error: cannot run non-existent: No such file or directory
> >     fatal: unable to start the pager: 'non-existent'
> >     0
> 
> OK. My initial reaction was "eh, who care? execve() failing is only one
> error mode, and we might see all kinds of failure modes from a missing
> or broken pager".
> 
> But this:
> 
> > Finally, it's worth noting that we are not changing the behavior if the
> > command specified in GIT_PAGER is a shell command.  In such cases, it
> > is:
> > 
> >     $ GIT_PAGER=:\;non-existent t/test-terminal.perl git log
> >     :;non-existent: 1: non-existent: not found
> >     died of signal 13 at t/test-terminal.perl line 33.
> 
> ...shows what happens in those other cases, and you are making things
> more consistent. So that seems reasonable to me.
> 
> > The behavior change we're introducing in this commit affects two tests
> > in t7006, which is a good sign regarding test coverage and requires us
> > to address it.
> > 
> > The first test is 'git skips paging non-existing command'.  This test
> > comes from f7991f01f2 (t7006: clean up SIGPIPE handling in trace2 tests,
> > 2021-11-21,) where a modification was made to a test that was originally
> > introduced in c24b7f6736 (pager: test for exit code with and without
> > SIGPIPE, 2021-02-02).  That original test was, IMHO, in the same
> > direction we're going in this commit.
> 
> Yeah, the point of f7991f01f2 was just to clean up the tests. The
> modification was only documenting what Git happened to do for that case
> now, and not meant as an endorsement of the behavior. ;) So I have no
> problem changing it.
> 
> > The second test being affected is: 'non-existent pager doesnt cause
> > crash', introduced in f917f57f40 (pager: fix crash when pager program
> > doesn't exist, 2021-11-24).  As its name states, it has the intention of
> > checking that we don't introduce a regression that produces a crash when
> > GIT_PAGER points to a nonexistent program.
> > 
> > This test could be considered redundant nowadays, due to us already
> > having several tests checking implicitly what a non-existent command in
> > GIT_PAGER produces.  However, let's maintain a good belt-and-suspenders
> > strategy; adapt it to the new world.
> 
> OK. I would also be happy to see it go. The crash was about reusing the
> pager child_process struct, and no we know that cannot happen. Either we
> run the pager or we immediately bail. I think that the code change in
> that commit could also be reverted (to always re-init the child
> process), but it's probably more defensive to keep it.

Yeah.  The name is what took most of my attention, I have to admit.  A
test named like "check that it doesn't crash" is defensive. ;)

Let's keep it.

Thanks for your review.