Re: [PATCH 07/12] merge-tree: support including merge messages in output

From: Christian Couder <christian.couder@gmail.com>
Date: Jan 26, 2022, 10:42 UTC
Message-ID: <CAP8UFD0fRTw0Uh6oWNtCdotRb3F6fvZnpUwm=vT0qzmfeuBvEQ@mail.gmail.com>
In-Reply-To: <2f296aeeefbf8340cfb8b7fa4fef5ad49c8b4aa1.1642888562.git.gitgitgadget@gmail.com>

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

Show 9 quoted lines

>  EXIT STATUS
>  -----------
> @@ -72,7 +102,8 @@ be used as a part of a series of steps such as
>
>  However, it does not quite fit into the same category of low-level
>  plumbing commands since the possibility of merge conflicts give it a
> -much higher chance of the command not succeeding.
> +much higher chance of the command not succeeding (and NEWTREE containing
> +a bunch of stuff other than just a toplevel tree).

Is this hunk really related to this commit or should it go into a previous commit?

Show 8 quoted lines

> @@ -440,22 +441,30 @@ static int real_merge(struct merge_tree_options *o,
>                 commit_list_insert(j->item, &merge_bases);
>
>         merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
> -       printf("%s\n", oid_to_hex(&result.tree->object.oid));
> +
>         if (result.clean < 0)
>                 die(_("failure to merge"));

So this addresses the comment I made in a previous commit related to the fact that if result.clean < 0 we might not have a valid tree that we can print. I think though that it would be better if that was addressed in a previous commit.

> -       else if (!result.clean)
> -               printf(_("Conflicts!\n"));

Ok, so we don't print "Conflicts!\n" now, which makes me wonder if we should have printed it in the first place in previous commits.

>         if (o.real && o.trivial)
>                 die(_("--write-tree and --trivial-merge are incompatible"));
> +       if (!o.real && original_argc < argc)
> +               die(_("--write-tree must be specified if any other options are"));

Is this necessary? It looks to me like another thing that would be simplified if we were just adding a new command...