On Mon, Feb 29, 2016 at 11:32 AM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> 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.
Yeah I am a bit scared too, so I'll do patches on top for further fixes
after one last reroll, fixing the issues below.
>
> [...]
>> --- 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
Thanks for reminding me of that practice!
>
> [...]
>> @@ -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.
I don't think it is common enough yet.
Thanks,
Stefan
>
> Thanks and hope that helps,
> Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html