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

Commit Message

Elijah Newren Jan. 22, 2022, 9:55 p.m. UTC

From: Elijah Newren <[e-mail]>

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 <[e-mail]>
---
 builtin/merge-tree.c | 67 ++++++++++++++++++++++++++++++++++++++------
 git.c                |  2 +-
 2 files changed, 59 insertions(+), 10 deletions(-)

Comments

René Scharfe Jan. 23, 2022, 8:05 a.m. UTC | #1

Am 22.01.22 um 22:55 schrieb Elijah Newren via GitGitGadget:
> From: Elijah Newren <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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);

This is unnecessary; parse_options() handles -h already.

> +
> +	/* 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"));

12909b6b8a (i18n: turn "options are incompatible" into "cannot be used
together", 2022-01-05) standardized messages of that kind; let's stick
to that to simplify translation:

		die(_("options '%s' and '%s' cannot be used together"),
		    "--write-tree", "--trivial-merge");

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

Ævar Arnfjörð Bjarmason Jan. 24, 2022, 9:46 a.m. UTC | #2

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

> From: Elijah Newren <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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 },

Elijah Newren Jan. 24, 2022, 4:43 p.m. UTC | #3

On Sun, Jan 23, 2022 at 12:05 AM René Scharfe <[e-mail]> wrote:
>
> Am 22.01.22 um 22:55 schrieb Elijah Newren via GitGitGadget:
...
> > +     /* Check for a request for basic help */
> > +     if (argc == 2 && !strcmp(argv[1], "-h"))
> > +             usage_with_options(merge_tree_usage, mt_options);
>
> This is unnecessary; parse_options() handles -h already.
>
> > +
> > +     /* 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"));
>
> 12909b6b8a (i18n: turn "options are incompatible" into "cannot be used
> together", 2022-01-05) standardized messages of that kind; let's stick
> to that to simplify translation:
>
>                 die(_("options '%s' and '%s' cannot be used together"),
>                     "--write-tree", "--trivial-merge");

Ah, thanks for both the pointers; will fix.

Elijah Newren Jan. 24, 2022, 4:54 p.m. UTC | #4

On Mon, Jan 24, 2022 at 1:50 AM Ævar Arnfjörð Bjarmason
<[e-mail]> wrote:
>
> On Sat, Jan 22 2022, Elijah Newren via GitGitGadget wrote:
>
...
> > +     /* 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.

Yep, sure was cargo-culted from somewhere else (my parse-options usage
always is), but I'm pretty sure it was from another place also using
parse-options.  Probably one of these 15 places:

 $ comm -12 <(git grep -l parse-options builtin/ | sort) <(git grep -l
strcmp.*-h\\b builtin/ | sort)
builtin/am.c
builtin/branch.c
builtin/checkout-index.c
builtin/checkout--worker.c
builtin/commit.c
builtin/commit-tree.c
builtin/gc.c
builtin/ls-files.c
builtin/merge.c
builtin/merge-tree.c
builtin/rebase.c
builtin/rev-parse.c
builtin/sparse-checkout.c
builtin/submodule--helper.c
builtin/update-index.c

> > +     /* 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).

TIL.  Thanks.

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

Not sure what you mean; I already route to sub-functions.  But I
should definitely add PARSE_OPT_STOP_AT_NON_OPTION; it's unfortunate
that it's not the default.

Patch

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);
+
+	/* 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"));
+	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);
+	}
+
+	/* 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 },