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

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Thu, 03 Feb 2022 03:05:27 +0100

On Wed, Feb 02 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.

Maybe that ship has sailed, but just my 0.02: I thought this whole thing
was much less confusing with your initial merge-tree-ort proposal at
https://lore.kernel.org/git/CABPp-BEeBpJoU4yXdfA6vRAYVAUbd2gRhEV6j4VEqoqcu=[email-masked]/;
I.e. the end-state of merge-tree.c is that you end up reading largely
unrelated code (various static functions only used by one side or
another).

But maybe that's all water under the bridge etc, however...

>  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 we really need to make this interface more special-casey by
auto-guessing based on argc what argument you want? I.e. instead of
usage like:

	N_("git merge-tree [--write-tree] <branch1> <branch2>"),
	N_("git merge-tree [--trivial-merge] <base-tree> <branch1> <branch2>"),

Wouldn't it be simpler to just have the equivalent of:

	# old
        git merge-tree ...
        # new
        git merge-tree --new-thing ...

And not have to look at ... to figure out if we're dispatching to the
new or old thing.