On Mon, Jul 28, 2025 at 11:26 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "D. Ben Knoble" <ben.knoble+github@xxxxxxxxx> writes:
>
> > 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.
Woops; definitely meant for that to be clearer. Will touch up.
>
> > Signed-off-by: D. Ben Knoble <ben.knoble+github@xxxxxxxxx>
> > ---
> > 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
>
> > 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.
Yeah, but that relies on folks reading the prototype, no? I wanted it
to be easier to read at the call sites (_without_ special tooling,
preferably).
I'll snip the rest, though, due to your downthread suggestion to use
#define's instead, which I think gives the result I want without extra
churn in other places.
> > @@ -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.
Ah, indeed: I wasn't sure where to draw the line there.
