git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Martin Ågren" <martin.agren@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Johannes Schindelin <johannes.schindelin@gmx.de>,
	Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>, Dan Jacques <dnj@google.com>
Subject: Re: [PATCH 1/3] gettext: avoid initialization if the locale dir is not present
Date: Fri, 20 Apr 2018 12:54:35 +0200	[thread overview]
Message-ID: <CAN0heSpda1ZnFXgoCEgxGdBk-JYUSPAV0A=mQYMtOq7w8x+5=w@mail.gmail.com> (raw)
In-Reply-To: <87o9iedwqn.fsf@evledraar.gmail.com>

On 20 April 2018 at 11:59, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> On Fri, Apr 20 2018, Johannes Schindelin wrote:
>
>> The runtime of a simple `git.exe version` call on Windows is currently
>> dominated by the gettext setup, adding a whopping ~150ms to the ~210ms
>> total.
>>
>> Given that this cost is added to each and every git.exe invocation goes
>> through common-main's invocation of git_setup_gettext(), and given that
>> scripts have to call git.exe dozens, if not hundreds, of times, this is
>> a substantial performance penalty.
>>
>> This is particularly pointless when considering that Git for Windows
>> ships without localization (to keep the installer's size to a bearable
>> ~34MB): all that time setting up gettext is for naught.
>>
>> So let's be smart about it and skip setting up gettext if the locale
>> directory is not even present.

> If you don't ship git for windows with gettext or a podir, then compile
> with NO_GETTEXT, then we won't even compile this function you're
> patching here. See line 30 and beyond of gettext.h.
>
> Are you instead compiling git for windows with gettext support with an
> optional add-on package for i18n, so then this podir conditional would
> pass?
>
> In any case, if this is actually the desired behavior I think it's worth
> clearly explaining the interaction with NO_GETTEXT in the commit
> message, and if you *actually* don't want NO_GETTEXT I think it's useful
> to guard this with something like MAYBE_GETTEXT on top, so this code
> doesn't unintentionally hide installation errors on other
> platforms. I.e. something like:
>
>         int have_podir = is_directory(podir);
>         if (!have_podir)
> #ifdef MAYBE_GETTEXT
>                 return;
> #else
>                 BUG("Broken installation, can't find '%s'!", podir);
> #endif

Is it fair to say that such a broken installation would function more or
less well before and after Dscho's patch, and that your approach would
render such an installation quite useless? Do we have some other similar
cases where we go BUG("I could not find a resource. I could manage
fairly well without it, but you seemed to want it when you installed
me.")? I wonder what other well-respected software do if they can't find
their translations.

I suppose the logic could be the other way round, i.e., with a flag
REALLY_REQUIRE_GETTEXT_AT_RUNTIME. But I also wonder if a user who does
not notice that the installation is broken without us screaming BUG in
their face, really minds the "brokenness" that much.

Disclaimer: I do not use translated Git, nor other non-English-language
software.

Martin

  reply	other threads:[~2018-04-20 10:54 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-20  8:03 [PATCH 0/3] Some add-on patches on top of dj/runtime-prefix Johannes Schindelin
2018-04-20  8:03 ` [PATCH 1/3] gettext: avoid initialization if the locale dir is not present Johannes Schindelin
2018-04-20  9:59   ` Ævar Arnfjörð Bjarmason
2018-04-20 10:54     ` Martin Ågren [this message]
2018-04-20 11:18       ` Ævar Arnfjörð Bjarmason
2018-04-20 19:35         ` Johannes Schindelin
2018-04-21  7:43           ` Ævar Arnfjörð Bjarmason
2018-04-21 10:17             ` Johannes Schindelin
2018-04-20  8:03 ` [PATCH 2/3] git_setup_gettext: plug memory leak Johannes Schindelin
2018-04-20  8:04 ` [PATCH 3/3] Avoid multiple PREFIX definitions Johannes Schindelin
2018-04-22 15:32   ` Philip Oakley
2018-04-21 11:13 ` [PATCH v2 0/3] Some add-on patches on top of dj/runtime-prefix Johannes Schindelin
2018-04-21 11:14   ` [PATCH v2 1/3] gettext: avoid initialization if the locale dir is not present Johannes Schindelin
2018-04-21 11:14   ` [PATCH v2 2/3] git_setup_gettext: plug memory leak Johannes Schindelin
2018-04-21 11:18   ` [PATCH v2 3/3] Avoid multiple PREFIX definitions Johannes Schindelin
2018-04-24  1:44   ` [PATCH v2 0/3] Some add-on patches on top of dj/runtime-prefix Junio C Hamano
2018-04-24  1:50     ` Junio C Hamano
2018-04-24  2:41       ` Junio C Hamano
2018-04-24 14:48         ` Johannes Schindelin
2018-04-25  1:28           ` Junio C Hamano
2018-04-25  7:46             ` Johannes Schindelin

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='CAN0heSpda1ZnFXgoCEgxGdBk-JYUSPAV0A=mQYMtOq7w8x+5=w@mail.gmail.com' \
    --to=martin.agren@gmail.com \
    --cc=avarab@gmail.com \
    --cc=dnj@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=johannes.schindelin@gmx.de \
    /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).