merge-tree: accept 3 trees as arguments

When specifying a merge base explicitly, there is actually no good reason why the inputs need to be commits: that's only needed if the merge base has to be deduced from the commit graph.

This commit is best viewed with `--color-moved
--color-moved-ws=allow-indentation-change`.

Signed-off-by Johannes Schindelin <[email protected]>

---

From: Johannes Schindelin <[email protected]>

When specifying a merge base explicitly, there is actually no good reason why the inputs need to be commits: that's only needed if the merge base has to be deduced from the commit graph.

I was asked to implement this at $dayjob and it seems like a feature that might be useful to other users, too.

builtin/merge-tree.c +18 -17

index 3bdec53fbe5..cbd8e15af6d 100644
@@ -429,35 +429,43 @@ static int real_merge(struct merge_tree_options *o,
 	struct merge_options opt;
 
 	copy_merge_options(&opt, &o->merge_options);
-	parent1 = get_merge_parent(branch1);
-	if (!parent1)
-		help_unknown_ref(branch1, "merge-tree",
-				 _("not something we can merge"));
-
-	parent2 = get_merge_parent(branch2);
-	if (!parent2)
-		help_unknown_ref(branch2, "merge-tree",
-				 _("not something we can merge"));
-
 	opt.show_rename_progress = 0;
 
 	opt.branch1 = branch1;
 	opt.branch2 = branch2;
 
 	if (merge_base) {
-		struct commit *base_commit;
 		struct tree *base_tree, *parent1_tree, *parent2_tree;
 
-		base_commit = lookup_commit_reference_by_name(merge_base);
-		if (!base_commit)
-			die(_("could not lookup commit '%s'"), merge_base);
+		/*
+		 * We actually only need the trees because we already
+		 * have a merge base.
+		 */
+		struct object_id base_oid, head_oid, merge_oid;
+
+		if (repo_get_oid_treeish(the_repository, merge_base, &base_oid))
+			die(_("could not parse as tree '%s'"), merge_base);
+		base_tree = parse_tree_indirect(&base_oid);
+		if (repo_get_oid_treeish(the_repository, branch1, &head_oid))
+			die(_("could not parse as tree '%s'"), branch1);
+		parent1_tree = parse_tree_indirect(&head_oid);
+		if (repo_get_oid_treeish(the_repository, branch2, &merge_oid))
+			die(_("could not parse as tree '%s'"), branch2);
+		parent2_tree = parse_tree_indirect(&merge_oid);
 
 		opt.ancestor = merge_base;
-		base_tree = repo_get_commit_tree(the_repository, base_commit);
-		parent1_tree = repo_get_commit_tree(the_repository, parent1);
-		parent2_tree = repo_get_commit_tree(the_repository, parent2);
 		merge_incore_nonrecursive(&opt, base_tree, parent1_tree, parent2_tree, &result);
 	} else {
+		parent1 = get_merge_parent(branch1);
+		if (!parent1)
+			help_unknown_ref(branch1, "merge-tree",

t/t4301-merge-tree-write-tree.sh +0 -0

index 12ac4368736..71f21bb834f 100755

From: Johannes Schindelin <[email protected]>

When specifying a merge base explicitly, there is actually no good reason why the inputs need to be commits: that's only needed if the merge base has to be deduced from the commit graph.

This commit is best viewed with `--color-moved
--color-moved-ws=allow-indentation-change`.

From: Johannes Schindelin <[email protected]>

When specifying a merge base explicitly, there is actually no good reason why the inputs need to be commits: that's only needed if the merge base has to be deduced from the commit graph.

This commit is best viewed with `--color-moved
--color-moved-ws=allow-indentation-change`.

Signed-off-by Johannes Schindelin <[email protected]>

---
merge-tree: accept 3 trees as arguments

I was asked to implement this at $dayjob and it seems like a feature that might be useful to other users, too.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1647%2Fdscho%2Fallow-merge-tree-to-accept-3-trees-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1647/dscho/allow-merge-tree-to-accept-3-trees-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1647

 Documentation/git-merge-tree.txt |  5 +++-
 builtin/merge-tree.c             | 42 +++++++++++++++++++-------------
 t/t4301-merge-tree-write-tree.sh |  8 ++++++
 3 files changed, 37 insertions(+), 18 deletions(-)

Documentation/git-merge-tree.txt +0 -0

index b50acace3bc..214e30c70ba 100644

Documentation/git-merge-tree.txt +3 -0

        Instead of finding the merge-bases for <branch1> and <branch2>,
        specify a merge-base for the merge, and specifying multiple bases is
        currently not supported. This option is incompatible with `--stdin`.
++
+As the merge-base is provided directly, <branch1> and <branch2> do not need
+o specify commits; it is sufficient if they specify trees.
 
 [[OUTPUT]]
 OUTPUT

builtin/merge-tree.c +0 -4

index 3bdec53fbe5..cbd8e15af6d 100644
@@ -429,35 +429,43 @@ static int real_merge(struct merge_tree_options *o,
        struct merge_options opt;
 
        copy_merge_options(&opt, &o->merge_options);
-       parent1 = get_merge_parent(branch1);
-       if (!parent1)
-               help_unknown_ref(branch1, "merge-tree",
-                                _("not something we can merge"));

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

bbbbbbb

Quoted 2 lines

---merge-base=<commit>::
+--merge-base=<tree-ish>::

A very minor point, but any chance we can just use <tree>, like git-restore does? I've never liked the '-ish' that we use as it seems to throw users, and I think they understand that they can use a commit or a tag where a tree is expected

Quoted 2 lines

        opt.branch1 = branch1;
        opt.branch2 = branch2;

If branch1 and branch2 refer to trees, then when users hit conflicts they'll see e.g.

<<<<<<< aaaaaaa
somecode();
=======
othercode();

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

bbbbbbb

but aaaaaaa and bbbbbbb are not commits that they can find.

That raises the question: if the user passes trees in, should we require helpful names be provided as additional parameters?

Or are
the usecases such that we don't expect callers to have any useful information about where these trees come from and these suboptimal conflicts are the best we can do?

Quoted 14 lines · Johannes Schindelin via GitGitGadget

 Documentation/git-merge-tree.txt |  5 +++-
 builtin/merge-tree.c             | 42 +++++++++++++++++++-------------
 t/t4301-merge-tree-write-tree.sh |  8 ++++++
 3 files changed, 37 insertions(+), 18 deletions(-)

Documentation/git-merge-tree.txt +0 -0

index b50acace3bc..214e30c70ba 100644
@@ -64,10 +64,13 @@ OPTIONS
        share no common history.  This flag can be given to override that
        check and make the merge proceed anyway.
 

Documentation/git-merge-tree.txt OPTIONS · line 67

Quoted 2 lines

Documentation/git-merge-tree.txt +1 -1

---merge-base=<commit>::
+--merge-base=<tree-ish>::

A very minor point, but any chance we can just use <tree>, like git-restore does? I've never liked the '-ish' that we use as it seems to throw users, and I think they understand that they can use a commit or a tag where a tree is expected

Quoted 12 lines

builtin/merge-tree.c +0 -4

index 3bdec53fbe5..cbd8e15af6d 100644
@@ -429,35 +429,43 @@ static int real_merge(struct merge_tree_options *o,
        struct merge_options opt;
 
        copy_merge_options(&opt, &o->merge_options);
-       parent1 = get_merge_parent(branch1);
-       if (!parent1)
-               help_unknown_ref(branch1, "merge-tree",
-                                _("not something we can merge"));

builtin/merge-tree.c static int real_merge(struct merge_tree_options *o, · line 435

Quoted 10 lines

builtin/merge-tree.c +0 -6

-
-       parent2 = get_merge_parent(branch2);
-       if (!parent2)
-               help_unknown_ref(branch2, "merge-tree",
-                                _("not something we can merge"));
-
        opt.show_rename_progress = 0;
 
        opt.branch1 = branch1;
        opt.branch2 = branch2;

If branch1 and branch2 refer to trees, then when users hit conflicts they'll see e.g.

<<<<<<< aaaaaaa
somecode();
=======
othercode();

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

bbbbbbb

but aaaaaaa and bbbbbbb are not commits that they can find.

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 5 lines · writes:

From: Johannes Schindelin <[email protected]>

When specifying a merge base explicitly, there is actually no good reason why the inputs need to be commits: that's only needed if the merge base has to be deduced from the commit graph.

yes, Yes, YES, YEAHHHHH!

Quoted 5 lines

t/t4301-merge-tree-write-tree.sh +0 -0

index 12ac4368736..71f21bb834f 100755

t/t4301-merge-tree-write-tree.sh test_expect_success 'check the input format when --stdin is passed' ' · line 954

Quoted 10 lines

t/t4301-merge-tree-write-tree.sh +7 -0

@@ -945,4 +945,12 @@ test_expect_success 'check the input format when --stdin is passed' '
 	test_cmp expect actual
 '
 
+test_expect_success '--merge-base with tree OIDs' '
+	git merge-tree --merge-base=side1^ side1 side3 >tree &&
+	tree=$(cat tree) &&
+	git merge-tree --merge-base=side1^^{tree} side1^{tree} side3^{tree} >tree2 &&
+	tree2=$(cat tree2) &&
+	test $tree = $tree2
+'

You do not need $tree and $tree2 variables that would make it harder to diagnose a failure case when we break merge-tree. You have tree and tree2 as files, and I think it is sufficient to do

	git merge-tree ... >result-from-commits &&
	git merge-tree ... >result-from-trees &&
	test_cmp result-from-commits result-from-trees

Quoted 6 lines

Quoted 2 lines

        opt.branch1 = branch1;
        opt.branch2 = branch2;

If branch1 and branch2 refer to trees, then when users hit conflicts they'll see e.g.

Quoted 7 lines

<<<<<<< aaaaaaa
somecode();
=======
othercode();

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

bbbbbbb

but aaaaaaa and bbbbbbb are not commits that they can find.

Correct. They are what they fed as two trees to be merged, aren't they? They (or the script that drives merge-tree and fed these two trees) should be recognise these two trees, as long as they are told that is what we show, no?

Quoted 2 lines

That raises the question: if the user passes trees in, should we require helpful names be provided as additional parameters?

If the user wants to give human-readable name to override whatever the code would internally produce, such new options may help them, and should probably apply equally to input that happens to be a commit (or a tag that is a tree-ish) as well, I would think.

Quoted 4 lines

Or are
the usecases such that we don't expect callers to have any useful information about where these trees come from and these suboptimal conflicts are the best we can do?

I do not necessarily think the output is "suboptimal" from the point of view of our users, exactly because that is what they fed into the command themselves.

Hi Elijah,

Quoted 3 lines · Elijah Newren

Quoted 1 line · Johannes Schindelin via GitGitGadget

That's funny: I asked Victoria Dye to look over the patch, and she pointed out the exact opposite: I had written <tree> and she remarked that most of Git's manual pages would call this a <tree-ish> :-)

On another funny note, I tried to establish the term "ent" for this category almost 222 months ago because I also disliked the "-ish" convention:
https://lore.kernel.org/git/Pine.LNX.4.63.0508051655480.8418@wgmdd8.biozentrum.uni-wuerzburg.de/

Quoted 32 lines

Quoted 12 lines

builtin/merge-tree.c +0 -4

index 3bdec53fbe5..cbd8e15af6d 100644
@@ -429,35 +429,43 @@ static int real_merge(struct merge_tree_options *o,
        struct merge_options opt;
 
        copy_merge_options(&opt, &o->merge_options);
-       parent1 = get_merge_parent(branch1);
-       if (!parent1)
-               help_unknown_ref(branch1, "merge-tree",
-                                _("not something we can merge"));

builtin/merge-tree.c static int real_merge(struct merge_tree_options *o, · line 435

Quoted 10 lines

builtin/merge-tree.c +0 -6

-
-       parent2 = get_merge_parent(branch2);
-       if (!parent2)
-               help_unknown_ref(branch2, "merge-tree",
-                                _("not something we can merge"));
-
        opt.show_rename_progress = 0;
 
        opt.branch1 = branch1;
        opt.branch2 = branch2;

If branch1 and branch2 refer to trees, then when users hit conflicts they'll see e.g.

<<<<<<< aaaaaaa
somecode();
=======
othercode();

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

bbbbbbb

Quoted 1 line

but aaaaaaa and bbbbbbb are not commits that they can find.

That is true. And it is not totally obvious to many users that they could then call git show aaaaaaa:file to see the full pre-image on the first-parent side.

On the other hand, the label that is shown is precisely what the user specified on the command-line.

For example:

        $ git merge-tree --merge-base=v2.42.0:t v2.43.0~11:t v2.43.0~10^2:t

will result in the following conflict markers:

        $ git show 021c3ce211:t0091-bugreport.sh
        [...]
        <<<<<<< v2.43.0~11:t
                grep usage output &&
                test_path_is_missing git-bugreport-*
        '

        test_expect_success 'incorrect positional arguments abort with usage and hint' '
                test_must_fail git bugreport false 2>output &&
                grep usage output &&
                grep false output &&
        =======
                test_grep usage output &&
        >>>>>>> v2.43.0~10^2:t
        [...]

which I personally find very pleasing output.

Besides, the manual page of git merge-tree says in no sugar-coated words:

Do NOT look through the resulting toplevel tree to try to find which files conflict; [...]

:-)

Quoted 6 lines · Junio C Hamano

...

Quoted 1 line

but aaaaaaa and bbbbbbb are not commits that they can find.

Correct. They are what they fed as two trees to be merged, aren't they? They (or the script that drives merge-tree and fed these two trees) should be recognise these two trees, as long as they are told

Quoted 1 line

that is what we show, no?

...
Yeah, I was worried though that the end user wasn't the one running the git-merge-tree command, and was trying to dig more into usage cases.