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.
