Re: [PATCH v2 1/5] wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> -	if (setenv(GIT_DIR_ENVIRONMENT, path, 1))
>> -		die(_("could not set GIT_DIR to '%s'"), path);
>> +	xsetenv(GIT_DIR_ENVIRONMENT, path, 1);
>> ...
>> +int xsetenv(const char *name, const char *value, int overwrite)
>> +{
>> +	if (!name)
>> +		die("xsetenv() got a NULL name, setenv() would return EINVAL");
>> +	if (setenv(name, value, overwrite))
>> +		die_errno("setenv(%s, '%s', %d) failed", name, value, overwrite);
>> +	return 0;
>> +}
>> +
>> +int xunsetenv(const char *name)
>> +{
>> +	if (!name)
>> +		die("xunsetenv() got a NULL name, xunsetenv() would return EINVAL");
>> +	if (!unsetenv(name))
>> +		die_errno("unsetenv(%s) failed", name);
>> +	return 0;
>> +}
>
> None of the existing callers have the "NULL name gets shown a
> special error".  If we would get EINVAL and die anyway, there is any
> need to add such an extra check that is always performed, no?
>
> As there seems no justification for it in the proposed log message,
> I'd have to say this is another "I'd do so while we are at it even
> though it has no reason to be there to support this topic" change.
>
> With explanation, perhaps these addtions would make sense.  If you
> wanted to protect the printf-like die_errno() from name==NULL, the
> cost of the check should be borne by the error codepath.
>
> IOW,
>
>     if (!unsetenv(name))
> 	die_errno(_("unsetenv(%s) failed"), name ? name : "<NULL given>");
>
> or something along that line, perhaps?  That won't need extra
> justification, as we are not adding a mysterious feature that gives
> a NULL name any special error status.