Re: [PATCH] pager: do not leak "GIT_PAGER_IN_USE" to the pager

From: Jeff King <peff@peff.net>
Date: Jul 3, 2015, 17:56 UTC
Message-ID: <20150703175627.GA9223@peff.net>
In-Reply-To: <xmqqk2uh5fd6.fsf@gitster.dls.corp.google.com>

On Fri, Jul 03, 2015 at 10:18:45AM -0700, Junio C Hamano wrote:

> Since 2e6c01e2, we export GIT_PAGER_IN_USE so that a process that

I imagine you mean 2e6c012e here.

Show 14 quoted lines

> becomes the upstream of the spawned pager can still tell that we
> have spawned the pager and decide to do colored output even when
> its output no longer goes to a terminal (i.e. isatty(1)).
> 
> But we forgot to clear it from the enviornment of the spawned pager.
> This is not a problem in a sane world, but if you have a handful of
> thousands Git users in your organization, somebody is bound to do
> strange things, e.g. typing "!<ENTER>" instead of 'q' to get control
> back from $LESS.  GIT_PAGER_IN_USE is still set in that subshell
> spawned by "less", and all sorts of interesting things starts
> happening, e.g. "git diff | cat" starts coloring its output.
> 
> We can clear the environment variable in the half of the fork that
> runs the pager to avoid the confusion.

I think this is a reasonable fix. I guess it would be possible that some pager would want to react differently depending on the variable, but I could not think of a useful case. And certainly your pager, being the pager itself, can assume that the pager is in use. ;) At the very worst, somebody can set GIT_PAGER="GIT_PAGER_IN_USE=1 my-pager" if they truly want to do something bizarre.

So,

Acked-by: Jeff King <peff@peff.net>

-Peff