[PATCH] wrapper: remove xunsetenv()

[PATCH] wrapper: remove xunsetenv()

[Carlo Marcelo Arenas Belón]

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 <rsbecker@nexbridge.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 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

Re: [PATCH] wrapper: remove xunsetenv()

[Jeff King]

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

RE: [PATCH] wrapper: remove xunsetenv()

[rsbecker]

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:
> 
> > 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)?

At least on NonStop x86, it is 

       int unsetenv(const char *name);

--Randall

Re: [PATCH] wrapper: remove xunsetenv()

[Carlo Arenas]

On Fri, Oct 29, 2021 at 2:43 PM <rsbecker@nexbridge.com> wrote:
>
> 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:
> >
> > > 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.

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

Re: [PATCH] wrapper: remove xunsetenv()

[Junio C Hamano]

Carlo Arenas <carenas@gmail.com> writes:

> On Fri, Oct 29, 2021 at 2:43 PM <rsbecker@nexbridge.com> wrote:
>>
>> 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:
>> >
>> > > 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.
>
>> > 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 ;)

I do not plan to have *ANYTHING* I first see today in -rc0.  Not
even near 'next'.  No way.

Thanks.

Re: [PATCH] wrapper: remove xunsetenv()

[Jeff King]

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:
> 
> > 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)?

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

Re: [PATCH] wrapper: remove xunsetenv()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Ahh, OK.  Well, we will hear from them soon enough.  It is not like
this is anything urgent.

Thanks.

Re: [PATCH] wrapper: remove xunsetenv()

[Jeff King]

On Fri, Oct 29, 2021 at 02:58:48PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> Ahh, OK.  Well, we will hear from them soon enough.  It is not like
> this is anything urgent.

Yeah, I am OK proceeding along those lines, and seeing if anybody
screams (though perhaps dropping xunsetenv() for -rc0 makes sense in the
interim).

-Peff

Re: [PATCH] wrapper: remove xunsetenv()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Oct 29, 2021 at 02:58:48PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > 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.
>> 
>> Ahh, OK.  Well, we will hear from them soon enough.  It is not like
>> this is anything urgent.
>
> Yeah, I am OK proceeding along those lines, and seeing if anybody
> screams (though perhaps dropping xunsetenv() for -rc0 makes sense in the
> interim).

Ahh, ok, the use of unsetenv() that assumes a modern unsetenv() is a
regression during this cycle.

Let's queue this then.

Thanks.

-- >8 --
From: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Subject: [PATCH] wrapper: remove xunsetenv()

Remove the unused wrapper function.

Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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-1021-g744e6a0a52

Re: [PATCH] wrapper: remove xunsetenv()

[Jeff King]

On Fri, Oct 29, 2021 at 03:03:39PM -0700, Junio C Hamano wrote:

> > Yeah, I am OK proceeding along those lines, and seeing if anybody
> > screams (though perhaps dropping xunsetenv() for -rc0 makes sense in the
> > interim).
> 
> Ahh, ok, the use of unsetenv() that assumes a modern unsetenv() is a
> regression during this cycle.
> 
> Let's queue this then.

Yeah, exactly. Thanks.

-Peff

RE: [PATCH] wrapper: remove xunsetenv()

[rsbecker]

On October 29, 2021 5:59 PM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> Ahh, OK.  Well, we will hear from them soon enough.  It is not like this is
> anything urgent.

Well... maybe for some of us 😉

Re: [PATCH] wrapper: remove xunsetenv()

[Junio C Hamano]

<rsbecker@nexbridge.com> writes:

> On October 29, 2021 5:59 PM, Junio C Hamano wrote:
>> Jeff King <peff@peff.net> writes:
>> 
>> > 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.
>> 
>> Ahh, OK.  Well, we will hear from them soon enough.  It is not like this is
>> anything urgent.
>
> Well... maybe for some of us 😉

Heh, but we all know you are capable of locally patching ;-).

RE: [PATCH] wrapper: remove xunsetenv()

[rsbecker]

On October 29, 2021 6:29 PM, Junio C Hamano wrote:
> <rsbecker@nexbridge.com> writes:
> 
> > On October 29, 2021 5:59 PM, Junio C Hamano wrote:
> >> Jeff King <peff@peff.net> writes:
> >>
> >> > 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.
> >>
> >> Ahh, OK.  Well, we will hear from them soon enough.  It is not like
> >> this is anything urgent.
> >
> > Well... maybe for some of us 😉
> 
> Heh, but we all know you are capable of locally patching ;-).

Our CI/CD uses Jenkins and depends directly on the git repo, like normal people 😉

RE: [PATCH] wrapper: remove xunsetenv()

[rsbecker]

On October 29, 2021 5:27 PM, 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.
> 
> Remove the unused wrapper function.
> 
> Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  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

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

Re: [PATCH] wrapper: remove xunsetenv()

[Carlo Arenas]

On Fri, Oct 29, 2021 at 2:37 PM <rsbecker@nexbridge.com> 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

RE: [PATCH] wrapper: remove xunsetenv()

[rsbecker]

On October 29, 2021 5:43 PM, Carlo Arenas wrote:
> On Fri, Oct 29, 2021 at 2:37 PM <rsbecker@nexbridge.com> 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.

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

Re: [PATCH] wrapper: remove xunsetenv()

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

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

It sounds like barking up a wrong tree.  unsetenv() is supposed to
signal success with 0 and failure with -1, and the compat/
implementation is broken, not the caller that tries to be nice and
check the error return from the system function it calls.

Not that adding the unused wrapper, and leaving it unused, was a
wise decision in hindsight, though.