git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Duy Nguyen <pclouds@gmail.com>
Cc: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Git Mailing List" <git@vger.kernel.org>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Vasco Almeida" <vascomalmeida@sapo.pt>,
	"Jiang Xin" <worldhello.net@gmail.com>
Subject: Re: [PATCH] Poison gettext with the Ook language
Date: Wed, 24 Oct 2018 19:54:03 +0200	[thread overview]
Message-ID: <87woq7b58k.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <CACsJy8Dex3VYEXmvRZv5_ot1-cwjJtir=kvupzKe7-Z2qPZw+Q@mail.gmail.com>


On Wed, Oct 24 2018, Duy Nguyen wrote:

> On Tue, Oct 23, 2018 at 6:45 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> >> The effect of what I'm suggesting here, and which my WIP patch in
>> >> <875zxtd59e.fsf@evledraar.gmail.com> implements is that we'd do a
>> >> one-time getenv() for each process that prints a _() message that we
>> >> aren't doing now, and for each message call a function that would check
>> >> a boolean "are we in poison mode" static global variable.
>> >
>> > Just don't do the getenv() inside _() even if you just do it one time.
>> > getenv() may invalidate whatever value from the previous call. I would
>> > not want to find out my code broke because of an getenv() inside some
>> > innocent _()...
>>
>> How would any code break? We have various getenv("GIT_TEST_*...")
>> already that work the same way. Unless you set that in the environment
>> the function is idempotent, and I don't see how anyone would do that
>> accidentally.
>
> I didn't check those GIT_TEST_ but I think the difference is in
> complexity. When you write
>
>  var = getenv("foo");
>  complexFunction();
>
> you probably start to feel scary and wrap getenv() with a strdup()
> because you usually don't know exactly what complexFunction() can call
> (and even if you do, you can't be sure what it may call in the
> future).
>
> The person who writes
>
>  printf(_("%s"), getenv("foo"));
>
> may not go through the same thought process as with complexFunction().
> If _() calls getenv(), because you the order of parameter evaluation
> is unspecified, you cannot be sure if getenv("foo") will be called
> before or after the one inside _(). One of them may screw up the
> other.

Ah, you mean because getenv() is not reentrant such a construct may
cause us to return something else entirely for getenv("bar") (e.g. in
this case the value for getenv("bar")).

Is that something you or anyone else has seen in practice? My intuition
is that while POSIX doesn't make that promise it isn't likely that
there's any implementation that would mutate the env purely on a
getenv(), but setenv() (munging some static "env" area in-place) might
be another matter.

But we could always call use_gettext_poison() really early from
git_setup_gettext() (called from our main()) if we're worried about
this, it would then set the static "poison_requested" variable and we
wouldn't call getenv() again, e.g. if we're later calling it in the
middle of multithreaded code, or within the same same sequence point.

>> > And we should still respect NO_GETTEXT, not doing any extra work when
>> > it's defined.
>>
>> This is not how any of our NO_* defines work. *Sometimes* defining them
>> guarantees you do no extra work, but in other cases we've decided that
>> bending over backwards with #ifdef in various places isn't worth it.
>
> I guess it boils down to "worth it". For me #ifdef NO_GETTEXT would be
> limited to gettext.h and it's not that much work. But you do the
> actual work. You decide.

Yeah the ifdef is pretty small and not really worth worrynig about, the
main benefit is being able to run tests in this mode without
recompiling.

I hadn't been running with GETTEXT_POISON when I build git because I
hadn't been bothered to build twice just for it, but am now running it
alongside other GIT_TEST_* modes.

  reply	other threads:[~2018-10-24 17:54 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-22 15:36 [PATCH] Poison gettext with the Ook language Nguyễn Thái Ngọc Duy
2018-10-22 20:22 ` SZEDER Gábor
2018-10-22 20:22   ` [PATCH 1/8] test-lib.sh: preserve GIT_GETTEXT_POISON from the environment SZEDER Gábor
2018-10-22 20:22   ` [PATCH 2/8] gettext: don't poison if GIT_GETTEXT_POISON is set but empty SZEDER Gábor
2018-10-22 20:38     ` Ævar Arnfjörð Bjarmason
2018-10-22 20:22   ` [PATCH 3/8] lib-rebase: loosen GETTEXT_POISON check in fake editor SZEDER Gábor
2018-10-22 20:22   ` [PATCH 4/8] gettext: #ifdef away GETTEXT POISON-related code from _() and Q_() SZEDER Gábor
2018-10-22 20:22   ` [PATCH 5/8] gettext: put "# GETTEXT POISON #" string literal into a macro SZEDER Gábor
2018-10-22 20:22   ` [PATCH 6/8] gettext: use an enum for the mode of GETTEXT POISONing SZEDER Gábor
2018-10-22 20:22   ` [PATCH 7/8] gettext: introduce GIT_GETTEXT_POISON=scrambled SZEDER Gábor
2018-10-23 14:44     ` Duy Nguyen
2018-10-22 20:22   ` [PATCH 8/8] travis-ci: run GETTEXT POISON build job in scrambled mode, too SZEDER Gábor
2018-10-23 14:37   ` [PATCH] Poison gettext with the Ook language Duy Nguyen
2018-10-27 10:11   ` Jakub Narebski
2018-10-22 20:52 ` Johannes Schindelin
2018-10-22 21:09 ` Ævar Arnfjörð Bjarmason
2018-10-22 21:46   ` Ævar Arnfjörð Bjarmason
2018-10-22 23:04   ` Junio C Hamano
2018-10-23 21:01     ` [PATCH] i18n: make GETTEXT_POISON a runtime option Ævar Arnfjörð Bjarmason
2018-10-24  5:45       ` Junio C Hamano
2018-10-24  7:44         ` Jeff King
2018-10-25  1:00           ` Junio C Hamano
2018-10-25  1:09             ` Jeff King
2018-10-25  1:24               ` Ramsay Jones
2018-10-25 21:23                 ` Jeff King
2018-10-26 19:20                   ` Ævar Arnfjörð Bjarmason
2018-10-27  6:59                     ` Jeff King
2018-10-27 10:42                       ` Ævar Arnfjörð Bjarmason
2018-10-24 11:47         ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2018-11-01 19:31           ` [PATCH v3] " Ævar Arnfjörð Bjarmason
2018-11-02  3:11             ` Junio C Hamano
2018-11-02 16:37             ` SZEDER Gábor
2018-11-08 20:26               ` Ævar Arnfjörð Bjarmason
2018-11-08 20:51                 ` SZEDER Gábor
2018-11-08  3:24             ` Junio C Hamano
2018-11-08 19:12               ` Eric Sunshine
2018-11-08 21:15             ` [PATCH v4 0/2] " Ævar Arnfjörð Bjarmason
2018-11-08 21:15             ` [PATCH v4 1/2] " Ævar Arnfjörð Bjarmason
2018-11-08 21:15             ` [PATCH v4 2/2] Makefile: ease dynamic-gettext-poison transition Ævar Arnfjörð Bjarmason
2018-10-23  9:30   ` [PATCH] Poison gettext with the Ook language Johannes Schindelin
2018-10-23 10:17     ` Ævar Arnfjörð Bjarmason
2018-10-23 11:07       ` Johannes Schindelin
2018-10-23 15:00       ` Duy Nguyen
2018-10-23 16:45         ` Ævar Arnfjörð Bjarmason
2018-10-24 14:41           ` Duy Nguyen
2018-10-24 17:54             ` Ævar Arnfjörð Bjarmason [this message]
2018-10-25  3:52             ` Junio C Hamano
2018-10-25  6:20               ` Jeff King
2018-10-27  6:55                 ` 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=87woq7b58k.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=pclouds@gmail.com \
    --cc=szeder.dev@gmail.com \
    --cc=vascomalmeida@sapo.pt \
    --cc=worldhello.net@gmail.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).