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

From: Junio C Hamano <junkio@cox.net>
Date: May 22, 2007, 22:19 UTC
Message-ID: <7v1wh88prw.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20070522214754.GD30871@steel.home>

Alex Riesen <raa.lkml@gmail.com> writes:

> 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.

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);

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:

	if (cmd->env[0] != '!')
        	putenv(cmd->env);
	else
		unsetenv(cmd->env + 1);

The former look cleaner but less efficient; we are going to exec so I do not think micro-optimization would matter at all, so my suggestion would be to do the strchr().