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

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Mon, 24 Jan 2022 10:46:13 +0100

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

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