Re: [PATCH v2] pager: fix crash when pager program doesn't exist

On 11/22, Ævar Arnfjörð Bjarmason wrote:

quoted

I think an alternate direction of simply getting rid of "argv" is better
in this case, and I've just submitted a topic to do that:
https://lore.kernel.org/git/cover-0.5-00000000000-20211122T153605Z-avarab@gmail.com/ (local)

Well, this is my first interaction with git's source, so I can't say for
sure, but that solution seems more complete indeed.

quoted

It still leave us with this oddity:

   $ ~/g/git/git -c pager.show=INVALID_PAGER show  HEAD
   error: cannot run INVALID_PAGER: No such file or directory
   error: cannot run INVALID_PAGER: No such file or directory

But that was the case before that topic (if we hadn't
crashed/segfaulted), and with your proposed change here.

Yes, I did find it weird on my initial debugging, but didn't care at
first.

Now, looking again, it's because git (main command) have a higher
precedence on pager preference, so it tries to setup/run the pager
before running subcommands.

An issue I've hit now is, if we don't want to fallback to any other
setting, this works:

diff --git a/pager.c b/pager.c
index d93304527d62..b528bbd644b5 100644
--- a/pager.c
+++ b/pager.c

@@ -110,6 +110,14 @@ void setup_pager(void)
        if (!pager)
                return;

+       /*
+        * There's already a pager set up and running.
+        * Regardless if it was successful or not, we shouldn't try running
+        * it again.
+        */
+       if (pager_in_use())
+               return;
+
        /*
         * After we redirect standard output, we won't be able to use an ioctl
         * to get the terminal size. Let's grab it now, and then set $COLUMNS

However, IMHO it would make sense to try pager.<subcommand> if a
previous attempt failed, e.g.:

$ git config pager.show my-valid-pager
$ GIT_PAGER=invalid-pager git -p show

So this will first try invalid-pager, fail, and not try again, with the
above patch. As a user, I would expect that my-valid-pager to be run in
case invalid-pager failed.

What do you think?