[off-list ref] 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?
On October 29, 2021 4:35 PM, Junio C Hamano:
[off-list ref] writes:
quoted
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 ...
quoted
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.
quoted
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?
On October 29, 2021 4:50 PM, I wrote:
To: 'Junio C Hamano' <redacted>
Cc: git@vger.kernel.org
Subject: RE: [Bug] wrapper.c uses unportable unsetenv
On October 29, 2021 4:35 PM, Junio C Hamano:
quoted
[off-list ref] writes:
<snip>
quoted
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.