{"thread":{"id":"61658","subject":"[PATCH] pager: die when paging to non-existing command","startedAt":"2024-06-20T17:25:46Z","lastAt":"2024-06-24T07:36:12Z","messageCount":17,"participants":["Rubén Justo","Junio C Hamano","Johannes Sixt","Jeff King","Phillip Wood","Dragan Simic","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"497416","messageId":"f7106878-5ec5-4fe7-940b-2fb1d9707f7d@gmail.com","threadId":"61658","inReplyTo":null,"subject":"[PATCH] pager: die when paging to non-existing command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-06-20T17:25:43Z","receivedAt":"2024-06-20T17:25:46Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"When trying to execute a non-existent program from GIT_PAGER, we display\nan error.  However, we also send the complete text to the terminal\nand return a successful exit code.  This can be confusing for the user\nand the displayed error could easily become obscured by a lengthy\ntext.\n\nFor example, here the error message would be very far above after\nsending 50 MB of text:\n\n    $ GIT_PAGER=non-existent t/test-terminal.perl git log | wc -c\n    error: cannot run non-existent: No such file or directory\n    50314363\n\nLet's make the error clear by aborting the process and return an error\nso that the user can easily correct their mistake.\n\nThis will be the result of the change:\n\n    $ GIT_PAGER=non-existent t/test-terminal.perl git log | wc -c\n    error: cannot run non-existent: No such file or directory\n    fatal: unable to start the pager: 'non-existent'\n    0\n\nThe behavior change we're introducing in this commit affects two tests\nin t7006, which is a good sign regarding test coverage and requires us\nto address it.\n\nThe first test is 'git skips paging non-existing command'.  This test\ncomes from f7991f01f2 (t7006: clean up SIGPIPE handling in trace2 tests,\n2021-11-21,) where a modification was made to a test that was originally\nintroduced in c24b7f6736 (pager: test for exit code with and without\nSIGPIPE, 2021-02-02).  That original test was, IMHO, in the same\ndirection we're going in this commit.\n\nAt any rate, this test obviously needs to be adjusted to check the new\nbehavior we are introducing.  Do it.\n\nThe second test being affected is: 'non-existent pager doesnt cause\ncrash', introduced in f917f57f40 (pager: fix crash when pager program\ndoesn't exist, 2021-11-24).  As its name states, it has the intention of\nchecking that we don't introduce a regression that produces a crash when\nGIT_PAGER points to a nonexistent program.\n\nThis test could be considered redundant nowadays, due to us already\nhaving several tests checking implicitly what a non-existent command in\nGIT_PAGER produces.  However, let's maintain a good belt-and-suspenders\nstrategy; adapt it to the new world.\n\nFinally, it's worth noting that we are not changing the behavior if the\ncommand specified in GIT_PAGER is a shell command.  In such cases, it\nis:\n\n    $ GIT_PAGER=:\\;non-existent t/test-terminal.perl git log\n    :;non-existent: 1: non-existent: not found\n    died of signal 13 at t/test-terminal.perl line 33.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n pager.c          |  2 +-\n t/t7006-pager.sh | 15 +++------------\n 2 files changed, 4 insertions(+), 13 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex e9e121db69..e4291cd0aa 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -137,7 +137,7 @@ void setup_pager(void)\n \tpager_process.in = -1;\n \tstrvec_push(&pager_process.env, \"GIT_PAGER_IN_USE\");\n \tif (start_command(&pager_process))\n-\t\treturn;\n+\t\tdie(\"unable to start the pager: '%s'\", pager);\n \n \t/* original process continues, but writes to the pipe */\n \tdup2(pager_process.in, 1);\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex e56ca5b0fa..80ffed59d9 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -725,18 +725,9 @@ test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '\n \ttest_path_is_file pager-used\n '\n \n-test_expect_success TTY 'git skips paging nonexisting command' '\n-\ttest_when_finished \"rm trace.normal\" &&\n+test_expect_success TTY 'git errors when asked to execute nonexisting pager' '\n \ttest_config core.pager \"does-not-exist\" &&\n-\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n-\texport GIT_TRACE2 &&\n-\ttest_when_finished \"unset GIT_TRACE2\" &&\n-\n-\ttest_terminal git log &&\n-\n-\tgrep child_exit trace.normal >child-exits &&\n-\ttest_line_count = 1 child-exits &&\n-\tgrep \" code:-1 \" child-exits\n+\ttest_must_fail test_terminal git log\n '\n \n test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '\n@@ -762,7 +753,7 @@ test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '\n \n test_expect_success TTY 'non-existent pager doesnt cause crash' '\n \ttest_config pager.show invalid-pager &&\n-\ttest_terminal git show\n+\ttest_must_fail test_terminal git show\n '\n \n test_done\n-- \n2.45.2.562.g334133e685\n"},{"id":"497427","messageId":"xmqqsex7tp0c.fsf@gitster.g","threadId":"61658","inReplyTo":"f7106878-5ec5-4fe7-940b-2fb1d9707f7d@gmail.com","subject":"Re: [PATCH] pager: die when paging to non-existing command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-20T19:04:03Z","receivedAt":"2024-06-20T19:04:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> Finally, it's worth noting that we are not changing the behavior if the\n> command specified in GIT_PAGER is a shell command.  In such cases, it\n> is:\n>\n>     $ GIT_PAGER=:\\;non-existent t/test-terminal.perl git log\n>     :;non-existent: 1: non-existent: not found\n>     died of signal 13 at t/test-terminal.perl line 33.\n\nIOW, the behaviours between the case where pager is spawned via the\nshell and bypassing the shell are different , and the case where the\nshell is involved behaves in a way that is easier to realize the\nmistake, so change the other case to match.  WHich makes sense.\n\nThis seems to be an ancient regression introduced in bfdd9ffd\n(Windows: Make the pager work., 2007-12-08), which did not really\naffect anybody but MinGW users, but ea27a18c (spawn pager via\nrun_command interface, 2008-07-22) inherited the \"if we failed to\nstart the pager, just silently return\" from it when non-MinGW code\nwas unified to use the run_command() codepath (the latter is\nattributed to Peff, which I presume is the reason why you cc'ed\nhim?).\n\n> Signed-off-by: Rubén Justo <rjusto@gmail.com>\n> ---\n>  pager.c          |  2 +-\n>  t/t7006-pager.sh | 15 +++------------\n>  2 files changed, 4 insertions(+), 13 deletions(-)\n>\n> diff --git a/pager.c b/pager.c\n> index e9e121db69..e4291cd0aa 100644\n> --- a/pager.c\n> +++ b/pager.c\n> @@ -137,7 +137,7 @@ void setup_pager(void)\n>  \tpager_process.in = -1;\n>  \tstrvec_push(&pager_process.env, \"GIT_PAGER_IN_USE\");\n>  \tif (start_command(&pager_process))\n> -\t\treturn;\n> +\t\tdie(\"unable to start the pager: '%s'\", pager);\n\nIf this error string is not used elsewhere, it probably is a good\nidea to \"revert\" to the original error message lost by ea27a18c,\nwhich was:\n\n\t\tdie(\"unable to execute pager '%s'\", pager);\n\nBut I do not think of a reason why we want to avoid dying here.\n\nJust in case there is a reason why we should instead silently return\non MinGW, I'll Cc the author of bfdd9ffd, though.\n\nWill queue.  Thanks.\n"},{"id":"497431","messageId":"73b9a923-c3d6-46e8-b050-e8a93b9757a2@gmail.com","threadId":"61658","inReplyTo":"xmqqsex7tp0c.fsf@gitster.g","subject":"Re: [PATCH] pager: die when paging to non-existing command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-06-20T20:22:30Z","receivedAt":"2024-06-20T20:22:33Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Thu, Jun 20, 2024 at 12:04:03PM -0700, Junio C Hamano wrote:\n\n> > +\t\tdie(\"unable to start the pager: '%s'\", pager);\n> \n> If this error string is not used elsewhere, it probably is a good\n> idea to \"revert\" to the original error message lost by ea27a18c,\n> which was:\n> \n> \t\tdie(\"unable to execute pager '%s'\", pager);\n\nMakes sense.  Let me know if you need me to reroll.\n\n> Just in case there is a reason why we should instead silently return\n> on MinGW, I'll Cc the author of bfdd9ffd, though.\n\nYup.  I did notice the MINGW conditions in t7006 but, to be honest, I\nhadn't thought about this.  Thank you for considering it and seeking\nconfirmation.\n"},{"id":"497433","messageId":"xmqq34p7tjhd.fsf@gitster.g","threadId":"61658","inReplyTo":"73b9a923-c3d6-46e8-b050-e8a93b9757a2@gmail.com","subject":"Re: [PATCH] pager: die when paging to non-existing command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-20T21:03:26Z","receivedAt":"2024-06-20T21:03:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> On Thu, Jun 20, 2024 at 12:04:03PM -0700, Junio C Hamano wrote:\n>\n>> > +\t\tdie(\"unable to start the pager: '%s'\", pager);\n>> \n>> If this error string is not used elsewhere, it probably is a good\n>> idea to \"revert\" to the original error message lost by ea27a18c,\n>> which was:\n>> \n>> \t\tdie(\"unable to execute pager '%s'\", pager);\n>\n> Makes sense.  Let me know if you need me to reroll.\n>\n>> Just in case there is a reason why we should instead silently return\n>> on MinGW, I'll Cc the author of bfdd9ffd, though.\n>\n> Yup.  I did notice the MINGW conditions in t7006 but, to be honest, I\n> hadn't thought about this.  Thank you for considering it and seeking\n> confirmation.\n\nAfter these questions are answered satisfactory, can you send an\nupdated version to conclude the topic?\n\nThanks.\n"},{"id":"497436","messageId":"ba5965c2-9f1c-4dd2-a2c5-e1bde832766c@kdbg.org","threadId":"61658","inReplyTo":"xmqqsex7tp0c.fsf@gitster.g","subject":"Re: [PATCH] pager: die when paging to non-existing command","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2024-06-20T22:17:22Z","receivedAt":"2024-06-20T22:17:26Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 20.06.24 um 21:04 schrieb Junio C Hamano:\n> Just in case there is a reason why we should instead silently return\n> on MinGW, I'll Cc the author of bfdd9ffd, though.\n\nI don't think there is a reason. IIRC, originally on Windows, failing to\nstart a pager would still let Git operate normally, just without paged\noutput. I might have regarded this as better than to fail the operation.\n\n-- Hannes\n\n"},{"id":"497438","messageId":"xmqqplsbqm2l.fsf@gitster.g","threadId":"61658","inReplyTo":"ba5965c2-9f1c-4dd2-a2c5-e1bde832766c@kdbg.org","subject":"Re: [PATCH] pager: die when paging to non-existing command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-20T22:35:46Z","receivedAt":"2024-06-20T22:35:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> Am 20.06.24 um 21:04 schrieb Junio C Hamano:\n>> Just in case there is a reason why we should instead silently return\n>> on MinGW, I'll Cc the author of bfdd9ffd, though.\n>\n> I don't think there is a reason. IIRC, originally on Windows, failing to\n> start a pager would still let Git operate normally, just without paged\n> output. I might have regarded this as better than to fail the operation.\n\nThe \"better keep going than to fail\" is what Rubén finds worse, so\nboth sides are quite understandable.\n\nIt is unlikely that real-world users are taking advantage of the\nfact.  If they do not want their invocation of Git command paged,\n\"GIT_PAGER=cat git foo\" is just as easy as \"GIT_PAGER=no git foo\",\nand if it was done by mistake to configure a non-working pager\n(e.g., configure core.pager to the program xyzzy and then\nuninstalling xyzzy without realizing you still have users), fixing\nit would be a one-time operation either way (you update core.pager\nor you reinstall xyzzy), so I would say that it is better to make\nthe failure more stand out.\n\nThanks for a quick response.\n"},{"id":"497466","messageId":"20240621064020.GB2105230@coredump.intra.peff.net","threadId":"61658","inReplyTo":"f7106878-5ec5-4fe7-940b-2fb1d9707f7d@gmail.com","subject":"Re: [PATCH] pager: die when paging to non-existing command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-21T06:40:20Z","receivedAt":"2024-06-21T06:40:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 20, 2024 at 07:25:43PM +0200, Rubén Justo wrote:\n\n> When trying to execute a non-existent program from GIT_PAGER, we display\n> an error.  However, we also send the complete text to the terminal\n> and return a successful exit code.  This can be confusing for the user\n> and the displayed error could easily become obscured by a lengthy\n> text.\n> \n> For example, here the error message would be very far above after\n> sending 50 MB of text:\n> \n>     $ GIT_PAGER=non-existent t/test-terminal.perl git log | wc -c\n>     error: cannot run non-existent: No such file or directory\n>     50314363\n> \n> Let's make the error clear by aborting the process and return an error\n> so that the user can easily correct their mistake.\n> \n> This will be the result of the change:\n> \n>     $ GIT_PAGER=non-existent t/test-terminal.perl git log | wc -c\n>     error: cannot run non-existent: No such file or directory\n>     fatal: unable to start the pager: 'non-existent'\n>     0\n\nOK. My initial reaction was \"eh, who care? execve() failing is only one\nerror mode, and we might see all kinds of failure modes from a missing\nor broken pager\".\n\nBut this:\n\n> Finally, it's worth noting that we are not changing the behavior if the\n> command specified in GIT_PAGER is a shell command.  In such cases, it\n> is:\n> \n>     $ GIT_PAGER=:\\;non-existent t/test-terminal.perl git log\n>     :;non-existent: 1: non-existent: not found\n>     died of signal 13 at t/test-terminal.perl line 33.\n\n...shows what happens in those other cases, and you are making things\nmore consistent. So that seems reasonable to me.\n\n> The behavior change we're introducing in this commit affects two tests\n> in t7006, which is a good sign regarding test coverage and requires us\n> to address it.\n> \n> The first test is 'git skips paging non-existing command'.  This test\n> comes from f7991f01f2 (t7006: clean up SIGPIPE handling in trace2 tests,\n> 2021-11-21,) where a modification was made to a test that was originally\n> introduced in c24b7f6736 (pager: test for exit code with and without\n> SIGPIPE, 2021-02-02).  That original test was, IMHO, in the same\n> direction we're going in this commit.\n\nYeah, the point of f7991f01f2 was just to clean up the tests. The\nmodification was only documenting what Git happened to do for that case\nnow, and not meant as an endorsement of the behavior. ;) So I have no\nproblem changing it.\n\n> The second test being affected is: 'non-existent pager doesnt cause\n> crash', introduced in f917f57f40 (pager: fix crash when pager program\n> doesn't exist, 2021-11-24).  As its name states, it has the intention of\n> checking that we don't introduce a regression that produces a crash when\n> GIT_PAGER points to a nonexistent program.\n> \n> This test could be considered redundant nowadays, due to us already\n> having several tests checking implicitly what a non-existent command in\n> GIT_PAGER produces.  However, let's maintain a good belt-and-suspenders\n> strategy; adapt it to the new world.\n\nOK. I would also be happy to see it go. The crash was about reusing the\npager child_process struct, and no we know that cannot happen. Either we\nrun the pager or we immediately bail. I think that the code change in\nthat commit could also be reverted (to always re-init the child\nprocess), but it's probably more defensive to keep it.\n\n-Peff\n"},{"id":"497467","messageId":"20240621065127.GC2105230@coredump.intra.peff.net","threadId":"61658","inReplyTo":"xmqqplsbqm2l.fsf@gitster.g","subject":"Re: [PATCH] pager: die when paging to non-existing command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-21T06:51:27Z","receivedAt":"2024-06-21T06:51:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 20, 2024 at 03:35:46PM -0700, Junio C Hamano wrote:\n\n> Johannes Sixt <j6t@kdbg.org> writes:\n> \n> > Am 20.06.24 um 21:04 schrieb Junio C Hamano:\n> >> Just in case there is a reason why we should instead silently return\n> >> on MinGW, I'll Cc the author of bfdd9ffd, though.\n> >\n> > I don't think there is a reason. IIRC, originally on Windows, failing to\n> > start a pager would still let Git operate normally, just without paged\n> > output. I might have regarded this as better than to fail the operation.\n> \n> The \"better keep going than to fail\" is what Rubén finds worse, so\n> both sides are quite understandable.\n> \n> It is unlikely that real-world users are taking advantage of the\n> fact.  If they do not want their invocation of Git command paged,\n> \"GIT_PAGER=cat git foo\" is just as easy as \"GIT_PAGER=no git foo\",\n> and if it was done by mistake to configure a non-working pager\n> (e.g., configure core.pager to the program xyzzy and then\n> uninstalling xyzzy without realizing you still have users), fixing\n> it would be a one-time operation either way (you update core.pager\n> or you reinstall xyzzy), so I would say that it is better to make\n> the failure more stand out.\n\nThe compelling thing to me is that just about every other failure mode\nof the pager will result in a SIGPIPE, so the \"be nice with a\nnon-working pager\" trick really only applies to the very narrow case of\nexecve() failing.\n\nI did assume that a bogus option like:\n\n  # oops, there is no -l option!\n  GIT_PAGER='less -l' git log\n\nwould be a plausible such misconfiguration, but to my surprise \"less\"\nprints \"hey, there is no -l option\" and then pages anyway. How helpful. :)\n\nBut something like:\n\n  # oops, there is no -X option!\n  GIT_PAGER='cat -X' git log\n\nyields just:\n\n  cat: invalid option -- 'X'\n  Try 'cat --help' for more information.\n\nwith no other output. It's a little confusing if you don't realize that\n\"cat\" is the pager. We obviously don't want to complain about SIGPIPE,\nbecause it's common for the user to simply exit the pager without\nreading all of the possible data. It might be nice if we printed some\nmessage when the pager exits non-zero, but I'd worry there might be\nfalse positives, depending on the behavior of various pagers.\n\n-Peff\n"},{"id":"497478","messageId":"ab50c9e6-e43a-47fb-b64a-136d6a768f75@gmail.com","threadId":"61658","inReplyTo":"xmqqsex7tp0c.fsf@gitster.g","subject":"Re: [PATCH] pager: die when paging to non-existing command","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-21T11:28:10Z","receivedAt":"2024-06-21T11:28:09Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 20/06/2024 20:04, Junio C Hamano wrote:\n> Rubén Justo <rjusto@gmail.com> writes:\n>> --- a/pager.c\n>> +++ b/pager.c\n>> @@ -137,7 +137,7 @@ void setup_pager(void)\n>>   \tpager_process.in = -1;\n>>   \tstrvec_push(&pager_process.env, \"GIT_PAGER_IN_USE\");\n>>   \tif (start_command(&pager_process))\n>> -\t\treturn;\n>> +\t\tdie(\"unable to start the pager: '%s'\", pager);\n> \n> If this error string is not used elsewhere, it probably is a good\n> idea to \"revert\" to the original error message lost by ea27a18c,\n> which was:\n> \n> \t\tdie(\"unable to execute pager '%s'\", pager);\n\nEither way I think we want to mark the message for translation\n\nBest Wishes\n\nPhillip\n"},{"id":"497504","messageId":"c9112fd358340cd4adce3cb65b00c444@manjaro.org","threadId":"61658","inReplyTo":"xmqqplsbqm2l.fsf@gitster.g","subject":"Re: [PATCH] pager: die when paging to non-existing command","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-06-21T17:11:59Z","receivedAt":"2024-06-21T17:12:07Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-06-21 00:35, Junio C Hamano wrote:\n> Johannes Sixt <j6t@kdbg.org> writes:\n> \n>> Am 20.06.24 um 21:04 schrieb Junio C Hamano:\n>>> Just in case there is a reason why we should instead silently return\n>>> on MinGW, I'll Cc the author of bfdd9ffd, though.\n>> \n>> I don't think there is a reason. IIRC, originally on Windows, failing \n>> to\n>> start a pager would still let Git operate normally, just without paged\n>> output. I might have regarded this as better than to fail the \n>> operation.\n> \n> The \"better keep going than to fail\" is what Rubén finds worse, so\n> both sides are quite understandable.\n> \n> It is unlikely that real-world users are taking advantage of the\n> fact.  If they do not want their invocation of Git command paged,\n> \"GIT_PAGER=cat git foo\" is just as easy as \"GIT_PAGER=no git foo\",\n> and if it was done by mistake to configure a non-working pager\n> (e.g., configure core.pager to the program xyzzy and then\n> uninstalling xyzzy without realizing you still have users), fixing\n> it would be a one-time operation either way (you update core.pager\n> or you reinstall xyzzy), so I would say that it is better to make\n> the failure more stand out.\n\nTo me, failing when the configured pager cannot be executed is the\nway to go.  Basically, if an invalid pager is configured, we're\nactually obliged to produce a failure, simply because we have to\nfollow and apply the configuration strictly.  This also applies\nto (partially) invalid configurations.\n"},{"id":"497523","messageId":"7c749c2f-803d-4e97-b4f4-a97c681ed102@gmail.com","threadId":"61658","inReplyTo":"20240621064020.GB2105230@coredump.intra.peff.net","subject":"Re: [PATCH] pager: die when paging to non-existing command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-06-21T21:11:06Z","receivedAt":"2024-06-21T21:11:09Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Fri, Jun 21, 2024 at 02:40:20AM -0400, Jeff King wrote:\n\n> > When trying to execute a non-existent program from GIT_PAGER, we display\n> > an error.  However, we also send the complete text to the terminal\n> > and return a successful exit code.  This can be confusing for the user\n> > and the displayed error could easily become obscured by a lengthy\n> > text.\n> > \n> > For example, here the error message would be very far above after\n> > sending 50 MB of text:\n> > \n> >     $ GIT_PAGER=non-existent t/test-terminal.perl git log | wc -c\n> >     error: cannot run non-existent: No such file or directory\n> >     50314363\n> > \n> > Let's make the error clear by aborting the process and return an error\n> > so that the user can easily correct their mistake.\n> > \n> > This will be the result of the change:\n> > \n> >     $ GIT_PAGER=non-existent t/test-terminal.perl git log | wc -c\n> >     error: cannot run non-existent: No such file or directory\n> >     fatal: unable to start the pager: 'non-existent'\n> >     0\n> \n> OK. My initial reaction was \"eh, who care? execve() failing is only one\n> error mode, and we might see all kinds of failure modes from a missing\n> or broken pager\".\n> \n> But this:\n> \n> > Finally, it's worth noting that we are not changing the behavior if the\n> > command specified in GIT_PAGER is a shell command.  In such cases, it\n> > is:\n> > \n> >     $ GIT_PAGER=:\\;non-existent t/test-terminal.perl git log\n> >     :;non-existent: 1: non-existent: not found\n> >     died of signal 13 at t/test-terminal.perl line 33.\n> \n> ...shows what happens in those other cases, and you are making things\n> more consistent. So that seems reasonable to me.\n> \n> > The behavior change we're introducing in this commit affects two tests\n> > in t7006, which is a good sign regarding test coverage and requires us\n> > to address it.\n> > \n> > The first test is 'git skips paging non-existing command'.  This test\n> > comes from f7991f01f2 (t7006: clean up SIGPIPE handling in trace2 tests,\n> > 2021-11-21,) where a modification was made to a test that was originally\n> > introduced in c24b7f6736 (pager: test for exit code with and without\n> > SIGPIPE, 2021-02-02).  That original test was, IMHO, in the same\n> > direction we're going in this commit.\n> \n> Yeah, the point of f7991f01f2 was just to clean up the tests. The\n> modification was only documenting what Git happened to do for that case\n> now, and not meant as an endorsement of the behavior. ;) So I have no\n> problem changing it.\n> \n> > The second test being affected is: 'non-existent pager doesnt cause\n> > crash', introduced in f917f57f40 (pager: fix crash when pager program\n> > doesn't exist, 2021-11-24).  As its name states, it has the intention of\n> > checking that we don't introduce a regression that produces a crash when\n> > GIT_PAGER points to a nonexistent program.\n> > \n> > This test could be considered redundant nowadays, due to us already\n> > having several tests checking implicitly what a non-existent command in\n> > GIT_PAGER produces.  However, let's maintain a good belt-and-suspenders\n> > strategy; adapt it to the new world.\n> \n> OK. I would also be happy to see it go. The crash was about reusing the\n> pager child_process struct, and no we know that cannot happen. Either we\n> run the pager or we immediately bail. I think that the code change in\n> that commit could also be reverted (to always re-init the child\n> process), but it's probably more defensive to keep it.\n\nYeah.  The name is what took most of my attention, I have to admit.  A\ntest named like \"check that it doesn't crash\" is defensive. ;)\n\nLet's keep it.\n\nThanks for your review.\n"},{"id":"497524","messageId":"0df06a80-723f-4ad7-9f2e-74c8fb5b8283@gmail.com","threadId":"61658","inReplyTo":"f7106878-5ec5-4fe7-940b-2fb1d9707f7d@gmail.com","subject":"[PATCH v2] pager: die when paging to non-existing command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-06-21T21:29:17Z","receivedAt":"2024-06-21T21:29:19Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"When trying to execute a non-existent program from GIT_PAGER, we display\nan error.  However, we also send the complete text to the terminal\nand return a successful exit code.  This can be confusing for the user\nand the displayed error could easily become obscured by a lengthy\ntext.\n\nFor example, here the error message would be very far above after\nsending 50 MB of text:\n\n    $ GIT_PAGER=non-existent t/test-terminal.perl git log | wc -c\n    error: cannot run non-existent: No such file or directory\n    50314363\n\nLet's make the error clear by aborting the process and return an error\nso that the user can easily correct their mistake.\n\nThis will be the result of the change:\n\n    $ GIT_PAGER=non-existent t/test-terminal.perl git log | wc -c\n    error: cannot run non-existent: No such file or directory\n    fatal: unable to start the pager: 'non-existent'\n    0\n\nThe behavior change we're introducing in this commit affects two tests\nin t7006, which is a good sign regarding test coverage and requires us\nto address it.\n\nThe first test is 'git skips paging non-existing command'.  This test\ncomes from f7991f01f2 (t7006: clean up SIGPIPE handling in trace2 tests,\n2021-11-21,) where a modification was made to a test that was originally\nintroduced in c24b7f6736 (pager: test for exit code with and without\nSIGPIPE, 2021-02-02).  That original test was, IMHO, in the same\ndirection we're going in this commit.\n\nAt any rate, this test obviously needs to be adjusted to check the new\nbehavior we are introducing.  Do it.\n\nThe second test being affected is: 'non-existent pager doesnt cause\ncrash', introduced in f917f57f40 (pager: fix crash when pager program\ndoesn't exist, 2021-11-24).  As its name states, it has the intention of\nchecking that we don't introduce a regression that produces a crash when\nGIT_PAGER points to a nonexistent program.\n\nThis test could be considered redundant nowadays, due to us already\nhaving several tests checking implicitly what a non-existent command in\nGIT_PAGER produces.  However, let's maintain a good belt-and-suspenders\nstrategy; adapt it to the new world.\n\nFinally, it's worth noting that we are not changing the behavior if the\ncommand specified in GIT_PAGER is a shell command.  In such cases, it\nis:\n\n    $ GIT_PAGER=:\\;non-existent t/test-terminal.perl git log\n    :;non-existent: 1: non-existent: not found\n    died of signal 13 at t/test-terminal.perl line 33.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n\nThis iteration, v2, is just to \"revert\" to the original error message\nlost by ea27a18c. \n\nFor those not yet used to it, the range-diff is at the end of the\nmessage. ;)\n\nThanks!\n\n\n pager.c          |  3 ++-\n t/t7006-pager.sh | 17 +++++------------\n 2 files changed, 7 insertions(+), 13 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex e9e121db69..f5b6dc9b60 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -1,6 +1,7 @@\n #include \"git-compat-util.h\"\n #include \"config.h\"\n #include \"editor.h\"\n+#include \"gettext.h\"\n #include \"pager.h\"\n #include \"run-command.h\"\n #include \"sigchain.h\"\n@@ -137,7 +138,7 @@ void setup_pager(void)\n \tpager_process.in = -1;\n \tstrvec_push(&pager_process.env, \"GIT_PAGER_IN_USE\");\n \tif (start_command(&pager_process))\n-\t\treturn;\n+\t\tdie(_(\"unable to execute pager '%s'\"), pager);\n \n \t/* original process continues, but writes to the pipe */\n \tdup2(pager_process.in, 1);\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex e56ca5b0fa..932c26cb45 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -725,18 +725,11 @@ test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '\n \ttest_path_is_file pager-used\n '\n \n-test_expect_success TTY 'git skips paging nonexisting command' '\n-\ttest_when_finished \"rm trace.normal\" &&\n+test_expect_success TTY 'git errors when asked to execute nonexisting pager' '\n+\ttest_when_finished \"rm -f err\" &&\n \ttest_config core.pager \"does-not-exist\" &&\n-\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n-\texport GIT_TRACE2 &&\n-\ttest_when_finished \"unset GIT_TRACE2\" &&\n-\n-\ttest_terminal git log &&\n-\n-\tgrep child_exit trace.normal >child-exits &&\n-\ttest_line_count = 1 child-exits &&\n-\tgrep \" code:-1 \" child-exits\n+\ttest_must_fail test_terminal git log 2>err &&\n+\ttest_grep \"unable to execute pager\" err\n '\n \n test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '\n@@ -762,7 +755,7 @@ test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '\n \n test_expect_success TTY 'non-existent pager doesnt cause crash' '\n \ttest_config pager.show invalid-pager &&\n-\ttest_terminal git show\n+\ttest_must_fail test_terminal git show\n '\n \n test_done\n\nRange-diff against v1:\n1:  5c7997810c ! 1:  95a2f36d18 pager: die when paging to non-existing command\n    @@ Commit message\n         Signed-off-by: Rubén Justo <rjusto@gmail.com>\n     \n      ## pager.c ##\n    +@@\n    + #include \"git-compat-util.h\"\n    + #include \"config.h\"\n    + #include \"editor.h\"\n    ++#include \"gettext.h\"\n    + #include \"pager.h\"\n    + #include \"run-command.h\"\n    + #include \"sigchain.h\"\n     @@ pager.c: void setup_pager(void)\n      \tpager_process.in = -1;\n      \tstrvec_push(&pager_process.env, \"GIT_PAGER_IN_USE\");\n      \tif (start_command(&pager_process))\n     -\t\treturn;\n    -+\t\tdie(\"unable to start the pager: '%s'\", pager);\n    ++\t\tdie(_(\"unable to execute pager '%s'\"), pager);\n      \n      \t/* original process continues, but writes to the pipe */\n      \tdup2(pager_process.in, 1);\n    @@ t/t7006-pager.sh: test_expect_success TTY 'git discards pager non-zero exit with\n     -test_expect_success TTY 'git skips paging nonexisting command' '\n     -\ttest_when_finished \"rm trace.normal\" &&\n     +test_expect_success TTY 'git errors when asked to execute nonexisting pager' '\n    ++\ttest_when_finished \"rm -f err\" &&\n      \ttest_config core.pager \"does-not-exist\" &&\n     -\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n     -\texport GIT_TRACE2 &&\n    @@ t/t7006-pager.sh: test_expect_success TTY 'git discards pager non-zero exit with\n     -\tgrep child_exit trace.normal >child-exits &&\n     -\ttest_line_count = 1 child-exits &&\n     -\tgrep \" code:-1 \" child-exits\n    -+\ttest_must_fail test_terminal git log\n    ++\ttest_must_fail test_terminal git log 2>err &&\n    ++\ttest_grep \"unable to execute pager\" err\n      '\n      \n      test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '\n-- \n2.45.2.562.g737041e583\n"},{"id":"497532","messageId":"xmqqed8pkhkn.fsf@gitster.g","threadId":"61658","inReplyTo":"ab50c9e6-e43a-47fb-b64a-136d6a768f75@gmail.com","subject":"Re: [PATCH] pager: die when paging to non-existing command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-21T23:21:44Z","receivedAt":"2024-06-21T23:21:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> On 20/06/2024 20:04, Junio C Hamano wrote:\n>> Rubén Justo <rjusto@gmail.com> writes:\n>>> --- a/pager.c\n>>> +++ b/pager.c\n>>> @@ -137,7 +137,7 @@ void setup_pager(void)\n>>>   \tpager_process.in = -1;\n>>>   \tstrvec_push(&pager_process.env, \"GIT_PAGER_IN_USE\");\n>>>   \tif (start_command(&pager_process))\n>>> -\t\treturn;\n>>> +\t\tdie(\"unable to start the pager: '%s'\", pager);\n>> If this error string is not used elsewhere, it probably is a good\n>> idea to \"revert\" to the original error message lost by ea27a18c,\n>> which was:\n>> \t\tdie(\"unable to execute pager '%s'\", pager);\n>\n> Either way I think we want to mark the message for translation\n\nGiven that none of the die() message in this file is marked for\nlocalization, I would strongly prefer to see this patch not to do\nso.  Possibly as part of a larger clean-up patch series, but not as\n\"while at it\" item for this fix.\n\nThanks.\n"},{"id":"497533","messageId":"6850f558-ad20-403a-ae1e-5b9826c53790@gmail.com","threadId":"61658","inReplyTo":"0df06a80-723f-4ad7-9f2e-74c8fb5b8283@gmail.com","subject":"[PATCH v3] pager: die when paging to non-existing command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-06-21T23:31:05Z","receivedAt":"2024-06-21T23:31:08Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"When trying to execute a non-existent program from GIT_PAGER, we display\nan error.  However, we also send the complete text to the terminal\nand return a successful exit code.  This can be confusing for the user\nand the displayed error could easily become obscured by a lengthy\ntext.\n\nFor example, here the error message would be very far above after\nsending 50 MB of text:\n\n    $ GIT_PAGER=non-existent t/test-terminal.perl git log | wc -c\n    error: cannot run non-existent: No such file or directory\n    50314363\n\nLet's make the error clear by aborting the process and return an error\nso that the user can easily correct their mistake.\n\nThis will be the result of the change:\n\n    $ GIT_PAGER=non-existent t/test-terminal.perl git log | wc -c\n    error: cannot run non-existent: No such file or directory\n    fatal: unable to start the pager: 'non-existent'\n    0\n\nThe behavior change we're introducing in this commit affects two tests\nin t7006, which is a good sign regarding test coverage and requires us\nto address it.\n\nThe first test is 'git skips paging non-existing command'.  This test\ncomes from f7991f01f2 (t7006: clean up SIGPIPE handling in trace2 tests,\n2021-11-21,) where a modification was made to a test that was originally\nintroduced in c24b7f6736 (pager: test for exit code with and without\nSIGPIPE, 2021-02-02).  That original test was, IMHO, in the same\ndirection we're going in this commit.\n\nAt any rate, this test obviously needs to be adjusted to check the new\nbehavior we are introducing.  Do it.\n\nThe second test being affected is: 'non-existent pager doesnt cause\ncrash', introduced in f917f57f40 (pager: fix crash when pager program\ndoesn't exist, 2021-11-24).  As its name states, it has the intention of\nchecking that we don't introduce a regression that produces a crash when\nGIT_PAGER points to a nonexistent program.\n\nThis test could be considered redundant nowadays, due to us already\nhaving several tests checking implicitly what a non-existent command in\nGIT_PAGER produces.  However, let's maintain a good belt-and-suspenders\nstrategy; adapt it to the new world.\n\nFinally, it's worth noting that we are not changing the behavior if the\ncommand specified in GIT_PAGER is a shell command.  In such cases, it\nis:\n\n    $ GIT_PAGER=:\\;non-existent t/test-terminal.perl git log\n    :;non-existent: 1: non-existent: not found\n    died of signal 13 at t/test-terminal.perl line 33.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n\nThis is a response to\nhttps://lore.kernel.org/git/xmqqed8pkhkn.fsf@gitster.g/\n\nRange-diff against v2:\n1:  95a2f36d18 ! 1:  60e852bffb pager: die when paging to non-existing command\n    @@ Commit message\n         Signed-off-by: Rubén Justo <rjusto@gmail.com>\n     \n      ## pager.c ##\n    -@@\n    - #include \"git-compat-util.h\"\n    - #include \"config.h\"\n    - #include \"editor.h\"\n    -+#include \"gettext.h\"\n    - #include \"pager.h\"\n    - #include \"run-command.h\"\n    - #include \"sigchain.h\"\n     @@ pager.c: void setup_pager(void)\n      \tpager_process.in = -1;\n      \tstrvec_push(&pager_process.env, \"GIT_PAGER_IN_USE\");\n      \tif (start_command(&pager_process))\n     -\t\treturn;\n    -+\t\tdie(_(\"unable to execute pager '%s'\"), pager);\n    ++\t\tdie(\"unable to execute pager '%s'\", pager);\n      \n      \t/* original process continues, but writes to the pipe */\n      \tdup2(pager_process.in, 1);\n\n pager.c          |  2 +-\n t/t7006-pager.sh | 17 +++++------------\n 2 files changed, 6 insertions(+), 13 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex e9e121db69..be6f4ee59f 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -137,7 +137,7 @@ void setup_pager(void)\n \tpager_process.in = -1;\n \tstrvec_push(&pager_process.env, \"GIT_PAGER_IN_USE\");\n \tif (start_command(&pager_process))\n-\t\treturn;\n+\t\tdie(\"unable to execute pager '%s'\", pager);\n \n \t/* original process continues, but writes to the pipe */\n \tdup2(pager_process.in, 1);\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex e56ca5b0fa..932c26cb45 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -725,18 +725,11 @@ test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '\n \ttest_path_is_file pager-used\n '\n \n-test_expect_success TTY 'git skips paging nonexisting command' '\n-\ttest_when_finished \"rm trace.normal\" &&\n+test_expect_success TTY 'git errors when asked to execute nonexisting pager' '\n+\ttest_when_finished \"rm -f err\" &&\n \ttest_config core.pager \"does-not-exist\" &&\n-\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n-\texport GIT_TRACE2 &&\n-\ttest_when_finished \"unset GIT_TRACE2\" &&\n-\n-\ttest_terminal git log &&\n-\n-\tgrep child_exit trace.normal >child-exits &&\n-\ttest_line_count = 1 child-exits &&\n-\tgrep \" code:-1 \" child-exits\n+\ttest_must_fail test_terminal git log 2>err &&\n+\ttest_grep \"unable to execute pager\" err\n '\n \n test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '\n@@ -762,7 +755,7 @@ test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '\n \n test_expect_success TTY 'non-existent pager doesnt cause crash' '\n \ttest_config pager.show invalid-pager &&\n-\ttest_terminal git show\n+\ttest_must_fail test_terminal git show\n '\n \n test_done\n-- \n2.45.1\n"},{"id":"497537","messageId":"f031c152-1b97-4598-92f3-a72aefd701a4@kdbg.org","threadId":"61658","inReplyTo":"6850f558-ad20-403a-ae1e-5b9826c53790@gmail.com","subject":"Re: [PATCH v3] pager: die when paging to non-existing command","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2024-06-22T07:08:42Z","receivedAt":"2024-06-22T07:09:00Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 22.06.24 um 01:31 schrieb Rubén Justo:\n> Let's make the error clear by aborting the process and return an error\n> so that the user can easily correct their mistake.\n> \n> This will be the result of the change:\n> \n>     $ GIT_PAGER=non-existent t/test-terminal.perl git log | wc -c\n>     error: cannot run non-existent: No such file or directory\n>     fatal: unable to start the pager: 'non-existent'\n>     0\n\nNot a big deal, but the error message cited here does not match the\nactual new text:\n\n>  \tif (start_command(&pager_process))\n> -\t\treturn;\n> +\t\tdie(\"unable to execute pager '%s'\", pager);\n\n-- Hannes\n\n"},{"id":"497542","messageId":"392deded-9eb2-42fa-b6f9-54c22d3ffd33@gmail.com","threadId":"61658","inReplyTo":"6850f558-ad20-403a-ae1e-5b9826c53790@gmail.com","subject":"[PATCH v4] pager: die when paging to non-existing command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-06-23T07:09:02Z","receivedAt":"2024-06-23T07:09:21Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"When trying to execute a non-existent program from GIT_PAGER, we display\nan error.  However, we also send the complete text to the terminal\nand return a successful exit code.  This can be confusing for the user\nand the displayed error could easily become obscured by a lengthy\ntext.\n\nFor example, here the error message would be very far above after\nsending 50 MB of text:\n\n    $ GIT_PAGER=non-existent t/test-terminal.perl git log | wc -c\n    error: cannot run non-existent: No such file or directory\n    50314363\n\nLet's make the error clear by aborting the process and return an error\nso that the user can easily correct their mistake.\n\nThis will be the result of the change:\n\n    $ GIT_PAGER=non-existent t/test-terminal.perl git log | wc -c\n    error: cannot run non-existent: No such file or directory\n    fatal: unable to execute pager 'non-existent'\n    0\n\nThe behavior change we're introducing in this commit affects two tests\nin t7006, which is a good sign regarding test coverage and requires us\nto address it.\n\nThe first test is 'git skips paging non-existing command'.  This test\ncomes from f7991f01f2 (t7006: clean up SIGPIPE handling in trace2 tests,\n2021-11-21,) where a modification was made to a test that was originally\nintroduced in c24b7f6736 (pager: test for exit code with and without\nSIGPIPE, 2021-02-02).  That original test was, IMHO, in the same\ndirection we're going in this commit.\n\nAt any rate, this test obviously needs to be adjusted to check the new\nbehavior we are introducing.  Do it.\n\nThe second test being affected is: 'non-existent pager doesnt cause\ncrash', introduced in f917f57f40 (pager: fix crash when pager program\ndoesn't exist, 2021-11-24).  As its name states, it has the intention of\nchecking that we don't introduce a regression that produces a crash when\nGIT_PAGER points to a nonexistent program.\n\nThis test could be considered redundant nowadays, due to us already\nhaving several tests checking implicitly what a non-existent command in\nGIT_PAGER produces.  However, let's maintain a good belt-and-suspenders\nstrategy; adapt it to the new world.\n\nFinally, it's worth noting that we are not changing the behavior if the\ncommand specified in GIT_PAGER is a shell command.  In such cases, it\nis:\n\n    $ GIT_PAGER=:\\;non-existent t/test-terminal.perl git log\n    :;non-existent: 1: non-existent: not found\n    died of signal 13 at t/test-terminal.perl line 33.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n\nError pointed out by Hannes in:\n\nhttps://lore.kernel.org/git/f031c152-1b97-4598-92f3-a72aefd701a4@kdbg.org\n\nThanks!\n\nRange-diff against v2:\n1:  60e852bffb ! 1:  93d6074a17 pager: die when paging to non-existing command\n    @@ Commit message\n     \n             $ GIT_PAGER=non-existent t/test-terminal.perl git log | wc -c\n             error: cannot run non-existent: No such file or directory\n    -        fatal: unable to start the pager: 'non-existent'\n    +        fatal: unable to execute pager 'non-existent'\n             0\n     \n         The behavior change we're introducing in this commit affects two tests\n\n pager.c          |  2 +-\n t/t7006-pager.sh | 17 +++++------------\n 2 files changed, 6 insertions(+), 13 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex e9e121db69..be6f4ee59f 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -137,7 +137,7 @@ void setup_pager(void)\n \tpager_process.in = -1;\n \tstrvec_push(&pager_process.env, \"GIT_PAGER_IN_USE\");\n \tif (start_command(&pager_process))\n-\t\treturn;\n+\t\tdie(\"unable to execute pager '%s'\", pager);\n \n \t/* original process continues, but writes to the pipe */\n \tdup2(pager_process.in, 1);\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex e56ca5b0fa..932c26cb45 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -725,18 +725,11 @@ test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '\n \ttest_path_is_file pager-used\n '\n \n-test_expect_success TTY 'git skips paging nonexisting command' '\n-\ttest_when_finished \"rm trace.normal\" &&\n+test_expect_success TTY 'git errors when asked to execute nonexisting pager' '\n+\ttest_when_finished \"rm -f err\" &&\n \ttest_config core.pager \"does-not-exist\" &&\n-\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n-\texport GIT_TRACE2 &&\n-\ttest_when_finished \"unset GIT_TRACE2\" &&\n-\n-\ttest_terminal git log &&\n-\n-\tgrep child_exit trace.normal >child-exits &&\n-\ttest_line_count = 1 child-exits &&\n-\tgrep \" code:-1 \" child-exits\n+\ttest_must_fail test_terminal git log 2>err &&\n+\ttest_grep \"unable to execute pager\" err\n '\n \n test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '\n@@ -762,7 +755,7 @@ test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '\n \n test_expect_success TTY 'non-existent pager doesnt cause crash' '\n \ttest_config pager.show invalid-pager &&\n-\ttest_terminal git show\n+\ttest_must_fail test_terminal git show\n '\n \n test_done\n-- \n2.45.1\n"},{"id":"497557","messageId":"ecf29882-c192-e6f5-64f9-ac4cedb5d85d@gmx.de","threadId":"61658","inReplyTo":"ba5965c2-9f1c-4dd2-a2c5-e1bde832766c@kdbg.org","subject":"Re: [PATCH] pager: die when paging to non-existing command","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-06-24T07:35:58Z","receivedAt":"2024-06-24T07:36:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Fri, 21 Jun 2024, Johannes Sixt wrote:\n\n> Am 20.06.24 um 21:04 schrieb Junio C Hamano:\n> > Just in case there is a reason why we should instead silently return\n> > on MinGW, I'll Cc the author of bfdd9ffd, though.\n>\n> I don't think there is a reason. IIRC, originally on Windows, failing to\n> start a pager would still let Git operate normally, just without paged\n> output. I might have regarded this as better than to fail the operation.\n\nI recall regarding this a much better idea back then, too, because it\nwas quite finicky to convince the MinGW variant of Git to play nice with\nthe MSys variant of the pager.\n\nIn the meantime, things have become a lot more robust and consider it a\nnet improvement to the change the behavior to _not_ silently continue if\nthe pager failed to start.\n\nCiao,\nJohannes\n"}]}