Re: [PATCH 0/7] More i18n fixes

["Jean-Noël AVILA" <jn.avila@free.fr>]

On Monday, 21 March 2022 14:59:24 CET Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Mar 21 2022, Johannes Sixt wrote:
> 
> > Am 20.03.22 um 22:54 schrieb Jean-Noël Avila via GitGitGadget:
> >> This is another i18n PR (and hopefully the last for a while).
> >> 
> >> As usual, the intent is kept the same: curbing the number of strings to
> >> translate, remove constant, error prone parts out of the way, trying in some
> >> sense to "put a precedent" so that the template strings can be reused later.
> >
> > I feel that many of the example conversions look like sentence lego
> > because there remains only one English word, e.g., "'%s' failed". The
> > converted code does not leave a hint for the translators what the %s
> > will be. Is it a command, a function name, somehting else? Even if the
> > hint was provided, different translations may be required depending on
> > the substituted entity. Did you investigate the existing translations
> > whether all of them can be converted to the new scheme?
> >
> >> This series has also a RFC status: can "bad argument" messages be merged
> >> with unrecognized argument?
> >
> > The cases that patch 7/7 transforms look like they need not keep
> > "unrecognized argument", but can be converted to "bad argument".
> >
> > Disclaimer: neither am I a translator nor a user of a translated Git.
> 
> Just to add to this:
> 
>  - Careful use of sentence lego is OK, but e.g. in my native language a
>    command-line option would use a male noun article, whereas commands
>    would be feminine.
> 
>    (I still haven't submitted an Icelandic translation, but this applies
>    in general).
> 
>    As a result string like "'%s' failed" can be *workable*, i.e. you can
>    translate it assuming you'll get any arbitrary string, but the
>    translation will often be rather tortured.
> 
>    So it's much preferred (and this also goes to Johannes's comment) to
>    instead do e.g.:
> 
>        "failed to run the '%s' command"
>        "failed to use the '%s' argument"
> 
>    Or whatever, and e.g. for:
> 	
> 	-		return strbuf_addf_ret(err, -1, _("%%(objecttype) does not take arguments"));
> 	+		return strbuf_addf_ret(err, -1, _("%s does not take arguments"), "%(objecttype)");
> 
>    Instead say "the '%s' format does not...", i.e. disambiguate with
>    "format".
> 
>  - While perfect shouldn't be the enemy of the good, it would be most
>    welcome to improve some of the warts revealed by these messages,
>    notably that e.g. the "failed to run X command" don't report
>    errno. E.g. this in git.c is a good template (except for the "\n" we
>    should ideally get rid of):
> 
>        _("failed to run command '%s': %s\n")

OK. 

> 
>  - On that topic, it would be really useful to see if we can unify some
>    of these with *existing* po/git.pot messaging, I don't know if that's
>    part of your workflow, but in some cases I've seen we can either
>    tweak wording slightly to match an existing message, or could further
>    unify some existing similar messages.
> 

That's part of the workflow, although not hand-made, but greped. There are two types of factorizations:
* strings where introducing a placeholder creates duplicates
* strings that basically mean the same, but expressed differently (require human parsing)

>  - Even if we say "failed to run git-apply" or whatever now we should
>    really be adding quotes to these as we convert them. In some cases
>    the changes that (good):
> 	
> 	-		die(_("git-http-push failed"));
> 	+		die(_("'%s' failed"), "git-http-push");
> 
>    But not in others (bad):
> 	
> 	-		res = error(_("Bad bisect_write argument: %s"), state);
> 	+		res = error(_("bad %s argument: %s"), "bisect_write", state);
> 
>    I.e. that should be 'bad '%s' argument. And also on the "unify" point
>    above, e.g. grep.c has this:
> 
>        grep.c: die("bad %s argument: %s", opt, arg);
> 
>    So we could covert that one to "bad '%s' argument: '%s"" while we're
>    at it...
> 
> - In some cases there's ucase to lcase conversions, like Bad->bad above
>   (good), but others are missed, e.g. (also missing quotes as noted
>   above):
> 
> 	-		die(_("Server does not support --shallow-since"));
> 	+		die(_("Server does not support %s"), "--shallow-since");
> 
>  - On quotes, let's consistently use '' quotes, and not e.g.g:
> 	
> 	-		die(_("`scalar list` does not take arguments"));
> 	+		die(_("%s does not take arguments"), "`scalar list`");
> 
> 

OK for disambiguation, lowercasing and quoting all placeholders.
BTW, disambiguation may have the side effect of revealing where the type of placeholders may not be matching the types of replaced variables.


Will reroll.

Thanks.