RE: [Bug] wrapper.c uses unportable unsetenv

From: rsbecker@nexbridge.com <rsbecker@nexbridge.com>
Date: Oct 29, 2021, 20:49 UTC
Message-ID: <013501d7cd06$8c8281e0$a58785a0$@nexbridge.com>
In-Reply-To: <xmqqsfwjk1s3.fsf@gitster.g>

On October 29, 2021 4:35 PM, Junio C Hamano:

Show 11 quoted lines

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

Show 5 quoted lines

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

Show 28 quoted lines

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