| Message ID | 20211029212705.31721-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6fc527a8d05141335799f27bfbaab28c8625c31b |
| Headers | show |
| Series | wrapper: remove xunsetenv() | expand |
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: > 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
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
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
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
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
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
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.
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.
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.
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
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
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
<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
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
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
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
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(-)