On Wed, Jan 26, 2022 at 2:42 AM Christian Couder
<christian.couder@xxxxxxxxx> wrote:
>
> On Sat, Jan 22, 2022 at 10:56 PM Elijah Newren via GitGitGadget
> <gitgitgadget@xxxxxxxxx> 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.
> > @@ -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.
> > 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.
