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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 22, 2021, 17:10 UTC
Message-ID: <211122.861r38yuun.gmgdl@evledraar.gmail.com>
In-Reply-To: <20211122164635.6zrqjqow4xa7idnn@cyberdelia>

On Mon, Nov 22 2021, Enzo Matsumiya wrote:

Show 58 quoted lines

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

I think a better approach is to just die early and not fallback if the user's pager setting is broken. I.e. let's not second-guess their explicit configuration, trust it, and if it doesn't work die() or error().

We do fallback on emitting to stdout, so perhaps there's a reason to do more exhaustive fallbacks here, I'm not really sure about the above.

The use of pager_in_use() in the above patch smells iffy though, we're able to do that because we did the setenv() of GIT_PAGER_IN_USE=true.

We then use that to check if we set it up ourselves and skip setting it up, but any other program invoked by us will be fooled by GIT_PAGER_IN_USE=true. Maybe that's intentional, but won't we then expect a working pager in some cases (maybe not)...

It seems non-obvious at best, perhaps we should push a list of failed pagers, or just the one failed one if we're not doing fallbacks..

Presumably we can invoke N git <something>, where those <something> have different pager.<something> config...