Re: [PATCH] Add ability to specify environment extension to run_command

From: Alex Riesen <raa.lkml@gmail.com>
Date: May 22, 2007, 23:14 UTC
Message-ID: <20070522231442.GM30871@steel.home>
In-Reply-To: <7v1wh88prw.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano, Wed, May 23, 2007 00:19:47 +0200:

Show 15 quoted lines

> > Others already discussed the issue. Just to be sure, I reimplemented
> > that comfortable putenv with unsetenv: if an environment entry ends
> > with a "=" it will be unset.
> 
> Although combination of putenv and unsetenv gives a somewhat
> queasy feeling for obvious reasons, I'll let it pass.  As we
> are coming up with an interface that uses only one string per
> environment element, that is probably a sensible thing to do,
> rather than trying to do the "historically correct" pairing of
> setenv/unsetenv.
> 
> However, I do not think "VAR=" to unset it is a good interface.
> Having an environment variable whose value happens to be an
> empty string and not having the variable at all are two
> different things.

Right

Show 7 quoted lines

> Because you _scan_ the whole string in your patch to see if it
> ends with = anyway, a trivial improvement would be to do:
> 
> 	if (strchr(cmd->env, '='))
>                 putenv(cmd->env);
> 	else
>         	unsetenv(cmd->env);

I like this one. The env field in struct child_process and run_command will have to mention it in comments (in run-command.h), it's kind of special.

> If you do not mind such a special syntax (e.g. "VAR="), I would
> suggest doing that as a prefix (e.g. "!VAR") and do:

Nah, !VAR is a _working_ environment variable name.

    int main(int argc, char *argv[], char *envp[])
    {
	    const char *argv1[] = {"/usr/bin/perl", "-e", "print $ENV{'!VAR'}", NULL};
	    const char *envp1[] = {"!VAR=value", NULL};
	    execve(*argv, (char**)argv1, (char**)envp1);
	    return 0;
    }

    $ gcc ... && ./a.out
    value

Someone could want it. We surely could use "=", though :)