From: Kaartic Sivaraam Date: Fri, 30 Oct 2026 21:08:57 GMT Subject: Re: [PATCH v3 2/4] parse-options: extract lookup_long_opt() from parse_long_opt() Message-ID: <02ba1b9d-b598-454b-897f-cb33ba190b13@gmail.com> In-Reply-To: <20261012074413.337183-3-christian.couder@gmail.com> On 10/12/26 13:14, Christian Couder wrote: > In a following commit, an early scan of the command line will need to > look up long options the same way parse_options() does, including > unambiguous abbreviations and negated forms, but without any side > effect: it must neither set option values nor call die() or error(). > > parse_long_opt() can't be used for that, as it does both the look up > and the actions that follow it: it calls get_value() on the option it > finds, die()s when abbreviations are disallowed, and error()s on an > ambiguous abbreviation. > > Let's extract a new lookup_long_opt() helper from parse_long_opt() to > perform only the look up part, and let parse_long_opt() act on what > lookup_long_opt() returns. > > There is no behavior change intended. In particular an exact match is > still preferred to any abbreviation, abbreviations are still rejected > when disallowed only if there is no exact match, and the ambiguity > error message is unchanged. > Neatly explained! > diff --git a/parse-options.c b/parse-options.c > index d7cbb5a343..a98b958423 100644 > --- a/parse-options.c > +++ b/parse-options.c > @@ -516,16 +516,46 @@ static void register_abbrev(struct parse_opt_ctx_t *p, > abbrev->flags = flags; > } > > -static enum parse_opt_result parse_long_opt( > - struct parse_opt_ctx_t *p, const char *arg, > - const struct option *options) > +enum long_opt_lookup { > + LONG_OPT_EXACT, /* exact match found */ > + LONG_OPT_ABBREV, /* unambiguous abbreviation found */ > + LONG_OPT_AMBIGUOUS, /* more than one candidate abbreviation */ > + LONG_OPT_UNKNOWN, /* nothing matches */ > +}; > + > +/* > + * Look up the long option named by `arg` (without its leading "--") > + * without acting on it. (Neither prints nor dies.) > + * > + * Return value: > + * - On LONG_OPT_EXACT, `found` is the exact matching option and its flags > + * (including OPT_UNSET for a negated form), and `stuck` points after > + * the '=' or is NULL. > + * - On LONG_OPT_ABBREV, `found` is the unambiguous abbreviation, same as > + * above for its flags and `stuck`. > + * - On LONG_OPT_AMBIGUOUS, `found` and `other` are two candidates of an > + * ambiguous abbreviation. > + * - On LONG_OPT_UNKNOWN, nothing matches, no result. > + * > + * `other` is only meaningful for LONG_OPT_AMBIGUOUS. > + */ > +static enum long_opt_lookup lookup_long_opt(struct parse_opt_ctx_t *p, > + const char *arg, > + const struct option *options, > + struct parsed_option *found, > + struct parsed_option *other, > + const char **stuck) > { > + enum opt_parsed flags = OPT_LONG; > const char *arg_end = strchrnul(arg, '='); > const char *arg_start = arg; > - enum opt_parsed flags = OPT_LONG; > int arg_starts_with_no_no = 0; > - struct parsed_option abbrev = { .option = NULL, .flags = OPT_LONG }; > - struct parsed_option ambiguous = { .option = NULL, .flags = OPT_LONG }; > + > + found->option = NULL; > + found->flags = OPT_LONG; > + other->option = NULL; > + other->flags = OPT_LONG; > + *stuck = NULL; > > if (skip_prefix(arg_start, "no-", &arg_start)) { > if (skip_prefix(arg_start, "no-", &arg_start)) > @@ -554,43 +584,70 @@ static enum parse_opt_result parse_long_opt( > > if (skip_prefix(arg_start, long_name, &rest)) { > if (*rest == '=') > - p->opt = rest + 1; > + *stuck = rest + 1; > else if (*rest) > continue; > - return get_value(p, options, flags ^ opt_flags); > + found->option = options; > + found->flags = flags ^ opt_flags; > + return LONG_OPT_EXACT; > } > > /* abbreviated? */ > if (!strncmp(long_name, arg_start, arg_end - arg_start)) > register_abbrev(p, options, flags ^ opt_flags, > - &abbrev, &ambiguous); > + found, other); > > /* negated and abbreviated very much? */ > if (allow_unset && starts_with("no-", arg)) > register_abbrev(p, options, OPT_UNSET ^ opt_flags, > - &abbrev, &ambiguous); > + found, other); > } > > - if (disallow_abbreviated_options && (ambiguous.option || abbrev.option)) > + if (other->option) > + return LONG_OPT_AMBIGUOUS; > + > + if (found->option) { > + if (*arg_end) > + *stuck = arg_end + 1; > + return LONG_OPT_ABBREV; > + } > + > + return LONG_OPT_UNKNOWN; > +} > + > +static enum parse_opt_result parse_long_opt(struct parse_opt_ctx_t *p, > + const char *arg, > + const struct option *options) > +{ > + struct parsed_option found; > + struct parsed_option other; > + const char *stuck; > + enum long_opt_lookup result = lookup_long_opt(p, arg, options, > + &found, &other, &stuck); > + Would it not be better to consume the `enum long_op_lookup` return value via a switch rather than having a set of if statements to cover all the cases? > + if (result == LONG_OPT_UNKNOWN) > + return PARSE_OPT_UNKNOWN; > + > + if (disallow_abbreviated_options && > + (result == LONG_OPT_ABBREV || result == LONG_OPT_AMBIGUOUS)) { > + const char *arg_end = strchrnul(arg, '='); > die("disallowed abbreviated or ambiguous option '%.*s'", > (int)(arg_end - arg), arg); > + } > > - if (ambiguous.option) { > - error(_("ambiguous option: %s " > - "(could be --%s%s or --%s%s)"), > - arg, > - (ambiguous.flags & OPT_UNSET) ? "no-" : "", > - ambiguous.option->long_name, > - (abbrev.flags & OPT_UNSET) ? "no-" : "", > - abbrev.option->long_name); > + if (result == LONG_OPT_AMBIGUOUS) { > + error(_("ambiguous option: %s (could be --%s%s or --%s%s)"), arg, > + (other.flags & OPT_UNSET) ? "no-" : "", other.option->long_name, > + (found.flags & OPT_UNSET) ? "no-" : "", found.option->long_name); > return PARSE_OPT_HELP_ERROR; > } > - if (abbrev.option) { > - if (*arg_end) > - p->opt = arg_end + 1; > - return get_value(p, abbrev.option, abbrev.flags); > + > + if (result == LONG_OPT_ABBREV || result == LONG_OPT_EXACT) { > + p->opt = stuck; > + return get_value(p, found.option, found.flags); > } > - return PARSE_OPT_UNKNOWN; > + > + BUG("unknown enum long_opt_lookup value '%d'", result); > } > > static enum parse_opt_result parse_nodash_opt(struct parse_opt_ctx_t *p, Rest of the patch looks good to me. -- Sivaraam