RFC: In-core git merge-tree ("Server side merges")

Quoted 3 lines · Ævar Arnfjörð Bjarmason

On Thu, Feb 03 2022, Elijah Newren wrote:

[...]

Quoted 19 lines

Quoted 9 lines

...also, even if we did require the --write-tree flag, we'd still have to look at argc. Since the option parsing handles both modes, someone could leave off --write-tree, but include a bunch of other options that only make sense with --write-tree. Individually checking the setting of every extra flag along with write_tree is a royal pain and I don't want to repeat that for each new option added. Simply checking argc allows you to provide an error message if the user does that.

Quoted 9 lines

(And I think it's sad that in Git we often forgot to warn and notify users of options that are only functional with certain other arguments; it makes it harder for users to figure out, and has in the past even made it harder for other developers to figure out what was meant and how things are to be used. I think I've seen multiple Git devs be confused over ls-files --directory and --no-empty-directory options, assuming they'd do something sensible for tracked files, when in fact those arguments are simply ignored because they are only modifiers for how untracked files are treated.)

Quoted 4 lines

There's a much simpler way to do what you're trying to do here which is to only parse --write-tree, and as soon as you have that pass off two one function or the other, and have those functions call parse_options().

But that makes --write-tree a mandatory argument when trying to use that mode, right? If so, that is not a simpler way to do what I'm trying to do at all; it breaks my intended usage.

--write-tree is a documentation-only construct that users should never have to pass.

Also, what happens if we remove the --trivial-merge flag and its whole mode after a sufficient deprecation period? Would the --write-tree parameter remain required in your model to select the only existing mode, simply due to us having gone through a transition period?

On Thu, Feb 03 2022, Elijah Newren wrote:

Quoted 11 lines

But that makes --write-tree a mandatory argument when trying to use that mode, right? If so, that is not a simpler way to do what I'm trying to do at all; it breaks my intended usage.

--write-tree is a documentation-only construct that users should never have to pass.

Also, what happens if we remove the --trivial-merge flag and its whole mode after a sufficient deprecation period? Would the --write-tree parameter remain required in your model to select the only existing mode, simply due to us having gone through a transition period?

You can have your cake and eat it too by running parse_optionss() N number of times. Although perhaps in this case the end result isn't worth it.

I was hoping this could be a simpler case of a subcommand dispatch, and perhaps it can still be generalized to that.

If the "trivial" mode never takes options and always 3 argv elements, we could just run parse_options() for it with no options, after checking that we have 3 arguments, and none start with '-'.

But the below is a generalization of this I tried out just now, it passes all your tests, and means that whenever you add new options you don't need to keep saying "no, not with the trivial mode" for each one.

Basically we run parse_options() once with the full set of options, and save away argc/argv (note the lack of strvec_clear() there, that's a TODO memory leak).

Then we've got a o.mode, which along with argc is the *only* thing we pay attention to at that point.

Then we dispatch to the "trivial" or "write" functions, which do parse_options() again, this time with only their options.

It means that e.g. this now works as expected:

./git merge-tree --trivial-merge -z origin/{master,next,seen}
error: unknown switch `z'
usage: git merge-tree [--trivial-merge] <base-tree> <branch1> <branch2>

--trivial-merge do a trivial merge only

I.e. we error out, with your verison we'll just ignore the -z.

Your "--trivial-merge is incompatible with all other options" doesn't work as you expect, and is buggy whether you want to go this route or not, as the added tests show.

Basically it does nothing at all. Because if you add --foo we'll die before we get there, parse_options() will die for us.

But if you do --trivial-merge -z your argc/argv will be trimmed, because -z is a known option. So your check is doing nothing.

So, here it is in all its glory :) A bit nasty for sure, but IMO preferrable to an ever expanding list of "X isn't compatible with A".

builtin/merge-tree.c +98 -43

index 58c0ddc5a32..1d47912816d 100644
@@ -12,6 +12,7 @@
 #include "exec-cmd.h"
 #include "merge-blobs.h"
 #include "quote.h"
+#include "strvec.h"
 
 static int line_termination = '\n';
 

@@ -371,13 +372,66 @@ static void *get_tree_descriptor(struct repository *r,
        return buf;
 }
 
-static int trivial_merge(const char *base,
-                        const char *branch1,
-                        const char *branch2)
+struct merge_tree_options {
+       int mode;
+       int allow_unrelated_histories;
+       int show_messages;
+       int exclude_modes_oids_stages;
+};
+
+#define BUILTIN_MERGE_TREE_USAGE_WRITE \
+               N_("git merge-tree [--write-tree] [<options>] <branch1> <branch2>")
+#define BUILTIN_MERGE_TREE_USAGE_TRIVIAL \
+               N_("git merge-tree [--trivial-merge] <base-tree> <branch1> <branch2>")
+
+#define BUILTIN_MERGE_TREE_OPT_CMDMODE_TRIVIAL \
+               OPT_CMDMODE(0, "trivial-merge", &o.mode, \
+                           N_("do a trivial merge only"), 't')
+
+#define BUILTIN_MERGE_TREE_OPT_CMDMODE_WRITE \
+               OPT_CMDMODE(0, "write-tree", &o.mode, \
+                           N_("do a real merge instead of a trivial merge"), \
+                           'w')
+
+#define BUILTIN_MERGE_TREE_OPT_WRITE \
+               BUILTIN_MERGE_TREE_OPT_CMDMODE_WRITE, \
+               OPT_BOOL(0, "messages", &o.show_messages, \
+                        N_("also show informational/conflict messages")), \
+               OPT_SET_INT('z', NULL, &line_termination, \
+                           N_("separate paths with the NUL character"), '\0'), \
+               OPT_BOOL_F('l', "exclude-modes-oids-stages", \
+                          &o.exclude_modes_oids_stages, \
+                          N_("list conflicted files without modes/oids/stages"), \
+                          PARSE_OPT_NONEG), \
+               OPT_BOOL_F(0, "allow-unrelated-histories", \
+                          &o.allow_unrelated_histories, \
+                          N_("allow merging unrelated histories"), \
+                          PARSE_OPT_NONEG)
+
+static int trivial_merge(int argc, const char **argv, const char *prefix)
 {
        struct repository *r = the_repository;
        struct tree_desc t[3];
        void *buf1, *buf2, *buf3;
+       struct merge_tree_options o = { 0 };
+       const char * const usage[] = {
+               BUILTIN_MERGE_TREE_USAGE_TRIVIAL,
+               NULL,
+       };
+       struct option options[] = {
+               BUILTIN_MERGE_TREE_OPT_CMDMODE_TRIVIAL,
+               OPT_END()
+       };
+       const char *base, *branch1, *branch2;
+
+       argc = parse_options(argc, argv, prefix, options, usage,
+                            PARSE_OPT_STOP_AT_NON_OPTION);
+       if (argc != 3)
+               BUG("should have ensured remaining argc == 3 already! Got %d", argc);
+
+       base = argv[0];
+       branch1 = argv[1];
+       branch2 = argv[2];;
 
        buf1 = get_tree_descriptor(r, t+0, base);
        buf2 = get_tree_descriptor(r, t+1, branch1);

@@ -391,24 +445,34 @@ static int trivial_merge(const char *base,
        return 0;
 }
 
-struct merge_tree_options {
-       int mode;
-       int allow_unrelated_histories;
-       int show_messages;
-       int exclude_modes_oids_stages;
-};
-
-static int real_merge(struct merge_tree_options *o,
-                     const char *branch1, const char *branch2,
-                     const char *prefix)
+static int real_merge(int argc, const char **argv, const char *prefix)
 {
        struct commit *parent1, *parent2;
        struct commit_list *common;
        struct commit_list *merge_bases = NULL;
        struct commit_list *j;
+       struct merge_tree_options o = { .show_messages = 1 };
        struct merge_options opt;
        struct merge_result result = { 0 };
 
+       const char * const usage[] = {
+               BUILTIN_MERGE_TREE_USAGE_WRITE,
+               NULL,
+       };
+       struct option options[] = {
+               BUILTIN_MERGE_TREE_OPT_CMDMODE_WRITE,
+               BUILTIN_MERGE_TREE_OPT_WRITE,
+               OPT_END()
+       };
+       const char *branch1, *branch2;
+
+       argc = parse_options(argc, argv, prefix, options, usage,
+                            PARSE_OPT_STOP_AT_NON_OPTION);
+       if (argc != 2)
+               BUG("should have ensured remaining argc == 2 already! Got %d", argc);
+       branch1 = argv[0];
+       branch2 = argv[1];
+
        parent1 = get_merge_parent(branch1);
        if (!parent1)
                help_unknown_ref(branch1, "merge-tree",

@@ -431,7 +495,7 @@ static int real_merge(struct merge_tree_options *o,
         * merge_incore_recursive in merge-ort.h
         */
        common = get_merge_bases(parent1, parent2);
-       if (!common && !o->allow_unrelated_histories)
+       if (!common && !o.allow_unrelated_histories)
                die(_("refusing to merge unrelated histories"));
        for (j = common; j; j = j->next)
                commit_list_insert(j->item, &merge_bases);

@@ -440,8 +504,8 @@ static int real_merge(struct merge_tree_options *o,
        if (result.clean < 0)
                die(_("failure to merge"));
 
-       if (o->show_messages == -1)
-               o->show_messages = !result.clean;
+       if (o.show_messages == -1)
+               o.show_messages = !result.clean;
 
        puts(oid_to_hex(&result.tree->object.oid));
        if (!result.clean) {

@@ -453,7 +517,7 @@ static int real_merge(struct merge_tree_options *o,
                for (i = 0; i < conflicted_files.nr; i++) {
                        const char *name = conflicted_files.items[i].string;
                        struct stage_info *c = conflicted_files.items[i].util;
-                       if (!o->exclude_modes_oids_stages)
+                       if (!o.exclude_modes_oids_stages)
                                printf("%06o %s %d\t",
                                       c->mode, oid_to_hex(&c->oid), c->stage);
                        else if (last && !strcmp(last, name))

@@ -464,7 +528,7 @@ static int real_merge(struct merge_tree_options *o,
                }
                string_list_clear(&conflicted_files, 1);
        }
-       if (o->show_messages) {
+       if (o.show_messages) {
                putchar(line_termination);
                merge_display_update_messages(&opt, &result, stdout);
        }

@@ -474,40 +538,32 @@ static int real_merge(struct merge_tree_options *o,
 
 int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 {
-       struct merge_tree_options o = { .show_messages = -1 };
+       struct merge_tree_options o;
        int expected_remaining_argc;
-       int original_argc;
-
+       int original_argc = argc;
+       struct strvec original_args = STRVEC_INIT;
        const char * const merge_tree_usage[] = {
-               N_("git merge-tree [--write-tree] [<options>] <branch1> <branch2>"),
-               N_("git merge-tree [--trivial-merge] <base-tree> <branch1> <branch2>"),
+               BUILTIN_MERGE_TREE_USAGE_WRITE,
+               BUILTIN_MERGE_TREE_USAGE_TRIVIAL,
                NULL
        };
        struct option mt_options[] = {
-               OPT_CMDMODE(0, "write-tree", &o.mode,
-                           N_("do a real merge instead of a trivial merge"),
-                           'w'),
-               OPT_CMDMODE(0, "trivial-merge", &o.mode,
-                           N_("do a trivial merge only"), 't'),
-               OPT_BOOL(0, "messages", &o.show_messages,
-                        N_("also show informational/conflict messages")),
-               OPT_SET_INT('z', NULL, &line_termination,
-                           N_("separate paths with the NUL character"), '\0'),
-               OPT_BOOL_F('l', "exclude-modes-oids-stages",
-                          &o.exclude_modes_oids_stages,
-                          N_("list conflicted files without modes/oids/stages"),
-                          PARSE_OPT_NONEG),
-               OPT_BOOL_F(0, "allow-unrelated-histories",
-                          &o.allow_unrelated_histories,
-                          N_("allow merging unrelated histories"),
-                          PARSE_OPT_NONEG),
+               BUILTIN_MERGE_TREE_OPT_CMDMODE_TRIVIAL,
+               BUILTIN_MERGE_TREE_OPT_CMDMODE_WRITE,
+               BUILTIN_MERGE_TREE_OPT_WRITE,
                OPT_END()
        };
 
+       /* We only care about deciding "o.mode" here */
+       o.mode = 0;
+       /*
+        * We need our original argv, and
+        * PARSE_OPT_KEEP_{ARGV0,UNKNOWN} would do the wrong thing
+        */
+       strvec_pushv(&original_args, argv);
        /* Parse arguments */
-       original_argc = argc;
        argc = parse_options(argc, argv, prefix, mt_options,
-                            merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+                            merge_tree_usage, 0);
        if (o.mode) {
                expected_remaining_argc = (o.mode == 'w' ? 2 : 3);
                if (argc != expected_remaining_argc)

@@ -517,12 +573,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
                        usage_with_options(merge_tree_usage, mt_options);
                o.mode = (argc == 2 ? 'w' : 't');
        }
-       if (o.mode == 't' && original_argc < argc)

t/t4301-merge-tree-write-tree.sh +1 -0

index 4de089d976d..749bdb6862d 100755
@@ -92,6 +92,18 @@ test_expect_success 'Barf on too many arguments' '
        grep "^usage: git merge-tree" expect
 '
 
+for opt in $(git merge-tree --git-completion-helper-all)

Quoted 11 lines · Ævar Arnfjörð Bjarmason

Quoted 7 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

Let merge-tree accept a --write-tree parameter for choosing real merges instead of trivial merges, and accept an optional --trivial-merge option to get the traditional behavior. Note that these accept different numbers of arguments, though, so these names need not actually be used.

Quoted 6 lines

Maybe that ship has sailed, but just my 0.02: I thought this whole thing was much less confusing with your initial merge-tree-ort proposal at
https://lore.kernel.org/git/CABPp-BEeBpJoU4yXdfA6vRAYVAUbd2gRhEV6j4VEqoqcu=FGSw@mail.gmail.com/;
I.e. the end-state of merge-tree.c is that you end up reading largely unrelated code (various static functions only used by one side or another).

Christian's merge-tree-ort proposal?

Quoted 46 lines

But maybe that's all water under the bridge etc, however...

Quoted 24 lines

int cmd_merge_tree(int argc, const char **argv, const char *prefix) {

builtin/merge-tree.c +19 -3

-     if (argc != 4)
-             usage(merge_tree_usage);
-     return trivial_merge(argc, argv);
+     struct merge_tree_options o = { 0 };
+     int expected_remaining_argc;
+
+     const char * const merge_tree_usage[] = {
+             N_("git merge-tree [--write-tree] <branch1> <branch2>"),
+             N_("git merge-tree [--trivial-merge] <base-tree> <branch1> <branch2>"),
+             NULL
+     };
+     struct option mt_options[] = {
+             OPT_CMDMODE(0, "write-tree", &o.mode,
+                         N_("do a real merge instead of a trivial merge"),
+                         'w'),
+             OPT_CMDMODE(0, "trivial-merge", &o.mode,
+                         N_("do a trivial merge only"), 't'),
+             OPT_END()
+     };
+
+     /* Parse arguments */
+     argc = parse_options(argc, argv, prefix, mt_options,

builtin/merge-tree.c static int trivial_merge(int argc, const char **argv) · line 428

Quoted 10 lines

builtin/merge-tree.c +10 -0

+                          merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+     if (o.mode) {
+             expected_remaining_argc = (o.mode == 'w' ? 2 : 3);
+             if (argc != expected_remaining_argc)
+                     usage_with_options(merge_tree_usage, mt_options);
+     } else {
+             if (argc < 2 || argc > 3)
+                     usage_with_options(merge_tree_usage, mt_options);
+             o.mode = (argc == 2 ? 'w' : 't');
+     }

Do we really need to make this interface more special-casey by auto-guessing based on argc what argument you want? I.e. instead of usage like:

        N_("git merge-tree [--write-tree] <branch1> <branch2>"),
        N_("git merge-tree [--trivial-merge] <base-tree> <branch1> <branch2>"),

Wouldn't it be simpler to just have the equivalent of:

Quoted 7 lines

        # old
        git merge-tree ...
        # new
        git merge-tree --new-thing ...

And not have to look at ... to figure out if we're dispatching to the new or old thing.

You seem to be focusing on code simplicity? Sure, that'd be simpler code, it'd just be a less useful feature.

I think passing --write-tree all the time would be an annoyance. I don't see why anyone would ever use the other mode. However, for as long as both exist in the manual, it makes the manual easier to explain to users, and example testcases more self-documenting by having the flag there. That's the sole purpose of the flag.

I'm never going to actually use it when I invoke it from the command line. And I suspect most would leave it off.

Let merge-tree accept a --write-tree parameter for choosing real merges instead of trivial merges, and accept an optional --trivial-merge option to get the traditional behavior. Note that these accept different numbers of arguments, though, so these names need not actually be used.

Note that real merges differ from trivial merges in that they handle:
- three way content merges
- recursive ancestor consolidation
- renames
- proper directory/file conflict handling
- etc.
Basically all the stuff you'd expect from git merge, just without updating the index and working tree. The initial shell added here does nothing more than die with "real merges are not yet implemented", but that will be fixed in subsequent commits.

Signed-off-by Elijah Newren <[email protected]>

---
 builtin/merge-tree.c | 61 +++++++++++++++++++++++++++++++++++++-------
 git.c                |  2 +-
 2 files changed, 53 insertions(+), 10 deletions(-)

builtin/merge-tree.c +23 -9

index 914ec960b7e..e98ec8a9f1d 100644
@@ -3,13 +3,12 @@
 #include "tree-walk.h"
 #include "xdiff-interface.h"
 #include "object-store.h"
+#include "parse-options.h"
 #include "repository.h"
 #include "blob.h"
 #include "exec-cmd.h"
 #include "merge-blobs.h"
 
-static const char merge_tree_usage[] = "git merge-tree <base-tree> <branch1> <branch2>";
-
 struct merge_list {
 	struct merge_list *next;
 	struct merge_list *link;	/* other stages for this object */

@@ -366,15 +365,17 @@ static void *get_tree_descriptor(struct repository *r,
 	return buf;
 }
 
-static int trivial_merge(int argc, const char **argv)
+static int trivial_merge(const char *base,
+			 const char *branch1,
+			 const char *branch2)
 {
 	struct repository *r = the_repository;
 	struct tree_desc t[3];
 	void *buf1, *buf2, *buf3;
 
-	buf1 = get_tree_descriptor(r, t+0, argv[1]);
-	buf2 = get_tree_descriptor(r, t+1, argv[2]);
-	buf3 = get_tree_descriptor(r, t+2, argv[3]);
+	buf1 = get_tree_descriptor(r, t+0, base);
+	buf2 = get_tree_descriptor(r, t+1, branch1);
+	buf3 = get_tree_descriptor(r, t+2, branch2);
 	trivial_merge_trees(t, "");
 	free(buf1);
 	free(buf2);

@@ -384,9 +385,51 @@ static int trivial_merge(int argc, const char **argv)
 	return 0;
 }
 
+struct merge_tree_options {
+	int mode;
+};
+
+static int real_merge(struct merge_tree_options *o,
+		      const char *branch1, const char *branch2)
+{
+	die(_("real merges are not yet implemented"));
+}
+
 int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 {
-	if (argc != 4)
-		usage(merge_tree_usage);
-	return trivial_merge(argc, argv);
+	struct merge_tree_options o = { 0 };
+	int expected_remaining_argc;
+
+	const char * const merge_tree_usage[] = {
+		N_("git merge-tree [--write-tree] <branch1> <branch2>"),
+		N_("git merge-tree [--trivial-merge] <base-tree> <branch1> <branch2>"),

builtin/merge-tree.c +10 -0

+
+	/* Parse arguments */
+	argc = parse_options(argc, argv, prefix, mt_options,
+			     merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+	if (o.mode) {
+		expected_remaining_argc = (o.mode == 'w' ? 2 : 3);
+		if (argc != expected_remaining_argc)
+			usage_with_options(merge_tree_usage, mt_options);
+	} else {
+		if (argc < 2 || argc > 3)

builtin/merge-tree.c static int trivial_merge(int argc, const char **argv) · line 415

Quoted 13 lines

builtin/merge-tree.c +13 -0

+	const char * const merge_tree_usage[] = {
+		N_("git merge-tree [--write-tree] <branch1> <branch2>"),
+		N_("git merge-tree [--trivial-merge] <base-tree> <branch1> <branch2>"),
+		NULL
+	};
+	struct option mt_options[] = {
+		OPT_CMDMODE(0, "write-tree", &o.mode,
+			    N_("do a real merge instead of a trivial merge"),
+			    'w'),
+		OPT_CMDMODE(0, "trivial-merge", &o.mode,
+			    N_("do a trivial merge only"), 't'),
+		OPT_END()
+	};

In the review club last week I had mentioned I thought OPT_CMDMODE worked well with enums. I found some a reasonably nice example in builtin/replace.c:cmd_replace(), although I have some Opinions about the enum declaration placement there. Regardless, I think using an enum instead of a single character would make this more readable - otherwise I need to remember what 'w' means when I'm reasoning about how many args to expect below.

builtin/merge-tree.c +21 -1

@@ -392,7 +395,46 @@ struct merge_tree_options {
 static int real_merge(struct merge_tree_options *o,
 		      const char *branch1, const char *branch2)
 {
-	die(_("real merges are not yet implemented"));
+	struct commit *parent1, *parent2;
+	struct commit_list *common;
+	struct commit_list *merge_bases = NULL;
+	struct commit_list *j;
+	struct merge_options opt;
+	struct merge_result result = { 0 };
+
+	parent1 = get_merge_parent(branch1);
+	if (!parent1)
+		help_unknown_ref(branch1, "merge-tree",
+				 _("not something we can merge"));
+
+	parent2 = get_merge_parent(branch2);
+	if (!parent2)
+		help_unknown_ref(branch2, "merge-tree",
+				 _("not something we can merge"));
+
+	init_merge_options(&opt, the_repository);
+
+	opt.show_rename_progress = 0;
+

Quoted 26 lines · Elijah Newren via GitGitGadget

builtin/merge-tree.c +21 -1

@@ -392,7 +395,46 @@ struct merge_tree_options {
 static int real_merge(struct merge_tree_options *o,
                    const char *branch1, const char *branch2)
 {
-     die(_("real merges are not yet implemented"));
+     struct commit *parent1, *parent2;
+     struct commit_list *common;
+     struct commit_list *merge_bases = NULL;
+     struct commit_list *j;
+     struct merge_options opt;
+     struct merge_result result = { 0 };
+
+     parent1 = get_merge_parent(branch1);
+     if (!parent1)
+             help_unknown_ref(branch1, "merge-tree",
+                              _("not something we can merge"));
+
+     parent2 = get_merge_parent(branch2);
+     if (!parent2)
+             help_unknown_ref(branch2, "merge-tree",
+                              _("not something we can merge"));
+
+     init_merge_options(&opt, the_repository);
+
+     opt.show_rename_progress = 0;
+

builtin/merge-tree.c struct merge_tree_options { · line 428

Quoted 10 lines

builtin/merge-tree.c +10 -0

+     opt.branch1 = branch1;
+     opt.branch2 = branch2;
+
+     /*
+      * Get the merge bases, in reverse order; see comment above
+      * merge_incore_recursive in merge-ort.h
+      */
+     common = get_merge_bases(parent1, parent2);
+     if (!common)
+             die(_("refusing to merge unrelated histories"));

It appears to me that "merge-tree" in this mode, with the above code, cannot be used as a workhorse to implement server-side cherry-pick (or revert), which needs to allow the user to specify an arbitrary "common ancestor", instead of computing on its own.

To replay the change made by commit A on top of commit X (i.e. "cherry-pick A on X"), we have to be able to say "compute the three-way merge between A and X, pretending as if A^ were their common ancestor". The story is the same for revert---we compute three-way merge between A^ and X, pretending as if A were their common ancestor.

The above interface into this function, sadly, does not seem to allow such a request, unless I am missing something.

And if I am correct, it is a shame---after all, the point of the merge-trees command is to take three trees and run a three-way merge, and not being able to merge three "trees" and require "commits" makes this mode much less useful than its potential.

Yes, you are reading right. I think the cherry-pick/rebase replacement actually deserves a separate command from what merges should use; replaying a sequence of commits just has a number of UI differences and abilities that I think pull it in a different direction.

Quoted 5 lines · Elijah Newren

Yes, you are reading right. I think the cherry-pick/rebase replacement actually deserves a separate command from what merges should use; replaying a sequence of commits just has a number of UI differences and abilities that I think pull it in a different direction.

I completely disagree. Each individual step in a sequence of replaying commits in order (or in reverse order) should be scriptable as a single merge-tree that takes "apply the change to go from A^ to A on X". Sequencing and placing UI around it is a job for the script that drives merge-tree.

Quoted 9 lines · Junio C Hamano

Quoted 5 lines · Elijah Newren

Yes, you are reading right. I think the cherry-pick/rebase replacement actually deserves a separate command from what merges should use; replaying a sequence of commits just has a number of UI differences and abilities that I think pull it in a different direction.

Quoted 5 lines

I completely disagree. Each individual step in a sequence of replaying commits in order (or in reverse order) should be scriptable as a single merge-tree that takes "apply the change to go from A^ to A on X". Sequencing and placing UI around it is a job for the script that drives merge-tree.

Adding such an ability to merge-tree would be trivial -- it basically involves just two things: (1) accepting one extra argument, and (2) calling merge_incore_nonrecursive() instead of merge_incore_recursive().

However, I think forking a subprocess for every merge of a series of commits is a completely unreasonable overhead, so even if we provide such an option to merge-tree, I still want a separate plumbing-ish tool that does non-worktree/non-index replaying of commits which is not written as a driver of merge-tree. That other tool should just call merge_incore_nonrecursive() directly. And such a tool, since it should handle an arbitrary number of commits, should certainly be able to handle just one commit. From that angle, it feels like adding another mode to merge-tree would just be a partial duplication of the other tool.

However, if the other tool doesn't obviate the need for this additional mode (perhaps it ends up being forced to be too porcelain-ish insteading of plumbing-ish?), or folks really just want another merge-tree mode, I'm happy to add one along with the tool I submit later. Does that sound reasonable to you, or is there something you're still objecting to that I've missed?

Quoted 21 lines · Elijah Newren

Quoted 9 lines · Junio C Hamano

Quoted 5 lines · Elijah Newren

Yes, you are reading right. I think the cherry-pick/rebase replacement actually deserves a separate command from what merges should use; replaying a sequence of commits just has a number of UI differences and abilities that I think pull it in a different direction.

Quoted 5 lines

I completely disagree. Each individual step in a sequence of replaying commits in order (or in reverse order) should be scriptable as a single merge-tree that takes "apply the change to go from A^ to A on X". Sequencing and placing UI around it is a job for the script that drives merge-tree.

Adding such an ability to merge-tree would be trivial -- it basically involves just two things: (1) accepting one extra argument, and (2) calling merge_incore_nonrecursive() instead of merge_incore_recursive().

Quoted 10 lines

However, I think forking a subprocess for every merge of a series of commits is a completely unreasonable overhead, so even if we provide such an option to merge-tree, I still want a separate plumbing-ish tool that does non-worktree/non-index replaying of commits which is not written as a driver of merge-tree. That other tool should just call merge_incore_nonrecursive() directly. And such a tool, since it should handle an arbitrary number of commits, should certainly be able to handle just one commit. From that angle, it feels like adding another mode to merge-tree would just be a partial duplication of the other tool.

I wonder how the UI of a tool that does non-worktree/non-index cherry-picks will look like. I'd expect it to produce the same output as merge-tree, except cherry-pick should probably output a commit OID, not a tree.

Maybe we want a unified command that produces commits from any sequence of merge/cherry-pick/revert/reword steps. The obvious UI would use something like the rebase-todo list as input. For example:

        $ echo '
        pick commit1
        reword commit2  # edit commit message in $GIT_EDITOR
        merge commit3 -m "log message"
        ' | git create-commit commit0
        <OID of final commit>

we start from commit0 and apply steps one-by-one. Obviously, one unsolved problem is how to pass parameters like commit messages if no editor should be invoked (my sketch uses -m).
If any of the steps fails when merging merge, then we get the tree with conflicts

        $ echo '
        pick commit1
        pick commit2
        pick commit-that-does-not-apply
        ' | git create-commit commit0
        <OID of commit after step 2>
        <OID of toplevel tree after failed merge>
        <Conflicted file info>
        <Informational messages>

Replaying a series of commits might look like this:

        $ echo 'pick commit1 ^commit0' | git create-commit new-base

I'm concluding that this is a difficult UI problem

and having a merge-tree
command that accepts a "common ancestor" parameter could make it easier to experiment. Of course that depends on who is experimenting.

Quoted 69 lines · Johannes Altmanninger

Quoted 21 lines · Elijah Newren

Quoted 9 lines · Junio C Hamano

Quoted 5 lines · Elijah Newren

Yes, you are reading right. I think the cherry-pick/rebase replacement actually deserves a separate command from what merges should use; replaying a sequence of commits just has a number of UI differences and abilities that I think pull it in a different direction.

Quoted 5 lines

I completely disagree. Each individual step in a sequence of replaying commits in order (or in reverse order) should be scriptable as a single merge-tree that takes "apply the change to go from A^ to A on X". Sequencing and placing UI around it is a job for the script that drives merge-tree.

Adding such an ability to merge-tree would be trivial -- it basically involves just two things: (1) accepting one extra argument, and (2) calling merge_incore_nonrecursive() instead of merge_incore_recursive().

Quoted 10 lines

However, I think forking a subprocess for every merge of a series of commits is a completely unreasonable overhead, so even if we provide such an option to merge-tree, I still want a separate plumbing-ish tool that does non-worktree/non-index replaying of commits which is not written as a driver of merge-tree. That other tool should just call merge_incore_nonrecursive() directly. And such a tool, since it should handle an arbitrary number of commits, should certainly be able to handle just one commit. From that angle, it feels like adding another mode to merge-tree would just be a partial duplication of the other tool.

I wonder how the UI of a tool that does non-worktree/non-index cherry-picks will look like. I'd expect it to produce the same output as merge-tree, except cherry-pick should probably output a commit OID, not a tree.

Maybe we want a unified command that produces commits from any sequence of merge/cherry-pick/revert/reword steps. The obvious UI would use something like the rebase-todo list as input. For example:

        $ echo '
        pick commit1
        reword commit2  # edit commit message in $GIT_EDITOR
        merge commit3 -m "log message"
        ' | git create-commit commit0
        <OID of final commit>

we start from commit0 and apply steps one-by-one. Obviously, one unsolved problem is how to pass parameters like commit messages if no editor should be invoked (my sketch uses -m).
If any of the steps fails when merging merge, then we get the tree with conflicts

        $ echo '
        pick commit1
        pick commit2
        pick commit-that-does-not-apply
        ' | git create-commit commit0
        <OID of commit after step 2>
        <OID of toplevel tree after failed merge>
        <Conflicted file info>
        <Informational messages>

Replaying a series of commits might look like this:

        $ echo 'pick commit1 ^commit0' | git create-commit new-base

Quoted 1 line

I'm concluding that this is a difficult UI problem

I agree. I've got a lot of thoughts on it, and some work in progress towards it (https://github.com/newren/git/tree/replay -- _very_ hacky, not even close to alpha quality, lots of fixup commits, todo comments, random brain dump files added to the tree, based on a previous round of this patch series, not updated for weeks, etc., etc.)

Quoted 3 lines

and having a merge-tree command that accepts a "common ancestor" parameter could make it easier to experiment. Of course that depends on who is experimenting.

I think that would result in experiments and eventually full-blown scripts designed around forking subprocesses for every merge, and pushes us back into the world of having a scripted-rebase again. Yes, I know people can transliterate shell back to C; it seems to always be done as a half-way measure with the forking just being done from C or have other UI-warts guided by the shell design. In fact, *that* was the primary reason for me not providing a merge-tree option based on merge_incore_nonrecursive(), despite how trivial it'd be to provide it. If someone wanted a merge_incore_nonrecursive() mode for merge-tree for reasons other than attempting to build a rebase/cherry-pick replacement based on it, then I'd be much happier to provide it.

If someone wants to experiment with what a plumbing-ish rebase/cherry-pick would look like, the _right_ way to do it would be making using of merge_incore_nonrecursive() directly. If they want example code, I already provided some a year and a half ago and got it merged into git.git in the form of t/helper/test-fast-rebase.c. My "replay" branch is based on that code, but (a) moves it from t/helper to a real builtin, (b) removes the hardcoded very strict input, (c) removes the line of code doing the index & working tree updates, and
(d) modifies the output to be a more plumbing-ish style.

We'll certainly have discussions on what that should look like. But a plumbing-ish replacement for merge was much simpler, and made sense to do first. I would prefer to concentrate on getting that hammered down first. Then I'll start discussions on a plumbing-ish rebase/cherry-pick. And if that doesn't fulfill all the needs that folks think they want out of merge-tree, then we can add a merge_incore_nonrecursive()-based mode to merge-tree. It's all coming, but having fought transliterations-of-scripts in merge-recursive.c, sequencer.c, stash.c, rebase.c, etc. for years I really, really don't want any more of that. Let's end that insanity.

Hi,

On Thu, 3 Feb 2022, Elijah Newren wrote:

Quoted 5 lines

I agree. I've got a lot of thoughts on it, and some work in progress towards it (https://github.com/newren/git/tree/replay -- _very_ hacky, not even close to alpha quality, lots of fixup commits, todo comments, random brain dump files added to the tree, based on a previous round of this patch series, not updated for weeks, etc., etc.)

Just chiming in that I find that very exciting. But it's a tangent, and slightly distracting from the topic at hand, so I would like to ask to focus back on server-side merges.

Quoted 10 lines

We'll certainly have discussions on what that should look like. But a plumbing-ish replacement for merge was much simpler, and made sense to do first. I would prefer to concentrate on getting that hammered down first. Then I'll start discussions on a plumbing-ish rebase/cherry-pick. And if that doesn't fulfill all the needs that folks think they want out of merge-tree, then we can add a merge_incore_nonrecursive()-based mode to merge-tree. It's all coming, but having fought transliterations-of-scripts in merge-recursive.c, sequencer.c, stash.c, rebase.c, etc. for years I really, really don't want any more of that. Let's end that insanity.

Being the driving force behind many a "built-in-ification" of scripted commands, I wholeheartedly agree. You can still see the fall-out of designing commands in a scripted fashion, without any way to represent data structures other than strings. I wish we had come up with a better design to prototype commands than to write shell scripts. But I have to admit that even I do not have any better idea than to work on a proper API for libgit.a (which has historically invariably seen push-back from Junio).

While I agree that this discussion is a valuable one, right now I would like to focus on getting the server-side merges done, and once that has happened, move on to the replay/sequencer/API discussion (which will probably be a big one, not so much for technical reasons but more for all too human ones).

I wonder how the UI of a tool that does non-worktree/non-index cherry-picks will look like. I'd expect it to produce the same output as merge-tree, except cherry-pick should probably output a commit OID, not a tree.

Adding such an ability to merge-tree would be trivial -- it basically involves just two things: (1) accepting one extra argument, and (2) calling merge_incore_nonrecursive() instead of merge_incore_recursive().

However, I think forking a subprocess for every merge of a series of commits is a completely unreasonable overhead, so even if we provide such an option to merge-tree, I still want a separate plumbing-ish tool that does non-worktree/non-index replaying of commits which is not written as a driver of merge-tree. That other tool should just call merge_incore_nonrecursive() directly. And such a tool, since it should handle an arbitrary number of commits, should certainly be able to handle just one commit. From that angle, it feels like adding another mode to merge-tree would just be a partial duplication of the other tool.

Quoted 5 lines · Elijah Newren

Adding such an ability to merge-tree would be trivial -- it basically involves just two things: (1) accepting one extra argument, and (2) calling merge_incore_nonrecursive() instead of merge_incore_recursive().

Quoted 10 lines

However, I think forking a subprocess for every merge of a series of commits is a completely unreasonable overhead, so even if we provide such an option to merge-tree, I still want a separate plumbing-ish tool that does non-worktree/non-index replaying of commits which is not written as a driver of merge-tree. That other tool should just call merge_incore_nonrecursive() directly. And such a tool, since it should handle an arbitrary number of commits, should certainly be able to handle just one commit. From that angle, it feels like adding another mode to merge-tree would just be a partial duplication of the other tool.

The above does not make much sense to me.

I am hearing that "multi-step cherry-picks and reverts need to be fast and we need something like sequencer that is all written in C,

and single-step cherry-pick is merely a special case that does not deserve a plumbing".

But that argument leads to "and the same something-like-sequencer that is all written in C would need '--rebase-merges' that can pick multi-step merge sequences, and single-step merge does not deserve a plumbing", which is an argument against this topic that is utterly absurd.

So why isn't your objection not equally absurd against having a single step cherry-pick or revert primitive as a plumbing?

The objection you are arguing against is not my position. In fact, I'm not even objecting to having a single-step cherry-pick, I'm objecting to providing it _now_, which I thought would have been clear from the portion of my email you snipped ("...I'm happy to add [a single step cherry-pick primitive] along with the tool I submit later...").

Quoted 6 lines · Elijah Newren

The objection you are arguing against is not my position. In fact, I'm not even objecting to having a single-step cherry-pick, I'm objecting to providing it _now_, which I thought would have been clear from the portion of my email you snipped ("...I'm happy to add [a single step cherry-pick primitive] along with the tool I submit later...").

The entry point into the in-core merge machinery of ort already knows how to accept externally defined merge-base(s) to bypass the "caller didn't give us the merge base, so let's figure them out by using the two heads being merged" logic, so it just felt backwards *not* to have a vanilla three-way merge that can take three trees and be used for merge, cherry-pick or revert as the single primitive in the very beginning before we talk about multi-step operations.

So, I guess I am still not getting where the "I'm happy to _add_" part comes from. If we start with a primitive (internally callable from C) "here are three trees O A B, do the three-way merge", then there is nothing to "add" at all to expose a single-step cherry-pick. In fact, to the users of merge-tree, the result does not have to have any fixed meaning. If they pass common ancestors as the merge bases as Os and the current HEAD and the other branch as A and B, they get a merge. If they pass the commit to be picked as B, current HEAD as A and B's parent as O, they get a cherry-pick.

Perhaps starting from "You are allowed to give me two commits A B, and I do not let you specify the commit O to use as a common 'ancestor'" is the root cause of making this thing feel backwards. I agree with the goal of having an all-incore machinery that can do a merge. I just do not see the reason why you have to build it in a way that cannot be reused for other two directions of merge-y operations.

So, am I correct to understand that what bugs you is actually merge-recursive's and merge-ort's API? That you don't want these two types of merges to have different entry points, and that there should in fact only be one?

It is more like
...

Quoted 2 lines · Junio C Hamano

It is more like
...

Actually, I misspoke. It is a bit different.

In my mind, the building block hierarchy would have been

(1) Take three tree objects A, B, and O, and do the three-way merge. No history relation is assumed among them.

(2) Take two tree objects A and B, with one or more commit objects Os; use (2) recursively to reduce Os into a single O and then apply (1) on A, B and O.

(3) Take two commit objects A and B. Compute Os out of A and B and use (2) once to merge A and B.

I think the basic primitive that should be exposed to an external world (read: plumbing) this year, after all years of experience with merge-recursive, should be (2), not (1).

If you have (2), then (3) is trivially possible (it is just a single call to get_merge_base()). "git merge-tree A B" without having to spell out bases is so convenent and you do not have to write "git merge-tree A B $(git merge-base --all A B)", so I am OK for it to exist, but it is not essential.

If you have (2) and exposed (2) as the primitive plumbing, cherry-pick and revert would be a narrow special case of passing only one O to the machinery.

And coming from the above point of view, exposing (3) as the primitive plumbing to scripters and system builders, and later having to _add_ support to allow (2), felt backwards. It should be trivial for us to make (2) available before we can even offer (3), but what is happening to this new plumbing command goes in the opposite order.

It may be, as you said, the problem the underlying ort API has that somehow makes it harder to expose (2), in which case, yes, I think that is what bugs me.

Quoted 8 lines · Junio C Hamano

Quoted 5 lines · Elijah Newren

Adding such an ability to merge-tree would be trivial -- it basically involves just two things: (1) accepting one extra argument, and (2) calling merge_incore_nonrecursive() instead of merge_incore_recursive().

Quoted 10 lines

Quoted 10 lines

However, I think forking a subprocess for every merge of a series of commits is a completely unreasonable overhead, so even if we provide such an option to merge-tree, I still want a separate plumbing-ish tool that does non-worktree/non-index replaying of commits which is not written as a driver of merge-tree. That other tool should just call merge_incore_nonrecursive() directly. And such a tool, since it should handle an arbitrary number of commits, should certainly be able to handle just one commit. From that angle, it feels like adding another mode to merge-tree would just be a partial duplication of the other tool.

Quoted 5 lines

The above does not make much sense to me.

I am hearing that "multi-step cherry-picks and reverts need to be fast and we need something like sequencer that is all written in C,

Yes, I agree with that part so far. jj is kicking our butt on rebase speed; I'm not sure if we can catch it, but it'd be nice to see us not be more than a hundred times slower.

Quoted 2 lines

and single-step cherry-pick is merely a special case that does not deserve a plumbing".

Well, apparently I failed at communication if that's what you heard. Perhaps I can step back and provide my high-level goals, and then mention how this series fits in. My high-level goals:

* new sequencer-like replay tool, including multiple abilities
today's rebase/cherry-pick tools don't have
* enable folks to use merging machinery for server side operations
(merge, rebase, cherry-pick, revert)
* do not repeat or encourage the rebase-as-shell-script mistakes of yesteryear
* somehow split this up into reviewable chunks

Now, in particular, the "merge divergent branches" piece seemed like a really simple portion of the problem space for which I could get some early feedback without having to address the whole problem space all at once, and which doesn't seem to have any downside risk.

And even with my attempt to narrow it in scope, and even despite lots of early feedback from the Git Virtual Contributor Summit six months ago, it's been nearly two months of active discussions including all kinds of intrinsic and tangential points about the UI and design. Why try to prematurely widen the scope? Can we just focus on merging divergent branches for now, and cover the rest later?

Quoted 8 lines

But that argument leads to "and the same something-like-sequencer that is all written in C would need '--rebase-merges' that can pick multi-step merge sequences, and single-step merge does not deserve a plumbing", which is an argument against this topic that is utterly absurd.

So why isn't your objection not equally absurd against having a single step cherry-pick or revert primitive as a plumbing?

The objection you are arguing against is not my position. In fact, I'm not even objecting to having a single-step cherry-pick, I'm objecting to providing it _now_, which I thought would have been clear from the portion of my email you snipped ("...I'm happy to add [a single step cherry-pick primitive] along with the tool I submit later..."). Since that wasn't clear, and since that wasn't my only communication failure here, let me attempt to be clearer about my objection(s):

1. I'm really trying to pick off a small piece of the problem space and get feedback on it without unnecessarily complicating things with unrelated issues. Thus, this series is _only_ about merging branches that have diverged, and leaves commit replaying for later.

2. Two folks have chimed in about the single step cherry-pick, and the ONLY reason given for wanting such a thing was to create a rebasing/cherry-picking script which was driven by repeatedly invoking this low-level primitive command. That's also the only usecase I can currently think of for such a primitive. To me, that means providing such a low-level command now would be likely to result in the rebase-as-a-script mistake of yesteryear. I think we can avoid that pitfall by first providing a tool that avoids the repeatedly-fork-git-subprocesses model. (Also, providing a low-level single-step cherry-pick command also has the added negative of further distracting from the focus on merging divergent branches.)

3. The merge primitive in this series is useful completely independent of any rebasing script (it would not be used solely for rebasing merges, if it's used for that purpose at all, as evidenced by the fact that dscho is already trying to use it for doing new real merges).

4. Once we have a git-replay tool that can replay a sequence of commits, there _might_ not be a need for a single commit replaying primitive. If we provided one as you and Johannes Altimanninger were asking for, and it turned out to be deemed useless because the later tool I provide can do everything it can and more, haven't we just wasted time in providing it? And perhaps also wasted future time as we then have work to do to deprecate and remove the new command or mode? (NOTE: I did *not* say there was "no need" for a single-commit replaying primitive -- I said there "might not" be a need.)

Also, since you bring up --rebase-merges, there's an additional point about it that might be relevant:

5. While you could implement a naive --rebase-merges in terms of a primitive for merging divergent branches (or vice-versa, i.e. implement merging divergent branches from a naive --rebase-merges implementation), I think replaying merges more intelligently[*] is actually a distinct operation from doing a new merge of divergent branches and that you probably can't implement one in terms of the other. (I'm not certain on this, and definitely don't want to argue the finer points on it while my implementation is still half-baked, but I really do think they are different things right now.)

[*] https://lore.kernel.org/git/CABPp-BHp+d62dCyAaJfh1cZ8xVpGyb97mZryd02aCOX=Qn=Ltw@mail.gmail.com/

Quoted 5 lines · Elijah Newren

Adding such an ability to merge-tree would be trivial -- it basically involves just two things: (1) accepting one extra argument, and (2) calling merge_incore_nonrecursive() instead of merge_incore_recursive().

Quoted 10 lines

However, I think forking a subprocess for every merge of a series of commits is a completely unreasonable overhead, so even if we provide such an option to merge-tree, I still want a separate plumbing-ish tool that does non-worktree/non-index replaying of commits which is not written as a driver of merge-tree. That other tool should just call merge_incore_nonrecursive() directly. And such a tool, since it should handle an arbitrary number of commits, should certainly be able to handle just one commit. From that angle, it feels like adding another mode to merge-tree would just be a partial duplication of the other tool.

The above does not make much sense to me.

I am hearing that "multi-step cherry-picks and reverts need to be fast and we need something like sequencer that is all written in C, and single-step cherry-pick is merely a special case that does not deserve a plumbing".

But that argument leads to "and the same something-like-sequencer that is all written in C would need '--rebase-merges' that can pick multi-step merge sequences, and single-step merge does not deserve a plumbing", which is an argument against this topic that is utterly absurd.

So why isn't your objection not equally absurd against having a single step cherry-pick or revert primitive as a plumbing?

Documentation/git-merge-tree.txt +49 -12

index 58731c19422..569485815a0 100644
@@ -3,26 +3,73 @@ git-merge-tree(1)
 
 NAME
 ----
-git-merge-tree - Show three-way merge without touching index
+git-merge-tree - Perform merge without touching index or working tree
 
 
 SYNOPSIS
 --------
 [verse]
-'git merge-tree' <base-tree> <branch1> <branch2>
+'git merge-tree' [--write-tree] <branch1> <branch2>
+'git merge-tree' [--trivial-merge] <base-tree> <branch1> <branch2> (deprecated)
 
 DESCRIPTION
 -----------
-Reads three tree-ish, and output trivial merge results and
-conflicting stages to the standard output.  This is similar to
-what three-way 'git read-tree -m' does, but instead of storing the
-results in the index, the command outputs the entries to the
-standard output.
-
-This is meant to be used by higher level scripts to compute
-merge results outside of the index, and stuff the results back into the
-index.  For this reason, the output from the command omits
-entries that match the <branch1> tree.
+
+Performs a merge, but does not make any new commits and does not read
+from or write to either the working tree or index.
+
+The second form is deprecated and supported only for backward
+compatibility.  It will likely be removed in the future, and will not
+be discussed further in this manual.
+
+The first form will merge the two branches, doing a real merge.  A real
+merge is distinguished from a trivial merge in that it includes:
+
+  * three way content merges of individual files
+  * rename detection
+  * proper directory/file conflict handling
+  * recursive ancestor consolidation (i.e. when there is more than one
+    merge base, creating a virtual merge base by merging the merge bases)
+  * etc.
+
+After the merge completes, it will create a new toplevel tree object.
+See `OUTPUT` below for details.
+
+OUTPUT
+------
+
+For either a successful or conflicted merge, the output from
+git-merge-tree is simply one line:
+
+	<OID of toplevel tree>
+
+The printed tree object corresponds to what would be checked out in
+the working tree at the end of `git merge`, and thus may have files
+with conflict markers in them.
+
+EXIT STATUS
+-----------
+
+For a successful, non-conflicted merge, the exit status is 0.  When the
+merge has conflicts, the exit status is 1.  If the merge is not able to
+complete (or start) due to some kind of error, the exit status is
+something other than 0 or 1.
+
+USAGE NOTES
+-----------
+
+git-merge-tree was written to be low-level plumbing, similar to
+hash-object, mktree, commit-tree, update-ref, and mktag.  Thus, it could

On 2022.02.02 07:34, Elijah Newren via GitGitGadget wrote:

Quoted 12 lines

Documentation/git-merge-tree.txt +12 -0

+USAGE NOTES
+-----------
+
+git-merge-tree was written to be low-level plumbing, similar to
+hash-object, mktree, commit-tree, update-ref, and mktag.  Thus, it could
+be used as a part of a series of steps such as
+
+       NEWTREE=$(git merge-tree --write-tree $BRANCH1 $BRANCH2)
+       test $? -eq 0 || die "There were conflicts..."
+       NEWCOMMIT=$(git commit-tree $NEWTREE -p $BRANCH1 -p $BRANCH2)
+       git update-ref $BRANCH1 $NEWCOMMIT
+

Documentation/git-merge-tree.txt git-merge-tree(1) · line 72

Quoted 3 lines

Documentation/git-merge-tree.txt +3 -0

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

I found this final paragraph confusing. It seems to be hinting at some conclusion it expects readers to make, but I haven't been able to figure out what. Could this be made more explicit, or perhaps dropped altogether?

From: Elijah Newren <[email protected]>

This modifies the new display_update_messages() function to allow printing to somewhere other than stdout. It also consolidates the location of the diff_warn_rename_limit() message with the rest of the CONFLICT and other update messages to all go to the same stream.

Signed-off-by Elijah Newren <[email protected]>

---
 merge-ort.c | 9 +++++----
 merge-ort.h | 3 ++-
 2 files changed, 7 insertions(+), 5 deletions(-)

merge-ort.c +2 -1

index 82d2faf5bf9..d28d1721d14 100644
@@ -4236,7 +4236,8 @@ static int record_conflicted_index_entries(struct merge_options *opt)
 }
 
 void merge_display_update_messages(struct merge_options *opt,
-				   struct merge_result *result)
+				   struct merge_result *result,
+				   FILE *stream)
 {
 	struct merge_options_internal *opti = result->priv;
 	struct hashmap_iter iter;

@@ -4263,13 +4264,13 @@ void merge_display_update_messages(struct merge_options *opt,
 	for (i = 0; i < olist.nr; ++i) {
 		struct strbuf *sb = olist.items[i].util;

Quoted 36 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

This modifies the new display_update_messages() function to allow printing to somewhere other than stdout. It also consolidates the location of the diff_warn_rename_limit() message with the rest of the CONFLICT and other update messages to all go to the same stream.

Signed-off-by Elijah Newren <[email protected]>

---
 merge-ort.c | 9 +++++----
 merge-ort.h | 3 ++-
 2 files changed, 7 insertions(+), 5 deletions(-)

merge-ort.c +3 -2

index 82d2faf5bf9..d28d1721d14 100644
@@ -4236,7 +4236,8 @@ static int record_conflicted_index_entries(struct merge_options *opt)
 }
 
 void merge_display_update_messages(struct merge_options *opt,
-                                struct merge_result *result)
+                                struct merge_result *result,
+                                FILE *stream)
 {
      struct merge_options_internal *opti = result->priv;
      struct hashmap_iter iter;

@@ -4263,13 +4264,13 @@ void merge_display_update_messages(struct merge_options *opt,
      for (i = 0; i < olist.nr; ++i) {
              struct strbuf *sb = olist.items[i].util;
 
-             printf("%s", sb->buf);
+             strbuf_write(sb, stream);
      }
      string_list_clear(&olist, 0);
 

merge-ort.c void merge_display_update_messages(struct merge_options *opt, · line 4273

Quoted 4 lines

merge-ort.c +1 -1

      /* Also include needed rename limit adjustment now */
      diff_warn_rename_limit("merge.renamelimit",
-                            opti->renames.needed_limit, 0, stderr);
+                            opti->renames.needed_limit, 0, stream);

At the tip of this series I tried to s/stream/stderr/g this, and t4301-merge-tree-write-tree.sh passes, doesn't this warning_fp() special behavior need a test somewhere?

I assumed that warning_fp() would be using vreportf() in usage.c, but it's not, it's just falling back to the equivalent of fprintf(out, ...), no? I don't really see why 05/15 and parts of 06/15 & this are needed over a much simpler simple helper macro like the below. applied on top of this series.

I would get it if the point was to actually use the full usage.c machinery, but we're just either calling warning(), or printing a formatted string to a file FILE *. There's no need to go through usage.c for that, and adding an API to it that behaves like this new warning_fp() is really confusing.

I.e. an API in usage.c that allowed warning to a given FD would be trying to replace the "2" in the write_in_full() call in vreportf(), I would think.

diff.c +7 -0

index 28368110147..4cf67e93dea 100644
@@ -6377,14 +6377,21 @@ static const char rename_limit_advice[] =
 N_("you may want to set your %s variable to at least "
    "%d and retry the command.");
 
+#define warning_fp(out, fmt, ...) do { \
+       if (out == stderr) \
+               warning(fmt, __VA_ARGS__); \
+       else \
+               fprintf(out, fmt, __VA_ARGS__); \
+} while (0)
+

diff.c +0 -0

        else
                return;
 

git-compat-util.h +0 -1

index 64ba60e5c71..d70ce142861 100644
@@ -475,7 +475,6 @@ int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
 int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
 void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
-void warning_fp(FILE *out, const char *warn, ...) __attribute__((format (printf, 2, 3)));
 
 #ifndef NO_OPENSSL
 #ifdef APPLE_COMMON_CRYPTO

usage.c +0 -14

index 0bfd2c603c0..c7d233b0de9 100644
@@ -253,20 +253,6 @@ void warning(const char *warn, ...)
        va_end(params);
 }
 
-void warning_fp(FILE *out, const char *warn, ...)
-{
-       va_list params;
-
-       va_start(params, warn);
-       if (out == stderr)
-               warn_routine(warn, params);
-       else {
-               vfprintf(out, warn, params);
-               fputc('\n', out);
-       }
-       va_end(params);
-}
-
 /* Only set this, ever, from t/helper/, when verifying that bugs are caught. */
 int BUG_exit_code;

Quoted 39 lines · Ævar Arnfjörð Bjarmason

Quoted 36 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

This modifies the new display_update_messages() function to allow printing to somewhere other than stdout. It also consolidates the location of the diff_warn_rename_limit() message with the rest of the CONFLICT and other update messages to all go to the same stream.

Signed-off-by Elijah Newren <[email protected]>

---
 merge-ort.c | 9 +++++----
 merge-ort.h | 3 ++-
 2 files changed, 7 insertions(+), 5 deletions(-)

merge-ort.c +3 -2

index 82d2faf5bf9..d28d1721d14 100644
@@ -4236,7 +4236,8 @@ static int record_conflicted_index_entries(struct merge_options *opt)
 }
 
 void merge_display_update_messages(struct merge_options *opt,
-                                struct merge_result *result)
+                                struct merge_result *result,
+                                FILE *stream)
 {
      struct merge_options_internal *opti = result->priv;
      struct hashmap_iter iter;

@@ -4263,13 +4264,13 @@ void merge_display_update_messages(struct merge_options *opt,
      for (i = 0; i < olist.nr; ++i) {
              struct strbuf *sb = olist.items[i].util;
 
-             printf("%s", sb->buf);
+             strbuf_write(sb, stream);
      }
      string_list_clear(&olist, 0);
 

Quoted 8 lines

merge-ort.c void merge_display_update_messages(struct merge_options *opt, · line 4273

Quoted 4 lines

merge-ort.c +1 -1

      /* Also include needed rename limit adjustment now */
      diff_warn_rename_limit("merge.renamelimit",
-                            opti->renames.needed_limit, 0, stderr);
+                            opti->renames.needed_limit, 0, stream);

At the tip of this series I tried to s/stream/stderr/g this, and t4301-merge-tree-write-tree.sh passes, doesn't this warning_fp() special behavior need a test somewhere?

That's a fair point, but...this gets back to my cover letter comments about patches 5, 6, and 8. They implement a code feature that seems useful in general...but which Dscho and Christian didn't like using in this particular command; they just wanted all output on stdout.

So, it's hard to add a test, because there's no code anywhere that exercises it in this series anymore. I originally wanted this feature in my remerge-diff series, but the idea of conflict headers made me punt it to this series. I wanted it for this series, but Dscho and Christian didn't. I could have punted again, but decided the underlying want kept coming up and decided to not excise it -- especially since Dscho was helping improve it. And Junio commented that he liked the idea[1].

[1] https://lore.kernel.org/git/[email protected]/

But yeah, it does leave it feeling slightly odd that we implemented a feature that nothing is currently using. Maybe these 3 should be split off into their own series? Still wouldn't have a test yet, though.

Quoted 5 lines

I assumed that warning_fp() would be using vreportf() in usage.c, but it's not, it's just falling back to the equivalent of fprintf(out, ...), no? I don't really see why 05/15 and parts of 06/15 & this are needed over a much simpler simple helper macro like the below. applied on top of this series.

That macro is simple? I thought I basically understood Dscho's code, but looking at what you did with diff_warn_rename_limit(), I think I'm lost.

Quoted 5 lines

I would get it if the point was to actually use the full usage.c machinery, but we're just either calling warning(), or printing a formatted string to a file FILE *. There's no need to go through usage.c for that, and adding an API to it that behaves like this new warning_fp() is really confusing.

Because the formatted string being printed to the file won't have the same "warning: " prefix that is normally added to stuff in usage?

That's a fair point; that does have a bit of a consistency problem. And I'd rather the messages were consistent regardless of where they are printed.

Quoted 3 lines

I.e. an API in usage.c that allowed warning to a given FD would be trying to replace the "2" in the write_in_full() call in vreportf(), I would think.

Hmm, makes sense.

Quoted 20 lines

diff.c +7 -0

index 28368110147..4cf67e93dea 100644
@@ -6377,14 +6377,21 @@ static const char rename_limit_advice[] =
 N_("you may want to set your %s variable to at least "
    "%d and retry the command.");
 
+#define warning_fp(out, fmt, ...) do { \
+       if (out == stderr) \
+               warning(fmt, __VA_ARGS__); \
+       else \
+               fprintf(out, fmt, __VA_ARGS__); \
+} while (0)
+
 void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc,
                            FILE *out)
 {
        fflush(stdout);
        if (degraded_cc)

diff.c static const char rename_limit_advice[] = · line 6394

Quoted 5 lines

diff.c +2 -2

-               warning_fp(out, _(degrade_cc_to_c_warning));
+               warning_fp(out, _(degrade_cc_to_c_warning), NULL);
        else if (needed)
-               warning_fp(out, _(rename_limit_warning));
+               warning_fp(out, _(rename_limit_warning), NULL);

Why do the only callers have a NULL parameter here? Is this one of those va_list/va_args things I never bothered to properly learn?

Quoted 1 line · Elijah Newren

On Wed, Feb 2, 2022 at 6:01 PM Ævar Arnfjörð Bjarmason <[email protected]> wrote:

Quoted 8 lines

Quoted 5 lines

I would get it if the point was to actually use the full usage.c machinery, but we're just either calling warning(), or printing a formatted string to a file FILE *. There's no need to go through usage.c for that, and adding an API to it that behaves like this new warning_fp() is really confusing.

Because the formatted string being printed to the file won't have the same "warning: " prefix that is normally added to stuff in usage?

But the pre-image doesn't add that either. We're just calling vfprintf(), not our own vreportf().

Quoted 3 lines

That's a fair point; that does have a bit of a consistency problem. And I'd rather the messages were consistent regardless of where they are printed.

I think that makes sense, that's why I added die_message() recently. If you meant to print a "warning: " prefix I think it would also be fine in this case to just do it inline. See prior art at:

    git grep '"(fatal|error|warning):' -- '*.c'

Quoted 20 lines

Quoted 20 lines

diff.c +7 -0

index 28368110147..4cf67e93dea 100644
@@ -6377,14 +6377,21 @@ static const char rename_limit_advice[] =
 N_("you may want to set your %s variable to at least "
    "%d and retry the command.");
 
+#define warning_fp(out, fmt, ...) do { \
+       if (out == stderr) \
+               warning(fmt, __VA_ARGS__); \
+       else \
+               fprintf(out, fmt, __VA_ARGS__); \
+} while (0)
+
 void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc,
                            FILE *out)
 {
        fflush(stdout);
        if (degraded_cc)

Quoted 8 lines

diff.c N_("you may want to set your %s variable to at least "

Quoted 5 lines

diff.c +2 -2

-               warning_fp(out, _(degrade_cc_to_c_warning));
+               warning_fp(out, _(degrade_cc_to_c_warning), NULL);
        else if (needed)
-               warning_fp(out, _(rename_limit_warning));
+               warning_fp(out, _(rename_limit_warning), NULL);

Why do the only callers have a NULL parameter here? Is this one of those va_list/va_args things I never bothered to properly learn?

That's wrong (I blame tiredness last night),an actual working version is produced below. Clang accepted my broken code, but gcc rightly yells about it:

Note that both your pre-image, my macro version and Johannes's linked-to-above are technically buggy in that they treat a non-formatting format as a formatting format. I.e. we should use warning("%s", msg) in that case, not warning(msg).

See 927dc330705 (advice.h: add missing __attribute__((format)) & fix usage, 2021-07-13) for a similar bug/fix.

Quoted 4 lines · Ævar Arnfjörð Bjarmason

Quoted 1 line · Elijah Newren

On Wed, Feb 2, 2022 at 6:01 PM Ævar Arnfjörð Bjarmason <[email protected]> wrote:

[...]

Quoted 11 lines

Quoted 8 lines

Quoted 5 lines

I would get it if the point was to actually use the full usage.c machinery, but we're just either calling warning(), or printing a formatted string to a file FILE *. There's no need to go through usage.c for that, and adding an API to it that behaves like this new warning_fp() is really confusing.

Because the formatted string being printed to the file won't have the same "warning: " prefix that is normally added to stuff in usage?

But the pre-image doesn't add that either. We're just calling vfprintf(), not our own vreportf().

Right, I'm saying that I thought you were reporting the original patch as buggy because it doesn't produce the same message when given a different stream; it'll omit the "warning: " prefix. And I was agreeing that it was buggy for those reasons.

Or was there a different reason you didn't like that function being in usage.c?

Quoted 9 lines

Quoted 3 lines

That's a fair point; that does have a bit of a consistency problem. And I'd rather the messages were consistent regardless of where they are printed.

I think that makes sense, that's why I added die_message() recently. If you meant to print a "warning: " prefix I think it would also be fine in this case to just do it inline. See prior art at:

    git grep '"(fatal|error|warning):' -- '*.c'

So, making diff_warn_rename_limit() stop using warning(), and just always directly writing out and including "warning:" in its message?

I'm wondering if that might cause problems if there are any existing callers of diff_warn_rename_limit() that might also be using set_warn_routine() (e.g. perhaps apply.c?). Of course, those callers probably couldn't handle anything other than the default stream. Hmm...

Quoted 29 lines

Quoted 20 lines

Quoted 20 lines

diff.c +7 -0

index 28368110147..4cf67e93dea 100644
@@ -6377,14 +6377,21 @@ static const char rename_limit_advice[] =
 N_("you may want to set your %s variable to at least "
    "%d and retry the command.");
 
+#define warning_fp(out, fmt, ...) do { \
+       if (out == stderr) \
+               warning(fmt, __VA_ARGS__); \
+       else \
+               fprintf(out, fmt, __VA_ARGS__); \
+} while (0)
+
 void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc,
                            FILE *out)
 {
        fflush(stdout);
        if (degraded_cc)

Quoted 8 lines

diff.c N_("you may want to set your %s variable to at least "

Quoted 5 lines

diff.c +2 -2

-               warning_fp(out, _(degrade_cc_to_c_warning));
+               warning_fp(out, _(degrade_cc_to_c_warning), NULL);
        else if (needed)
-               warning_fp(out, _(rename_limit_warning));
+               warning_fp(out, _(rename_limit_warning), NULL);

Why do the only callers have a NULL parameter here? Is this one of those va_list/va_args things I never bothered to properly learn?

Quoted 3 lines

That's wrong (I blame tiredness last night),an actual working version is produced below. Clang accepted my broken code, but gcc rightly yells about it:

Well, seeing the new code makes me feel better as it makes more sense to me now. ;-)

Quoted 5 lines

Note that both your pre-image, my macro version and Johannes's linked-to-above are technically buggy in that they treat a non-formatting format as a formatting format. I.e. we should use warning("%s", msg) in that case, not warning(msg).

Quoted 2 lines

See 927dc330705 (advice.h: add missing __attribute__((format)) & fix usage, 2021-07-13) for a similar bug/fix.

Good point.

Man, what a can of worms this all is. Maybe I really should just drop patches 5, 6, and 8 for now...

Quoted 18 lines · Elijah Newren

Quoted 4 lines · Ævar Arnfjörð Bjarmason

Quoted 1 line · Elijah Newren

On Wed, Feb 2, 2022 at 6:01 PM Ævar Arnfjörð Bjarmason <[email protected]> wrote:

[...]

Quoted 11 lines

Quoted 8 lines

Quoted 5 lines

I would get it if the point was to actually use the full usage.c machinery, but we're just either calling warning(), or printing a formatted string to a file FILE *. There's no need to go through usage.c for that, and adding an API to it that behaves like this new warning_fp() is really confusing.

Because the formatted string being printed to the file won't have the same "warning: " prefix that is normally added to stuff in usage?

But the pre-image doesn't add that either. We're just calling vfprintf(), not our own vreportf().

Quoted 6 lines

Right, I'm saying that I thought you were reporting the original patch as buggy because it doesn't produce the same message when given a different stream; it'll omit the "warning: " prefix. And I was agreeing that it was buggy for those reasons.

Or was there a different reason you didn't like that function being in usage.c?

Maybe it was accidentally a bug report :) But no, I was just observing that it was odd that it was in usage.c when it seemingly had almost nothing to do with what that API accomplishes.

But maybe the underlying issue is that the "warning: " part is missing here. But I didn't mean to report that/missed it.

Quoted 10 lines

Quoted 9 lines

Quoted 3 lines

That's a fair point; that does have a bit of a consistency problem. And I'd rather the messages were consistent regardless of where they are printed.

I think that makes sense, that's why I added die_message() recently. If you meant to print a "warning: " prefix I think it would also be fine in this case to just do it inline. See prior art at:

    git grep '"(fatal|error|warning):' -- '*.c'

Quoted 8 lines

So, making diff_warn_rename_limit() stop using warning(), and just always directly writing out and including "warning:" in its message?

I'm wondering if that might cause problems if there are any existing callers of diff_warn_rename_limit() that might also be using set_warn_routine() (e.g. perhaps apply.c?). Of course, those callers probably couldn't handle anything other than the default stream. Hmm...

Using set_warn_routine() is the "right" way to do it currently, and with or without a "warning: " prefix the current API use is "wrong" if the purpose is to have it behave nicely with the pluggable usage.c API.

But of course that may not be the goal at all, i.e. I think here we've probably stopped caring about usage.c's formatting, logging etc. entirely, and are just emitting a string.

Just like serve.c emits "E <msg>" or whatever (and not with error()).

Quoted 41 lines

Quoted 29 lines

Quoted 20 lines

Quoted 20 lines

diff.c +7 -0

index 28368110147..4cf67e93dea 100644
@@ -6377,14 +6377,21 @@ static const char rename_limit_advice[] =
 N_("you may want to set your %s variable to at least "
    "%d and retry the command.");
 
+#define warning_fp(out, fmt, ...) do { \
+       if (out == stderr) \
+               warning(fmt, __VA_ARGS__); \
+       else \
+               fprintf(out, fmt, __VA_ARGS__); \
+} while (0)
+
 void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc,
                            FILE *out)
 {
        fflush(stdout);
        if (degraded_cc)

Quoted 8 lines

diff.c N_("you may want to set your %s variable to at least "

Quoted 5 lines

diff.c +2 -2

-               warning_fp(out, _(degrade_cc_to_c_warning));
+               warning_fp(out, _(degrade_cc_to_c_warning), NULL);
        else if (needed)
-               warning_fp(out, _(rename_limit_warning));
+               warning_fp(out, _(rename_limit_warning), NULL);

Why do the only callers have a NULL parameter here? Is this one of those va_list/va_args things I never bothered to properly learn?

Quoted 3 lines

That's wrong (I blame tiredness last night),an actual working version is produced below. Clang accepted my broken code, but gcc rightly yells about it:

Well, seeing the new code makes me feel better as it makes more sense to me now. ;-)

Quoted 5 lines

Note that both your pre-image, my macro version and Johannes's linked-to-above are technically buggy in that they treat a non-formatting format as a formatting format. I.e. we should use warning("%s", msg) in that case, not warning(msg).

Quoted 7 lines

Quoted 2 lines

See 927dc330705 (advice.h: add missing __attribute__((format)) & fix usage, 2021-07-13) for a similar bug/fix.

Good point.

Man, what a can of worms this all is. Maybe I really should just drop patches 5, 6, and 8 for now...

Yeah, I really think it's worth it to just sprinkle a tiny bit of if/else (or a macro) here and print to stderr inline or not. We can make some use of some usage.c when there's good reason to do so, but this bit just seems like a needless digression.

I hope all of this has helped somewhat ...

Quoted 11 lines · Ævar Arnfjörð Bjarmason

Quoted 2 lines · Elijah Newren

Man, what a can of worms this all is. Maybe I really should just drop patches 5, 6, and 8 for now...

Yeah, I really think it's worth it to just sprinkle a tiny bit of if/else (or a macro) here and print to stderr inline or not. We can make some use of some usage.c when there's good reason to do so, but this bit just seems like a needless digression.

Quoted 1 line

I hope all of this has helped somewhat ...

Absolutely; thanks for reviewing! These parts may just end up in me dropping some patches for now (since they're not actually being used anyway), but I think it's all good feedback.

Hi Elijah,

Quoted 14 lines · Elijah Newren

Quoted 12 lines · Ævar Arnfjörð Bjarmason

Quoted 2 lines · Elijah Newren

Man, what a can of worms this all is. Maybe I really should just drop patches 5, 6, and 8 for now...

Yeah, I really think it's worth it to just sprinkle a tiny bit of if/else (or a macro) here and print to stderr inline or not. We can make some use of some usage.c when there's good reason to do so, but this bit just seems like a needless digression.

I hope all of this has helped somewhat ...

Quoted 3 lines

Absolutely; thanks for reviewing! These parts may just end up in me dropping some patches for now (since they're not actually being used anyway), but I think it's all good feedback.

So we dropped some useful patches future-proofing merge-tree for the sake of appeasing a refactoring with no immediately obvious benefit? I really don't like that direction.

Quoted 23 lines · Johannes Schindelin

Hi Elijah,

Quoted 17 lines · Elijah Newren

Quoted 6 lines · Ævar Arnfjörð Bjarmason

Quoted 2 lines · Elijah Newren

Man, what a can of worms this all is. Maybe I really should just drop patches 5, 6, and 8 for now...

Quoted 6 lines

Yeah, I really think it's worth it to just sprinkle a tiny bit of if/else (or a macro) here and print to stderr inline or not. We can make some use of some usage.c when there's good reason to do so, but this bit just seems like a needless digression.

I hope all of this has helped somewhat ...

Absolutely; thanks for reviewing! These parts may just end up in me dropping some patches for now (since they're not actually being used anyway), but I think it's all good feedback.

Quoted 3 lines

So we dropped some useful patches future-proofing merge-tree for the sake of appeasing a refactoring with no immediately obvious benefit? I really don't like that direction.

Even before any of Ævar's comments, I had already noted on my cover letter[1] that "to be honest, patches 5, 6, & 8 may be less relevant since we're now including these messages on stdout anyway" -- so I was already wondering if I should defer them to some future series.

From: Elijah Newren <[email protected]>

Much like git merge updates the index with information of the form
(mode, oid, stage, name)
provide this output for conflicted files for merge-tree as well. Provide an --exclude-modes-oids-stages/-l option for users to exclude

+--exclude-oids-and-modes::

Quoted 6 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

Much like git merge updates the index with information of the form
(mode, oid, stage, name)
provide this output for conflicted files for merge-tree as well. Provide an --exclude-modes-oids-stages/-l option for users to exclude

This.

Quoted 1 line

+--exclude-oids-and-modes::

No longer matches this.