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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 17, 2021, 16:57 UTC
Message-ID: <xmqq35q3i1hi.fsf@gitster.g>
In-Reply-To: <patch-v2-1.5-49706b26642-20210916T182918Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 21 quoted lines

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