git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: <rsbecker@nexbridge.com>
Cc: <git@vger.kernel.org>
Subject: Re: [Bug] wrapper.c uses unportable unsetenv
Date: Fri, 29 Oct 2021 13:35:08 -0700	[thread overview]
Message-ID: <xmqqsfwjk1s3.fsf@gitster.g> (raw)
In-Reply-To: <012301d7cd01$90428960$b0c79c20$@nexbridge.com> (rsbecker@nexbridge.com's message of "Fri, 29 Oct 2021 16:14:17 -0400")

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



  reply	other threads:[~2021-10-29 20:35 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-29 20:14 [Bug] wrapper.c uses unportable unsetenv rsbecker
2021-10-29 20:35 ` Junio C Hamano [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqsfwjk1s3.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=rsbecker@nexbridge.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).