Re: [PATCH 1/5] parseopt: extract subcommand handling from parse_options_step()

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

Jiamu Sun <39@barroit.sh> writes:

> -static enum parse_opt_result parse_subcommand(const char *arg,
> -					      const struct option *options)

So, this function used to return either PARSE_OPT_SUBCOMMAND or
PARSE_OPT_UNKNOWN, and the caller (in an "switch ()" statement in
the parse_options_step() we see below) expected only these two.  We
now instead ...

> +static int parse_subcommand(const char *arg, const struct option *options)

... make it return either 0 (success - we found a subcommand) or -1
(failure - we did not).  The updated caller does use the return
value in "if ()" condition to return early when we found a
subcommand successfully, which is a lot more straight-forward than
the original "switch()".  The original is even worse in that it
enumerates other PARSE_OPT_* values that are possible at the time of
writing it, instead of catching everything else with "default:",
which makes the switch() statement a maintenance burden.  Getting
rid of that switch and clarifying that there are only two possible
outcome from this function alone is a good enough justification to
have this clean-up patch.  Very well done.


Editing the hunk to show only the postimage reveals that the new
implementation has CodingGuidelines violation ...

>  {
> +	for (; options->type != OPTION_END; options++) {
> +		if (options->type != OPTION_SUBCOMMAND ||
> +		    strcmp(options->long_name, arg))
> +			continue;
>  
> +		parse_opt_subcommand_fn **opt_val = options->value;

... here.  Move the variable definition up in the block introduced
by the for(;;) statement, perhaps?

> +		*opt_val = options->subcommand_fn;
> +
> +		return 0;
> +	}
> +
> +	return -1;
> +}
> +


And the part of the caller of parse_subcommand() that used to be the
switch() statement in the parse_options_step() is now extracted into
yet another helper function here.

> +static enum parse_opt_result handle_subcommand(struct parse_opt_ctx_t *ctx,
> +					       const char *arg,
> +					       const struct option *options,
> +					       const char * const usagestr[])
> +{
> +	int err = parse_subcommand(arg, options);
> +
> +	if (!err)
> +		return PARSE_OPT_SUBCOMMAND;
> +
> +	/*
> +	 * arg is neither a short or long option nor a subcommand.  Since this
> +	 * command has a default operation mode, we have to treat this arg and
> +	 * all remaining args as args meant to that default operation mode.
> +	 * So we are done parsing.
> +	 */

Thanks to being a small helper function, we lost two levels of
indentation which made this comment a lot more readable with
reflowing the lines ;-)

> +	if (ctx->flags & PARSE_OPT_SUBCOMMAND_OPTIONAL)
> +		return PARSE_OPT_DONE;
> +
> +	error(_("unknown subcommand: `%s'"), arg);
> +	usage_with_options(usagestr, options);
>  }

It is clear that there is no unintended behaviour change around this
helper function and parse_subcommand().  Very nice.

> @@ -990,37 +1016,16 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
>  		if (*arg != '-' || !arg[1]) {
>  			if (parse_nodash_opt(ctx, arg, options) == 0)
>  				continue;
> +
>  			if (!ctx->has_subcommands) {
>  				if (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION)
>  					return PARSE_OPT_NON_OPTION;
>  				ctx->out[ctx->cpidx++] = ctx->argv[0];
>  				continue;
> +
> +			} else {
> +				return handle_subcommand(ctx, arg,
> +							 options, usagestr);
>  			}
>  		}

Editing the hunk to only show the postimage shows us that the blank
line at the end of the "if ()" block is funny.  Drop it.  Or even
better, as the block either returns or continues anyway, lose the
"else" block, perhaps?  Which will make the above read like so:

			if (!ctx->has_subcommands) {
				if (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION)
                                	return PARSE_OPT_NON_OPTION;
				ctx->out[ctx->cpidx++] = ctx->argv[0];
				continue;
			}

                        return has_subcommands(ctx, arg, options, usagestr);

or swapping the logic the other way around, i.e.,

			if (ctx->has_subcommands)
				return has_subcommands(ctx, arg,
						       options, usagestr);

                        if (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION)
                                return PARSE_OPT_NON_OPTION;

                        ctx->out[ctx->cpidx++] = ctx->argv[0];
                        continue;

may make the result even easier to read, perhaps?