diff mbox series

[v2,03/13] merge-tree: add option parsing and initial shell for real merge function

Message ID 63f42df21aec5bda50e4414493eb59dcb64e5558.1643479633.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series In-core git merge-tree ("Server side merges") | expand

Commit Message

Elijah Newren Jan. 29, 2022, 6:07 p.m. UTC 
From: Elijah Newren <newren@gmail.com>

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@gmail.com>
---
 builtin/merge-tree.c | 61 +++++++++++++++++++++++++++++++++++++-------
 git.c                |  2 +-
 2 files changed, 53 insertions(+), 10 deletions(-)

Comments

Junio C Hamano Feb. 2, 2022, 9:30 p.m. UTC | #1 
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +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>"),
> +		NULL
> +	};
> +	struct option mt_options[] = {
> +		OPT_CMDMODE(0, "write-tree", &o.mode,
> +			    N_("do a real merge instead of a trivial merge"),
> +			    'w'),

Given the length of the second line in the usage[] array, it would
make more sense to have "'w')," on the same line to make it match
better with the following one.

> +		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,
> +			     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');
> +	}

We are not planning to have tons of different command modes, but the
above looks quite brittle in assuming that 'w' and 't' are the only
ones, and not having an easy way to extend the part without major
rewrite when the assumption will have to be broken.  I wonder:

	switch (o.cmd_mode) {
        default:
		BUG("unexpected cmdmode %c", o.cmd_mode);
	case 0:
		switch (argc) {
		default:
			usage_with_options(merge_tree_usage, mt_options);
		case 2: 
			o.cmd_mode = 'w';
			break;
		case 3:
			o.cmd_mode = 't';
			break;
		}
                expected_remaining_argc = argc;
		break;
	case 'w':
		expected_remaining_argc = 2;
                break;
	case 't':
		expected_remaining_argc = 3;
 		break;
	}

        if (argc != expected_remaining_argc)
		usage_with_options(merge_tree_usage, mt_options);

even though it is a tad longer with more boilerplate, is easlier to
manage.

> +	/* Do the relevant type of merge */
> +	if (o.mode == 'w')
> +		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 },

This affects git_support_parseopt_helper function in the completion
script, but it by itself is very unlikely to break existing tests on
the completion, as the bit only affects how the command line options
are completed, and the command didn't have any command line options
to be tested before this series ;-)

>  	{ "mktag", cmd_mktag, RUN_SETUP | NO_PARSEOPT },
>  	{ "mktree", cmd_mktree, RUN_SETUP },
>  	{ "multi-pack-index", cmd_multi_pack_index, RUN_SETUP },
diff mbox series

Patch

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 914ec960b7e..e98ec8a9f1d 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,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>"),
+		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,
+			     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 the relevant type of merge */
+	if (o.mode == 'w')
+		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 },