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

From: Jeff King <peff@peff.net>
Date: Sep 2, 2022, 17:13 UTC
Message-ID: <YxI5qBylRhj1jsEv@coredump.intra.peff.net>
In-Reply-To: <20220902050621.94381-1-oystwa@gmail.com>

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

Show 14 quoted lines

> 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