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

On Sun, Sep 19, 2021 at 10:47:15AM +0200, Ævar Arnfjörð Bjarmason wrote:

Add fatal wrappers for setenv() and unsetenv(). In d7ac12b25d3 (Add
set_git_dir() function, 2007-08-01) we started checking its return
value, and since 48988c4d0c3 (set_git_dir: die when setenv() fails,
2018-03-30) we've had set_git_dir_1() die if we couldn't set it.

Let's provide a wrapper for both, this will be useful in many other
places, a subsequent patch will make another use of xsetenv().

Makes sense.

We could make these return "void" (as far as I can tell there's no
other x*() wrappers that needed to make that decision before),
i.e. our "return 0" is only to indicate that we didn't error, which we
would have died on. Let's return "int" instead to be consistent with
the C library function signatures, including for any future code that
expects a pointer to a setenv()-like function.

This may be a little over-clever ;). It is cute, but returning an int
makes xsetenv a drop-in replacement for setenv. Which is nice, but it
makes it all too-easy to take code like:

  if (setenv(...) < 0)
    die(_("..."));

and replace it with

  if (xsetenv(...) < 0)

which makes the whole conditional redundant, since the wrappers are
guaranteed not to return an error.

In other words, I like the idea that s/setenv/x&/ causes a compile-time
error, and returning an int from these wrappers prevents that from
happening.

This may be a little too-theoretical, and you're certainly free to
disagree, just my $0.02.

I think it would be OK skip the NULL check of the "name" here for the
calls to die_errno(). Almost all of our setenv() callers are taking a
constant string hardcoded in the source as the first argument, and for
the rest we can probably assume they've done the NULL check
themselves. Even if they didn't, modern C libraries are forgiving
about it (e.g. glibc formatting it as "(null)"), on those that aren't,
well, we were about to die anyway. But let's include the check anyway
for good measure.

This I think is a good call. I agree in practice that most times we'd be
just fine to pass null to printf() (as we have seen from 88617d11f9
(multi-pack-index: fix potential segfault without sub-command,
2021-07-19) ;)). But there's no reason to rely on risky assumptions when
it's easy to avoid doing so.

For what it's worth, I find these new messages a little wordy. Maybe
we should just sticky "could not (un)set %s"?

Thanks,
Taylor