On Wed, Jan 26, 2022 at 2:42 AM Christian Couder
[off-list ref] wrote:

On Sat, Jan 22, 2022 at 10:56 PM Elijah Newren via GitGitGadget
[off-list ref] wrote:

quoted

 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?

It's meant to first be related to this commit, though as you pointed
out below, I had some accidental stuff not cleaned out of the earlier
commit.

quoted

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

quoted

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

Yep, good flag on both of these last two comments.  When I was fixing
this up I didn't squash it back in early enough.  Thanks for reading
carefully.

quoted

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

I think the code is harder to read than it should be.  I changed it to:

    if (o.mode == 't' && original_argc < argc)
        die(_("--trivial-merge is incompatible with all other options"));

which I think is clearer; it points out that it's only about the
deprecated --trivial-merge option and how it's incompatible with all
other options.