Re: [PATCH 1/2] pager: move pager-specific setup into the build

From: Jeff King <peff@peff.net>
Date: Aug 1, 2016, 17:24 UTC
Message-ID: <20160801172459.wz7wigmytxpfvs22@sigill.intra.peff.net>
In-Reply-To: <20160801014303.x5j3dqumcmrkyc76@vauxhall.crustytoothpaste.net>

On Mon, Aug 01, 2016 at 01:43:03AM +0000, brian m. carlson wrote:

Show 32 quoted lines

> On Mon, Aug 01, 2016 at 01:05:56AM +0000, Eric Wong wrote:
> > +static void setup_pager_env(struct argv_array *env)
> > +{
> > +	const char *pager_env = stringify(PAGER_ENV);
> > +
> > +	while (*pager_env) {
> > +		struct strbuf buf = STRBUF_INIT;
> > +		const char *cp = strchrnul(pager_env, '=');
> > +
> > +		if (!*cp)
> > +			die("malformed build-time PAGER_ENV");
> > +		strbuf_add(&buf, pager_env, cp - pager_env);
> > +		cp = strchrnul(pager_env, ' ');
> > +		if (!getenv(buf.buf)) {
> > +			strbuf_reset(&buf);
> > +			strbuf_add(&buf, pager_env, cp - pager_env);
> > +			argv_array_push(env, strbuf_detach(&buf, NULL));
> > +		}
> > +		strbuf_reset(&buf);
> > +		while (*cp && *cp == ' ')
> > +			cp++;
> > +		pager_env = cp;
> > +	}
> 
> So it looks like this function splits on spaces but doesn't provide any
> escaping mechanism.  Is there any case in which we want to accept
> environment variables containing whitespace?  I ask this as someone that
> has EDITOR set to "gvim -f" on occasion and seeing how tools sometimes
> handle that poorly.
> 
> Even without that, I think this series is probably an improvement over
> the status quo.

I'm not too worried about spaces here. This is a resurrection of an old discussion, and in all that time, I think the only realistic suggestions for built-in values have been pretty tame.

If this were used to parse arbitrary user-provided runtime values, I'd be more concerned. But I'm not sure why we would need that. Your $EDITOR example is arbitrary shell code, and we let the shell handle it (modulo some efficiency shortcuts). Likewise, fancy runtime things should go in GIT_PAGER, where you can not only set options with spaces, but do fancy things like pipes, shell functions, etc.

The use of stringify() here is funny to me; I think there is a cpp tokenizing step in the middle that will do things like gobble up whitespace (but I'm not sure if it has other possible effects). I think our more usual method here would be to C-quote in the Makefile (with the equivalent of 's/\\/\\\\/g; s/"/\\"/g'), and then pass it to the compiler as a string literal, like -DPAGER_ENV=\"$(PAGER_ENV_CQ_SQ\".

-Peff