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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 19, 2022, 06:39 UTC
Message-ID: <xmqqzgh5d2s5.fsf@gitster.g>
In-Reply-To: <20220718214349.3379328-1-calvinwan@google.com>

Calvin Wan <calvinwan@google.com> writes:

> Changes since v4:
>  - Rebased onto gitster/master
>  - Fixed test cases to work with only merge-ort (and not merge-recursive)

That's not a log-message material. You could have probably written scissors ...

> == Description ==

--- >8 ---

... here, perhaps?

Show 78 quoted lines

> When attempting to merge in a superproject with conflicting submodule
> pointers that cannot be fast-forwarded or trivially resolved, the merge
> fails and git prints the following error:
>
> Failed to merge submodule <submodule>
> CONFLICT (submodule): Merge conflict in <submodule>
> Automatic merge failed; fix conflicts and then commit the result.
>
> Git is left in a conflicted state, which requires the user to:
>  1. merge submodules or update submodules to an already existing
> 	commit that reflects the merge
>  2. add submodules changes to the superproject
>  3. finish merging superproject
> These steps are non-obvious for newer submodule users to figure out
> based on the error message and neither `git submodule status` nor `git
> status` provide any useful pointers. 
>
> Update error message to match the steps above. If git does not detect a 
> merge resolution, the following is printed:
>
> ====
>
> Failed to merge submodule <submodule>
> CONFLICT (submodule): Merge conflict in <submodule>
> Recursive merging with submodules currently only supports trivial cases.
> Please manually handle the merging of each conflicted submodule.
> This can be accomplished with the following steps:
>  - go to submodule (<submodule>), and either merge commit <commit>
> or update to an existing commit which has merged those changes
>  - come back to superproject, and `git add <submodule>` to record the above merge or update
>  - resolve any other conflicts in the superproject
>  - commit the resulting index in the superproject
> Automatic merge failed; fix conflicts and then commit the result.
>
> ====
>
> If git detects a possible merge resolution, the following is printed:
>
> ====
>
> Failed to merge submodule sub, but a possible merge resolution exists:
>     <commit> Merge branch '<branch1>' into <branch2>
>
> CONFLICT (submodule): Merge conflict in <submodule>
> Recursive merging with submodules currently only supports trivial cases.
> Please manually handle the merging of each conflicted submodule.
> This can be accomplished with the following steps:
> To manually complete the merge:
>  - go to submodule (<submodule>), and either merge commit <commit>
> or update to an existing commit which has merged those changes
> such as one listed above
>  - come back to superproject, and `git add <submodule>` to record the above merge or update
>  - resolve any other conflicts in the superproject
>  - commit the resulting index in the superproject
> Automatic merge failed; fix conflicts and then commit the result.
>
> ====
>
> If git detects multiple possible merge resolutions, the following is printed:
>
> ====
>
> Failed to merge submodule sub, but multiple possible merges exist:
>     <commit> Merge branch '<branch1>' into <branch2>
>     <commit> Merge branch '<branch1>' into <branch3>
>
> CONFLICT (submodule): Merge conflict in <submodule>
> Recursive merging with submodules currently only supports trivial cases.
> Please manually handle the merging of each conflicted submodule.
> This can be accomplished with the following steps:
> To manually complete the merge:
>  - go to submodule (<submodule>), and either merge commit <commit>
> or update to an existing commit which has merged those changes
> such as one listed above
>  - come back to superproject, and `git add <submodule>` to record the above merge or update
>  - resolve any other conflicts in the superproject
>  - commit the resulting index in the superproject
> Automatic merge failed; fix conflicts and then commit the result.

But cutting the cruft at the top is still not enough, because the below are not log-message material, either.

These extraneous stuff may help reviewers while the patch is being polished, but once committed to our history, readers of "git log" should not have to care what other attempts were made that did not become part of the final hstory. The log message proper should be understandable standalone.

The stuff to help reviewers who may have seen earlier round are usually written in the cover letter, or after the three-dash line.

Show 22 quoted lines

> == Previous Changes ==
>
> Changes since v3:
> ...
>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> ---
>  merge-ort.c                 | 56 +++++++++++++++++++++++++++++++++++++
>  t/t6437-submodule-merge.sh  | 38 +++++++++++++++++++++----
>  t/t7402-submodule-rebase.sh |  9 ++++--
>  3 files changed, 95 insertions(+), 8 deletions(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 01f150ef3b..125ee3c0d1 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> ...
> +	if (csub && csub->nr > 0) {
> +		int i;
> +		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"));

This makes me wonder if these "helpful but verbose" messages should use the advise mechanism. Also, those reviewers who care about l10n may suggest use of printf_ln() to lose the LF at the end of these messages (i.e. not just the above one, but others we see below as well).

Show 29 quoted lines

> +		for (i = 0; i < csub->nr; i++) {
> +			printf(_(" - go to submodule (%s), and either merge commit %s\n"
> +				    "or update to an existing commit which has merged those changes\n"),
> +					csub->items[i].path,
> +					csub->items[i].oid);
> +			if (csub->items[i].resolution_exists)
> +				printf(_("such as one listed above\n"));
> +		}
> +		printf(_(" - come back to superproject, and `git add"));
> +		for (i = 0; i < csub->nr; i++)
> +			printf(_(" %s"), csub->items[i].path);
> +		printf(_("` to record the above merge or update \n"
> +			" - resolve any other conflicts in the superproject\n"
> +			" - commit the resulting index in the superproject\n"));
> +	}
> +}
> +
>  void merge_display_update_messages(struct merge_options *opt,
>  				   int detailed,
>  				   struct merge_result *result)
> @@ -4461,6 +4515,8 @@ void merge_display_update_messages(struct merge_options *opt,
>  	}
>  	string_list_clear(&olist, 0);
>  
> +	print_submodule_conflict_suggestion(&opti->conflicted_submodules);
> +
>  	/* Also include needed rename limit adjustment now */
>  	diff_warn_rename_limit("merge.renamelimit",
>  			       opti->renames.needed_limit, 0);

Thanks.