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

From: Elijah Newren <newren@gmail.com>
Date: Jan 24, 2022, 16:54 UTC
Message-ID: <CABPp-BG1UXDLVh4_F_TQJmiM4=fFMOLFo5k27=MghbBPWPkL7A@mail.gmail.com>
In-Reply-To: <220124.86lez5ihso.gmgdl@evledraar.gmail.com>

On Mon, Jan 24, 2022 at 1:50 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

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

...

Show 7 quoted lines

> > +     /* 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.

Yep, sure was cargo-culted from somewhere else (my parse-options usage always is), but I'm pretty sure it was from another place also using parse-options. Probably one of these 15 places:

 $ comm -12 <(git grep -l parse-options builtin/ | sort) <(git grep -l
strcmp.*-h\\b builtin/ | sort)
builtin/am.c
builtin/branch.c
builtin/checkout-index.c
builtin/checkout--worker.c
builtin/commit.c
builtin/commit-tree.c
builtin/gc.c
builtin/ls-files.c
builtin/merge.c
builtin/merge-tree.c
builtin/rebase.c
builtin/rev-parse.c
builtin/sparse-checkout.c
builtin/submodule--helper.c
builtin/update-index.c

Show 9 quoted lines

> > +     /* 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).

TIL.  Thanks.

Show 14 quoted lines

> > +     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.

Not sure what you mean; I already route to sub-functions. But I should definitely add PARSE_OPT_STOP_AT_NON_OPTION; it's unfortunate that it's not the default.