| Message ID | pull.1397.v2.git.1666958144017.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] merge-tree.c: add --merge-base=<commit> option | expand |
Hi, On Fri, Oct 28, 2022 at 4:55 AM Kyle Zhao via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Kyle Zhao <kylezhao@tencent.com> > > 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. 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. > Signed-off-by: Kyle Zhao <kylezhao@tencent.com> > --- > 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. I think you mean the other way around (using merge_incore_nonrecursive() instead of merge_incore_recursive() when merge-base is specified). > + if (o->base_commit) { > + struct tree *base_tree, *parent1_tree, *parent2_tree; > + > + opt.ancestor = "specified merge base"; It is a specified merge base, but that won't help the user much when they get conflict markers if they attempt to investigate them. Since the specified merge base is a commit the user will know, and in fact one they named on the command line, we should use that name. So, I'd expect this to be set to o->merge_base. > @@ -544,8 +561,14 @@ 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); > + } Personally, I think of the code before "/* Do the relevant type of merge */" as a continuation of option parsing (i.e. sanity checking arguments and determining defaults and whatnot), and I think the last five lines above are more option parsing. From that angle, I think it'd make sense to move these lines out above this block (before or after determining o.mode). But this may well just be personal preference; what you have certainly works. > return real_merge(&o, argv[0], argv[1], prefix); > + } > else > return trivial_merge(argv[0], argv[1], argv[2]); > } > diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh > index 013b77144bd..64bfe6f4a41 100755 > --- a/t/t4301-merge-tree-write-tree.sh > +++ b/t/t4301-merge-tree-write-tree.sh > @@ -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 Much simpler. :-) > + ) && > + # 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 In particular, you are testing here that file2 does NOT appear despite the fact that it was part of c3. That makes sense, but I'm not sure casual readers of this code would catch that. It might be worth adding a comment to make it easier to follow for future test readers. > + ) > +' > + > test_done > > base-commit: 5af5e54106e20f65c913550c80aec3186b859e9b 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.
diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt index d6c356740ef..e762209b76d 100644 --- a/Documentation/git-merge-tree.txt +++ b/Documentation/git-merge-tree.txt @@ -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 ------ diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c index ae5782917b9..089ea8fac81 100644 --- a/builtin/merge-tree.c +++ b/builtin/merge-tree.c @@ -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; + const struct commit *base_commit; }; static int real_merge(struct merge_tree_options *o, @@ -430,16 +432,26 @@ 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->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); + } - merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result); if (result.clean < 0) die(_("failure to merge")); @@ -478,6 +490,7 @@ 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; @@ -505,6 +518,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", + &merge_base, + N_("commit"), + N_("specify a merge-base for the merge")), OPT_END() }; @@ -544,8 +561,14 @@ 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]); } diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh index 013b77144bd..64bfe6f4a41 100755 --- a/t/t4301-merge-tree-write-tree.sh +++ b/t/t4301-merge-tree-write-tree.sh @@ -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