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

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

Jeff King <peff@peff.net> writes:

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

There isn't.  While I was writing [*], I wondered if the two calls
next to each other for opt3 and opt4 want to be combined to opt7.

  *  https://lore.kernel.org/git/xmqq1pbkefh0.fsf@gitster.g/



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

Yes.  I like that and that is why the static inlines are kept to
cover the most common cases.

I think I can do without [1/2], by the way.

 - die_for_incompatible_optN() (2 <= N <= 4) will keep accepting N
   pairs of <int, const char *>

 - die_for_incompatible_opts() will take pairs of <int, const char *>,
   expects "int" to be 0 (not set), 1 (set), or EOF==-1 (sentinel).

 - static inline void die_for_incompatible_opt2() emulation layer
   will call die_for_incompatible_opts(!!opt1, opt1_name, !!opt2,
   opt2_name, EOF).  Similarly for opt3() and opt4() variants.

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

Very true.

We could switch to dynamic allocations immediately after we see
option[] filled, as we are committed to die() at that point and can
afford to waste cycles.  That way, for die_for_incompatible_opt10()
when the end-user uses 7 of them, we can fill option[4], switch to
dynamic allocation to collect all 7 of them and report.

The reason I chose not to is primarily because we cannot use the
existing message templates in that case, hurting i18n/l10n.