On Mon, Nov 22 2021, Enzo Matsumiya wrote: > 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@xxxxxxxxx/ > > 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...
