Re: [PATCH v2 1/3] pretty.c: rework describe options parsing for better extensibility

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Oct 25, 2021 at 9:36 PM Eli Schwartz <eschwartz@archlinux.org> wrote:
> It contains option arguments only, not options. We would like to add
> option support here too, but to do that we need to distinguish between
> different types of options.
>
> Lay out the groundwork for distinguishing between bools, strings, etc.
> and move the central logic (validating values and pushing new arguments
> to *args) into the successful match, because that will be fairly
> conditional on what type of argument is being parsed.
>
> Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
> ---
> diff --git a/pretty.c b/pretty.c
> @@ -1216,28 +1216,37 @@ int format_set_trailers_options(struct process_trailer_options *opts,
>  static size_t parse_describe_args(const char *start, struct strvec *args)
>  {
> +       struct {
> +               char *name;
> +               enum { OPT_STRING } type;
> +       }  option[] = {
> +               { "exclude", OPT_STRING },
> +               { "match", OPT_STRING },
> +       };
>         const char *arg = start;
>
>         for (;;) {
> +               int found = 0;
>                 const char *argval;
>                 size_t arglen = 0;
>                 int i;
>
> +               for (i = 0; !found && i < ARRAY_SIZE(option); i++) {
> +                       switch(option[i].type) {
> +                       case OPT_STRING:
> +                               if (match_placeholder_arg_value(arg, option[i].name, &arg,
> +                                                               &argval, &arglen) && arglen) {
> +                                       if (!arglen)
> +                                               return 0;

I may be missing something obvious, but how will it be possible for:

    if (!arglen)
        return 0;

to trigger if the `if` immediately above it:

    if (... && arglen) {

 has already asserted that `arglen` is not 0?

> +                                       strvec_pushf(args, "--%s=%.*s", option[i].name, (int)arglen, argval);
> +                                       found = 1;
> +                               }
>                                 break;
>                         }
>                 }
> +               if (!found)
>                         break;

The use of `found` to break out of a loop from within a `switch` seems
a bit clunky. An alternative would be to `goto` a label...

>         }
>         return arg - start;

... which could be introduced just before the `return`. Of course,
this is highly subjective, so not necessarily worth changing.