Re: [PATCH v6] submodule merge: update conflict error message
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jul 27, 2022, 09:20 UTC
- Message-ID
- <220727.86ilnilxfv.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <20220726210020.3397249-1-calvinwan@google.com>
On Tue, Jul 26 2022, Calvin Wan wrote:
Aside from what Elijah pointed out already...
> + > + /* field that holds submodule conflict information */ > + struct string_list conflicted_submodules;
Looks good!
Show 10 quoted lines
> cleanup:
> + if (!ret) {
> + struct string_list *csub = &opt->priv->conflicted_submodules;
> + char *util;
> + const char *abbrev;
> +
> + abbrev = repo_find_unique_abbrev(&subrepo, b, DEFAULT_ABBREV);
> + util = xstrdup(abbrev);
> +
> + string_list_append(csub, path)->util = util;Elijah pointed out that these don't need to be dup'd at all, and you should follow that advice. I'm not really familiar at all with this code (compared to him).
FWIW the "util" here could be dropped in any case, in my version it was because we were making a struct, but the idiom for this would just be:
string_list_append(....)->util = xstrdup(...);
> + printf(_(" - go to submodule (%s), and either merge commit %s\n"
> + " or update to an existing commit which has merged those changes\n"),
> + item->string, abbrev);FWIW what I mentioned in v5 was to arrange this so that " - " or whatever would be _()'d separately, so translators wouldn't need to worry about the formatting...
> + printf(" git add %s", item->string);
> + first = 0;
> + } else {
> + printf(" \\\n %s", item->string);And if we're translating *some* whitespace we should translate all of it. In RTL languages the a string like " foo" needs to be translated as "foo ". I.e. the whitespace from the "right" side of your terminal.
That was what I was pointing out in the object-name.c code, usage_with_options_internal() is another example.
> + }
> + printf(_("\n\n to record the above merge or update\n"We can add \n\n unconditionally, no need to put it in the translation.