On Fri, Jan 28, 2022 at 8:37 AM Johannes Schindelin
[off-list ref] wrote:

Hi Elijah,

On Sat, 22 Jan 2022, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>

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.

quoted

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.

quoted

      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!