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

[Enzo Matsumiya <ematsumiya@suse.de>]

On 11/22, Ævar Arnfjörð Bjarmason wrote:
>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/

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

>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?


Cheers,

Enzo