Re: [PATCH 2/3] archive: implement protocol v2 archive command

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 14, 2018, 14:31 UTC
Message-ID: <87d0tgkv55.fsf@evledraar.gmail.com>
In-Reply-To: <20180914060552.GD219147@aiede.svl.corp.google.com>

On Fri, Sep 14 2018, Jonathan Nieder wrote:

Show 23 quoted lines

> Ævar Arnfjörð Bjarmason wrote:
>> On Wed, Sep 12 2018, Stefan Beller wrote:
>
>>> Would asking for a setlocale() on the server side be an unreasonable
>>> feature request for the capabilities (in a follow up patch, and then not
>>> just for archive but also fetch/push, etc.)?
>>
>> This would be very nice to have, but as you suggest in some follow-up
>> change.
>
> Indeed, I think we've gone pretty far afield from the goal of this
> patch series.
>
>> I think though that instead of doing setlocale() it would be better to
>> pass some flag saying we're operating in a machine-readable mode, and
>> then we'd (as part of the protocol defintion) say we're going to emit
>> GIT_ERR_UPLOAD_ARCHIVE_EXPECTED_DELIM_PACKET or whatever.
>
> I think you're suggesting client-side message generation, and that is
> one way to handle internationalization of server output.
>
> The main downside is when the server really does want to provide a
> custom error message.  For that, we'd need

Yeah you can't do it for everything. E.g. hooks will want to spew out custom messages, and this hypothetical protocol extension won't have codes for those. So you'll still need to pass $LANG along.

Show 14 quoted lines

>  1. To propagate LANG to the server, so it knows what human language
>     to generate messages in.
>
>  2. On the server side, to produce messages in that language if
>     available, with an appropriate fallback if not.
>
> We've been thinking of doing at least (1) using the same trick as
> server-options use (cramming it into client capabilities).
>
> It is difficult to use setlocale for this because it affects the whole
> program (problematic for a threaded server) and affects features like
> collation order instead of just message generation (problematic for
> many things).  Does gettext have a variant that takes a locale_t
> argument?

No, its API is fairly crappy and depends on setlocale().

Keep in mind though that we're not tied to that API. E.g. one way to work around this problem is to simply loop through all the languages we have translations for at server startup, for each one call setlocale() and gettext(), and save the result in a hash table for runtime lookup, then you'd just call sprintf(hash[language][message_id], ...) at runtime.

That's all libintl is really doing under the hood, in a roundabout way where calls to setlocale() determine what table we're looking things up in.

The reason I opted to go for gettext to begin with was mainly a) it was there b) the ubiquitous availability of tooling for translators when it comes to the *.po files.

But the API for looking things up at runtime is fairly small, and easy to replace. We could e.g. replace all of our own gettext.[ch] wrapper with something that works somewhat like what I described above, with some extra build step to extract the relevant data from the *.mo files (or parse the *.po directly).

Show 11 quoted lines

> [...]
>>  4) Aside from translation purposes, getting a machine-readable
>>     "push/pull" etc. mode would be very handy. E.g. now you need to
>>     parse stderr to see why exactly your push failed (hook denied, or
>>     non-fast-forward, or non-fast-forward where there was a lock race
>>     condition? ...).
>
> Indeed, this is a good reason to provide error codes instead of (in
> the case where the message doesn't add anything to it) or alongside
> (in case the error message is more specialized) human-oriented error
> messages.