* [Bug] wrapper.c uses unportable unsetenv @ 2021-10-29 20:14 rsbecker 2021-10-29 20:35 ` Junio C Hamano 0 siblings, 1 reply; 7+ messages in thread From: rsbecker @ 2021-10-29 20:14 UTC (permalink / raw) To: git The unsetenv()/setenv(overwrite) calls are not 100% portable - as in not on all POSIX implementations. It breaks the build on some of the NonStop platforms. This will change in a year or two but I really don't want to fall behind on git releases. This was introduced at 3540c71 but I was on vacation when it happened so did not catch it during reviews - my apologies for that. Is it critical that this be called or can we #ifdef it away if it isn't supported for a build? The #if is exactly this: wrapper.c@150 + #if (_TANDEM_ARCH_ > 3 || (_TANDEM_ARCH_ == 3 && __L_Series_RVU >= 2010)) if (setenv(name, value, overwrite)) die_errno(_("could not setenv '%s'"), name ? name : "(null)"); + #endif wrapper.c@154 + #if (_TANDEM_ARCH_ > 3 || (_TANDEM_ARCH_ == 3 && __L_Series_RVU >= 2010)) if (!unsetenv(name)) die_errno(_("could not unsetenv '%s'"), name ? name : "(null)"); + #endif -Randall ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bug] wrapper.c uses unportable unsetenv 2021-10-29 20:14 [Bug] wrapper.c uses unportable unsetenv rsbecker @ 2021-10-29 20:35 ` Junio C Hamano 2021-10-29 20:49 ` rsbecker 0 siblings, 1 reply; 7+ messages in thread From: Junio C Hamano @ 2021-10-29 20:35 UTC (permalink / raw) To: rsbecker; +Cc: git <rsbecker@nexbridge.com> writes: > The unsetenv()/setenv(overwrite) calls are not 100% portable - as in not on > all POSIX implementations. It breaks the build on some of the NonStop > platforms. This will change in a year or two but I really don't want to fall > behind on git releases. > > This was introduced at 3540c71 but I was on vacation when it happened so did > not catch it during reviews - my apologies for that. I do not quite understand. xsetenv() does use the three-arg setenv(), but that is not anything new. It merely replaced a call to the same three-arg setenv() in environment.c that have already been there, introduced by d7ac12b2 (Add set_git_dir() function, 2007-08-01). You may argue that 3540c71 has done a shoddy job by introducing xunsetenv() without adding any caller, and to this day, we do not have a single caller to the wrapper, but we already have a few calls to unsetenv() that is compiled unconditionally. So if you built any version of Git, you must have had these somehow "available" in your build (e.g. your system headers may have made them a no-op), and I'd expect you'd keep doing the same to locally work it around on the platform side, without ... > Is it critical that this be called or can we #ifdef it away if it isn't > supported for a build? The #if is exactly this: ... doing something like this in the generic part of the code. Please don't do this. > wrapper.c@150 > + #if (_TANDEM_ARCH_ > 3 || (_TANDEM_ARCH_ == 3 && __L_Series_RVU >= 2010)) > if (setenv(name, value, overwrite)) > die_errno(_("could not setenv '%s'"), name ? name : > "(null)"); > + #endif > > wrapper.c@154 > + #if (_TANDEM_ARCH_ > 3 || (_TANDEM_ARCH_ == 3 && __L_Series_RVU >= 2010)) > if (!unsetenv(name)) > die_errno(_("could not unsetenv '%s'"), name ? name : > "(null)"); > + #endif There are compat/setenv.c and compat/unsetenv.c to be used when NO_SETENV and NO_UNSETENV are defined. Is that how you built your Git earlier since 2007, perhaps? ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [Bug] wrapper.c uses unportable unsetenv 2021-10-29 20:35 ` Junio C Hamano @ 2021-10-29 20:49 ` rsbecker 2021-10-29 21:10 ` rsbecker 0 siblings, 1 reply; 7+ messages in thread From: rsbecker @ 2021-10-29 20:49 UTC (permalink / raw) To: 'Junio C Hamano'; +Cc: git On October 29, 2021 4:35 PM, Junio C Hamano: > <rsbecker@nexbridge.com> writes: > > > The unsetenv()/setenv(overwrite) calls are not 100% portable - as in > > not on all POSIX implementations. It breaks the build on some of the > > NonStop platforms. This will change in a year or two but I really > > don't want to fall behind on git releases. > > > > This was introduced at 3540c71 but I was on vacation when it happened > > so did not catch it during reviews - my apologies for that. > > I do not quite understand. xsetenv() does use the three-arg setenv(), but that is > not anything new. It merely replaced a call to the same three-arg setenv() in > environment.c that have already been there, introduced by d7ac12b2 (Add > set_git_dir() function, 2007-08-01). > > You may argue that 3540c71 has done a shoddy job by introducing > xunsetenv() without adding any caller, and to this day, we do not have a single > caller to the wrapper, but we already have a few calls to unsetenv() that is > compiled unconditionally. > > So if you built any version of Git, you must have had these somehow "available" > in your build (e.g. your system headers may have made them a no-op), and I'd > expect you'd keep doing the same to locally work it around on the platform side, > without ... > > > Is it critical that this be called or can we #ifdef it away if it > > isn't supported for a build? The #if is exactly this: > > ... doing something like this in the generic part of the code. > Please don't do this. > > > wrapper.c@150 > > + #if (_TANDEM_ARCH_ > 3 || (_TANDEM_ARCH_ == 3 && __L_Series_RVU >= > > + 2010)) > > if (setenv(name, value, overwrite)) > > die_errno(_("could not setenv '%s'"), name ? name : > > "(null)"); > > + #endif > > > > wrapper.c@154 > > + #if (_TANDEM_ARCH_ > 3 || (_TANDEM_ARCH_ == 3 && __L_Series_RVU >= > > + 2010)) > > if (!unsetenv(name)) > > die_errno(_("could not unsetenv '%s'"), name ? name : > > "(null)"); > > + #endif > > > There are compat/setenv.c and compat/unsetenv.c to be used when > NO_SETENV and NO_UNSETENV are defined. Is that how you built your Git > earlier since 2007, perhaps? We have defined NO_SETENV and NO_UNSETENV since I have been maintaining it, so I don't get how we are getting into this situation. I was planning on removing NO_SETENV when the OS caught up for that on our minimum support builds next year. NO_UNSETENV needs to stick around for bit longer. The setenv() code is actually fine. It is unsetenv() that is causing problems. Should not git-compat-util.h be included in wrapper.c so that we reference gitunsetenv? ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [Bug] wrapper.c uses unportable unsetenv 2021-10-29 20:49 ` rsbecker @ 2021-10-29 21:10 ` rsbecker 2021-10-29 21:42 ` Junio C Hamano 0 siblings, 1 reply; 7+ messages in thread From: rsbecker @ 2021-10-29 21:10 UTC (permalink / raw) To: rsbecker, 'Junio C Hamano'; +Cc: git On October 29, 2021 4:50 PM, I wrote: > To: 'Junio C Hamano' <gitster@pobox.com> > Cc: git@vger.kernel.org > Subject: RE: [Bug] wrapper.c uses unportable unsetenv > > On October 29, 2021 4:35 PM, Junio C Hamano: > > <rsbecker@nexbridge.com> writes: > > <snip> > > There are compat/setenv.c and compat/unsetenv.c to be used when > > NO_SETENV and NO_UNSETENV are defined. Is that how you built your Git > > earlier since 2007, perhaps? > > We have defined NO_SETENV and NO_UNSETENV since I have been maintaining > it, so I don't get how we are getting into this situation. I was planning on > removing NO_SETENV when the OS caught up for that on our minimum support > builds next year. NO_UNSETENV needs to stick around for bit longer. The > setenv() code is actually fine. It is unsetenv() that is causing problems. > Should not git-compat-util.h be included in wrapper.c so that we reference > gitunsetenv? The actual issue is this: if (!unsetenv(name)) ^ "/home/ituglib/randall/git/wrapper.c", line 156: error(134): expression must have arithmetic or pointer type This is with NO_UNSETENV = YesPlease. It makes no sense to me why this isn't compiling. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bug] wrapper.c uses unportable unsetenv 2021-10-29 21:10 ` rsbecker @ 2021-10-29 21:42 ` Junio C Hamano 2021-10-29 21:50 ` Johannes Schindelin 0 siblings, 1 reply; 7+ messages in thread From: Junio C Hamano @ 2021-10-29 21:42 UTC (permalink / raw) To: rsbecker; +Cc: git <rsbecker@nexbridge.com> writes: > The actual issue is this: > > if (!unsetenv(name)) > ^ > "/home/ituglib/randall/git/wrapper.c", line 156: error(134): expression > must have arithmetic or pointer type > > This is with NO_UNSETENV = YesPlease. It makes no sense to me why this isn't > compiling. Ah, compat/unsetenv.c is broken. gitunsetenv() should return int, not void. ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----- Subject: unsetenv(3) returns int, not void This compatilibity implementation has been returning a wrong type, ever since 731043fd (Add compat/unsetenv.c ., 2006-01-25) added to the system, yet nobody noticed it in the past 16 years, presumably because no code checks failures in their unsetenv() calls. Sigh. For now, make it always succeed. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- compat/unsetenv.c | 2 +- git-compat-util.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git c/compat/unsetenv.c w/compat/unsetenv.c index bf5fd7063b..46d49c2c5e 100644 --- c/compat/unsetenv.c +++ w/compat/unsetenv.c @@ -1,6 +1,6 @@ #include "../git-compat-util.h" -void gitunsetenv (const char *name) +int gitunsetenv(const char *name) { #if !defined(__MINGW32__) extern char **environ; diff --git c/git-compat-util.h w/git-compat-util.h index b46605300a..0f7e369a5d 100644 --- c/git-compat-util.h +++ w/git-compat-util.h @@ -726,7 +726,7 @@ char *gitmkdtemp(char *); #ifdef NO_UNSETENV #define unsetenv gitunsetenv -void gitunsetenv(const char *); +int gitunsetenv(const char *); #endif #ifdef NO_STRCASESTR ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Bug] wrapper.c uses unportable unsetenv 2021-10-29 21:42 ` Junio C Hamano @ 2021-10-29 21:50 ` Johannes Schindelin 2021-10-29 21:57 ` Junio C Hamano 0 siblings, 1 reply; 7+ messages in thread From: Johannes Schindelin @ 2021-10-29 21:50 UTC (permalink / raw) To: Junio C Hamano; +Cc: rsbecker, git Hi Junio, On Fri, 29 Oct 2021, Junio C Hamano wrote: > ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----- > Subject: unsetenv(3) returns int, not void > > This compatilibity implementation has been returning a wrong type, > ever since 731043fd (Add compat/unsetenv.c ., 2006-01-25) added to > the system, yet nobody noticed it in the past 16 years, presumably > because no code checks failures in their unsetenv() calls. Sigh. > > For now, make it always succeed. Makes sense, but... > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > compat/unsetenv.c | 2 +- > git-compat-util.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git c/compat/unsetenv.c w/compat/unsetenv.c > index bf5fd7063b..46d49c2c5e 100644 > --- c/compat/unsetenv.c > +++ w/compat/unsetenv.c > @@ -1,6 +1,6 @@ > #include "../git-compat-util.h" > > -void gitunsetenv (const char *name) > +int gitunsetenv(const char *name) > { > #if !defined(__MINGW32__) > extern char **environ; ... but we are missing this: @@ -24,4 +24,5 @@ void gitunsetenv (const char *name) ++dst; } environ[dst] = NULL; + return 0; } Thanks, Dscho > diff --git c/git-compat-util.h w/git-compat-util.h > index b46605300a..0f7e369a5d 100644 > --- c/git-compat-util.h > +++ w/git-compat-util.h > @@ -726,7 +726,7 @@ char *gitmkdtemp(char *); > > #ifdef NO_UNSETENV > #define unsetenv gitunsetenv > -void gitunsetenv(const char *); > +int gitunsetenv(const char *); > #endif > > #ifdef NO_STRCASESTR > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bug] wrapper.c uses unportable unsetenv 2021-10-29 21:50 ` Johannes Schindelin @ 2021-10-29 21:57 ` Junio C Hamano 0 siblings, 0 replies; 7+ messages in thread From: Junio C Hamano @ 2021-10-29 21:57 UTC (permalink / raw) To: Johannes Schindelin; +Cc: rsbecker, git Johannes Schindelin <johannes.schindelin@gmail.com> writes: > @@ -24,4 +24,5 @@ void gitunsetenv (const char *name) > ++dst; > } > environ[dst] = NULL; > + return 0; > } Yup, that hunk (with a blank before the "return") is already in my "real" version. Thanks for spotting. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-10-29 21:57 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-29 20:14 [Bug] wrapper.c uses unportable unsetenv rsbecker 2021-10-29 20:35 ` Junio C Hamano 2021-10-29 20:49 ` rsbecker 2021-10-29 21:10 ` rsbecker 2021-10-29 21:42 ` Junio C Hamano 2021-10-29 21:50 ` Johannes Schindelin 2021-10-29 21:57 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).