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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
