RE: [Bug] wrapper.c uses unportable unsetenv

[<rsbecker@nexbridge.com>]

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?