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, 18:19 UTC
Message-ID: <xmqqseiguqig.fsf@gitster.g>
In-Reply-To: <xmqqjz3sxro3.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 26 quoted lines

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

And with that lessor impact change, you could still add a smaller change to help callers (you do not need any change to the callee, which uses biased parameter names for these two), if you wanted to (though as I said this is internal implementation detail of the parse_options API, so you do not have to). For example, you could do

#define USAGE_FULL 1 #define USAGE_TO_STDERR 1

without anything else and do

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

			return usage_with_options_internal(ctx,	usagestr, options,
							   USAGE_FULL, 0);

which may make it easier to follow. The point is that you be more verbose only when you do a non-standard thing.

And without enum, of course you do not need any change like below.

Show 7 quoted lines

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