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

From: Stefan Beller <sbeller@google.com>
Date: Feb 29, 2016, 20:40 UTC
Message-ID: <CAGZ79ka7Wrc4kzXWwi8c+6JC0FVC45g=fu-F6QxXFe37sm5GMQ@mail.gmail.com>
In-Reply-To: <20160229193222.GX28749@google.com>

On Mon, Feb 29, 2016 at 11:32 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 6 quoted lines

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

Show 29 quoted lines

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

Show 14 quoted lines

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