Re: [PATCH 2/2] die_for_incompatible_opts(): accept more than four options

[Jeff King <peff@peff.net>]

On Wed, Aug 26, 2026 at 04:31:52PM -0700, Junio C Hamano wrote:

> To avoid allocation costs, the implementation reports only the first
> four mutually incompatible options used.
> 
> This behavior is deliberate.  If a set of ten options were mutually
> exclusive and a user specified seven of them at once, they would be
> told that the first four cannot be used together.  If the user then
> tries the remaining three, the same error for the remaining three
> would be reported.  It is dubious that there is any practical
> downside to not reporting all seven incompatible options at once,
> especially given that there are other three mutually incompatible
> options that the user will not be told about with this message
> anyway.

It took me a minute to understand why we would even want to have an
arbitrary-sized input if we are capping at 4 anyway. The answer is that
we are capping at 4 options _that the user actually specified_. But the
input can be the total set of conflicting options, which is greater. OK.

Really we could cap at 2 if we wanted to be technically correct, but it
might annoy the user to find each pair iteratively.

So that makes sense. Of course the follow-on question is whether any
callers actually want to pass more than 4 options. I don't see any
patches adding new calls.

> -void die_for_incompatible_opt4(const char *opt1_name, int opt1,
> -			       const char *opt2_name, int opt2,
> -			       const char *opt3_name, int opt3,
> -			       const char *opt4_name, int opt4)

One nice thing about foo4() without varargs is that the compiler will
tell you if you messed it up. The obvious downside being that you have
to count in order to avoid messing it up. ;)

But now we can forget the NULL terminator and cause a runtime problem.
So we probably want LAST_ARG_MUST_BE_NULL in the header file here:

> +void die_for_incompatible_opts(const char *opt1_name, int opt1, ...);

The rest of the patch looks OK, but just a few observations.

> +void die_for_incompatible_opts(const char *opt1_name, int opt1, ...)
>  {
> -	int count = 0;
> +	unsigned count = 0;
>  	const char *options[4];
> +	va_list ap;
> +
> +	va_start(ap, opt1);
>  
>  	if (opt1)
>  		options[count++] = opt1_name;
> -	if (opt2)
> -		options[count++] = opt2_name;
> -	if (opt3)
> -		options[count++] = opt3_name;
> -	if (opt4)
> -		options[count++] = opt4_name;
> +	while (count < ARRAY_SIZE(options)) {

Using ARRAY_SIZE() is nice, because we could in theory bump this 4
later. Though sadly here:

>  	switch (count) {
>  	case 4:
>  		die(_("options '%s', '%s', '%s', and '%s' cannot be used together"),
> -		    opt1_name, opt2_name, opt3_name, opt4_name);
> +		    options[0], options[1], options[2], options[3]);

we still hard-code various count values. It probably would be fine to
allocate a buffer for the message, though I guess that pushes
translators into lego-land.

> +static inline void die_for_incompatible_opt4(const char *opt1_name, int opt1,
> +					     const char *opt2_name, int opt2,
> +					     const char *opt3_name, int opt3,
> +					     const char *opt4_name, int opt4)
> +{
> +	die_for_incompatible_opts(opt1_name, opt1,
> +				  opt2_name, opt2,
> +				  opt3_name, opt3,
> +				  opt4_name, opt4, NULL);
> +}

OK, now we wrap the arbitrary-sized version. The "3" and "2" variants
could probably be cleaned up slightly by calling it, too, rather than
passing dummy 0/"" values.

-Peff