[PATCH] wrapper: remove xunsetenv()

Platforms that are using the git compatibility layer for unsetenv
use void as a return value for unsetenv(), so any function that checks
for a return value will fail to build.

Remove the unused wrapper function.

Reported-by: Randall S. Becker <redacted>
Signed-off-by: Carlo Marcelo Arenas Belón <redacted>
---
 git-compat-util.h | 1 -
 wrapper.c         | 6 ------
 2 files changed, 7 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 141bb86351..d70ce14286 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h

@@ -879,7 +879,6 @@ char *xstrndup(const char *str, size_t len);
 void *xrealloc(void *ptr, size_t size);
 void *xcalloc(size_t nmemb, size_t size);
 void xsetenv(const char *name, const char *value, int overwrite);
-void xunsetenv(const char *name);
 void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
 const char *mmap_os_err(void);
 void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_t offset);

diff --git a/wrapper.c b/wrapper.c
index 1460d4e27b..36e12119d7 100644
--- a/wrapper.c
+++ b/wrapper.c

@@ -151,12 +151,6 @@ void xsetenv(const char *name, const char *value, int overwrite)
 		die_errno(_("could not setenv '%s'"), name ? name : "(null)");
 }
 
-void xunsetenv(const char *name)
-{
-	if (!unsetenv(name))
-		die_errno(_("could not unsetenv '%s'"), name ? name : "(null)");
-}
-
 /*
  * Limit size of IO chunks, because huge chunks only cause pain.  OS X
  * 64-bit is buggy, returning EINVAL if len >= INT_MAX; and even in

-- 
2.33.1.1200.g715dc68e71

On Fri, Oct 29, 2021 at 02:27:05PM -0700, Carlo Marcelo Arenas Belón wrote:

Platforms that are using the git compatibility layer for unsetenv
use void as a return value for unsetenv(), so any function that checks
for a return value will fail to build.

Good catch.

Remove the unused wrapper function.

I don't mind removing this if nobody is using it, but doesn't your first
paragraph argue that our definition of gitunsetenv() is just wrong?
I.e., it should return an int, even if it is always "0"?

Or is it a portability question? I.e., are there platforms where
unsetenv() also returns void, in which case we must make sure nobody
ever looks at its return value (and xunsetenv() is therefore a wrong
direction)?

-Peff

On October 29, 2021 5:27 PM, Carlo Marcelo Arenas Belón wrote:

I will be submitting a separate patch to turn off NO_SETENV and NO_UNSETENV for the NonStop x86 platform, where the calls have been supported since October 2020. The ia64 platform will have to continue to use the compat layer.

Thank you for solving this.

Randall

On Fri, Oct 29, 2021 at 2:37 PM [off-list ref] wrote:

I will be submitting a separate patch to turn off NO_SETENV and NO_UNSETENV for the NonStop x86 platform, where the calls have been supported since October 2020. The ia64 platform will have to continue to use the compat layer.

The right place to add that logic is most likely in config.mak.uname;
see all the other conditions that match based on version as a
guideline.

Carlo

On October 29, 2021 5:37 PM, Jeff King wrote:

On Fri, Oct 29, 2021 at 02:27:05PM -0700, Carlo Marcelo Arenas Belón wrote:

quoted

Platforms that are using the git compatibility layer for unsetenv use
void as a return value for unsetenv(), so any function that checks for
a return value will fail to build.

Good catch.

quoted

Remove the unused wrapper function.

I don't mind removing this if nobody is using it, but doesn't your first paragraph
argue that our definition of gitunsetenv() is just wrong?
I.e., it should return an int, even if it is always "0"?

Or is it a portability question? I.e., are there platforms where
unsetenv() also returns void, in which case we must make sure nobody ever
looks at its return value (and xunsetenv() is therefore a wrong direction)?

At least on NonStop x86, it is 

       int unsetenv(const char *name);

--Randall

On October 29, 2021 5:43 PM, Carlo Arenas wrote:

On Fri, Oct 29, 2021 at 2:37 PM [off-list ref] wrote:

quoted

I will be submitting a separate patch to turn off NO_SETENV and

NO_UNSETENV for the NonStop x86 platform, where the calls have been
supported since October 2020. The ia64 platform will have to continue to use
the compat layer.

The right place to add that logic is most likely in config.mak.uname; see all the
other conditions that match based on version as a guideline.

Already there. I just want to make sure everything is fine on the older box. This will be it, but I'm looking at whether I can get rid of any other switches at the same time:

diff --git a/config.mak.uname b/config.mak.uname
index 3236a491..fdcc4690 100644
--- a/config.mak.uname
+++ b/config.mak.uname

@@ -569,8 +569,11 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
        NO_STRCASESTR = YesPlease
        NO_MEMMEM = YesPlease
        NO_STRLCPY = YesPlease
-       NO_SETENV = YesPlease
-       NO_UNSETENV = YesPlease
+       ifeq ($(uname_R),J06)
+               # setenv and unsetenv are not supported on J-series
+               NO_SETENV = YesPlease
+               NO_UNSETENV = YesPlease
+       endif
        NO_MKDTEMP = YesPlease
        # Currently libiconv-1.9.1.
        OLD_ICONV = UnfortunatelyYes

On Fri, Oct 29, 2021 at 2:43 PM [off-list ref] wrote:

On October 29, 2021 5:37 PM, Jeff King wrote:

quoted

On Fri, Oct 29, 2021 at 02:27:05PM -0700, Carlo Marcelo Arenas Belón wrote:

quoted

Remove the unused wrapper function.

I don't mind removing this if nobody is using it, but doesn't your first paragraph
argue that our definition of gitunsetenv() is just wrong?
I.e., it should return an int, even if it is always "0"?

I couldn't figure the intent Jason had when this code was added in
2006, but considering how Junio suggested using void for the wrapper,
my guess is that we really wanted to make sure nobody will consider
errors for that function as actionable.

quoted

Or is it a portability question? I.e., are there platforms where
unsetenv() also returns void, in which case we must make sure nobody ever
looks at its return value (and xunsetenv() is therefore a wrong direction)?

At least on NonStop x86, it is

       int unsetenv(const char *name);

I don't think there is any platform that had anything but int, and so
I agree with you that it would be much better if the compatibility
layer returns 0, but as you pointed out, this was the safest approach
considering we are 1 day after rc0 ;)

Carlo

On Fri, Oct 29, 2021 at 05:37:16PM -0400, Jeff King wrote:

On Fri, Oct 29, 2021 at 02:27:05PM -0700, Carlo Marcelo Arenas Belón wrote:

quoted

Platforms that are using the git compatibility layer for unsetenv
use void as a return value for unsetenv(), so any function that checks
for a return value will fail to build.

Good catch.

quoted

Remove the unused wrapper function.

I don't mind removing this if nobody is using it, but doesn't your first
paragraph argue that our definition of gitunsetenv() is just wrong?
I.e., it should return an int, even if it is always "0"?

Or is it a portability question? I.e., are there platforms where
unsetenv() also returns void, in which case we must make sure nobody
ever looks at its return value (and xunsetenv() is therefore a wrong
direction)?

Looks like Junio just posted such a patch in the other thread.

However, according to the unsetenv() manpage:

  Prior to glibc 2.2.2, unsetenv() was prototyped as returning void;
  more recent glibc versions follow the POSIX.1-compliant prototype
  shown in the SYNOPSIS.

So it is POSIX to return an int, but that gives us at least one platform
where unsetenv() returns void (or used to). glibc 2.2.2 is 2001-era, so
that may be old enough that we don't care. But it makes me wonder if
other older or obscure platforms will run into this.

-Peff