merge-tree.c: add --merge-base=<commit> option

From: Kyle Zhao <[email protected]>

This option allows users to specify a merge-base commit for the merge.

It will give our callers more flexibility to use the git merge-tree. For example:

    git merge-tree --merge-base=<sha1>^1 source-branch <sha1>

This allows us to implement git cherry-pick in bare repositories.

---
merge-tree.c: add --merge-base= option

Thanks for Elijah's work. I'm very excited that merge-ort is integrated into the git merge-tree, which means that we can use merge-ort in bare repositories to optimize merge performance.

In this patch, I introduce a new --merge-base=<commit> option to allow callers to specify a merge-base for the merge. This may allow users to implement git cherry-pick and git rebase in bare repositories with git merge-tree cmd.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1397%2Fkeyu98%2Fkz%2Fmerge-tree-option-merge-base-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1397/keyu98/kz/merge-tree-option-merge-base-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1397

 Documentation/git-merge-tree.txt |  4 +++
 builtin/merge-tree.c             | 23 +++++++++++++----
 t/t4301-merge-tree-write-tree.sh | 44 ++++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 5 deletions(-)

index d6c356740ef..e762209b76d 100644
@@ -64,6 +64,10 @@ OPTIONS
 	share no common history.  This flag can be given to override that
 	check and make the merge proceed anyway.
 
+--merge-base=<commit>::
+	Instead of finding the merge-bases for <branch1> and <branch2>,
+	specify a merge-base for the merge.
+
 [[OUTPUT]]
 OUTPUT
 ------

index ae5782917b9..adc461f00f3 100644
@@ -3,6 +3,7 @@
 #include "tree-walk.h"
 #include "xdiff-interface.h"
 #include "help.h"
+#include "commit.h"
 #include "commit-reach.h"
 #include "merge-ort.h"
 #include "object-store.h"

@@ -402,6 +403,7 @@ struct merge_tree_options {
 	int allow_unrelated_histories;
 	int show_messages;
 	int name_only;
+	char* merge_base;
 };
 
 static int real_merge(struct merge_tree_options *o,

@@ -430,11 +432,18 @@ static int real_merge(struct merge_tree_options *o,
 	opt.branch1 = branch1;
 	opt.branch2 = branch2;
 
-	/*
-	 * Get the merge bases, in reverse order; see comment above
-	 * merge_incore_recursive in merge-ort.h
-	 */
-	merge_bases = get_merge_bases(parent1, parent2);
+	if (o->merge_base) {
+		struct commit *c = lookup_commit_reference_by_name(o->merge_base);
+		if (!c)
+			die(_("could not lookup commit %s"), o->merge_base);
+		commit_list_insert(c, &merge_bases);
+	} else {
+		/*
+		 * Get the merge bases, in reverse order; see comment above
+		 * merge_incore_recursive in merge-ort.h
+		 */
+		merge_bases = get_merge_bases(parent1, parent2);
+	}
 	if (!merge_bases && !o->allow_unrelated_histories)
 		die(_("refusing to merge unrelated histories"));
 	merge_bases = reverse_commit_list(merge_bases);

@@ -505,6 +514,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 			   &o.allow_unrelated_histories,
 			   N_("allow merging unrelated histories"),
 			   PARSE_OPT_NONEG),
+		OPT_STRING(0, "merge-base",
+			 &o.merge_base,
+			 N_("commit"),
+			 N_("specify a merge-base commit for the merge")),
 		OPT_END()
 	};
 

base-commit: db29e6bbaed156ae9025ff0474fd788e58230367

gitgitgadget

[...]

This was my thought too; but would we be painting ourselves into a corner if we ever want to make merge-tree support octopus merges?

Also, why did you write $(git merge-base branch^ branch) rather than just branch^ ?

Yeah, I don't think that'd be too hard...if we could rule out ever supporting octopus merges in merge-tree (which I'm not so sure is a good assumption). Otherwise, we might need to figure out the appropriate backward-compatible input parsing (and output format changes?)

True, the above suggestion was especially bad as it would not be possible to extend to support multiple bases *and* octopus at the same time. Consider it scrapped. Taking zero or more --base-commit options explicitly from the command line would be a better interface.

Just to be explicit which one is doing what.

I'd prefer an approach to tackle one thing at a time while making sure we leave the door open for the future ones. I think the backend interface from "merge" to external strategies use a separator to signal the boundary between the heads being merged (i.e. branchN above) and the bases being used, which is easy to parse and support multiple bases and octopus at the same time.

As to making it easy to implement "cherry-pick", I do not think you should dogmatically forbid it on the basis for merge-tree from the command line is inherently one mergy operation per invocation. You will have the --stdin interface, and a way forward may be a notation to refer to previous results in the --stdin input stream. That way, a single invocation of merge-tree "server" can be fed a sequence of 3-way merges that are actually steps of multi-commit cherry-pick, that merge the differences of multiple commits on top of the previous result, one step at a time.

IOW, as long as you make sure --stdin interface is rich enough to allow you do what people would do a sequence of "git merge-tree" single shot invocations (perhaps even driven by a script), you would be OK, I would think.

Ooh, the separator idea sounds like a good solution.

I agree that this kind of functionality probably makes sense for inclusion from a completeness perspective, but I still maintain that if its sole purpose is implementing a cherry-pick command then it's at best an ill-advised or interim hack and we should instead do something better. While combining this patch with --stdin does solve the "single operation per invocation" issue, that's only one of six issues I listed in my email to Kyle about why this is the wrong base for building a cherry-pick alternative.

Thanks for your reviews.

Referring to Newren's reply, if we need to implement octopus merges for git-merge-tree in the future, still need a new option, so I haven't modified it yet.

Done.

Hi Kyle!

Thanks for the interest in this area of the code. There's lots of interesting history you're probably not aware of here...

Note that this has been implemented previously. I may have implemented it previously and ripped it out (about a year ago), or maybe I just avoided it once I ran across the reasons below. Dscho also implemented it over at
https://lore.kernel.org/git/[email protected]/

You may want to take a look at his code for comparison, especially the merge_incore_nonrecursive() stuff.

This capability is something we may ultimately want for completeness, but it definitely should not be marketed as a building block for implementing cherry-pick or rebase. There are several reasons for that:

Performance reasons:
* it's a design with a separate fork for every commit to be picked,
which is quite wasteful. cherry-pick and rebase should do better.
* it is explicitly single-commit replaying, which discards the
remembering renames optimization I placed into merge-ort. We should have something series-aware so it can take advantage of that.

Usability/Design reasons:
* with cherry-picking or rebasing you often have a sequence of
commits. Picking them one at a time not only has the performance issues above, but also introduces usability and design questions around the creation of commits from the toplevel trees created by merge-tree. For normal merges, we have no idea what the commit message should be without explicit directives and user editing; this is reflected in the fact that git merge has 9 different flags for controlling how to specify the commit message or components of it or how to tweak it or whatever. I really don't want to copy all of that logic from git-merge into git-merge-tree, and merge-tree is therefore just a dry-run merge. However, in the case of cherry-picking or rebasing, we *do* have a commit message (and author) that we can just re-use. But if we make merge-tree handle the commit message and create commits, you are fundamentally changing the output style of merge-tree that we have carefully documented (and promised backward compatibility on). Figuring out how to extend the output and when (and do we also allow that for non-specified merge bases) probably requires some careful thought.
* a cherry-pick or rebase tool actually might want to do something
clever with sequences of replayed commits. For example, if later commits in the series have commit messages that refer to earlier commits in the series by their (abbreviated) commit hash, it would be nice to update those (abbreviated) commit hashes to instead refer to the replayed version of the earlier commit. But that fundamentally requires operating on a series rather than an individual commit.

Correctness/capability reason:
* this patch is fundamentally limited to replaying regular commits.
Replaying merge-commits correctly doesn't fit within this design; it needs a different framework.

Maintenance reason:
* suggesting this code as a basis for cherry-pick or rebase is
likely to lead to yet another shell-scripted rebase; we've been trying to generally nuke shell scripts from Git for a variety of reasons. It'd be sad to regress here.

My apologies for having very limited git time (which is often entirely used up just in reviewing/responding to patches and other queries on the list instead of on new features like this, or maybe on making some nice slides for a conference); if I had more time, I think git-replay could have easily been done many months ago (or perhaps even last year). Then you'd have the high level tool you need for server side cherry-picking and rebasing. But, I haven't had much time. So, it makes sense that folks might want to use an interim hack while waiting for the more robust tool to materialize.

merge_incore_nonrecursive() stuff.

Thanks. I'll take a look later.

Thank you for your very detailed explanation, I feel like I have a deeper understanding of rebase and cherry-pick.

git-replay sounds great, looking forward to it one day.

Done.

Done. I've changed it to merge_incore_recursive(). It works, but I don't know if it's written correctly, especially opt.ancestor.

Done.

It's useful, which makes the test clearer.

From: Kyle Zhao <[email protected]>

This patch will give our callers more flexibility to use git merge-tree, such as:

    git merge-tree --write-tree --merge-base=branch^ HEAD branch

It would cherry-pick the commit at the tip of the branch on top of the current commit even if the repository is bare.

---
merge-tree: allow specifying a base commit when --write-tree is passed

Thanks for Elijah's work. I'm very excited that merge-ort is integrated into the git merge-tree, which means that we can use merge-ort in bare repositories to optimize merge performance.

In this patch, I introduce a new --merge-base=<commit> option to allow callers to specify a merge-base for the merge. This may allow users to implement git cherry-pick and git rebase in bare repositories with git merge-tree cmd.

Changes since v1:

* Changed merge_incore_nonrecursive() to merge_incore_recursive() when merge-base is specified.
* Fixed c style problem.
* Moved commit lookup/die logic out to the parsing logic in cmd_merge_tree().
* use test_commit for test

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1397%2Fkeyu98%2Fkz%2Fmerge-tree-option-merge-base-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1397/keyu98/kz/merge-tree-option-merge-base-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1397

Range-diff vs v1:

1: 965e544c849 ! 1: ab4e5d5ad08 merge-tree.c: add --merge-base=<commit> option

     @@ Metadata
       ## Commit message ##
          merge-tree.c: add --merge-base=<commit> option
      
     -    This option allows users to specify a merge-base commit for the merge.
     +    This patch will give our callers more flexibility to use `git merge-tree`,
     +    such as:
      
     -    It will give our callers more flexibility to use the `git merge-tree`.
     -    For example:
     +        git merge-tree --write-tree --merge-base=branch^ HEAD branch
      
     -        git merge-tree --merge-base=<sha1>^1 source-branch <sha1>
     -
     -    This allows us to implement `git cherry-pick` in bare repositories.
     +    It would cherry-pick the commit at the tip of the branch on top of the
     +    current commit even if the repository is bare.
      
          Signed-off-by: Kyle Zhao <[email protected]>
      
     @@ builtin/merge-tree.c: struct merge_tree_options {
       	int allow_unrelated_histories;
       	int show_messages;
       	int name_only;
     -+	char* merge_base;
     ++	const struct commit *base_commit;
       };
       
       static int real_merge(struct merge_tree_options *o,
     @@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
      -	 * merge_incore_recursive in merge-ort.h
      -	 */
      -	merge_bases = get_merge_bases(parent1, parent2);
     -+	if (o->merge_base) {
     -+		struct commit *c = lookup_commit_reference_by_name(o->merge_base);
     -+		if (!c)
     -+			die(_("could not lookup commit %s"), o->merge_base);
     -+		commit_list_insert(c, &merge_bases);
     +-	if (!merge_bases && !o->allow_unrelated_histories)
     +-		die(_("refusing to merge unrelated histories"));
     +-	merge_bases = reverse_commit_list(merge_bases);
     ++	if (o->base_commit) {
     ++		struct tree *base_tree, *parent1_tree, *parent2_tree;
     ++
     ++		opt.ancestor = "specified merge base";
     ++		base_tree = get_commit_tree(o->base_commit);
     ++		parent1_tree = get_commit_tree(parent1);
     ++		parent2_tree = get_commit_tree(parent2);
     ++		merge_incore_nonrecursive(&opt, base_tree, parent1_tree, parent2_tree, &result);
      +	} else {
      +		/*
      +		 * Get the merge bases, in reverse order; see comment above
      +		 * merge_incore_recursive in merge-ort.h
      +		 */
      +		merge_bases = get_merge_bases(parent1, parent2);
     ++		if (!merge_bases && !o->allow_unrelated_histories)
     ++			die(_("refusing to merge unrelated histories"));
     ++		merge_bases = reverse_commit_list(merge_bases);
     ++		merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
      +	}
     - 	if (!merge_bases && !o->allow_unrelated_histories)
     - 		die(_("refusing to merge unrelated histories"));
     - 	merge_bases = reverse_commit_list(merge_bases);
     + 
     +-	merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
     + 	if (result.clean < 0)
     + 		die(_("failure to merge"));
     + 
     +@@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
     + 
     + int cmd_merge_tree(int argc, const char **argv, const char *prefix)
     + {
     ++	const char *merge_base = NULL;
     + 	struct merge_tree_options o = { .show_messages = -1 };
     + 	int expected_remaining_argc;
     + 	int original_argc;
      @@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char *prefix)
       			   &o.allow_unrelated_histories,
       			   N_("allow merging unrelated histories"),
       			   PARSE_OPT_NONEG),
      +		OPT_STRING(0, "merge-base",
     -+			 &o.merge_base,
     ++			 &merge_base,
      +			 N_("commit"),
     -+			 N_("specify a merge-base commit for the merge")),
     ++			 N_("specify a merge-base for the merge")),
       		OPT_END()
       	};
       
     +@@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char *prefix)
     + 		usage_with_options(merge_tree_usage, mt_options);
     + 
     + 	/* Do the relevant type of merge */
     +-	if (o.mode == MODE_REAL)
     ++	if (o.mode == MODE_REAL) {
     ++		if (merge_base) {
     ++			o.base_commit = lookup_commit_reference_by_name(merge_base);
     ++			if (!o.base_commit)
     ++				die(_("could not lookup commit %s"), merge_base);
     ++		}
     + 		return real_merge(&o, argv[0], argv[1], prefix);
     ++	}
     + 	else
     + 		return trivial_merge(argv[0], argv[1], argv[2]);
     + }
      
       ## t/t4301-merge-tree-write-tree.sh ##
      @@ t/t4301-merge-tree-write-tree.sh: test_expect_success SANITY 'merge-ort fails gracefully in a read-only repository
       	test_must_fail git -C read-only merge-tree side1 side2
       '
       
     -+# specify merge-base as parent of branch2.
     -+# git merge-tree --merge-base=A O B
     -+#   Commit O: foo, bar
     -+#   Commit A: modify foo after Commit O
     -+#   Commit B: modify bar after Commit A
     -+#   Expected: foo is unchanged, modify bar
     -+
      +test_expect_success 'specify merge-base as parent of branch2' '
      +	# Setup
      +	git init base-b2-p && (
      +		cd base-b2-p &&
     -+		echo foo >foo &&
     -+		echo bar >bar &&
     -+		git add foo bar &&
     -+		git commit -m O &&
     -+
     -+		git branch O &&
     -+		git branch A &&
     -+
     -+		git checkout A &&
     -+		echo "A" >foo &&
     -+		git add foo &&
     -+		git commit -m A &&
     -+
     -+		git checkout -b B &&
     -+		echo "B" >bar &&
     -+		git add bar &&
     -+		git commit -m B
     ++		test_commit c1 file1 &&
     ++		test_commit c2 file2 &&
     ++		test_commit c3 file3
      +	) &&
      +	# Testing
      +	(
      +		cd base-b2-p &&
     -+		TREE_OID=$(git merge-tree --merge-base=A O B) &&
     ++		TREE_OID=$(git merge-tree --write-tree --merge-base=c2 c1 c3) &&
      +
      +		q_to_tab <<-EOF >expect &&
     -+		100644 blob $(git rev-parse B:bar)Qbar
     -+		100644 blob $(git rev-parse O:foo)Qfoo
     ++		100644 blob $(git rev-parse c1:file1)Qfile1
     ++		100644 blob $(git rev-parse c3:file3)Qfile3
      +		EOF
      +
      +		git ls-tree $TREE_OID >actual &&

 Documentation/git-merge-tree.txt |  4 +++
 builtin/merge-tree.c             | 43 ++++++++++++++++++++++++--------
 t/t4301-merge-tree-write-tree.sh | 23 +++++++++++++++++
 3 files changed, 60 insertions(+), 10 deletions(-)

index d6c356740ef..e762209b76d 100644
@@ -64,6 +64,10 @@ OPTIONS
 	share no common history.  This flag can be given to override that
 	check and make the merge proceed anyway.
 
+--merge-base=<commit>::
+	Instead of finding the merge-bases for <branch1> and <branch2>,
+	specify a merge-base for the merge.
+
 [[OUTPUT]]
 OUTPUT
 ------

index 013b77144bd..64bfe6f4a41 100755
@@ -819,4 +819,27 @@ test_expect_success SANITY 'merge-ort fails gracefully in a read-only repository
 	test_must_fail git -C read-only merge-tree side1 side2
 '
 
+test_expect_success 'specify merge-base as parent of branch2' '
+	# Setup
+	git init base-b2-p && (
+		cd base-b2-p &&
+		test_commit c1 file1 &&
+		test_commit c2 file2 &&
+		test_commit c3 file3
+	) &&
+	# Testing
+	(
+		cd base-b2-p &&
+		TREE_OID=$(git merge-tree --write-tree --merge-base=c2 c1 c3) &&
+
+		q_to_tab <<-EOF >expect &&
+		100644 blob $(git rev-parse c1:file1)Qfile1
+		100644 blob $(git rev-parse c3:file3)Qfile3
+		EOF
+
+		git ls-tree $TREE_OID >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done

base-commit: 5af5e54106e20f65c913550c80aec3186b859e9b

gitgitgadget

Hi,

Perhaps just "This does a merge of HEAD and branch, but uses branch^ as the merge-base."

Also, given that both Junio and I thought a positional argument might be better, it might be worth calling out that the reason for using an option flag instead of a positional argument is to allow additional commits passed to merge-tree to be handled via an octopus merge in the future.

I think you mean the other way around (using merge_incore_nonrecursive() instead of merge_incore_recursive() when merge-base is specified).

This should be rebased on top of en/merge-tree-sequence. Then, you need to either figure out how to modify the new --stdin flag to also accept a specified merge base in a way that permits future handling of octopus merges (it looks like Junio might have a good suggestion elsewhere in this thread), or else explicitly call out in the commit message why specified merge base support is only being added when --stdin is not specified.

From: Kyle Zhao <[email protected]>

This patch will give our callers more flexibility to use git merge-tree, such as:

    git merge-tree --write-tree --merge-base=branch^ HEAD branch

This does a merge of HEAD and branch, but uses branch^ as the merge-base.

And the reason why using an option flag instead of a positional argument is to allow additional commits passed to merge-tree to be handled via an octopus merge in the future.

---
merge-tree: allow specifying a base commit when --write-tree is passed

Thanks for Elijah's work. I'm very excited that merge-ort is integrated into the git merge-tree, which means that we can use merge-ort in bare repositories to optimize merge performance.

In this patch, I introduce a new --merge-base=<commit> option to allow callers to specify a merge-base for the merge. This may allow users to implement git cherry-pick and git rebase in bare repositories with git merge-tree cmd.

Changes since v1:

* Changed merge_incore_recursive() to merge_incore_nonrecursive() when merge-base is specified.
* Fixed c style problem.
* Moved commit lookup/die logic out to the parsing logic in cmd_merge_tree().
* use test_commit for test

Changes since v2:

* commit message
* Rebased on top of en/merge-tree-sequence.
* Set opt.ancestor to o->merge_base. Because opt.ancestor is a *char. To make it easier to pass parameters, I moved lookup_commit_reference_by_name() to real_ merge() again.
* Added test comment.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1397%2Fkeyu98%2Fkz%2Fmerge-tree-option-merge-base-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1397/keyu98/kz/merge-tree-option-merge-base-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1397

Range-diff vs v2:

1: ab4e5d5ad08 ! 1: 554cbde49e8 merge-tree.c: add --merge-base=<commit> option

     @@ Commit message
      
              git merge-tree --write-tree --merge-base=branch^ HEAD branch
      
     -    It would cherry-pick the commit at the tip of the branch on top of the
     -    current commit even if the repository is bare.
     +    This does a merge of HEAD and branch, but uses branch^ as the merge-base.
     +
     +    And the reason why using an option flag instead of a positional argument
     +    is to allow additional commits passed to merge-tree to be handled via an
     +    octopus merge in the future.
      
          Signed-off-by: Kyle Zhao <[email protected]>
      
     @@ builtin/merge-tree.c
       #include "merge-ort.h"
       #include "object-store.h"
      @@ builtin/merge-tree.c: struct merge_tree_options {
     - 	int allow_unrelated_histories;
       	int show_messages;
       	int name_only;
     -+	const struct commit *base_commit;
     + 	int use_stdin;
     ++	const char *merge_base;
       };
       
       static int real_merge(struct merge_tree_options *o,
     @@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
      -	if (!merge_bases && !o->allow_unrelated_histories)
      -		die(_("refusing to merge unrelated histories"));
      -	merge_bases = reverse_commit_list(merge_bases);
     -+	if (o->base_commit) {
     ++	if (o->merge_base) {
     ++		struct commit *base_commit;
      +		struct tree *base_tree, *parent1_tree, *parent2_tree;
      +
     -+		opt.ancestor = "specified merge base";
     -+		base_tree = get_commit_tree(o->base_commit);
     ++		base_commit = lookup_commit_reference_by_name(o->merge_base);
     ++		if (!base_commit)
     ++			die(_("could not lookup commit %s"), o->merge_base);
     ++
     ++		opt.ancestor = o->merge_base;
     ++		base_tree = get_commit_tree(base_commit);
      +		parent1_tree = get_commit_tree(parent1);
      +		parent2_tree = get_commit_tree(parent2);
      +		merge_incore_nonrecursive(&opt, base_tree, parent1_tree, parent2_tree, &result);
     @@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
       	if (result.clean < 0)
       		die(_("failure to merge"));
       
     -@@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
     - 
     - int cmd_merge_tree(int argc, const char **argv, const char *prefix)
     - {
     -+	const char *merge_base = NULL;
     - 	struct merge_tree_options o = { .show_messages = -1 };
     - 	int expected_remaining_argc;
     - 	int original_argc;
      @@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char *prefix)
     - 			   &o.allow_unrelated_histories,
     - 			   N_("allow merging unrelated histories"),
     + 			   &o.use_stdin,
     + 			   N_("perform multiple merges, one per line of input"),
       			   PARSE_OPT_NONEG),
      +		OPT_STRING(0, "merge-base",
     -+			 &merge_base,
     -+			 N_("commit"),
     -+			 N_("specify a merge-base for the merge")),
     ++			   &o.merge_base,
     ++			   N_("commit"),
     ++			   N_("specify a merge-base for the merge")),
       		OPT_END()
       	};
       
     -@@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char *prefix)
     - 		usage_with_options(merge_tree_usage, mt_options);
     - 
     - 	/* Do the relevant type of merge */
     --	if (o.mode == MODE_REAL)
     -+	if (o.mode == MODE_REAL) {
     -+		if (merge_base) {
     -+			o.base_commit = lookup_commit_reference_by_name(merge_base);
     -+			if (!o.base_commit)
     -+				die(_("could not lookup commit %s"), merge_base);
     -+		}
     - 		return real_merge(&o, argv[0], argv[1], prefix);
     -+	}
     - 	else
     - 		return trivial_merge(argv[0], argv[1], argv[2]);
     - }
      
       ## t/t4301-merge-tree-write-tree.sh ##
     -@@ t/t4301-merge-tree-write-tree.sh: test_expect_success SANITY 'merge-ort fails gracefully in a read-only repository
     - 	test_must_fail git -C read-only merge-tree side1 side2
     +@@ t/t4301-merge-tree-write-tree.sh: test_expect_success '--stdin with both a successful and a conflicted merge' '
     + 	test_cmp expect actual
       '
       
     ++# specify merge-base as parent of branch2
     ++# git merge-tree --write-tree --merge-base=c2 c1 c3
     ++#   Commit c1: add file1
     ++#   Commit c2: add file2 after c1
     ++#   Commit c3: add file3 after c2
     ++#   Expected: add file3, and file2 does NOT appear
     ++
      +test_expect_success 'specify merge-base as parent of branch2' '
      +	# Setup
      +	git init base-b2-p && (

 Documentation/git-merge-tree.txt |  4 ++++
 builtin/merge-tree.c             | 39 ++++++++++++++++++++++++--------
 t/t4301-merge-tree-write-tree.sh | 30 ++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 9 deletions(-)

index 04bcc416e6e..d9dacb2ce54 100644
@@ -64,6 +64,10 @@ OPTIONS
 	share no common history.  This flag can be given to override that
 	check and make the merge proceed anyway.
 
+--merge-base=<commit>::
+	Instead of finding the merge-bases for <branch1> and <branch2>,
+	specify a merge-base for the merge.
+
 [[OUTPUT]]
 OUTPUT
 ------

index fe853aa8f91..cd25aac6ba6 100644
@@ -3,6 +3,7 @@
 #include "tree-walk.h"
 #include "xdiff-interface.h"
 #include "help.h"
+#include "commit.h"
 #include "commit-reach.h"
 #include "merge-ort.h"
 #include "object-store.h"

@@ -403,6 +404,7 @@ struct merge_tree_options {
 	int show_messages;
 	int name_only;
 	int use_stdin;
+	const char *merge_base;
 };
 
 static int real_merge(struct merge_tree_options *o,

@@ -432,16 +434,31 @@ static int real_merge(struct merge_tree_options *o,
 	opt.branch1 = branch1;
 	opt.branch2 = branch2;
 
-	/*
-	 * Get the merge bases, in reverse order; see comment above
-	 * merge_incore_recursive in merge-ort.h
-	 */
-	merge_bases = get_merge_bases(parent1, parent2);
-	if (!merge_bases && !o->allow_unrelated_histories)
-		die(_("refusing to merge unrelated histories"));
-	merge_bases = reverse_commit_list(merge_bases);
+	if (o->merge_base) {
+		struct commit *base_commit;
+		struct tree *base_tree, *parent1_tree, *parent2_tree;
+
+		base_commit = lookup_commit_reference_by_name(o->merge_base);
+		if (!base_commit)
+			die(_("could not lookup commit %s"), o->merge_base);
+
+		opt.ancestor = o->merge_base;
+		base_tree = get_commit_tree(base_commit);
+		parent1_tree = get_commit_tree(parent1);
+		parent2_tree = get_commit_tree(parent2);
+		merge_incore_nonrecursive(&opt, base_tree, parent1_tree, parent2_tree, &result);
+	} else {
+		/*
+		 * Get the merge bases, in reverse order; see comment above
+		 * merge_incore_recursive in merge-ort.h
+		 */
+		merge_bases = get_merge_bases(parent1, parent2);
+		if (!merge_bases && !o->allow_unrelated_histories)
+			die(_("refusing to merge unrelated histories"));
+		merge_bases = reverse_commit_list(merge_bases);
+		merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
+	}
 
-	merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
 	if (result.clean < 0)
 		die(_("failure to merge"));
 

@@ -515,6 +532,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 			   &o.use_stdin,
 			   N_("perform multiple merges, one per line of input"),
 			   PARSE_OPT_NONEG),
+		OPT_STRING(0, "merge-base",
+			   &o.merge_base,
+			   N_("commit"),
+			   N_("specify a merge-base for the merge")),
 		OPT_END()
 	};
 

index cac85591b52..5b0073d3dcd 100755
@@ -860,4 +860,34 @@ test_expect_success '--stdin with both a successful and a conflicted merge' '
 	test_cmp expect actual
 '
 
+# specify merge-base as parent of branch2
+# git merge-tree --write-tree --merge-base=c2 c1 c3
+#   Commit c1: add file1
+#   Commit c2: add file2 after c1
+#   Commit c3: add file3 after c2
+#   Expected: add file3, and file2 does NOT appear
+
+test_expect_success 'specify merge-base as parent of branch2' '
+	# Setup
+	git init base-b2-p && (
+		cd base-b2-p &&
+		test_commit c1 file1 &&
+		test_commit c2 file2 &&
+		test_commit c3 file3
+	) &&
+	# Testing
+	(
+		cd base-b2-p &&
+		TREE_OID=$(git merge-tree --write-tree --merge-base=c2 c1 c3) &&
+
+		q_to_tab <<-EOF >expect &&
+		100644 blob $(git rev-parse c1:file1)Qfile1
+		100644 blob $(git rev-parse c3:file3)Qfile3
+		EOF
+
+		git ls-tree $TREE_OID >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done

base-commit: ec1edbcb56ac05e9980299b05924c5c1b51d68b4

gitgitgadget

Hi,

[...]

This looks pretty good; thanks for the many changes. One thing is still missing: you are now building on a commit that adds a --stdin option to merge-tree, which allows the user to specify what to merge on stdin rather than via other arguments on the command line. Since you're making merge-tree support specified merge base(s), it should also support that in conjunction with --stdin. So, see the "/* Handle stdin */" block and adjust it appropriately.

Once you do that, I think your series will be good to go.

Thanks for Elijah's work. I'm very excited that merge-ort is integrated into the git merge-tree, which means that we can use merge-ort in bare repositories to optimize merge performance.

In this patch, I introduce a new --merge-base=<commit> option to allow callers to specify a merge-base for the merge. This may allow users to implement git cherry-pick and git rebase in bare repositories with git merge-tree cmd.

Changes since v1:

* Changed merge_incore_recursive() to merge_incore_nonrecursive() when merge-base is specified.
* Fixed c style problem.
* Moved commit lookup/die logic out to the parsing logic in cmd_merge_tree().
* use test_commit for test

Changes since v2:

* commit message
* Rebased on top of en/merge-tree-sequence.
* Set opt.ancestor to o->merge_base. Because opt.ancestor is a *char. To make it easier to pass parameters, I moved lookup_commit_reference_by_name() to real_ merge() again.
* Added test comment.

Changes since v3:

* support --merge-base in conjunction with --stdin

Kyle Zhao (2):
merge-tree.c: add --merge-base=<commit> option merge-tree.c: support --merge-base in conjunction with --stdin

 Documentation/git-merge-tree.txt |  5 +++
 builtin/merge-tree.c             | 64 ++++++++++++++++++++++++++------
 t/t4301-merge-tree-write-tree.sh | 51 +++++++++++++++++++++++++
 3 files changed, 108 insertions(+), 12 deletions(-)

base-commit: ec1edbcb56ac05e9980299b05924c5c1b51d68b4
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1397%2Fkeyu98%2Fkz%2Fmerge-tree-option-merge-base-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1397/keyu98/kz/merge-tree-option-merge-base-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1397

Range-diff vs v3:

1: 554cbde49e8 ! 1: bba854fc8fa merge-tree.c: add --merge-base=<commit> option

     @@ builtin/merge-tree.c
       #include "merge-ort.h"
       #include "object-store.h"
      @@ builtin/merge-tree.c: struct merge_tree_options {
     - 	int show_messages;
     - 	int name_only;
     - 	int use_stdin;
     -+	const char *merge_base;
       };
       
       static int real_merge(struct merge_tree_options *o,
     ++		      const char *merge_base,
     + 		      const char *branch1, const char *branch2,
     + 		      const char *prefix)
     + {
      @@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
       	opt.branch1 = branch1;
       	opt.branch2 = branch2;
     @@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
      -	if (!merge_bases && !o->allow_unrelated_histories)
      -		die(_("refusing to merge unrelated histories"));
      -	merge_bases = reverse_commit_list(merge_bases);
     -+	if (o->merge_base) {
     ++	if (merge_base) {
      +		struct commit *base_commit;
      +		struct tree *base_tree, *parent1_tree, *parent2_tree;
      +
     -+		base_commit = lookup_commit_reference_by_name(o->merge_base);
     ++		base_commit = lookup_commit_reference_by_name(merge_base);
      +		if (!base_commit)
     -+			die(_("could not lookup commit %s"), o->merge_base);
     ++			die(_("could not lookup commit %s"), merge_base);
      +
     -+		opt.ancestor = o->merge_base;
     ++		opt.ancestor = merge_base;
      +		base_tree = get_commit_tree(base_commit);
      +		parent1_tree = get_commit_tree(parent1);
      +		parent2_tree = get_commit_tree(parent2);
     @@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
       	if (result.clean < 0)
       		die(_("failure to merge"));
       
     +@@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char *prefix)
     + 	struct merge_tree_options o = { .show_messages = -1 };
     + 	int expected_remaining_argc;
     + 	int original_argc;
     ++	const char *merge_base = NULL;
     + 
     + 	const char * const merge_tree_usage[] = {
     + 		N_("git merge-tree [--write-tree] [<options>] <branch1> <branch2>"),
      @@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char *prefix)
       			   &o.use_stdin,
       			   N_("perform multiple merges, one per line of input"),
       			   PARSE_OPT_NONEG),
      +		OPT_STRING(0, "merge-base",
     -+			   &o.merge_base,
     ++			   &merge_base,
      +			   N_("commit"),
      +			   N_("specify a merge-base for the merge")),
       		OPT_END()
       	};
       
     +@@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char *prefix)
     + 			if (!split[0] || !split[1] || split[2])
     + 				die(_("malformed input line: '%s'."), buf.buf);
     + 			strbuf_rtrim(split[0]);
     +-			result = real_merge(&o, split[0]->buf, split[1]->buf, prefix);
     ++			result = real_merge(&o, merge_base, split[0]->buf, split[1]->buf, prefix);
     + 			if (result < 0)
     + 				die(_("merging cannot continue; got unclean result of %d"), result);
     + 			strbuf_list_free(split);
     +@@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char *prefix)
     + 
     + 	/* Do the relevant type of merge */
     + 	if (o.mode == MODE_REAL)
     +-		return real_merge(&o, argv[0], argv[1], prefix);
     ++		return real_merge(&o, merge_base, argv[0], argv[1], prefix);
     + 	else
     + 		return trivial_merge(argv[0], argv[1], argv[2]);
     + }
      
       ## t/t4301-merge-tree-write-tree.sh ##
      @@ t/t4301-merge-tree-write-tree.sh: test_expect_success '--stdin with both a successful and a conflicted merge' '

-: ----------- > 2: db47fbc663e merge-tree.c: support --merge-base in conjunction with --stdin

gitgitgadget

From: Kyle Zhao <[email protected]>

This patch will give our callers more flexibility to use git merge-tree, such as:

    git merge-tree --write-tree --merge-base=branch^ HEAD branch

This does a merge of HEAD and branch, but uses branch^ as the merge-base.

And the reason why using an option flag instead of a positional argument is to allow additional commits passed to merge-tree to be handled via an octopus merge in the future.

---
 Documentation/git-merge-tree.txt |  4 +++
 builtin/merge-tree.c             | 44 ++++++++++++++++++++++++--------
 t/t4301-merge-tree-write-tree.sh | 30 ++++++++++++++++++++++
 3 files changed, 67 insertions(+), 11 deletions(-)

index 04bcc416e6e..d9dacb2ce54 100644
@@ -64,6 +64,10 @@ OPTIONS
 	share no common history.  This flag can be given to override that
 	check and make the merge proceed anyway.
 
+--merge-base=<commit>::
+	Instead of finding the merge-bases for <branch1> and <branch2>,
+	specify a merge-base for the merge.
+
 [[OUTPUT]]
 OUTPUT
 ------

index cac85591b52..5b0073d3dcd 100755
@@ -860,4 +860,34 @@ test_expect_success '--stdin with both a successful and a conflicted merge' '
 	test_cmp expect actual
 '
 
+# specify merge-base as parent of branch2
+# git merge-tree --write-tree --merge-base=c2 c1 c3
+#   Commit c1: add file1
+#   Commit c2: add file2 after c1
+#   Commit c3: add file3 after c2
+#   Expected: add file3, and file2 does NOT appear
+
+test_expect_success 'specify merge-base as parent of branch2' '
+	# Setup
+	git init base-b2-p && (
+		cd base-b2-p &&
+		test_commit c1 file1 &&
+		test_commit c2 file2 &&
+		test_commit c3 file3
+	) &&
+	# Testing
+	(
+		cd base-b2-p &&
+		TREE_OID=$(git merge-tree --write-tree --merge-base=c2 c1 c3) &&
+
+		q_to_tab <<-EOF >expect &&
+		100644 blob $(git rev-parse c1:file1)Qfile1
+		100644 blob $(git rev-parse c3:file3)Qfile3
+		EOF
+
+		git ls-tree $TREE_OID >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done

gitgitgadget

From: Kyle Zhao <[email protected]>

The previous change add "--merge-base" option in order to allow users to specify merge-base for the merge. But it doesn't compatible well with --stdin, because multiple batched merges can only have the same specified base.

This patch allows users to pass --merge-base option into the input line, such as:

    printf "--merge-base=<b3> <b1> <b2>" | git merge-tree --stdin

This does a merge of b1 and b2, and uses b3 as the merge-base.

---
 Documentation/git-merge-tree.txt |  3 ++-
 builtin/merge-tree.c             | 22 ++++++++++++++++++++--
 t/t4301-merge-tree-write-tree.sh | 21 +++++++++++++++++++++
 3 files changed, 43 insertions(+), 3 deletions(-)

index d9dacb2ce54..be6a11bbaec 100644
@@ -66,7 +66,8 @@ OPTIONS
 
 --merge-base=<commit>::
 	Instead of finding the merge-bases for <branch1> and <branch2>,
-	specify a merge-base for the merge.
+	specify a merge-base for the merge. When --stdin is passed, this
+	option should be passed into the input line.
 
 [[OUTPUT]]
 OUTPUT

index f402b807c0f..7a8049e7b0c 100644
@@ -551,16 +551,34 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 
 		if (o.mode == MODE_TRIVIAL)
 			die(_("--trivial-merge is incompatible with all other options"));
+		if (merge_base)
+			die(_("--merge-base should be passed into the input line"));
 		line_termination = '\0';
 		while (strbuf_getline_lf(&buf, stdin) != EOF) {
 			struct strbuf **split;
 			int result;
+			const char *input_merge_base = NULL;
+			const char *arg;
 
 			split = strbuf_split(&buf, ' ');
-			if (!split[0] || !split[1] || split[2])
+			if (!split[0] || !split[1])
 				die(_("malformed input line: '%s'."), buf.buf);
 			strbuf_rtrim(split[0]);
-			result = real_merge(&o, merge_base, split[0]->buf, split[1]->buf, prefix);
+
+			/* parse --merge-base=<commit> option */
+			arg = split[0]->buf;
+			if (skip_prefix(arg, "--merge-base=", &arg))
+				input_merge_base = arg;
+
+			if (input_merge_base && split[2] && !split[3]) {
+				strbuf_rtrim(split[1]);
+				result = real_merge(&o, input_merge_base, split[1]->buf, split[2]->buf, prefix);
+			} else if (!input_merge_base && !split[2]) {
+				result = real_merge(&o, NULL, split[0]->buf, split[1]->buf, prefix);
+			} else {
+				die(_("malformed input line: '%s'."), buf.buf);
+			}
+
 			if (result < 0)
 				die(_("merging cannot continue; got unclean result of %d"), result);
 			strbuf_list_free(split);

index 5b0073d3dcd..aec2c00b91f 100755
@@ -860,6 +860,13 @@ test_expect_success '--stdin with both a successful and a conflicted merge' '
 	test_cmp expect actual
 '
 
+
+test_expect_success '--merge-base is incompatible with --stdin' '
+	test_must_fail git merge-tree --merge-base=side1 --stdin 2>expect &&
+
+	grep "^fatal: --merge-base should be passed into the input line" expect
+'
+
 # specify merge-base as parent of branch2
 # git merge-tree --write-tree --merge-base=c2 c1 c3
 #   Commit c1: add file1

@@ -890,4 +897,18 @@ test_expect_success 'specify merge-base as parent of branch2' '
 	)
 '
 
+test_expect_success '--stdin with both a normal merge and a merge-base specified merge' '
+	cd base-b2-p &&
+	printf "c1 c3\n--merge-base=c2 c1 c3" | git merge-tree --stdin >actual &&
+
+	printf "1\0" >expect &&
+	git merge-tree --write-tree -z c1 c3 >>expect &&
+	printf "\0" >>expect &&
+
+	printf "1\0" >>expect &&
+	git merge-tree --write-tree -z --merge-base=c2 c1 c3 >>expect &&
+	printf "\0" >>expect &&
+	test_cmp expect actual
+'
+
 test_done

gitgitgadget

s/add/added/ ?

"it doesn't compatible well" doesn't parse for me.

Quoting from Documentation/SubmittingPatches:

"""
Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behavior.
"""

Perhaps something like:

"""
The previous commit added a --merge-base option in order to allow using a specified merge-base for the merge. Extend the input accepted by --stdin to also allow a specified merge-base with each merge requested. For example:

    printf "--merge-base=<b3> <b1> <b2>" | git merge-tree --stdin

does a merge of b1 and b2, and uses b3 as the merge-base. """

However, I'm a bit curious about using --merge-base= on the input line as opposed to just using a simpler marker; something like

    printf "<b3> -- <b1> <b2>" | git merge-tree --stdin

(which follows a precedent set by git-merge-recursive). Your version is a bit more self-documenting, but what if we want to allow users to specify multiple merge bases in the future (for use in passing to merge_incore_recursive())? Is it annoying to need to prefix each one?

Thanks for your review.

Done.

Done.

Actually, I thought about it at the time, but didn't think of any good input format. Your idea is great, I think I'll use it.

I just added the documentation for --input, please see what else needs to be improved.

Yeah, I have added the comment.

Thanks for the update. I haven't taken a close look, but queued what you have as a replacement on top of en/merge-tree-sequence.

Let's see what Elijah thinks...

Thanks,
Taylor

Thanks for Elijah's work. I'm very excited that merge-ort is integrated into the git merge-tree, which means that we can use merge-ort in bare repositories to optimize merge performance.

In this patch, I introduce a new --merge-base=<commit> option to allow callers to specify a merge-base for the merge. This may allow users to implement git cherry-pick and git rebase in bare repositories with git merge-tree cmd.

Changes since v1:

* Changed merge_incore_recursive() to merge_incore_nonrecursive() when merge-base is specified.
* Fixed c style problem.
* Moved commit lookup/die logic out to the parsing logic in cmd_merge_tree().
* use test_commit for test

Changes since v2:

* commit message
* Rebased on top of en/merge-tree-sequence.
* Set opt.ancestor to o->merge_base. Because opt.ancestor is a *char. To make it easier to pass parameters, I moved lookup_commit_reference_by_name() to real_ merge() again.
* Added test comment.

Changes since v3:

* support --merge-base in conjunction with --stdin

Kyle Zhao (2):
merge-tree.c: add --merge-base=<commit> option merge-tree.c: allow specifying the merge-base when --stdin is passed

 Documentation/git-merge-tree.txt | 16 ++++++++
 builtin/merge-tree.c             | 65 ++++++++++++++++++++++++++------
 t/t4301-merge-tree-write-tree.sh | 55 +++++++++++++++++++++++++++
 3 files changed, 124 insertions(+), 12 deletions(-)

base-commit: ec1edbcb56ac05e9980299b05924c5c1b51d68b4
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1397%2Fkeyu98%2Fkz%2Fmerge-tree-option-merge-base-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1397/keyu98/kz/merge-tree-option-merge-base-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1397

Range-diff vs v4:

1: bba854fc8fa ! 1: 01df0d1a6a7 merge-tree.c: add --merge-base=<commit> option
@@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char
OPT_END()
};

+@@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char *prefix) +
+ if (o.mode == MODE_TRIVIAL)
+ die(_("--trivial-merge is incompatible with all other options"));
++ if (merge_base)
++ die(_("--merge-base is incompatible with --stdin"));
+ line_termination = '\0';
+ while (strbuf_getline_lf(&buf, stdin) != EOF) {
+ struct strbuf **split;
@@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char *prefix)
if (!split[0] || !split[1] || split[2])
die(_("malformed input line: '%s'."), buf.buf);
2: db47fbc663e ! 2: 3bdfad03cca merge-tree.c: support --merge-base in conjunction with --stdin

     @@ Metadata
      Author: Kyle Zhao <[email protected]>
      
       ## Commit message ##
     -    merge-tree.c: support --merge-base in conjunction with --stdin
     +    merge-tree.c: allow specifying the merge-base when --stdin is passed
      
     -    The previous change add "--merge-base" option in order to allow users to
     -    specify merge-base for the merge. But it doesn't compatible well with
     -    --stdin, because multiple batched merges can only have the same specified
     -    base.
     +    The previous commit added a `--merge-base` option in order to allow
     +    using a specified merge-base for the merge.  Extend the input accepted
     +    by `--stdin` to also allow a specified merge-base with each merge
     +    requested.  For example:
      
     -    This patch allows users to pass --merge-base option into the input line,
     -    such as:
     +        printf "<b3> -- <b1> <b2>" | git merge-tree --stdin
      
     -        printf "--merge-base=<b3> <b1> <b2>" | git merge-tree --stdin
     -
     -    This does a merge of b1 and b2, and uses b3 as the merge-base.
     +    does a merge of b1 and b2, and uses b3 as the merge-base.
      
          Signed-off-by: Kyle Zhao <[email protected]>
      
     @@ Documentation/git-merge-tree.txt: OPTIONS
       --merge-base=<commit>::
       	Instead of finding the merge-bases for <branch1> and <branch2>,
      -	specify a merge-base for the merge.
     -+	specify a merge-base for the merge. When --stdin is passed, this
     -+	option should be passed into the input line.
     ++	specify a merge-base for the merge. This option is incompatible
     ++	with `--stdin`.
       
       [[OUTPUT]]
       OUTPUT
     +@@ Documentation/git-merge-tree.txt: with linkgit:git-merge[1]:
     +   * any messages that would have been printed to stdout (the
     +     <<IM,Informational messages>>)
     + 
     ++INPUT FORMAT
     ++------------
     ++'git merge-tree --stdin' input format is fully text based. Each line
     ++has this format:
     ++
     ++	[<base-commit> -- ]<branch1> <branch2>
     ++
     ++If one line is separated by `--`, the string before the separator is
     ++used for specifying a merge-base for the merge and the string after
     ++the separator describes the branches to be merged.
     ++
     + MISTAKES TO AVOID
     + -----------------
     + 
      
       ## builtin/merge-tree.c ##
      @@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char *prefix)
     - 
     - 		if (o.mode == MODE_TRIVIAL)
     - 			die(_("--trivial-merge is incompatible with all other options"));
     -+		if (merge_base)
     -+			die(_("--merge-base should be passed into the input line"));
     - 		line_termination = '\0';
       		while (strbuf_getline_lf(&buf, stdin) != EOF) {
       			struct strbuf **split;
       			int result;
      +			const char *input_merge_base = NULL;
     -+			const char *arg;
       
       			split = strbuf_split(&buf, ' ');
      -			if (!split[0] || !split[1] || split[2])
     @@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char
       				die(_("malformed input line: '%s'."), buf.buf);
       			strbuf_rtrim(split[0]);
      -			result = real_merge(&o, merge_base, split[0]->buf, split[1]->buf, prefix);
     ++			strbuf_rtrim(split[1]);
      +
     -+			/* parse --merge-base=<commit> option */
     -+			arg = split[0]->buf;
     -+			if (skip_prefix(arg, "--merge-base=", &arg))
     -+				input_merge_base = arg;
     ++			/* parse the merge-base */
     ++			if (!strcmp(split[1]->buf, "--")) {
     ++				input_merge_base = split[0]->buf;
     ++			}
      +
     -+			if (input_merge_base && split[2] && !split[3]) {
     -+				strbuf_rtrim(split[1]);
     -+				result = real_merge(&o, input_merge_base, split[1]->buf, split[2]->buf, prefix);
     ++			if (input_merge_base && split[2] && split[3] && !split[4]) {
     ++				strbuf_rtrim(split[2]);
     ++				strbuf_rtrim(split[3]);
     ++				result = real_merge(&o, input_merge_base, split[2]->buf, split[3]->buf, prefix);
      +			} else if (!input_merge_base && !split[2]) {
      +				result = real_merge(&o, NULL, split[0]->buf, split[1]->buf, prefix);
      +			} else {
     @@ t/t4301-merge-tree-write-tree.sh: test_expect_success '--stdin with both a succe
      +test_expect_success '--merge-base is incompatible with --stdin' '
      +	test_must_fail git merge-tree --merge-base=side1 --stdin 2>expect &&
      +
     -+	grep "^fatal: --merge-base should be passed into the input line" expect
     ++	grep "^fatal: --merge-base is incompatible with --stdin" expect
      +'
      +
       # specify merge-base as parent of branch2
     @@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'specify merge-base as par
       	)
       '
       
     ++# Since the earlier tests have verified that individual merge-tree calls
     ++# are doing the right thing, this test case is only used to test whether
     ++# the input format is available.
     ++
      +test_expect_success '--stdin with both a normal merge and a merge-base specified merge' '
      +	cd base-b2-p &&
     -+	printf "c1 c3\n--merge-base=c2 c1 c3" | git merge-tree --stdin >actual &&
     ++	printf "c1 c3\nc2 -- c1 c3" | git merge-tree --stdin >actual &&
      +
      +	printf "1\0" >expect &&
      +	git merge-tree --write-tree -z c1 c3 >>expect &&

gitgitgadget

From: Kyle Zhao <[email protected]>

This patch will give our callers more flexibility to use git merge-tree, such as:

    git merge-tree --write-tree --merge-base=branch^ HEAD branch

This does a merge of HEAD and branch, but uses branch^ as the merge-base.

And the reason why using an option flag instead of a positional argument is to allow additional commits passed to merge-tree to be handled via an octopus merge in the future.

---
 Documentation/git-merge-tree.txt |  4 +++
 builtin/merge-tree.c             | 46 ++++++++++++++++++++++++--------
 t/t4301-merge-tree-write-tree.sh | 30 +++++++++++++++++++++
 3 files changed, 69 insertions(+), 11 deletions(-)

index 04bcc416e6e..d9dacb2ce54 100644
@@ -64,6 +64,10 @@ OPTIONS
 	share no common history.  This flag can be given to override that
 	check and make the merge proceed anyway.
 
+--merge-base=<commit>::
+	Instead of finding the merge-bases for <branch1> and <branch2>,
+	specify a merge-base for the merge.
+
 [[OUTPUT]]
 OUTPUT
 ------

index cac85591b52..5b0073d3dcd 100755
@@ -860,4 +860,34 @@ test_expect_success '--stdin with both a successful and a conflicted merge' '
 	test_cmp expect actual
 '
 
+# specify merge-base as parent of branch2
+# git merge-tree --write-tree --merge-base=c2 c1 c3
+#   Commit c1: add file1
+#   Commit c2: add file2 after c1
+#   Commit c3: add file3 after c2
+#   Expected: add file3, and file2 does NOT appear
+
+test_expect_success 'specify merge-base as parent of branch2' '
+	# Setup
+	git init base-b2-p && (
+		cd base-b2-p &&
+		test_commit c1 file1 &&
+		test_commit c2 file2 &&
+		test_commit c3 file3
+	) &&
+	# Testing
+	(
+		cd base-b2-p &&
+		TREE_OID=$(git merge-tree --write-tree --merge-base=c2 c1 c3) &&
+
+		q_to_tab <<-EOF >expect &&
+		100644 blob $(git rev-parse c1:file1)Qfile1
+		100644 blob $(git rev-parse c3:file3)Qfile3
+		EOF
+
+		git ls-tree $TREE_OID >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done

gitgitgadget

Thanks for the reviews, I'll fix it in the next patch.

Nice.

From: Kyle Zhao <[email protected]>

The previous commit added a --merge-base option in order to allow using a specified merge-base for the merge. Extend the input accepted by --stdin to also allow a specified merge-base with each merge requested. For example:

    printf "<b3> -- <b1> <b2>" | git merge-tree --stdin

does a merge of b1 and b2, and uses b3 as the merge-base.

---
 Documentation/git-merge-tree.txt | 14 +++++++++++++-
 builtin/merge-tree.c             | 21 +++++++++++++++++++--
 t/t4301-merge-tree-write-tree.sh | 25 +++++++++++++++++++++++++
 3 files changed, 57 insertions(+), 3 deletions(-)

index d9dacb2ce54..298c133fdb6 100644
@@ -66,7 +66,8 @@ OPTIONS
 
 --merge-base=<commit>::
 	Instead of finding the merge-bases for <branch1> and <branch2>,
-	specify a merge-base for the merge.
+	specify a merge-base for the merge. This option is incompatible
+	with `--stdin`.
 
 [[OUTPUT]]
 OUTPUT

@@ -220,6 +221,17 @@ with linkgit:git-merge[1]:
   * any messages that would have been printed to stdout (the
     <<IM,Informational messages>>)
 
+INPUT FORMAT
+------------
+'git merge-tree --stdin' input format is fully text based. Each line
+has this format:
+
+	[<base-commit> -- ]<branch1> <branch2>
+
+If one line is separated by `--`, the string before the separator is
+used for specifying a merge-base for the merge and the string after
+the separator describes the branches to be merged.
+
 MISTAKES TO AVOID
 -----------------
 

index 62c6d43cdb9..330f779e8bc 100644
@@ -557,12 +557,29 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 		while (strbuf_getline_lf(&buf, stdin) != EOF) {
 			struct strbuf **split;
 			int result;
+			const char *input_merge_base = NULL;
 
 			split = strbuf_split(&buf, ' ');
-			if (!split[0] || !split[1] || split[2])
+			if (!split[0] || !split[1])
 				die(_("malformed input line: '%s'."), buf.buf);
 			strbuf_rtrim(split[0]);
-			result = real_merge(&o, merge_base, split[0]->buf, split[1]->buf, prefix);
+			strbuf_rtrim(split[1]);
+
+			/* parse the merge-base */
+			if (!strcmp(split[1]->buf, "--")) {
+				input_merge_base = split[0]->buf;
+			}
+
+			if (input_merge_base && split[2] && split[3] && !split[4]) {
+				strbuf_rtrim(split[2]);
+				strbuf_rtrim(split[3]);
+				result = real_merge(&o, input_merge_base, split[2]->buf, split[3]->buf, prefix);
+			} else if (!input_merge_base && !split[2]) {
+				result = real_merge(&o, NULL, split[0]->buf, split[1]->buf, prefix);
+			} else {
+				die(_("malformed input line: '%s'."), buf.buf);
+			}
+
 			if (result < 0)
 				die(_("merging cannot continue; got unclean result of %d"), result);
 			strbuf_list_free(split);

index 5b0073d3dcd..03bb1cb8685 100755
@@ -860,6 +860,13 @@ test_expect_success '--stdin with both a successful and a conflicted merge' '
 	test_cmp expect actual
 '
 
+
+test_expect_success '--merge-base is incompatible with --stdin' '
+	test_must_fail git merge-tree --merge-base=side1 --stdin 2>expect &&
+
+	grep "^fatal: --merge-base is incompatible with --stdin" expect
+'
+
 # specify merge-base as parent of branch2
 # git merge-tree --write-tree --merge-base=c2 c1 c3
 #   Commit c1: add file1

@@ -890,4 +897,22 @@ test_expect_success 'specify merge-base as parent of branch2' '
 	)
 '
 
+# Since the earlier tests have verified that individual merge-tree calls
+# are doing the right thing, this test case is only used to test whether
+# the input format is available.
+
+test_expect_success '--stdin with both a normal merge and a merge-base specified merge' '
+	cd base-b2-p &&
+	printf "c1 c3\nc2 -- c1 c3" | git merge-tree --stdin >actual &&
+
+	printf "1\0" >expect &&
+	git merge-tree --write-tree -z c1 c3 >>expect &&
+	printf "\0" >>expect &&
+
+	printf "1\0" >>expect &&
+	git merge-tree --write-tree -z --merge-base=c2 c1 c3 >>expect &&
+	printf "\0" >>expect &&
+	test_cmp expect actual
+'
+
 test_done

gitgitgadget

Thanks for Elijah's work. I'm very excited that merge-ort is integrated into the git merge-tree, which means that we can use merge-ort in bare repositories to optimize merge performance.

In this patch, I introduce a new --merge-base=<commit> option to allow callers to specify a merge-base for the merge. This may allow users to implement git cherry-pick and git rebase in bare repositories with git merge-tree cmd.

Changes since v1:

* Changed merge_incore_recursive() to merge_incore_nonrecursive() when merge-base is specified.
* Fixed c style problem.
* Moved commit lookup/die logic out to the parsing logic in cmd_merge_tree().
* use test_commit for test

Changes since v2:

* commit message
* Rebased on top of en/merge-tree-sequence.
* Set opt.ancestor to o->merge_base. Because opt.ancestor is a *char. To make it easier to pass parameters, I moved lookup_commit_reference_by_name() to real_ merge() again.
* Added test comment.

Changes since v3:

* support --merge-base in conjunction with --stdin

Changes since v4:

* commit message
* added input format document
* changed the input format for specifying the merge-base when --stdin is passed
* changed the output when --stdin and --merge-base are used at the same time
* add comment for test

Changes since v5:

* improved test: remove the test repo after the test; avoid sub-shell.

Kyle Zhao (2):
merge-tree.c: add --merge-base=<commit> option merge-tree.c: allow specifying the merge-base when --stdin is passed

 Documentation/git-merge-tree.txt | 16 ++++++++
 builtin/merge-tree.c             | 65 ++++++++++++++++++++++++++------
 t/t4301-merge-tree-write-tree.sh | 61 ++++++++++++++++++++++++++++++
 3 files changed, 130 insertions(+), 12 deletions(-)

base-commit: ec1edbcb56ac05e9980299b05924c5c1b51d68b4
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1397%2Fkeyu98%2Fkz%2Fmerge-tree-option-merge-base-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1397/keyu98/kz/merge-tree-option-merge-base-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/1397

Range-diff vs v5:

1: 01df0d1a6a7 ! 1: 1cf1c69b8e8 merge-tree.c: add --merge-base=<commit> option

     @@ t/t4301-merge-tree-write-tree.sh: test_expect_success '--stdin with both a succe
      +
      +test_expect_success 'specify merge-base as parent of branch2' '
      +	# Setup
     -+	git init base-b2-p && (
     -+		cd base-b2-p &&
     -+		test_commit c1 file1 &&
     -+		test_commit c2 file2 &&
     -+		test_commit c3 file3
     -+	) &&
     ++	test_when_finished "rm -rf base-b2-p" &&
     ++	git init base-b2-p &&
     ++	test_commit -C base-b2-p c1 file1 &&
     ++	test_commit -C base-b2-p c2 file2 &&
     ++	test_commit -C base-b2-p c3 file3 &&
     ++
      +	# Testing
     -+	(
     -+		cd base-b2-p &&
     -+		TREE_OID=$(git merge-tree --write-tree --merge-base=c2 c1 c3) &&
     ++	TREE_OID=$(git -C base-b2-p merge-tree --write-tree --merge-base=c2 c1 c3) &&
      +
     -+		q_to_tab <<-EOF >expect &&
     -+		100644 blob $(git rev-parse c1:file1)Qfile1
     -+		100644 blob $(git rev-parse c3:file3)Qfile3
     -+		EOF
     ++	q_to_tab <<-EOF >expect &&
     ++	100644 blob $(git -C base-b2-p rev-parse c1:file1)Qfile1
     ++	100644 blob $(git -C base-b2-p rev-parse c3:file3)Qfile3
     ++	EOF
      +
     -+		git ls-tree $TREE_OID >actual &&
     -+		test_cmp expect actual
     -+	)
     ++	git -C base-b2-p ls-tree $TREE_OID >actual &&
     ++	test_cmp expect actual
      +'
      +
       test_done
 2:  3bdfad03cca ! 2:  40d56544e6e merge-tree.c: allow specifying the merge-base when --stdin is passed
     @@ t/t4301-merge-tree-write-tree.sh: test_expect_success '--stdin with both a succe
       # git merge-tree --write-tree --merge-base=c2 c1 c3
       #   Commit c1: add file1
      @@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'specify merge-base as parent of branch2' '
     - 	)
     + 	test_cmp expect actual
       '
       
      +# Since the earlier tests have verified that individual merge-tree calls
      +# are doing the right thing, this test case is only used to test whether
      +# the input format is available.
      +
     -+test_expect_success '--stdin with both a normal merge and a merge-base specified merge' '
     -+	cd base-b2-p &&
     -+	printf "c1 c3\nc2 -- c1 c3" | git merge-tree --stdin >actual &&
     ++test_expect_success 'check the input format when --stdin is passed' '
     ++	test_when_finished "rm -rf repo" &&
     ++	git init repo &&
     ++	test_commit -C repo c1 &&
     ++	test_commit -C repo c2 &&
     ++	test_commit -C repo c3 &&
     ++	printf "c1 c3\nc2 -- c1 c3\nc2 c3" | git -C repo merge-tree --stdin >actual &&
      +
      +	printf "1\0" >expect &&
     -+	git merge-tree --write-tree -z c1 c3 >>expect &&
     ++	git -C repo merge-tree --write-tree -z c1 c3 >>expect &&
      +	printf "\0" >>expect &&
      +
      +	printf "1\0" >>expect &&
     -+	git merge-tree --write-tree -z --merge-base=c2 c1 c3 >>expect &&
     ++	git -C repo merge-tree --write-tree -z --merge-base=c2 c1 c3 >>expect &&
      +	printf "\0" >>expect &&
     ++
     ++	printf "1\0" >>expect &&
     ++	git -C repo merge-tree --write-tree -z c2 c3 >>expect &&
     ++	printf "\0" >>expect &&
     ++
      +	test_cmp expect actual
      +'
      +

gitgitgadget

From: Kyle Zhao <[email protected]>

This patch will give our callers more flexibility to use git merge-tree, such as:

    git merge-tree --write-tree --merge-base=branch^ HEAD branch

This does a merge of HEAD and branch, but uses branch^ as the merge-base.

And the reason why using an option flag instead of a positional argument is to allow additional commits passed to merge-tree to be handled via an octopus merge in the future.

---
 Documentation/git-merge-tree.txt |  4 +++
 builtin/merge-tree.c             | 46 ++++++++++++++++++++++++--------
 t/t4301-merge-tree-write-tree.sh | 27 +++++++++++++++++++
 3 files changed, 66 insertions(+), 11 deletions(-)

index 04bcc416e6e..d9dacb2ce54 100644
@@ -64,6 +64,10 @@ OPTIONS
 	share no common history.  This flag can be given to override that
 	check and make the merge proceed anyway.
 
+--merge-base=<commit>::
+	Instead of finding the merge-bases for <branch1> and <branch2>,
+	specify a merge-base for the merge.
+
 [[OUTPUT]]
 OUTPUT
 ------

index cac85591b52..6db96ccbaae 100755
@@ -860,4 +860,31 @@ test_expect_success '--stdin with both a successful and a conflicted merge' '
 	test_cmp expect actual
 '
 
+# specify merge-base as parent of branch2
+# git merge-tree --write-tree --merge-base=c2 c1 c3
+#   Commit c1: add file1
+#   Commit c2: add file2 after c1
+#   Commit c3: add file3 after c2
+#   Expected: add file3, and file2 does NOT appear
+
+test_expect_success 'specify merge-base as parent of branch2' '
+	# Setup
+	test_when_finished "rm -rf base-b2-p" &&
+	git init base-b2-p &&
+	test_commit -C base-b2-p c1 file1 &&
+	test_commit -C base-b2-p c2 file2 &&
+	test_commit -C base-b2-p c3 file3 &&
+
+	# Testing
+	TREE_OID=$(git -C base-b2-p merge-tree --write-tree --merge-base=c2 c1 c3) &&
+
+	q_to_tab <<-EOF >expect &&
+	100644 blob $(git -C base-b2-p rev-parse c1:file1)Qfile1
+	100644 blob $(git -C base-b2-p rev-parse c3:file3)Qfile3
+	EOF
+
+	git -C base-b2-p ls-tree $TREE_OID >actual &&
+	test_cmp expect actual
+'
+
 test_done

gitgitgadget

From: Kyle Zhao <[email protected]>

The previous commit added a --merge-base option in order to allow using a specified merge-base for the merge. Extend the input accepted by --stdin to also allow a specified merge-base with each merge requested. For example:

    printf "<b3> -- <b1> <b2>" | git merge-tree --stdin

does a merge of b1 and b2, and uses b3 as the merge-base.

---
 Documentation/git-merge-tree.txt | 14 ++++++++++++-
 builtin/merge-tree.c             | 21 ++++++++++++++++++--
 t/t4301-merge-tree-write-tree.sh | 34 ++++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 3 deletions(-)

index d9dacb2ce54..298c133fdb6 100644
@@ -66,7 +66,8 @@ OPTIONS
 
 --merge-base=<commit>::
 	Instead of finding the merge-bases for <branch1> and <branch2>,
-	specify a merge-base for the merge.
+	specify a merge-base for the merge. This option is incompatible
+	with `--stdin`.
 
 [[OUTPUT]]
 OUTPUT

@@ -220,6 +221,17 @@ with linkgit:git-merge[1]:
   * any messages that would have been printed to stdout (the
     <<IM,Informational messages>>)
 
+INPUT FORMAT
+------------
+'git merge-tree --stdin' input format is fully text based. Each line
+has this format:
+
+	[<base-commit> -- ]<branch1> <branch2>
+
+If one line is separated by `--`, the string before the separator is
+used for specifying a merge-base for the merge and the string after
+the separator describes the branches to be merged.
+
 MISTAKES TO AVOID
 -----------------
 

index 62c6d43cdb9..330f779e8bc 100644
@@ -557,12 +557,29 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 		while (strbuf_getline_lf(&buf, stdin) != EOF) {
 			struct strbuf **split;
 			int result;
+			const char *input_merge_base = NULL;
 
 			split = strbuf_split(&buf, ' ');
-			if (!split[0] || !split[1] || split[2])
+			if (!split[0] || !split[1])
 				die(_("malformed input line: '%s'."), buf.buf);
 			strbuf_rtrim(split[0]);
-			result = real_merge(&o, merge_base, split[0]->buf, split[1]->buf, prefix);
+			strbuf_rtrim(split[1]);
+
+			/* parse the merge-base */
+			if (!strcmp(split[1]->buf, "--")) {
+				input_merge_base = split[0]->buf;
+			}
+
+			if (input_merge_base && split[2] && split[3] && !split[4]) {
+				strbuf_rtrim(split[2]);
+				strbuf_rtrim(split[3]);
+				result = real_merge(&o, input_merge_base, split[2]->buf, split[3]->buf, prefix);
+			} else if (!input_merge_base && !split[2]) {
+				result = real_merge(&o, NULL, split[0]->buf, split[1]->buf, prefix);
+			} else {
+				die(_("malformed input line: '%s'."), buf.buf);
+			}
+
 			if (result < 0)
 				die(_("merging cannot continue; got unclean result of %d"), result);
 			strbuf_list_free(split);

index 6db96ccbaae..22b43f0003e 100755
@@ -860,6 +860,13 @@ test_expect_success '--stdin with both a successful and a conflicted merge' '
 	test_cmp expect actual
 '
 
+
+test_expect_success '--merge-base is incompatible with --stdin' '
+	test_must_fail git merge-tree --merge-base=side1 --stdin 2>expect &&
+
+	grep "^fatal: --merge-base is incompatible with --stdin" expect
+'
+
 # specify merge-base as parent of branch2
 # git merge-tree --write-tree --merge-base=c2 c1 c3
 #   Commit c1: add file1

@@ -887,4 +894,31 @@ test_expect_success 'specify merge-base as parent of branch2' '
 	test_cmp expect actual
 '
 
+# Since the earlier tests have verified that individual merge-tree calls
+# are doing the right thing, this test case is only used to test whether
+# the input format is available.
+
+test_expect_success 'check the input format when --stdin is passed' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	test_commit -C repo c1 &&
+	test_commit -C repo c2 &&
+	test_commit -C repo c3 &&
+	printf "c1 c3\nc2 -- c1 c3\nc2 c3" | git -C repo merge-tree --stdin >actual &&
+
+	printf "1\0" >expect &&
+	git -C repo merge-tree --write-tree -z c1 c3 >>expect &&
+	printf "\0" >>expect &&
+
+	printf "1\0" >>expect &&
+	git -C repo merge-tree --write-tree -z --merge-base=c2 c1 c3 >>expect &&
+	printf "\0" >>expect &&
+
+	printf "1\0" >>expect &&
+	git -C repo merge-tree --write-tree -z c2 c3 >>expect &&
+	printf "\0" >>expect &&
+
+	test_cmp expect actual
+'
+
 test_done

gitgitgadget

Sorry for the delay; my Git time has sadly been quite limited. :-(

# are doing the right thing, this test case is only used to verify that
# we can also trigger merges via --stdin, and that when we do we get
# the same answer as running a bunch of separate merges.

Thanks for everyone's careful reviews.

In this patch, I introduce a new --merge-base=<commit> option to allow callers to specify a merge-base for the merge and extend the input accepted by --stdin to also allow a specified merge-base with each merge requested.

Regards, Kyle

Changes since v1:

* Changed merge_incore_recursive() to merge_incore_nonrecursive() when merge-base is specified.
* Fixed c style problem.
* Moved commit lookup/die logic out to the parsing logic in cmd_merge_tree().
* use test_commit for test

Changes since v2:

* commit message
* Rebased on top of en/merge-tree-sequence.
* Set opt.ancestor to o->merge_base. Because opt.ancestor is a *char. To make it easier to pass parameters, I moved lookup_commit_reference_by_name() to real_ merge() again.
* Added test comment.

Changes since v3:

* support --merge-base in conjunction with --stdin

Changes since v4:

* commit message
* added input format document
* changed the input format for specifying the merge-base when --stdin is passed
* changed the output when --stdin and --merge-base are used at the same time
* add comment for test

Changes since v5:

* improved test: remove the test repo after the test; avoid sub-shell.

Changes since v6:

* fixed comment of test

Kyle Zhao (2):
merge-tree.c: add --merge-base=<commit> option merge-tree.c: allow specifying the merge-base when --stdin is passed

 Documentation/git-merge-tree.txt | 16 ++++++++
 builtin/merge-tree.c             | 65 ++++++++++++++++++++++++++------
 t/t4301-merge-tree-write-tree.sh | 62 ++++++++++++++++++++++++++++++
 3 files changed, 131 insertions(+), 12 deletions(-)

base-commit: ec1edbcb56ac05e9980299b05924c5c1b51d68b4
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1397%2Fkeyu98%2Fkz%2Fmerge-tree-option-merge-base-v7
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1397/keyu98/kz/merge-tree-option-merge-base-v7
Pull-Request: https://github.com/gitgitgadget/git/pull/1397

Range-diff vs v6:

 1:  1cf1c69b8e8 = 1:  1cf1c69b8e8 merge-tree.c: add --merge-base=<commit> option
 2:  40d56544e6e ! 2:  48e55d4e97c merge-tree.c: allow specifying the merge-base when --stdin is passed

@@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'specify merge-base as par
'

+# Since the earlier tests have verified that individual merge-tree calls
-+# are doing the right thing, this test case is only used to test whether -+# the input format is available.
++# are doing the right thing, this test case is only used to verify that ++# we can also trigger merges via --stdin, and that when we do we get ++# the same answer as running a bunch of separate merges.
+
+test_expect_success 'check the input format when --stdin is passed' '
+ test_when_finished "rm -rf repo" &&

gitgitgadget

From: Kyle Zhao <[email protected]>

This patch will give our callers more flexibility to use git merge-tree, such as:

    git merge-tree --write-tree --merge-base=branch^ HEAD branch

This does a merge of HEAD and branch, but uses branch^ as the merge-base.

And the reason why using an option flag instead of a positional argument is to allow additional commits passed to merge-tree to be handled via an octopus merge in the future.

---
 Documentation/git-merge-tree.txt |  4 +++
 builtin/merge-tree.c             | 46 ++++++++++++++++++++++++--------
 t/t4301-merge-tree-write-tree.sh | 27 +++++++++++++++++++
 3 files changed, 66 insertions(+), 11 deletions(-)

index 04bcc416e6e..d9dacb2ce54 100644
@@ -64,6 +64,10 @@ OPTIONS
 	share no common history.  This flag can be given to override that
 	check and make the merge proceed anyway.
 
+--merge-base=<commit>::
+	Instead of finding the merge-bases for <branch1> and <branch2>,
+	specify a merge-base for the merge.
+
 [[OUTPUT]]
 OUTPUT
 ------

index cac85591b52..6db96ccbaae 100755
@@ -860,4 +860,31 @@ test_expect_success '--stdin with both a successful and a conflicted merge' '
 	test_cmp expect actual
 '
 
+# specify merge-base as parent of branch2
+# git merge-tree --write-tree --merge-base=c2 c1 c3
+#   Commit c1: add file1
+#   Commit c2: add file2 after c1
+#   Commit c3: add file3 after c2
+#   Expected: add file3, and file2 does NOT appear
+
+test_expect_success 'specify merge-base as parent of branch2' '
+	# Setup
+	test_when_finished "rm -rf base-b2-p" &&
+	git init base-b2-p &&
+	test_commit -C base-b2-p c1 file1 &&
+	test_commit -C base-b2-p c2 file2 &&
+	test_commit -C base-b2-p c3 file3 &&
+
+	# Testing
+	TREE_OID=$(git -C base-b2-p merge-tree --write-tree --merge-base=c2 c1 c3) &&
+
+	q_to_tab <<-EOF >expect &&
+	100644 blob $(git -C base-b2-p rev-parse c1:file1)Qfile1
+	100644 blob $(git -C base-b2-p rev-parse c3:file3)Qfile3
+	EOF
+
+	git -C base-b2-p ls-tree $TREE_OID >actual &&
+	test_cmp expect actual
+'
+
 test_done

gitgitgadget

I agree.

OPT_STRING only use the last value of "--merge-base".
It will mislead users, they may specify the merge-base multiple times, but found it doesn't work.

I went to check the api-parse-option.txt, but I didn't found an elegant solution to stop when the users uses the second "--merge-base". Did I miss it? Or we just need to mention this in the git-merge-tree.txt, such as:
--merge-base=<commit>::
Instead of finding the merge-bases for <branch1> and <branch2>,
- specify a merge-base for the merge.
+ specify a merge-base for the merge. This option doesn't support
+ being specified multiple times, only the last value you provide will be used.

Thanks,
Kyle

That's not even a fix, as it does not allow specifying more than one merge bases, is it?

Just like how builtin/merge-tree.c::real_merge() is prepared to handle parent1 and parent2 that have multiple merge bases and pass all of them to the underlying merge machinery, you can accumulate multiple strings using OPT_STRING_LIST() and use all of them as merge_bases, no?

I am a bit torn on this, though.

Because it uses lookup_commit_reference_by_name() to obtain base_commit in addition to parent1 and parent2, and then get_commit_tree() on them to get their trees, the real_merge() function in the posted patch is incapable of accepting a single "pretend as if this tree object is the common ancestor tree" and merging the two tree objects. But once that flaw is fixed, using merge_incore_nonrecursive() with an explicitly given "base", we can merge totally trees regardless of how the commits they are found in are related in the history, which is a very logical thing to do. And while operating in that mode, there is no way to accept more than one "base".

So, I would be PERFECTLY HAPPY if this new mode of operation can take only one "base" tree object, if it allows BASE, PARENT1, and PARENT2 to be all tree objects, not necessarily commit objects.

But if we insist that PARENT1 and PARENT2 must be commits, then I would find it very unsatisfying if we only took a single BASE that also must be a commit. If the merge-base has to be a tree or trees, then there is no way to perform recursive merge (because you cannot compute common ancestors across tree objects) , so it is perfectly justifiable to take only a single base (and error out upon seeing a second --merge-base=X option).

But it has to be a commit, then there is no justification to forbid recursive operation, and I do not see a good reason to take only one COMMON thing.

So, it is easy to say "let's take the current patch as a good first step", but it is unclear what the good second step would be.

We could correct the code to allow PARENT1, PARENT2 and BASE to be tree objects, not necessarily commits, when only a single merge-base is specified. That corrects the current flaw that tree objects cannot be used. And then when more than one BASE is given (or no BASE is given---which is the original code), we could correct the code to call merge_incore_recursive() instead.

But the amount of the effort to get there from the current codebase (without your patch) feels more or less the same ballpark as the patch in question, so...

Yeah, I think so.
In fact, I planned to implement a version that specifies *only one* merge-base at the beginning and made the format of this option would be possible to extend to support multiple bases and octopus at the same time.

I prefer this solution.

It means we need to support specifying multiple bases in the first version, which makes merge-tree command more complete. Since individual merge-tree support multiple bases, --stdin mode seems to need to support this too.

However, I can't think of when the user needs to manually specify multiple bases for a merge ;). In other words, do we really need to support multiple bases in the first version?

Thanks,
Kyle

From: Kyle Zhao <[email protected]>

The previous commit added a --merge-base option in order to allow using a specified merge-base for the merge. Extend the input accepted by --stdin to also allow a specified merge-base with each merge requested. For example:

    printf "<b3> -- <b1> <b2>" | git merge-tree --stdin

does a merge of b1 and b2, and uses b3 as the merge-base.

---
 Documentation/git-merge-tree.txt | 14 ++++++++++++-
 builtin/merge-tree.c             | 21 +++++++++++++++++--
 t/t4301-merge-tree-write-tree.sh | 35 ++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+), 3 deletions(-)

index d9dacb2ce54..298c133fdb6 100644
@@ -66,7 +66,8 @@ OPTIONS
 
 --merge-base=<commit>::
 	Instead of finding the merge-bases for <branch1> and <branch2>,
-	specify a merge-base for the merge.
+	specify a merge-base for the merge. This option is incompatible
+	with `--stdin`.
 
 [[OUTPUT]]
 OUTPUT

@@ -220,6 +221,17 @@ with linkgit:git-merge[1]:
   * any messages that would have been printed to stdout (the
     <<IM,Informational messages>>)
 
+INPUT FORMAT
+------------
+'git merge-tree --stdin' input format is fully text based. Each line
+has this format:
+
+	[<base-commit> -- ]<branch1> <branch2>
+
+If one line is separated by `--`, the string before the separator is
+used for specifying a merge-base for the merge and the string after
+the separator describes the branches to be merged.
+
 MISTAKES TO AVOID
 -----------------
 

index 62c6d43cdb9..330f779e8bc 100644
@@ -557,12 +557,29 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 		while (strbuf_getline_lf(&buf, stdin) != EOF) {
 			struct strbuf **split;
 			int result;
+			const char *input_merge_base = NULL;
 
 			split = strbuf_split(&buf, ' ');
-			if (!split[0] || !split[1] || split[2])
+			if (!split[0] || !split[1])
 				die(_("malformed input line: '%s'."), buf.buf);
 			strbuf_rtrim(split[0]);
-			result = real_merge(&o, merge_base, split[0]->buf, split[1]->buf, prefix);
+			strbuf_rtrim(split[1]);
+
+			/* parse the merge-base */
+			if (!strcmp(split[1]->buf, "--")) {
+				input_merge_base = split[0]->buf;
+			}
+
+			if (input_merge_base && split[2] && split[3] && !split[4]) {
+				strbuf_rtrim(split[2]);
+				strbuf_rtrim(split[3]);
+				result = real_merge(&o, input_merge_base, split[2]->buf, split[3]->buf, prefix);
+			} else if (!input_merge_base && !split[2]) {
+				result = real_merge(&o, NULL, split[0]->buf, split[1]->buf, prefix);
+			} else {
+				die(_("malformed input line: '%s'."), buf.buf);
+			}
+
 			if (result < 0)
 				die(_("merging cannot continue; got unclean result of %d"), result);
 			strbuf_list_free(split);

index 6db96ccbaae..a8983a0edcb 100755
@@ -860,6 +860,13 @@ test_expect_success '--stdin with both a successful and a conflicted merge' '
 	test_cmp expect actual
 '
 
+
+test_expect_success '--merge-base is incompatible with --stdin' '
+	test_must_fail git merge-tree --merge-base=side1 --stdin 2>expect &&
+
+	grep "^fatal: --merge-base is incompatible with --stdin" expect
+'
+
 # specify merge-base as parent of branch2
 # git merge-tree --write-tree --merge-base=c2 c1 c3
 #   Commit c1: add file1

@@ -887,4 +894,32 @@ test_expect_success 'specify merge-base as parent of branch2' '
 	test_cmp expect actual
 '
 
+# Since the earlier tests have verified that individual merge-tree calls
+# are doing the right thing, this test case is only used to verify that
+# we can also trigger merges via --stdin, and that when we do we get
+# the same answer as running a bunch of separate merges.
+
+test_expect_success 'check the input format when --stdin is passed' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	test_commit -C repo c1 &&
+	test_commit -C repo c2 &&
+	test_commit -C repo c3 &&
+	printf "c1 c3\nc2 -- c1 c3\nc2 c3" | git -C repo merge-tree --stdin >actual &&
+
+	printf "1\0" >expect &&
+	git -C repo merge-tree --write-tree -z c1 c3 >>expect &&
+	printf "\0" >>expect &&
+
+	printf "1\0" >>expect &&
+	git -C repo merge-tree --write-tree -z --merge-base=c2 c1 c3 >>expect &&
+	printf "\0" >>expect &&
+
+	printf "1\0" >>expect &&
+	git -C repo merge-tree --write-tree -z c2 c3 >>expect &&
+	printf "\0" >>expect &&
+
+	test_cmp expect actual
+'
+
 test_done

gitgitgadget

Thanks; this version looks good to me and is ready to merge down.

Thanks for the updated round. I applied it locally, now let's see what reviewers think of it...

Thanks,
Taylor

Oops ;-). I should have looked further down in my inbox before replying. Looks like this is ready to merge down according to Elijah, so let's start cooking this in 'next'.

Thanks again.

Thanks,
Taylor

Users may specify the merge-base multiple times, such as "git merge-tree --merge-base=X --merge-base=Y A B", but found it doesn't work.

This patch I fix the description of option "--merge-base".

By the way, the amount of the effort support specifying multiple bases from the codebase without these patches feels more or less the same ballpark as these patches.

One point worth discussing is whether manually specifying multiple bases is a rarely used feature and do we need to support it in the first version.

If needed, It might take a while for me to implement it, and the feature might be delayed to the version after 2.39.

Thanks, Kyle

Changes since v1:

* Changed merge_incore_recursive() to merge_incore_nonrecursive() when merge-base is specified.
* Fixed c style problem.
* Moved commit lookup/die logic out to the parsing logic in cmd_merge_tree().
* use test_commit for test

Changes since v2:

* commit message
* Rebased on top of en/merge-tree-sequence.
* Set opt.ancestor to o->merge_base. Because opt.ancestor is a *char. To make it easier to pass parameters, I moved lookup_commit_reference_by_name() to real_ merge() again.
* Added test comment.

Changes since v3:

* support --merge-base in conjunction with --stdin

Changes since v4:

* commit message
* added input format document
* changed the input format for specifying the merge-base when --stdin is passed
* changed the output when --stdin and --merge-base are used at the same time
* add comment for test

Changes since v5:

* improved test: remove the test repo after the test; avoid sub-shell.

Changes since v6:

* fixed comment of test

Changes since v7:

* docs: fix description of the --merge-base option

Kyle Zhao (3):
merge-tree.c: add --merge-base=<commit> option merge-tree.c: allow specifying the merge-base when --stdin is passed
docs: fix description of the --merge-base option

 Documentation/git-merge-tree.txt | 16 ++++++++
 builtin/merge-tree.c             | 65 ++++++++++++++++++++++++++------
 t/t4301-merge-tree-write-tree.sh | 62 ++++++++++++++++++++++++++++++
 3 files changed, 131 insertions(+), 12 deletions(-)

base-commit: ec1edbcb56ac05e9980299b05924c5c1b51d68b4
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1397%2Fkeyu98%2Fkz%2Fmerge-tree-option-merge-base-v8
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1397/keyu98/kz/merge-tree-option-merge-base-v8
Pull-Request: https://github.com/gitgitgadget/git/pull/1397

Range-diff vs v7:

 1:  1cf1c69b8e8 = 1:  1cf1c69b8e8 merge-tree.c: add --merge-base=<commit> option
 2:  48e55d4e97c = 2:  48e55d4e97c merge-tree.c: allow specifying the merge-base when --stdin is passed
 -:  ----------- > 3:  c21466d1db0 docs: fix description of the `--merge-base` option

gitgitgadget

From: Kyle Zhao <[email protected]>

This patch will give our callers more flexibility to use git merge-tree, such as:

    git merge-tree --write-tree --merge-base=branch^ HEAD branch

This does a merge of HEAD and branch, but uses branch^ as the merge-base.

And the reason why using an option flag instead of a positional argument is to allow additional commits passed to merge-tree to be handled via an octopus merge in the future.

---
 Documentation/git-merge-tree.txt |  4 +++
 builtin/merge-tree.c             | 46 ++++++++++++++++++++++++--------
 t/t4301-merge-tree-write-tree.sh | 27 +++++++++++++++++++
 3 files changed, 66 insertions(+), 11 deletions(-)

index 04bcc416e6e..d9dacb2ce54 100644
@@ -64,6 +64,10 @@ OPTIONS
 	share no common history.  This flag can be given to override that
 	check and make the merge proceed anyway.
 
+--merge-base=<commit>::
+	Instead of finding the merge-bases for <branch1> and <branch2>,
+	specify a merge-base for the merge.
+
 [[OUTPUT]]
 OUTPUT
 ------

index cac85591b52..6db96ccbaae 100755
@@ -860,4 +860,31 @@ test_expect_success '--stdin with both a successful and a conflicted merge' '
 	test_cmp expect actual
 '
 
+# specify merge-base as parent of branch2
+# git merge-tree --write-tree --merge-base=c2 c1 c3
+#   Commit c1: add file1
+#   Commit c2: add file2 after c1
+#   Commit c3: add file3 after c2
+#   Expected: add file3, and file2 does NOT appear
+
+test_expect_success 'specify merge-base as parent of branch2' '
+	# Setup
+	test_when_finished "rm -rf base-b2-p" &&
+	git init base-b2-p &&
+	test_commit -C base-b2-p c1 file1 &&
+	test_commit -C base-b2-p c2 file2 &&
+	test_commit -C base-b2-p c3 file3 &&
+
+	# Testing
+	TREE_OID=$(git -C base-b2-p merge-tree --write-tree --merge-base=c2 c1 c3) &&
+
+	q_to_tab <<-EOF >expect &&
+	100644 blob $(git -C base-b2-p rev-parse c1:file1)Qfile1
+	100644 blob $(git -C base-b2-p rev-parse c3:file3)Qfile3
+	EOF
+
+	git -C base-b2-p ls-tree $TREE_OID >actual &&
+	test_cmp expect actual
+'
+
 test_done

gitgitgadget

From: Kyle Zhao <[email protected]>

The previous commit added a --merge-base option in order to allow using a specified merge-base for the merge. Extend the input accepted by --stdin to also allow a specified merge-base with each merge requested. For example:

    printf "<b3> -- <b1> <b2>" | git merge-tree --stdin

does a merge of b1 and b2, and uses b3 as the merge-base.

---
 Documentation/git-merge-tree.txt | 14 ++++++++++++-
 builtin/merge-tree.c             | 21 +++++++++++++++++--
 t/t4301-merge-tree-write-tree.sh | 35 ++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+), 3 deletions(-)

index d9dacb2ce54..298c133fdb6 100644
@@ -66,7 +66,8 @@ OPTIONS
 
 --merge-base=<commit>::
 	Instead of finding the merge-bases for <branch1> and <branch2>,
-	specify a merge-base for the merge.
+	specify a merge-base for the merge. This option is incompatible
+	with `--stdin`.
 
 [[OUTPUT]]
 OUTPUT

@@ -220,6 +221,17 @@ with linkgit:git-merge[1]:
   * any messages that would have been printed to stdout (the
     <<IM,Informational messages>>)
 
+INPUT FORMAT
+------------
+'git merge-tree --stdin' input format is fully text based. Each line
+has this format:
+
+	[<base-commit> -- ]<branch1> <branch2>
+
+If one line is separated by `--`, the string before the separator is
+used for specifying a merge-base for the merge and the string after
+the separator describes the branches to be merged.
+
 MISTAKES TO AVOID
 -----------------
 

index 62c6d43cdb9..330f779e8bc 100644
@@ -557,12 +557,29 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 		while (strbuf_getline_lf(&buf, stdin) != EOF) {
 			struct strbuf **split;
 			int result;
+			const char *input_merge_base = NULL;
 
 			split = strbuf_split(&buf, ' ');
-			if (!split[0] || !split[1] || split[2])
+			if (!split[0] || !split[1])
 				die(_("malformed input line: '%s'."), buf.buf);
 			strbuf_rtrim(split[0]);
-			result = real_merge(&o, merge_base, split[0]->buf, split[1]->buf, prefix);
+			strbuf_rtrim(split[1]);
+
+			/* parse the merge-base */
+			if (!strcmp(split[1]->buf, "--")) {
+				input_merge_base = split[0]->buf;
+			}
+
+			if (input_merge_base && split[2] && split[3] && !split[4]) {
+				strbuf_rtrim(split[2]);
+				strbuf_rtrim(split[3]);
+				result = real_merge(&o, input_merge_base, split[2]->buf, split[3]->buf, prefix);
+			} else if (!input_merge_base && !split[2]) {
+				result = real_merge(&o, NULL, split[0]->buf, split[1]->buf, prefix);
+			} else {
+				die(_("malformed input line: '%s'."), buf.buf);
+			}
+
 			if (result < 0)
 				die(_("merging cannot continue; got unclean result of %d"), result);
 			strbuf_list_free(split);

index 6db96ccbaae..a8983a0edcb 100755
@@ -860,6 +860,13 @@ test_expect_success '--stdin with both a successful and a conflicted merge' '
 	test_cmp expect actual
 '
 
+
+test_expect_success '--merge-base is incompatible with --stdin' '
+	test_must_fail git merge-tree --merge-base=side1 --stdin 2>expect &&
+
+	grep "^fatal: --merge-base is incompatible with --stdin" expect
+'
+
 # specify merge-base as parent of branch2
 # git merge-tree --write-tree --merge-base=c2 c1 c3
 #   Commit c1: add file1

@@ -887,4 +894,32 @@ test_expect_success 'specify merge-base as parent of branch2' '
 	test_cmp expect actual
 '
 
+# Since the earlier tests have verified that individual merge-tree calls
+# are doing the right thing, this test case is only used to verify that
+# we can also trigger merges via --stdin, and that when we do we get
+# the same answer as running a bunch of separate merges.
+
+test_expect_success 'check the input format when --stdin is passed' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	test_commit -C repo c1 &&
+	test_commit -C repo c2 &&
+	test_commit -C repo c3 &&
+	printf "c1 c3\nc2 -- c1 c3\nc2 c3" | git -C repo merge-tree --stdin >actual &&
+
+	printf "1\0" >expect &&
+	git -C repo merge-tree --write-tree -z c1 c3 >>expect &&
+	printf "\0" >>expect &&
+
+	printf "1\0" >>expect &&
+	git -C repo merge-tree --write-tree -z --merge-base=c2 c1 c3 >>expect &&
+	printf "\0" >>expect &&
+
+	printf "1\0" >>expect &&
+	git -C repo merge-tree --write-tree -z c2 c3 >>expect &&
+	printf "\0" >>expect &&
+
+	test_cmp expect actual
+'
+
 test_done

gitgitgadget

From: Kyle Zhao <[email protected]>

---
 Documentation/git-merge-tree.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 298c133fdb6..88ee942101a 100644
@@ -66,8 +66,8 @@ OPTIONS
 
 --merge-base=<commit>::
 	Instead of finding the merge-bases for <branch1> and <branch2>,
-	specify a merge-base for the merge. This option is incompatible
-	with `--stdin`.
+	specify a merge-base for the merge, and specifying multiple bases is
+	currently not supported. This option is incompatible with `--stdin`.
 
 [[OUTPUT]]
 OUTPUT

gitgitgadget