From: Jiamu Sun <39@barroit.sh>
Date: Mon, 09 Mar 2026 01:56:48 GMT
Subject: Re: [PATCH 1/5] parseopt: extract subcommand handling from parse_options_step()
Message-ID: <SY0P300MB0801019DB36BC682AFDBF2C9CE79A@SY0P300MB0801.AUSP300.PROD.OUTLOOK.COM>
In-Reply-To: <xmqq1phtao03.fsf@gitster.g>

On Sun, Mar 08, 2026 at 04:40:44PM -0700, Junio C Hamano wrote:
> ... here.  Move the variable definition up in the block introduced
> by the for(;;) statement, perhaps?
> 
> > +		*opt_val = options->subcommand_fn;
> > +
> > +		return 0;
> > +	}

Agree. Actually, I forgot to check the CodingGuidelines.  Will fix the
coding style issue in v3.

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

Swapping the logic works better here. Returning early in the
ctx->has_subcommands path lets the rest of the block assume
!ctx->has_subcommands, so the extra check can go away. That makes
the code easier to read. Will do that.

-- 
Jiamu Sun <39@barroit.sh>