On Sat, Jan 22 2022, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@xxxxxxxxx>
>
> 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 <newren@xxxxxxxxx>
> ---
> builtin/merge-tree.c | 67 ++++++++++++++++++++++++++++++++++++++------
> git.c | 2 +-
> 2 files changed, 59 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index 914ec960b7e..33e47cc1534 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -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,57 @@ static int trivial_merge(int argc, const char **argv)
> return 0;
> }
>
> +struct merge_tree_options {
> + int real;
> + int trivial;
> +};
> +
> +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>"),
> + NULL
> + };
> + struct option mt_options[] = {
> + OPT_BOOL(0, "write-tree", &o.real,
> + N_("do a real merge instead of a trivial merge")),
> + OPT_BOOL(0, "trivial-merge", &o.trivial,
> + N_("do a trivial merge only")),
> + OPT_END()
> + };
> +
> + /* Check for a request for basic help */
> + if (argc == 2 && !strcmp(argv[1], "-h"))
> + usage_with_options(merge_tree_usage, mt_options);
Is this bit cargo-culted from something else, perhaps
non-parse-options.c usage? I don't think this is needed, the
parse_options() below intercepts "-h" by default.
> + /* Parse arguments */
> + 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"));
Shouldn't those two just be OPT_CMDMODE()? Then you get this
incompatibility checking for free. See 485fd2c3dae (cat-file: make
--batch-all-objects a CMDMODE, 2021-12-28).
> + if (o.real || o.trivial) {
> + expected_remaining_argc = (o.real ? 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.real = (argc == 2);
> + }
And this can also be done like this, but I wonder if using
PARSE_OPT_STOP_AT_NON_OPTION and then routing to a sub-function wouldn't
be better, i.e. to treat these like sub-commands if they've got
different arity etc.
> + /* Do the relevant type of merge */
> + if (o.real)
> + return real_merge(&o, argv[0], argv[1]);
> + else
> + return trivial_merge(argv[0], argv[1], argv[2]);
> }
> diff --git a/git.c b/git.c
> index 5ff21be21f3..6090a1289db 100644
> --- a/git.c
> +++ b/git.c
> @@ -558,7 +558,7 @@ static struct cmd_struct commands[] = {
> { "merge-recursive-ours", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
> { "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
> { "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
> - { "merge-tree", cmd_merge_tree, RUN_SETUP | NO_PARSEOPT },
> + { "merge-tree", cmd_merge_tree, RUN_SETUP },
> { "mktag", cmd_mktag, RUN_SETUP | NO_PARSEOPT },
> { "mktree", cmd_mktree, RUN_SETUP },
> { "multi-pack-index", cmd_multi_pack_index, RUN_SETUP },
