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

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