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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]