Re: [PATCH] i18n: add infrastructure for translating Git with gettext

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 16, 2011, 10:25 UTC
Message-ID: <CACBZZX4nypBW1agNw6NrC-7LBWbjZ1ycgpn-zvBsg0x4EDBD0g@mail.gmail.com>
In-Reply-To: <7vlirhx14x.fsf@alter.siamese.dyndns.org>

On Tue, Nov 15, 2011 at 09:04, Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Change the skeleton implementation of i18n in Git to one that can show
>> localized strings to users for our C, Shell and Perl programs using
>> either GNU libint or the Solaris gettext implementation.
>
> Happy ;-).
>
> Isn't it libintl, though?
>
>> This new internationalization support is enabled by default. If
>> gettext isn't available, or if Git is compiled with
>> NO_GETTEXT=YesPlease, Git fall back on its current behavior of showing
>
> s/fall/falls/;

I'll fix both of these speling errurs in a new version of the patch.

But I won't send it for at least a few days. I don't think there's much point as we're in no rush to apply this, and it might as well sit on-list for a few days to accumulate more review.

Show 15 quoted lines

>> This change is somewhat large because as well as adding a C, Shell and
>> Perl i18n interface we're adding a lot of tests for them, and for
>> those tests to work we need a skeleton PO file to actually test
>> translations.
>
> You _could_ split it up this way, I think, if you really wanted to:
>
>  * The mechanism, i.e. integration with libintl and gettext, without any
>   translated strings but with the .pot file, with tests to make sure in a
>   locale that is missing *.mo files for it, we get the default output;
>
>  * Sample translation for is_IS locale, with tests to make sure that we
>   get translated output in a locale that has *.mo files for it.
>
> But I am not sure if that is better.

I think it's better to keep it in one large patch. The changes to the core code are pretty minimal, most of what it's adding is new stuff that's only ever used if i18n itself is used.

Show 30 quoted lines

>> diff --git a/.gitignore b/.gitignore
>> index 8572c8c..c47f3a8 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -224,3 +224,4 @@
>>  *.pdb
>>  /Debug/
>>  /Release/
>> +/share/
>
> I somehow find it distasteful that this is overly wide; same with the
> change in Makefile to "rm -rf share/". I suspect it wouldn't be the case
> if it were limited to share/locale/ or something.
>
> But perhaps we would never ship anything in share/ and things in there
> will always come from elsewhere.
>
>> diff --git a/Makefile b/Makefile
>> index ee34eab..896f5fd 100644
>> --- a/Makefile
>> +++ b/Makefile
>> ...
>> @@ -2435,6 +2507,7 @@ clean:
>>       $(RM) $(TEST_PROGRAMS)
>>       $(RM) -r bin-wrappers
>>       $(RM) -r $(dep_dirs)
>> +     $(RM) -r po/git.pot share/
>
> It seems "distclean" tells us to clean po/git.pot, which hints at me that
> normal "clean" shouldn't?

I'm not quite sure what we should do with it.

Here's what we do with the sharedir currently:

 * It's used as the gitwebdir already, and is used also as the
   localedir with this patch:

    sharedir = $(prefix)/share
    gitwebdir = $(sharedir)/gitweb
    localedir = $(sharedir)/locale

 * Even then I can't find the option that makes gitweb put something
   in it, after a "make" with this patch:

    $ find share -type f
    share/locale/is/LC_MESSAGES/git.mo

 * It *is* important that we have this under a
   $SOMETHING/is/LC_MESSAGES/git.mo structure, since we set
   GIT_TEXTDOMAINDIR to $SOMETHING, and the gettext library will only
   understand something in this layout.

   But that $SOMETHING doesn't have to be $ROOT/share, it could be
   e.g. $ROOT/po/generated, which would make it not conflict with the
   gitwebdir.

 * Yeah, we shouldn't remove po/git.pot in "clean" as well as
   "distclean". I missed that it had been added in 92a684b916 while
   applying this patch.