Re: [PATCHv20 00/12] Expose submodule parallelism to the user

[Jonathan Nieder <jrnieder@gmail.com>]

Stefan Beller wrote:

> I added your suggestions as amending and as a new patch.

I think we're at the point where patches on top would work better.  I
admit I was a little scared to see another reroll.

[...]
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -299,10 +299,10 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
>  
>  	if (ce_stage(ce)) {
>  		if (suc->recursive_prefix) {
> -			strbuf_addf(out, "Skipping unmerged submodule %s/%s\n",
> +			strbuf_addf(out,_("Skipping unmerged submodule %s/%s\n"),

Missing space after comma.

Usual practice for i18n would be something like

	struct strbuf path = STRBUF_INIT;
	if (suc->recursive_prefix)
		strbuf_addf(&path, "%s/%s", suc->recursive_prefix, ce->name);
	else
		strbuf_addstr(&path, ce->name);

	strbuf_addf(out, _("Skipping unmerged submodule %s"), path.buf);
	strbuf_addch(out, '\n');
	strbuf_release(&path);

Reasons:
 - translators shouldn't have to worry about the trailing newline
 - minimizing number of strings to translate
 - minimizing the chance that a translator typo produces an invalid path

[...]
> @@ -319,7 +319,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
>  	if (suc->update.type == SM_UPDATE_NONE
>  	    || (suc->update.type == SM_UPDATE_UNSPECIFIED
>  		&& sub->update_strategy.type == SM_UPDATE_NONE)) {
> -		strbuf_addf(out, "Skipping submodule '%s'\n",
> +		strbuf_addf(out, _("Skipping submodule '%s'\n"),
>  			    displaypath);

Same issue here with the trailing \n.

If the strbuf_addf + strbuf_addch('\n') pattern is common enough, we
could introduce a helper (e.g. strbuf_addf_nl) to save typing.

Thanks and hope that helps,
Jonathan