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

Ævar Arnfjörð Bjarmason  [off-list ref] 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.