Re: [PATCH v5 00/11] Factorization of messages with similar meaning

From: Johannes Sixt <j6t@kdbg.org>
Date: Jan 5, 2022, 20:58 UTC
Message-ID: <478ed4c7-467d-384d-b6d9-68956dc39c41@kdbg.org>
In-Reply-To: <pull.1088.v5.git.1641412944.gitgitgadget@gmail.com>

Am 05.01.22 um 21:02 schrieb Jean-Noël Avila via GitGitGadget:

Show 30 quoted lines

> This series is a meager attempt at rationalizing a small fraction of the
> internationalized messages. Sorry in advance for the dull task of reviewing
> these insipide patches.
> 
> Doing so has some positive effects:
> 
>  * non-translatable constant strings are kept out of the way for translators
>  * messages with identical meaning are built identically
>  * the total number of messages to translate is decreased.
> 
> Changes since V1:
> 
>  * took into account the comments, except for ref-filter.c where the
>    proposed refactoring is not obvious.
>  * added even more strings to the "cannot be used together" crowd.
> 
> Changes since V2:
> 
>  * fixed change of behaviour in tag.c
>  * reverted sam changes as per Johannes Sixt comments
> 
> Changes since V3:
> 
>  * apply Oxford comma where needed
>  * switch all options to " '%s' " style where i18n is applied.
> 
> Changes since V4:
> 
>  * Apply changes by René on tag.c
>  * cosmetic changes

This round looks good to me, with one caveat: I am not a translator, nor do I use a translated version of Git. So, I haven't verified the claim that the number translatable strings was reduced greatly, nor whether there are any accidential duplicates due to typos. I infer the correctness only by looking at the changes.

There's one small nit visible in the range-diff; not worth a reroll IMHO:

Show 17 quoted lines

>   5:  a9d8a50d666 !  5:  ad58bc8d8a9 i18n: tag.c factorize i18n strings
>      @@ builtin/tag.c: int cmd_tag(int argc, const char **argv, const char *prefix)
>       -		die(_("--no-contains option is only allowed in list mode"));
>       -	if (filter.points_at.nr)
>       -		die(_("--points-at option is only allowed in list mode"));
>      +-	if (filter.reachable_from || filter.unreachable_from)
>      +-		die(_("--merged and --no-merged options are only allowed in list mode"));
>       +		only_in_list = "-n";
>       +	else if (filter.with_commit)
>       +		only_in_list = "--contains";
>      @@ builtin/tag.c: int cmd_tag(int argc, const char **argv, const char *prefix)
>       +		only_in_list = "--no-contains";
>       +	else if (filter.points_at.nr)
>       +		only_in_list = "--points-at";
>      ++	else if (filter.reachable_from)
>      ++		only_in_list = "--merged";
>      ++	else if  (filter.unreachable_from)

An extra blank after the 'if'.

Show 9 quoted lines

>      ++		only_in_list = "--no-merged";
>       +	if (only_in_list)
>       +		die(_("the '%s' option is only allowed in list mode"), only_in_list);
>      - 	if (filter.reachable_from || filter.unreachable_from)
>      --		die(_("--merged and --no-merged options are only allowed in list mode"));
>      -+		die(_("'--merged' and '--no-merged' options are only allowed in list mode"));
>        	if (cmdmode == 'd') {
>        		ret = delete_tags(argv);
>        		goto cleanup;

-- Hannes