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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Sep 17 2021, Junio C Hamano wrote:

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

Sure, I didn't think much about it when writing it.

I'd think skipping the translation would be fine here, but sure, will
include it. On second thought just a:

    die_errno(_("unsetenv(%s) failed"), name);

Should be fine. I.e. this is an internal-only function, we're
exceedingly unlikely to end up with a xsetenv(NULL, ...).

Even if we did I'd think the undefined behavior is OK here. In practice
modern C libraries are forgiving about it (e.g. glibc formatting it as
"(null)"), and if not we were about to die anyway...

But unless you explicitly Ack that undefined behavior bit I'll use your
version in a re-roll. Thanks.