Re: [PATCH 09/20] parse-options: add support for parsing subcommands

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

SZEDER Gábor <szeder.dev@gmail.com> writes:

> The approach is guided by the following observations:
> ...
> So in the end subcommand declaration and parsing would look something
> like this:
>
>     parse_opt_subcommand_fn *fn = NULL;
>     struct option builtin_commit_graph_options[] = {
>         OPT_STRING(0, "object-dir", &opts.obj_dir, N_("dir"),
>                    N_("the object directory to store the graph")),
>         OPT_SUBCOMMAND("verify", &fn, graph_verify),
>         OPT_SUBCOMMAND("write", &fn, graph_write),
>         OPT_END(),
>     };
>     argc = parse_options(argc, argv, prefix, options,
>                          builtin_commit_graph_usage, 0);
>     return fn(argc, argv, prefix);

All makes sense and exactly as I expected to see, after reading the
later steps [10-20/20] that make use of the facility.  Nicely designed.

> Here each OPT_SUBCOMMAND specifies the name of the subcommand and the
> function implementing it, and the same pointer to 'fn' where
> parse_options() will store the function associated with the given
> subcommand.

Puzzling.  Isn't parse_options() an read-only operation with respect
to the elements of the struct option array?  

With s/store/expect to find/, I would understand the above, though.

> There is no need to check whether 'fn' is non-NULL before
> invoking it:

Again, this is puzzling.  "There is no need to"---to whom does this
statement mean to advise?  The implementor of the new codepath IN
parse_options() to handle OPT_SUBCOMMAND()?

> if there were no subcommands given on the command line
> then the parse_options() call would error out and show usage.  

I think that you mean to say

    When one or more OPT_SUBCOMMAND elements exist in an array of
    struct option, parse_options() will error out if none of them
    triggers.

and that makes sense, but I cannot quite tell how it relates to the
previous sentence about fn possibly being NULL.

Ahh, OK.  Let's try to rephrase to see if I can make it easier to
understand.

    Here each OPT_SUBCOMMAND specifies ... with the given
    subcommand.

    After parse_options() returns, the variable 'fn' would have been
    updated to point at the function to call, if one of the
    subcommand specified by the OPT_SUBCOMMAND() is given on the
    command line.

    - When the PARSE_OPT_SUBCOMMAND_OPTIONAL flag is given to
      parse_options(), and no subcommand is given on the command
      line, the variable 'fn' is left intact.  This can be used to
      implement a command with the "default" subcommand by
      initializing the variable 'fn' to the default value.

    - Otherwise, parse_options() will error out and show usage, when
      no subcommand is given.

    - If more than one subcommands are given from the command line,
      parse_options() will stop at the first one, and treat the
      remainder of the command line as arguments to the subcommand.
    
> In case
> a command has a default operation mode, 'fn' should be initialized to
> the function implementing that mode, and parse_options() should be
> invoked with the PARSE_OPT_SUBCOMMAND_OPTIONAL flag.

OK.

> Some thoughts about the implementation:
>
>   - Arguably it is a bit weird that the same pointer to 'fn' have to
>     be specified as 'value' for each OPT_SUBCOMMAND, but we need a way
>     to tell parse_options() where to put the function associated with
>     the given subcommand, and I didn't like the alternatives:

I do not find this so disturbing.  This is similar to CMDMODE in
spirit in that only one has to be chosen.  CMDMODE needs to go an
extra mile to ensure that by checking the current value in the
variable pointed by the pointer because the parser does not stop
immediately after getting one, but SUBCOMMAND can afford to be
simpler because the parser immediately stops once a subcommand is
found.

In a sense, OPT_SUBCOMMAND() does not have to exist as the first-class
mechanism.  With just two primitives:

 - a flag that says "after seeing this option, immediately stop
   parsing".
 - something similar to OPT_SET_INT() but works with a function pointer.

we can go 80% of the way to implement OPT_SUBCOMMAND() as a thin
wrapper (there is "one of the OPT_SET_FUNC() must be triggered" that
needs new code specific to the need, which is the other 20%).

>   - I decided against automatically calling the subcommand function
>     from within parse_options(), because:
>
>       - There are commands that have to perform additional actions
>         after option parsing but before calling the function
>         implementing the specified subcommand.
>
>       - The return code of the subcommand is usually the return code
>         of the git command, but preserving the return code of the
>         automatically called subcommand function would have made the
>         API awkward.

Yes, the above makes perfect sense.  Also I suspect that we would
find some cases where being able to use two or more variables become
handy.

> +			case PARSE_OPT_COMPLETE:
> +			case PARSE_OPT_HELP:
> +			case PARSE_OPT_ERROR:
> +			case PARSE_OPT_DONE:
> +			case PARSE_OPT_NON_OPTION:
> +				BUG("parse_subcommand() cannot return these");
> +			}

"these" without giving the violating value?  A "%d" would be cheap
addition, but I see this was copied from elsewhere.

Thanks.