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

[Junio C Hamano <gitster@pobox.com>]

"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 },