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

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