Re: [PATCH 1/3] gettext: avoid initialization if the locale dir is not present

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  gettext.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/gettext.c b/gettext.c
> index baba28343c3..701355d66e7 100644
> --- a/gettext.c
> +++ b/gettext.c
> @@ -163,6 +163,9 @@ void git_setup_gettext(void)
>  	if (!podir)
>  		podir = system_path(GIT_LOCALE_PATH);
>
> +	if (!is_directory(podir))
> +		return;
> +
>  	bindtextdomain("git", podir);
>  	setlocale(LC_MESSAGES, "");
>  	setlocale(LC_TIME, "");

I wish we could fix the root cause and just make gettext faster (or ship
with a small shimmy of our own), but leaving that aside, I don't see how
this patch makes sense.

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