Re: [PATCH v3 03/15] merge-tree: add option parsing and initial shell for real merge function
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Feb 3, 2022, 02:05 UTC
- Message-ID
- <220203.86a6f87lbl.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <63f42df21aec5bda50e4414493eb59dcb64e5558.1643787281.git.gitgitgadget@gmail.com>
On Wed, Feb 02 2022, Elijah Newren via GitGitGadget wrote:
Show 7 quoted lines
> From: Elijah Newren <newren@gmail.com> > > 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=FGSw@mail.gmail.com/; 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...
Show 34 quoted lines
> 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.