On Mon, Jan 24, 2022 at 1:50 AM Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> wrote:
>
> On Sat, Jan 22 2022, Elijah Newren via GitGitGadget wrote:
>
...
> > + /* 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
> > + /* 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.
> > + 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.
