Re: [PATCH v2 2/4] parse-options: add a separate case for help output on error
From: Jeff King <hidden>
Date: 2026-07-02 08:38:08
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Wed, Jul 01, 2026 at 09:24:40PM +0000, brian m. carlson wrote:
However, there are some cases where we print help output because the user has provided ambiguous or invalid input, such as an ambiguous option, and we'll want to exit unsuccessfully there. Make this easier by defining a new return code, PARSE_OPT_HELP_ERROR, that can be used in this case, while reserving PARSE_OPT_HELP for those cases where the user has requested help directly.
Makes sense. We'd want to audit every spot that generates PARSE_OPT_HELP and see if it should be PARSE_OPT_HELP_ERROR. I only see one spot touched here:
quoted hunk ↗ jump to hunk
--- a/parse-options.c +++ b/parse-options.c@@ -583,7 +583,7 @@ static enum parse_opt_result parse_long_opt( ambiguous.option->long_name, (abbrev.flags & OPT_UNSET) ? "no-" : "", abbrev.option->long_name); - return PARSE_OPT_HELP; + return PARSE_OPT_HELP_ERROR; }
That one makes sense. The other site that generates it is within usage_with_options_internal(), which handles both asked-for "-h" and unexpected errors, but still always returns PARSE_OPT_HELP. Ah...it looks like you _do_ switch it in patch 4 (when the distinction between the two starts to make a difference). I think it should be done in this patch, though, since the point is generating the correct HELP/HELP_ERROR here (even though it does not yet matter). I wonder if we'd also want:
diff --git a/parse-options.c b/parse-options.c
index 742444eead..08c21d9fc0 100644
--- a/parse-options.c
+++ b/parse-options.c@@ -1373,7 +1373,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t parse_options_check_harder(opts); if (!usagestr) - return PARSE_OPT_HELP; + return err ? PARSE_OPT_HELP_ERROR : PARSE_OPT_HELP; if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL) fprintf(outfile, "cat <<\\EOF\n");
I can't figure out when we wouldn't have a usagestr, though. Perhaps not ever from parse-options itself, but only when called via usage_with_options() or something? That function does not look at our return value so it would not matter, but it feels like we should keep things consistent. -Peff