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

From: Elijah Newren <newren@gmail.com>
Date: Jan 29, 2022, 04:52 UTC
Message-ID: <CABPp-BGVCdSoVowgmM17xCiuYLwfGVLvLm=Ue8tPB+q+N3oB6g@mail.gmail.com>
In-Reply-To: <CAP8UFD0fRTw0Uh6oWNtCdotRb3F6fvZnpUwm=vT0qzmfeuBvEQ@mail.gmail.com>

On Wed, Jan 26, 2022 at 2:42 AM Christian Couder <christian.couder@gmail.com> wrote:

Show 16 quoted lines

>
> On Sat, Jan 22, 2022 at 10:56 PM Elijah Newren via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>
> >  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.

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

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.

Show 7 quoted lines

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