Re: [PATCH v3] merge-tree.c: add --merge-base=<commit> option
- From
Elijah Newren <newren@gmail.com>
- Date
- Nov 1, 2022, 01:08 UTC
- Message-ID
- <CABPp-BF1kjWMS7Te31FqGrHMNOQEGhG=e1g9ah29ZHWGYufbXA@mail.gmail.com>
- In-Reply-To
- <pull.1397.v3.git.1667014975042.gitgitgadget@gmail.com>
Hi,
On Fri, Oct 28, 2022 at 8:42 PM Kyle Zhao via GitGitGadget <gitgitgadget@gmail.com> wrote:
Show 49 quoted lines
> > 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 > > 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 <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_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 >
[...]
Show 133 quoted lines
> 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(-)
>
> diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
> index 04bcc416e6e..d9dacb2ce54 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 fe853aa8f91..cd25aac6ba6 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"
> @@ -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()
> };
>
> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
> index cac85591b52..5b0073d3dcd 100755
> --- a/t/t4301-merge-tree-write-tree.sh
> +++ b/t/t4301-merge-tree-write-tree.sh
> @@ -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: ec1edbcb56ac05e9980299b05924c5c1b51d68b4This 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.