Re: [PATCH v2] merge-tree.c: add --merge-base=<commit> option

From: Elijah Newren <newren@gmail.com>
Date: Oct 29, 2022, 01:32 UTC
Message-ID: <CABPp-BHiqW2zgt4UfnH5iJbOf6RzPnnw+=bzvRYdrLQs08hU5g@mail.gmail.com>
In-Reply-To: <pull.1397.v2.git.1666958144017.gitgitgadget@gmail.com>

Hi,

On Fri, Oct 28, 2022 at 4:55 AM Kyle Zhao via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 10 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
>
> 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.

Show 17 quoted lines

> 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.

Show 11 quoted lines

> @@ -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.

Show 20 quoted lines

>                 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.  :-)

Show 13 quoted lines

> +       ) &&
> +       # 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.

Show 6 quoted lines

> +       )
> +'
> +
>  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.