Re: [PATCH 2/4] parse-options: name flags passed to usage_with_options_internal
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 28, 2025, 15:26 UTC
- Message-ID
- <xmqqjz3sxro3.fsf@gitster.g>
- In-Reply-To
- <20250726165320.4039-3-ben.knoble+github@gmail.com>
"D. Ben Knoble" <ben.knoble+github@gmail.com> writes:
Show 5 quoted lines
> When reading or editing calls to usage_with_options_internal, it is > difficult to tell what trailing "0, 0", "0, 1", "1, 0" arguments mean > (NB there is never a "1, 1" case). > > Give the flags readable names to improve call-sites.
It is a good idea to explicitly say that this step introduces no change in behaviour, and only changes the way how these 0/1 are spelled.
Show 22 quoted lines
> Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
> ---
> parse-options.c | 32 ++++++++++++++++++++++----------
> 1 file changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index 5224203ffe..c3222cc9bb 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -953,10 +953,21 @@ static void free_preprocessed_options(struct option *options)
> free(options);
> }
>
> +enum usage_style {
> + style_normal = 0,
> + style_full = 1,
> +};
> +
> +enum usage_output {
> + to_out = 0,
> + to_err = 1,
> +};These are very much internal implementation detail, so I am not sure if this churn is a good thing, though.
For example, it ought to be sufficient, for the purpose of improved readability, to instead doing this
Show 6 quoted lines
> static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *, > const char * const *, > const struct option *, > - int, int); > + enum usage_style, > + enum usage_output);
just do
int full_usage, int usage_to_stderr);
here. Dropping the parameter names in the function prototype is allowed, and we encourage to do so in our codebase but _only_ when the meaning of each parameter is obvious from their type. The first 3 parameters we see above are of distinct types and except for the second one being the usage string given to the users, they should be obvious. But the last two unnamed integers are not obvious and they should have been spelled out---otherwise a developer who is adding a new callsite cannot work from the prototype alone and has to go to the implementation to figure out what to pass.
Adding two enums for this is a bit overkill, but is OK here locally.
Show 6 quoted lines
> @@ -1088,7 +1099,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx, > } > > if (internal_help && !strcmp(arg + 2, "help-all")) > - return usage_with_options_internal(ctx, usagestr, options, 1, 0); > + return usage_with_options_internal(ctx, usagestr, options, style_full, to_out);
But this is not an improvement as-is. Wrap long lines or the result is even harder to read.
Show 9 quoted lines
> @@ -1278,10 +1289,11 @@ static const struct option *find_option_by_long_name(const struct option *opts,
> static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *ctx,
> const char * const *usagestr,
> const struct option *opts,
> - int full, int err)
> + enum usage_style help_style,
> + enum usage_output to_where)
> {
> const struct option *all_opts = opts;> - FILE *outfile = err ? stderr : stdout; > + FILE *outfile = to_where == to_err ? stderr : stdout;
This one ...
> @@ -1327,7 +1339,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t > - if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL) > + if (to_where != to_err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL) > fprintf(outfile, "cat <<\\EOF\n");
... and this one become markedly harder to read. I think the primary reason is because unlike the original, the parameter names are not biased. "If we are doing full usage, do this" is far easier to grok than "If the "style" we are told to use is the "style_full", then do this", but use of "enum" inherently is about not making the variables and parameters of that enum type unbiased.
Show 7 quoted lines
> @@ -1373,7 +1385,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t > fprintf(outfile, "%s\n", _(opts->help)); > continue; > } > - if (!full && (opts->flags & PARSE_OPT_HIDDEN)) > + if (help_style != style_full && (opts->flags & PARSE_OPT_HIDDEN)) > continue;
Ditto.
Show 8 quoted lines
> if (need_newline) {
> @@ -1435,7 +1447,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
> }
> fputc('\n', outfile);
>
> - if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
> + if (to_where != to_err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
> fputs("EOF\n", outfile);Ditto.
One way to reduce this churn is to do
int err = (to_where == to_stderr); int full = (help_style == style_full);
at the very beginning of the function. Then you do not have to change the body of the function harder to read at all.
Thanks.