patchpager: die when paging to non-existing command

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

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.

At any rate, this test obviously needs to be adjusted to check the new behavior we are introducing. Do 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.

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.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 pager.c          |  2 +-
 t/t7006-pager.sh | 15 +++------------
 2 files changed, 4 insertions(+), 13 deletions(-)

Rubén Justo <rjusto@gmail.com> writes:

IOW, the behaviours between the case where pager is spawned via the shell and bypassing the shell are different , and the case where the shell is involved behaves in a way that is easier to realize the mistake, so change the other case to match. WHich makes sense.

This seems to be an ancient regression introduced in bfdd9ffd (Windows: Make the pager work., 2007-12-08), which did not really affect anybody but MinGW users, but ea27a18c (spawn pager via run_command interface, 2008-07-22) inherited the "if we failed to start the pager, just silently return" from it when non-MinGW code was unified to use the run_command() codepath (the latter is attributed to Peff, which I presume is the reason why you cc'ed him?).

If this error string is not used elsewhere, it probably is a good idea to "revert" to the original error message lost by ea27a18c, which was:

		die("unable to execute pager '%s'", pager);

But I do not think of a reason why we want to avoid dying here.

Just in case there is a reason why we should instead silently return on MinGW, I'll Cc the author of bfdd9ffd, though.

Will queue.  Thanks.

On Thu, Jun 20, 2024 at 12:04:03PM -0700, Junio C Hamano wrote:

Makes sense.  Let me know if you need me to reroll.

Yup. I did notice the MINGW conditions in t7006 but, to be honest, I hadn't thought about this. Thank you for considering it and seeking confirmation.

Rubén Justo <rjusto@gmail.com> writes:

After these questions are answered satisfactory, can you send an updated version to conclude the topic?

Thanks.

Am 20.06.24 um 21:04 schrieb Junio C Hamano:

I don't think there is a reason. IIRC, originally on Windows, failing to start a pager would still let Git operate normally, just without paged output. I might have regarded this as better than to fail the operation.

-- Hannes

Johannes Sixt <j6t@kdbg.org> writes:

The "better keep going than to fail" is what Rubén finds worse, so both sides are quite understandable.

It is unlikely that real-world users are taking advantage of the fact. If they do not want their invocation of Git command paged, "GIT_PAGER=cat git foo" is just as easy as "GIT_PAGER=no git foo", and if it was done by mistake to configure a non-working pager (e.g., configure core.pager to the program xyzzy and then uninstalling xyzzy without realizing you still have users), fixing it would be a one-time operation either way (you update core.pager or you reinstall xyzzy), so I would say that it is better to make the failure more stand out.

Thanks for a quick response.

On Thu, Jun 20, 2024 at 03:35:46PM -0700, Junio C Hamano wrote:

The compelling thing to me is that just about every other failure mode of the pager will result in a SIGPIPE, so the "be nice with a non-working pager" trick really only applies to the very narrow case of execve() failing.

I did assume that a bogus option like:

  # oops, there is no -l option!
  GIT_PAGER='less -l' git log

would be a plausible such misconfiguration, but to my surprise "less" prints "hey, there is no -l option" and then pages anyway. How helpful. :)

But something like:

  # oops, there is no -X option!
  GIT_PAGER='cat -X' git log

yields just:

  cat: invalid option -- 'X'
  Try 'cat --help' for more information.

with no other output. It's a little confusing if you don't realize that "cat" is the pager. We obviously don't want to complain about SIGPIPE, because it's common for the user to simply exit the pager without reading all of the possible data. It might be nice if we printed some message when the pager exits non-zero, but I'd worry there might be false positives, depending on the behavior of various pagers.

-Peff

On 2024-06-21 00:35, Junio C Hamano wrote:

To me, failing when the configured pager cannot be executed is the way to go. Basically, if an invalid pager is configured, we're actually obliged to produce a failure, simply because we have to follow and apply the configuration strictly. This also applies to (partially) invalid configurations.

Hi Hannes,

On Fri, 21 Jun 2024, Johannes Sixt wrote:

I recall regarding this a much better idea back then, too, because it was quite finicky to convince the MinGW variant of Git to play nice with the MSys variant of the pager.

In the meantime, things have become a lot more robust and consider it a net improvement to the change the behavior to _not_ silently continue if the pager failed to start.

Ciao, Johannes

On 20/06/2024 20:04, Junio C Hamano wrote:

Either way I think we want to mark the message for translation

Best Wishes

Phillip

Phillip Wood <phillip.wood123@gmail.com> writes:

Given that none of the die() message in this file is marked for localization, I would strongly prefer to see this patch not to do so. Possibly as part of a larger clean-up patch series, but not as "while at it" item for this fix.

Thanks.

On Thu, Jun 20, 2024 at 07:25:43PM +0200, Rubén Justo wrote:

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:

...shows what happens in those other cases, and you are making things more consistent. So that seems reasonable to me.

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.

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.

-Peff

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

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.

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

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.

At any rate, this test obviously needs to be adjusted to check the new behavior we are introducing. Do 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.

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.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

This iteration, v2, is just to "revert" to the original error message lost by ea27a18c.

For those not yet used to it, the range-diff is at the end of the message. ;)

Thanks!

 pager.c          |  3 ++-
 t/t7006-pager.sh | 17 +++++------------
 2 files changed, 7 insertions(+), 13 deletions(-)

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

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.

At any rate, this test obviously needs to be adjusted to check the new behavior we are introducing. Do 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.

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.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

This is a response to https://lore.kernel.org/git/xmqqed8pkhkn.fsf@gitster.g/

Range-diff against v2:
1:  95a2f36d18 ! 1:  60e852bffb pager: die when paging to non-existing command
    @@ Commit message
         Signed-off-by: Rubén Justo <rjusto@gmail.com>
     
      ## pager.c ##
    -@@
    - #include "git-compat-util.h"
    - #include "config.h"
    - #include "editor.h"
    -+#include "gettext.h"
    - #include "pager.h"
    - #include "run-command.h"
    - #include "sigchain.h"
     @@ pager.c: void setup_pager(void)
      	pager_process.in = -1;
      	strvec_push(&pager_process.env, "GIT_PAGER_IN_USE");
      	if (start_command(&pager_process))
     -		return;
    -+		die(_("unable to execute pager '%s'"), pager);
    ++		die("unable to execute pager '%s'", pager);
      
      	/* original process continues, but writes to the pipe */
      	dup2(pager_process.in, 1);

 pager.c          |  2 +-
 t/t7006-pager.sh | 17 +++++------------
 2 files changed, 6 insertions(+), 13 deletions(-)

Am 22.06.24 um 01:31 schrieb Rubén Justo:

Not a big deal, but the error message cited here does not match the actual new text:

-- Hannes

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 execute pager 'non-existent'
    0

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.

At any rate, this test obviously needs to be adjusted to check the new behavior we are introducing. Do 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.

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.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

Error pointed out by Hannes in:

https://lore.kernel.org/git/f031c152-1b97-4598-92f3-a72aefd701a4@kdbg.org

Thanks!

Range-diff against v2:
1:  60e852bffb ! 1:  93d6074a17 pager: die when paging to non-existing command
    @@ Commit message
     
             $ 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'
    +        fatal: unable to execute pager 'non-existent'
             0
     
         The behavior change we're introducing in this commit affects two tests

 pager.c          |  2 +-
 t/t7006-pager.sh | 17 +++++------------
 2 files changed, 6 insertions(+), 13 deletions(-)