Re: [PATCH 07/12] merge-tree: support including merge messages in output

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Elijah,

On Sat, 22 Jan 2022, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>
>
> When running `git merge-tree --write-tree`, we previously would only
> return an exit status reflecting the cleanness of a merge, and print out
> the toplevel tree of the resulting merge.  Merges also have
> informational messages, ("Auto-merging <PATH>", "CONFLICT (content):
> ...", "CONFLICT (file/directory)", etc.)  In fact, when non-content
> conflicts occur (such as file/directory, modify/delete, add/add with
> differing modes, rename/rename (1to2), etc.), these informational
> messages are often the only notification since these conflicts are not
> representable in the contents of the file.
>
> Add a --[no-]messages option so that callers can request these messages
> be included at the end of the output.  Include such messages by default
> when there are conflicts, and omit them by default when the merge is
> clean.

Makes sense.

> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index 0c19639594d..560640ad911 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -440,22 +441,30 @@ static int real_merge(struct merge_tree_options *o,
>  		commit_list_insert(j->item, &merge_bases);
>
>  	merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
> -	printf("%s\n", oid_to_hex(&result.tree->object.oid));
> +
>  	if (result.clean < 0)
>  		die(_("failure to merge"));
> -	else if (!result.clean)
> -		printf(_("Conflicts!\n"));
> +
> +	if (o->show_messages == -1)
> +		o->show_messages = !result.clean;
> +
> +	printf("%s\n", oid_to_hex(&result.tree->object.oid));
> +	if (o->show_messages) {
> +		printf("\n");
> +		merge_display_update_messages(&opt, &result, stdout);
> +	}

Excellent.

>  	merge_finalize(&opt, &result);
>  	return !result.clean; /* result.clean < 0 handled above */
>  }
>
>  int cmd_merge_tree(int argc, const char **argv, const char *prefix)
>  {
> -	struct merge_tree_options o = { 0 };
> +	struct merge_tree_options o = { .show_messages = -1 };
>  	int expected_remaining_argc;
> +	int original_argc;
>
>  	const char * const merge_tree_usage[] = {
> -		N_("git merge-tree [--write-tree] <branch1> <branch2>"),
> +		N_("git merge-tree [--write-tree] [<options>] <branch1> <branch2>"),
>  		N_("git merge-tree [--trivial-merge] <base-tree> <branch1> <branch2>"),
>  		NULL
>  	};
> @@ -464,6 +473,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
>  			 N_("do a real merge instead of a trivial merge")),
>  		OPT_BOOL(0, "trivial-merge", &o.trivial,
>  			 N_("do a trivial merge only")),
> +		OPT_BOOL(0, "messages", &o.show_messages,
> +			 N_("also show informational/conflict messages")),
>  		OPT_END()
>  	};
>
> @@ -472,10 +483,13 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
>  		usage_with_options(merge_tree_usage, mt_options);
>
>  	/* Parse arguments */
> +	original_argc = argc;
>  	argc = parse_options(argc, argv, prefix, mt_options,
>  			     merge_tree_usage, 0);
>  	if (o.real && o.trivial)
>  		die(_("--write-tree and --trivial-merge are incompatible"));
> +	if (!o.real && original_argc < argc)
> +		die(_("--write-tree must be specified if any other options are"));

Hmm. Well. Hmm.


I'd rather keep `--write-tree` neat and optional. What's wrong with
allowing

	git merge-tree --no-messages HEAD MERGE_HEAD

?

To be clear, I think we need this instead:

	if (o.trivial && o.show_messages >= 0)
		die(_("--trivial-merge is incompatible with additional options"));

I like the rest of the patch very much!

Thank you,
Dscho

>  	if (o.real || o.trivial) {
>  		expected_remaining_argc = (o.real ? 2 : 3);
>  		if (argc != expected_remaining_argc)
> diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh
> index e03688515c5..c34f8e6c1ed 100755
> --- a/t/t4301-merge-tree-real.sh
> +++ b/t/t4301-merge-tree-real.sh
> @@ -84,4 +84,25 @@ test_expect_success 'Barf on too many arguments' '
>  	grep "^usage: git merge-tree" expect
>  '
>
> +test_expect_success 'test conflict notices and such' '
> +	test_expect_code 1 git merge-tree --write-tree side1 side2 >out &&
> +	sed -e "s/[0-9a-f]\{40,\}/HASH/g" out >actual &&
> +
> +	# Expected results:
> +	#   "greeting" should merge with conflicts
> +	#   "numbers" should merge cleanly
> +	#   "whatever" has *both* a modify/delete and a file/directory conflict
> +	cat <<-EOF >expect &&
> +	HASH
> +
> +	Auto-merging greeting
> +	CONFLICT (content): Merge conflict in greeting
> +	Auto-merging numbers
> +	CONFLICT (file/directory): directory in the way of whatever from side1; moving it to whatever~side1 instead.
> +	CONFLICT (modify/delete): whatever~side1 deleted in side2 and modified in side1.  Version side1 of whatever~side1 left in tree.
> +	EOF
> +
> +	test_cmp expect actual
> +'
> +
>  test_done
> --
> gitgitgadget
>
>