Re: [PATCH v6] submodule merge: update conflict error message

["Ævar Arnfjörð Bjarmason" <avarab@gmail.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!

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