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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 28, 2022, 23:22 UTC
Message-ID: <220729.86y1wcixnz.gmgdl@evledraar.gmail.com>
In-Reply-To: <20220728211221.2913928-1-calvinwan@google.com>

On Thu, Jul 28 2022, Calvin Wan wrote:

Show 7 quoted lines

> For Elijah: Cleaned up the small nits and updated resolutions for
> those 4 cases we discussed.
>
> For Ævar: Apologies for misunderstanding your suggestions to make
> my messages easier for translators to work with. I have reformatted
> all of the messages to separate text vs formatting translations. Let
> me know if this is what you were expecting.

Let's take a look, and thanks for sticking with this...

Show 102 quoted lines

>
>  merge-ort.c                 | 112 ++++++++++++++++++++++++++++++++++--
>  t/t6437-submodule-merge.sh  |  78 +++++++++++++++++++++++--
>  t/t7402-submodule-rebase.sh |   9 ++-
>  3 files changed, 185 insertions(+), 14 deletions(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 01f150ef3b..4302e900ee 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -387,6 +387,9 @@ struct merge_options_internal {
>  
>  	/* call_depth: recursion level counter for merging merge bases */
>  	int call_depth;
> +
> +	/* field that holds submodule conflict information */
> +	struct string_list conflicted_submodules;
>  };
>  
>  struct version_info {
> @@ -517,6 +520,7 @@ enum conflict_and_info_types {
>  	CONFLICT_SUBMODULE_NOT_INITIALIZED,
>  	CONFLICT_SUBMODULE_HISTORY_NOT_AVAILABLE,
>  	CONFLICT_SUBMODULE_MAY_HAVE_REWINDS,
> +	CONFLICT_SUBMODULE_NULL_MERGE_BASE,
>  
>  	/* Keep this entry _last_ in the list */
>  	NB_CONFLICT_TYPES,
> @@ -570,6 +574,8 @@ static const char *type_short_descriptions[] = {
>  		"CONFLICT (submodule history not available)",
>  	[CONFLICT_SUBMODULE_MAY_HAVE_REWINDS] =
>  		"CONFLICT (submodule may have rewinds)",
> +	[CONFLICT_SUBMODULE_NULL_MERGE_BASE] =
> +		"CONFLICT (submodule no merge base)"
>  };
>  
>  struct logical_conflict_info {
> @@ -686,6 +692,8 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
>  
>  	mem_pool_discard(&opti->pool, 0);
>  
> +	string_list_clear(&opti->conflicted_submodules, 1);
> +
>  	/* Clean out callback_data as well. */
>  	FREE_AND_NULL(renames->callback_data);
>  	renames->callback_data_nr = renames->callback_data_alloc = 0;
> @@ -1744,26 +1752,40 @@ static int merge_submodule(struct merge_options *opt,
>  
>  	int i;
>  	int search = !opt->priv->call_depth;
> +	int sub_initialized = 1;
>  
>  	/* store fallback answer in result in case we fail */
>  	oidcpy(result, opt->priv->call_depth ? o : a);
>  
>  	/* we can not handle deletion conflicts */
> -	if (is_null_oid(o))
> -		return 0;
>  	if (is_null_oid(a))
> -		return 0;
> +		BUG("submodule deleted on one side; this should be handled outside of merge_submodule()"); 
>  	if (is_null_oid(b))
> -		return 0;
> +		BUG("submodule deleted on one side; this should be handled outside of merge_submodule()");
>  
> -	if (repo_submodule_init(&subrepo, opt->repo, path, null_oid())) {
> +	if ((sub_initialized = repo_submodule_init(&subrepo,
> +									opt->repo, path, null_oid()))) {
>  		path_msg(opt, CONFLICT_SUBMODULE_NOT_INITIALIZED, 0,
>  			 path, NULL, NULL, NULL,
>  			 _("Failed to merge submodule %s (not checked out)"),
>  			 path);
> +		/*
> +		 * NEEDSWORK: Since the steps to resolve this error are
> +		 * more involved than what is currently in 
> +		 * print_submodule_conflict_suggestion(), we return
> +		 * immediately rather than generating an error message
> +		 */
>  		return 0;
>  	}
>  
> +	if (is_null_oid(o)) {
> +		path_msg(opt, CONFLICT_SUBMODULE_NULL_MERGE_BASE, 0,
> +			 path, NULL, NULL, NULL,
> +			 _("Failed to merge submodule %s (no merge base)"),
> +			 path);
> +		goto cleanup;
> +	}
> +
>  	if (!(commit_o = lookup_commit_reference(&subrepo, o)) ||
>  	    !(commit_a = lookup_commit_reference(&subrepo, a)) ||
>  	    !(commit_b = lookup_commit_reference(&subrepo, b))) {
> @@ -1849,7 +1871,15 @@ static int merge_submodule(struct merge_options *opt,
>  
>  	object_array_clear(&merges);
>  cleanup:
> -	repo_clear(&subrepo);
> +	if (!opt->priv->call_depth && !ret) {
> +		struct string_list *csub = &opt->priv->conflicted_submodules;
> +
> +		string_list_append(csub, path)->util =
> +				xstrdup(repo_find_unique_abbrev(&subrepo, b, DEFAULT_ABBREV));

FWIW (since you may have changed this due to my comment) I meant (but maybe didn't make clear enough) in https://lore.kernel.org/git/220727.86ilnilxfv.gmgdl@evledraar.gmail.com/ that just getting rid of the "util" variable could trivially be done, so:

	struct string_list *csub = &opt->priv->conflicted_submodules;
	const char *abbrev;

	abbrev = repo_find_unique_abbrev(&subrepo, b, DEFAULT_ABBREV);

	string_list_append(csub, path)->util = xstrdup(abbrev);

What you have here is also just fine, but in case you traded the line variable for line wrapping I think it's just fine (and actually preferable) to just make an intermediate variable to avoid this sort of line wrapping.

Anyway, just clarifying...

> +static void print_submodule_conflict_suggestion(struct string_list *csub) {
> +	if (csub->nr > 0) {

Since the entire body of the function is guarded by this maybe avoid the indentation and:

	if (!csub->nr)
		return;

Show 15 quoted lines

> +		struct string_list_item *item;
> +		struct strbuf msg = STRBUF_INIT;
> +		struct strbuf tmp = STRBUF_INIT;
> +
> +		strbuf_addf(&tmp, _("Recursive merging with submodules currently only supports trivial cases."));
> +		strbuf_addf(&msg, "%s\n", tmp.buf);
> +		strbuf_release(&tmp);
> +
> +		strbuf_addf(&tmp, _("Please manually handle the merging of each conflicted submodule."));
> +		strbuf_addf(&msg, "%s\n", tmp.buf);
> +		strbuf_release(&tmp);
> +
> +		strbuf_addf(&tmp, _("This can be accomplished with the following steps:"));
> +		strbuf_addf(&msg, "%s\n", tmp.buf);
> +		strbuf_release(&tmp);

This was *almost* correct in your v6, i.e.:

	printf(_("Recursive merging with submodules currently only supports trivial cases.\n"
		 "Please manually handle the merging of each conflicted submodule.\n"
		 "This can be accomplished with the following steps:\n"));

What I meant with "We can add \n\n unconditionally, no need to put it in the translation." in https://lore.kernel.org/git/220727.86ilnilxfv.gmgdl@evledraar.gmail.com/ is that you should avoid adding formatting to translations themselves if possible.

i.e. what we're translating here is a full paragraph, so to a translator it doesn't matter if it ends with ":\n" or ":", so we can add the last "\n" unconditionalyl.

But we should *not* add the \n's within a single paragraph unconditionally, a translator needs to be able to translate that entire string.

Different languages split sentences differently, and different conventions in different languages & SVO v.s. OVS or wahtever (see https://en.wikipedia.org/wiki/Word_order) means that your last sentence might need to come first in some languages.

So I think this should just be:

	printf(_("Recursive merging with submodules currently only supports trivial cases.\n"
		 "Please manually handle the merging of each conflicted submodule.\n"
		 "This can be accomplished with the following steps:"));
	putchar('\n');

I.e. the "\n" within the pargraph are imporant, and translators need to be able to amend those, and perhaps some languages will have 2x, some 4x or whatever.

Whereas the "\n" at the end is something we can always add, because it's just a matter of how we're interpolating the paragraph into other output (think *nix terminal v.s. say HTML, just as an example).

Show 11 quoted lines

> +
> +		for_each_string_list_item(item, csub) {
> +			const char *abbrev= item->util;
> +			/*
> +			 * TRANSLATORS: This is a line of advice to resolve a merge conflict
> +			 * in a submodule. The second argument is the abbreviated id of the
> +			 * commit that needs to be merged.
> +			 * E.g. - go to submodule (sub), and either merge commit abc1234"
> +			 */
> +			strbuf_addf(&tmp, _("go to submodule (%s), and either merge commit %s"),
> +													item->string, abbrev);

...

Show 7 quoted lines

> +			strbuf_addf(&msg, _(" - %s"), tmp.buf);
> +			strbuf_addf(&msg, "\n");
> +			strbuf_release(&tmp);
> +			strbuf_addf(&tmp, _("or update to an existing commit which has merged those changes"));
> +			strbuf_addf(&msg, _("   %s"), tmp.buf);
> +			strbuf_addf(&msg, "\n");
> +			strbuf_release(&tmp);

Similarly this is worse, because we're now splitting up up a single sentence or paragraph into multiple translations.

FWIW I meant that you could write some helper function that would do something like this (in Perl, too lazy to write C now):

	perl -wE '
		my @lines = split /\n/, $ARGV[0];
		for (my $i = 0; $i < @lines; $i++) {
			my $pfx = !$i ? "- " : "  "; say "$pfx$lines[$i]";
	}' "$(printf "some multi\nline paragraph\nhere")"
	- some multi
	  line paragraph
	  here

I.e. the translator would see:

	_("some multi\nline paragraph\nhere")

Which would allow them to insert any amount of newlines, but you'd just have a utility function that:

 * Splits those lines by \n
 * Formats the first line by _("- %s\n")
 * Formats subsequent lines with _("  %s\n") 
 * The %s in those is a _()'d string.

I.e. what's happening at the bottom of show_ambiguous_object(). It's:

 * A relatively small amount of programming,
 * Lets translators focus only on these formatting questions for the
   translations that do the magic formatting, and those only need to be
   changed for RTL languages.

   I.e. German and English whill both want "- %s\n", but e.g. Hebrew
   will want "%s -\n".

 * Makes the code easier to read, because instead of adding formatting concerns to every string, you'd just:

	strbuf_addstr(&buf, add_fmt_list_item(i, _("blah bla\nblah blah\nblah.)));

> +		strbuf_release(&tmp);

When you use the strbuf API you should strbuf_reset() within a single scope if you want to "clear" it, not strbuf_release().

The strbuf_release() also works, but then it's a malloc()/free(), each time, with strbuf_reset() we don't free() it, we just set the len to zero, and the content to "\0", but remember how long it was ("alloc" in strbuf.h).

You then only do the strbuf_release() at the very end.