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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 8, 2026, 23:40 UTC
Message-ID: <xmqq1phtao03.fsf@gitster.g>
In-Reply-To: <SY0P300MB0801422323C4C4185B9617A1CE78A@SY0P300MB0801.AUSP300.PROD.OUTLOOK.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 ...

Show 7 quoted lines

>  {
> +	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?

Show 8 quoted lines

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

Show 16 quoted lines

> +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 ;-)

Show 6 quoted 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.

Show 16 quoted lines

> @@ -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?