Re: [PATCH] rev-parse: Detect missing opt-spec

[Jeff King <peff@peff.net>]

On Fri, Sep 02, 2022 at 07:06:21AM +0200, Øystein Walle wrote:

> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index b259d8990a..04958cf9a9 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -479,6 +479,9 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
>  		if (!s)
>  			s = help;
>  
> +		if (s == sb.buf)
> +			die(_("Missing opt-spec before option flags"));
> +
>  		if (s - sb.buf == 1) /* short option only */
>  			o->short_name = *sb.buf;
>  		else if (sb.buf[1] != ',') /* long option only */

I think this is the right thing to do, at least for now. Certainly it
catches the bug. It doesn't allow short or long option names to contain
any flag characters, but that's probably OK in practice.

I think one could make an argument that cmd_parseopt() should do a
better job of parsing left-to-right. The reason it missed this case is
that it calls strpbrk(), expecting to jump past the short/long option
names, but it jumps less far than expected.

If the parsing were more left-to-right, like:

  - start with pointer at beginning of sb.buf

  - look for acceptable character for short option, or "," for none;
    advance pointer if found, otherwise bail

  - look for syntactically valid long option name; advance pointer,
    otherwise bail

  - look for valid flags

then I think it becomes much easier to reason about what is valid for
each item. And we _could_ do things like allowing a short-option that is
also a flag-character, if we wanted to.

But IMHO such a refactoring can come later, or not at all. While it
might make the code a bit more clear, I don't think it meaningfully
improves the behavior. And either way, we should start with a minimal
and easy-to-verify fix like you have here.

-Peff