Re: [PATCH v2 2/4] parse-options: add a separate case for help output on error

[Jeff King <peff@peff.net>]

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:

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