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

From: Jeff King <peff@peff.net>
Date: Aug 29, 2026, 11:14 UTC
Message-ID: <20260829111418.GA40814@coredump.intra.peff.net>
In-Reply-To: <xmqqv78vbphh.fsf@gitster.g>

On Thu, Aug 27, 2026 at 07:35:38AM -0700, Junio C Hamano wrote:

Show 6 quoted lines

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

OK. I wonder if we're approaching churn here, but I don't have a strong feeling.

Show 11 quoted lines

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

Yeah, but then you can't get good compiler support, since I don't think there is an integer equivalent to LAST_ARG_MUST_BE_NULL. So the varargs interface feels less safe (and strictly worse since we are not actually helping any case that has more than 4 items).

If we're not actually exposing the varargs version and expect people to use the counted wrappers, then it's not as big a risk. But then I wonder what the value of the patch is.

Show 8 quoted lines

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

Yeah, that makes sense.

-Peff