Re: [PATCH 2/4] parse-options: name flags passed to usage_with_options_internal

[Junio C Hamano <gitster@pobox.com>]

"D. Ben Knoble" <ben.knoble+github@gmail.com> 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.

> 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

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


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

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

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

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