threads / patch / 56808

patchwrapper: remove xunsetenv()

Subject: [PATCH] wrapper: remove xunsetenv()

## tl;dr

17 messages between Oct 29, 2021 and Oct 30, 2021. Diffs are folded; open one to read it.

replies: 16people: 4as markdown or json

Carlo Marcelo Arenas Belón· Oct 29, 2021, 21:27 UTC · lore

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(-)
Show changes to 2 files +0 −7

git-compat-util.h, wrapper.c

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
Jeff King· Oct 29, 2021, 21:37 UTC · re: Carlo Marcelo Arenas Belón · lore

Re: [PATCH] wrapper: remove xunsetenv()

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
rsbecker@nexbridge.com· Oct 29, 2021, 21:43 UTC · re: Jeff King · lore

RE: [PATCH] wrapper: remove xunsetenv()

On October 29, 2021 5:37 PM, Jeff King wrote:
Show 17 quoted lines
> 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
Carlo Arenas· Oct 29, 2021, 21:50 UTC · re: rsbecker@nexbridge.com · lore

Re: [PATCH] wrapper: remove xunsetenv()

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

Show 7 quoted lines
> > 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
Junio C Hamano· Oct 29, 2021, 21:56 UTC · re: Carlo Arenas · lore

Re: [PATCH] wrapper: remove xunsetenv()

Carlo Arenas <carenas@gmail.com> writes:
Show 28 quoted lines
> 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.
Jeff King· Oct 29, 2021, 21:51 UTC · re: Jeff King · lore

Re: [PATCH] wrapper: remove xunsetenv()

On Fri, Oct 29, 2021 at 05:37:16PM -0400, Jeff King wrote:
Show 18 quoted lines
> 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
Junio C Hamano· Oct 29, 2021, 21:58 UTC · re: Jeff King · lore

Re: [PATCH] wrapper: remove xunsetenv()

Jeff King <peff@peff.net> writes:
Show 10 quoted lines
> 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.
Jeff King· Oct 29, 2021, 22:00 UTC · re: Junio C Hamano · lore

Re: [PATCH] wrapper: remove xunsetenv()

On Fri, Oct 29, 2021 at 02:58:48PM -0700, Junio C Hamano wrote:
Show 15 quoted lines
> 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
Junio C Hamano· Oct 29, 2021, 22:03 UTC · re: Jeff King · lore

Re: [PATCH] wrapper: remove xunsetenv()

Jeff King <peff@peff.net> writes:
Show 21 quoted lines
> 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(-)
Show changes to 2 files +0 −7

git-compat-util.h, wrapper.c

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
Jeff King· Oct 29, 2021, 23:11 UTC · re: Junio C Hamano · lore

Re: [PATCH] wrapper: remove xunsetenv()

On Fri, Oct 29, 2021 at 03:03:39PM -0700, Junio C Hamano wrote:
Show 8 quoted lines
> > 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
rsbecker@nexbridge.com· Oct 29, 2021, 22:01 UTC · re: Junio C Hamano · lore

RE: [PATCH] wrapper: remove xunsetenv()

On October 29, 2021 5:59 PM, Junio C Hamano wrote:
Show 15 quoted lines
> 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 😉
Junio C Hamano· Oct 29, 2021, 22:29 UTC · re: rsbecker@nexbridge.com · lore

Re: [PATCH] wrapper: remove xunsetenv()

<rsbecker@nexbridge.com> writes:
Show 18 quoted lines
> 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 ;-).
rsbecker@nexbridge.com· Oct 30, 2021, 00:13 UTC · re: Junio C Hamano · lore

RE: [PATCH] wrapper: remove xunsetenv()

On October 29, 2021 6:29 PM, Junio C Hamano wrote:
Show 22 quoted lines
> <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 😉
rsbecker@nexbridge.com· Oct 29, 2021, 21:37 UTC · re: Carlo Marcelo Arenas Belón · lore

RE: [PATCH] wrapper: remove xunsetenv()

On October 29, 2021 5:27 PM, Carlo Marcelo Arenas Belón wrote:
Show 41 quoted lines
> 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
Carlo Arenas· Oct 29, 2021, 21:43 UTC · re: rsbecker@nexbridge.com · lore

Re: [PATCH] wrapper: remove xunsetenv()

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
rsbecker@nexbridge.com· Oct 29, 2021, 21:48 UTC · re: Carlo Arenas · lore

RE: [PATCH] wrapper: remove xunsetenv()

On October 29, 2021 5:43 PM, Carlo Arenas wrote:
Show 9 quoted lines
> 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:
Show changes to config.mak.uname +5 −2
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
Junio C Hamano· Oct 29, 2021, 21:53 UTC · re: Carlo Marcelo Arenas Belón · lore

Re: [PATCH] wrapper: remove xunsetenv()

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.

← back to recent threads