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

From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Enzo Matsumiya" <ematsumiya@suse.de>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	git@vger.kernel.org
Subject: Re: [PATCH v2] pager: fix crash when pager program doesn't exist
Date: Mon, 22 Nov 2021 13:37:56 -0500	[thread overview]
Message-ID: <YZvjhF+KM/gzu7vN@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqq35nogijg.fsf@gitster.g>

On Mon, Nov 22, 2021 at 10:19:15AM -0800, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I know $GIT_PAGER trumps core.pager, which indicates between
> > equivalents, environment is taken as a stronger wish.  But I do not
> > mind if the order were updated to pager.<cmd> trumps $GIT_PAGER,
> > which in turn trumps core.pager, which in turn trumps $PAGER.
> 
> If such a precedence order makes it impossible to override a
> configured pager.<cmd> value at runtime, then it is a bad idea.  But
> luckily, we can do
> 
>     git -c "pager.<cmd>=<this one-shot pager>" cmd ...
> 
> to override a configured one, so perhaps it is OK.
> 
> I tend to agree with opinions I read elsewhere in this thread that
> it would be better not to do the fallback in the first place, but
> in this case, what I said I am OK with is when pager.<cmd> is
> defined, we do not even look at $GIT_PAGER or later choices, which
> is orthogonal.

FWIW, I also scratched my head while looking at this topic over seeing
$GIT_PAGER take precedence over a command-specific pager.

I do wonder if changing the override behavior would end up breaking some
scripted uses, though. E.g., imagine some program which does custom
markup of Git output, and ships with a wrapper script like:

  $ cat pretty-git
  #!/bin/sh
  GIT_PAGER='markup-git-output | less' "$@"

  $ pretty-git log -p

which would now behave differently if the user has pager.log set.

I dunno. That is perhaps a bit far-fetched. Most tools that I know of
like that (diff-highlight and diff-so-fancy) just tell you to set up
pager.log in the first place.

Just grepping around on GitHub, I don't see many uses, but this one[1] for
example might get confused:

  export GIT_PAGER=cat # disable pager when running interactively

So I dunno. It is probably unlikely to have much fallout. On the other
hand, I kind of wonder if the benefit is worth changing now.

-Peff

[0] https://github.com/node-inspector/node-inspector/blob/79e01c049286374f86dd560742a614019c02402f/tools/git-changelog.sh#L38