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

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

On Fri, Sep 14 2018, Jonathan Nieder wrote:

> Æ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.

>  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).

> [...]
>>  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.