Re: [PATCH 04/12] merge-tree: implement real merges

From: Christian Couder <christian.couder@gmail.com>
Date: Jan 26, 2022, 09:44 UTC
Message-ID: <CAP8UFD3_Dx2aZObbUp7dHkFCp5w9mJG-s03Soz=9YDU=yE2NoQ@mail.gmail.com>
In-Reply-To: <05bd17686e1404c81542b6bbf69dcd3decb83c5b.1642888562.git.gitgitgadget@gmail.com>

On Sat, Jan 22, 2022 at 10:56 PM Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 12 quoted lines

>
> From: Elijah Newren <newren@gmail.com>
>
> This adds the ability to perform real merges rather than just trivial
> merges (meaning handling three way content merges, recursive ancestor
> consolidation, renames, proper directory/file conflict handling, and so
> forth).  However, unlike `git merge`, the working tree and index are
> left alone and no branch is updated.
>
> The only output is:
>   - the toplevel resulting tree printed on stdout
>   - exit status of 0 (clean) or 1 (conflicts present)

The exit status can now actually be something other than 0 and 1 according to the doc and code below.

Show 5 quoted lines

> +Performs a merge, but does not make any new commits and does not read
> +from or write to either the working tree or index.
> +
> +The first form will merge the two branches, doing a full recursive
> +merge with rename detection.

Maybe this could already tell that the first form will also write a tree with the result of the merge (even in case of conflict) as this could help understand the reason why the associated option is called '--write-tree'. It could also help to say that we call such a merge a 'real' merge.

> The rest of this manual (other than the
> +next paragraph) describes the first form in more detail -- including
> +options, output format, exit status, and usage notes.

Show 15 quoted lines

>  static int real_merge(struct merge_tree_options *o,
>                       const char *branch1, const char *branch2)
>  {
> -       die(_("real merges are not yet implemented"));
> +       struct commit *parent1, *parent2;
> +       struct commit_list *common;
> +       struct commit_list *merge_bases = NULL;
> +       struct commit_list *j;
> +       struct merge_options opt;
> +       struct merge_result result = { 0 };
> +
> +       parent1 = get_merge_parent(branch1);
> +       if (!parent1)
> +               help_unknown_ref(branch1, "merge",
> +                                _("not something we can merge"));

The second argument is supposed to be the command (it's called "cmd"), so maybe "merge-tree" instead of "merge".

> +       parent2 = get_merge_parent(branch2);
> +       if (!parent2)
> +               help_unknown_ref(branch2, "merge",
> +                                _("not something we can merge"));

idem

> +       opt.show_rename_progress = 0;
> +
> +       opt.branch1 = merge_remote_util(parent1)->name; /* or just branch1? */
> +       opt.branch2 = merge_remote_util(parent2)->name; /* or just branch2? */

I think just:

       opt.branch1 = branch1
       opt.branch2 = branch2

might be better for users as it should show the name as it was passed to the command.

> +       merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
> +       printf("%s\n", oid_to_hex(&result.tree->object.oid));

I wonder if we can actually always output a valid tree when result.clean < 0. In case we might not, the printing should go a few lines below.

> +       if (result.clean < 0)
> +               die(_("failure to merge"));
> +       else if (!result.clean)

The "else" is not necessary above.

> +               printf(_("Conflicts!\n"));
> +       merge_finalize(&opt, &result);
> +       return !result.clean; /* result.clean < 0 handled above */
>  }

Show 5 quoted lines

> diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh
> new file mode 100755
> index 00000000000..e03688515c5
> --- /dev/null
> +++ b/t/t4301-merge-tree-real.sh

I wonder if it would be better named 't4301-merge-tree-write-tree.sh'...

> @@ -0,0 +1,87 @@
> +#!/bin/sh
> +
> +test_description='git merge-tree --write-tree'

... especially given this description.