On Fri, Jan 28, 2022 at 8:37 AM Johannes Schindelin
<Johannes.Schindelin@xxxxxx> wrote:
>
> Hi Elijah,
>
> On Sat, 22 Jan 2022, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@xxxxxxxxx>
> >
> > When running `git merge-tree --write-tree`, we previously would only
> > return an exit status reflecting the cleanness of a merge, and print out
> > the toplevel tree of the resulting merge. Merges also have
> > informational messages, ("Auto-merging <PATH>", "CONFLICT (content):
> > ...", "CONFLICT (file/directory)", etc.) In fact, when non-content
> > conflicts occur (such as file/directory, modify/delete, add/add with
> > differing modes, rename/rename (1to2), etc.), these informational
> > messages are often the only notification since these conflicts are not
> > representable in the contents of the file.
> >
> > Add a --[no-]messages option so that callers can request these messages
> > be included at the end of the output. Include such messages by default
> > when there are conflicts, and omit them by default when the merge is
> > clean.
>
> Makes sense.
>
> > diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> > index 0c19639594d..560640ad911 100644
> > --- a/builtin/merge-tree.c
> > +++ b/builtin/merge-tree.c
> > @@ -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"));
> > - else if (!result.clean)
> > - printf(_("Conflicts!\n"));
> > +
> > + if (o->show_messages == -1)
> > + o->show_messages = !result.clean;
> > +
> > + printf("%s\n", oid_to_hex(&result.tree->object.oid));
> > + if (o->show_messages) {
> > + printf("\n");
> > + merge_display_update_messages(&opt, &result, stdout);
> > + }
>
> Excellent.
>
> > merge_finalize(&opt, &result);
> > return !result.clean; /* result.clean < 0 handled above */
> > }
> >
> > int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> > {
> > - struct merge_tree_options o = { 0 };
> > + struct merge_tree_options o = { .show_messages = -1 };
> > int expected_remaining_argc;
> > + int original_argc;
> >
> > const char * const merge_tree_usage[] = {
> > - N_("git merge-tree [--write-tree] <branch1> <branch2>"),
> > + N_("git merge-tree [--write-tree] [<options>] <branch1> <branch2>"),
> > N_("git merge-tree [--trivial-merge] <base-tree> <branch1> <branch2>"),
> > NULL
> > };
> > @@ -464,6 +473,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> > N_("do a real merge instead of a trivial merge")),
> > OPT_BOOL(0, "trivial-merge", &o.trivial,
> > N_("do a trivial merge only")),
> > + OPT_BOOL(0, "messages", &o.show_messages,
> > + N_("also show informational/conflict messages")),
> > OPT_END()
> > };
> >
> > @@ -472,10 +483,13 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> > usage_with_options(merge_tree_usage, mt_options);
> >
> > /* Parse arguments */
> > + original_argc = argc;
> > argc = parse_options(argc, argv, prefix, mt_options,
> > merge_tree_usage, 0);
> > 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"));
>
> Hmm. Well. Hmm.
>
>
> I'd rather keep `--write-tree` neat and optional. What's wrong with
> allowing
>
> git merge-tree --no-messages HEAD MERGE_HEAD
>
> ?
Yeah, oops. Nothing is wrong with that...but do note that there is
something wrong with this:
git merge-tree --no-messages MERGE_BASE HEAD MERGE_HEAD
because the three argument form is signalling the deprecated trivial
merge, and the trivial merge code doesn't handle any options. You
were right to call out my code since I placed it too early and made it
a bit obtuse. I've changed it to:
if (o.mode == 't' && original_argc < argc)
die(_("--trivial-merge is incompatible with all other options"));
where o.mode is guaranteed to be set before that check.
> To be clear, I think we need this instead:
>
> if (o.trivial && o.show_messages >= 0)
> die(_("--trivial-merge is incompatible with additional options"));
Most of that is better, assuming the o.trivial check handles the
implicit case too. Thanks. I'm not so sure about the check on
o.show_messages, though, because then I'll have to keep adding
additional checks for each new additional option (and future patches
will add more). So, I think I'll keep the original_argc < argc part
of the check instead of that. :-)
> I like the rest of the patch very much!
Thanks!
