On Mon, Jul 28, 2025 at 11:26 AM Junio C Hamano [off-list ref] wrote:

"D. Ben Knoble" [off-list ref] writes:

quoted

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.

quoted

Signed-off-by: D. Ben Knoble <redacted>
---
 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

quoted

 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.

quoted

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