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

From: Emily Shaffer <[email-masked]>
Date: Mon, 7 Feb 2022 14:41:13 -0800

On Wed, Feb 02, 2022 at 07:34:29AM +0000, Elijah Newren via GitGitGadget wrote:
> 
> 
> 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 | 61 +++++++++++++++++++++++++++++++++++++-------
>  git.c                |  2 +-
>  2 files changed, 53 insertions(+), 10 deletions(-)
> 
> 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()
> +	};

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.

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

Sorry for the slow reply.

 - Emily