Re: [PATCH v2 1/4] i18n: factorize more 'incompatible options' messages

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 28, 2022, 23:58 UTC
Message-ID: <xmqqee4rtnts.fsf@gitster.g>
In-Reply-To: <7f46e276-b669-e8fe-21fd-6b43f5bfb33b@kdbg.org>

Johannes Sixt <j6t@kdbg.org> writes:

Show 36 quoted lines

>> +void die_if_incompatible_opt4(int opt1, const char *opt1_name,
>> +							  int opt2, const char *opt2_name,
>> +							  int opt3, const char *opt3_name,
>> +							  int opt4, const char *opt4_name)
>> +{
>> +	int count = 0;
>> +	const char *options[4];
>> +
>> +	if (opt1)
>> +		options[count++] = opt1_name;
>> +	if (opt2)
>> +		options[count++] = opt2_name;
>> +	if (opt3)
>> +		options[count++] = opt3_name;
>> +	if (opt4)
>> +		options[count++] = opt4_name;
>> +	switch (count) {
>> +	case 4:
>> +		die(_("options '%s', '%s', '%s', and '%s' cannot be used together"), opt1_name, opt2_name, opt3_name, opt4_name);
>> +		break;
>> +	case 3:
>> +		die(_("options '%s', '%s', and '%s' cannot be used together"), options[0], options[1], options[2]);
>> +		break;
>> +	case 2:
>> +		die(_("options '%s' and '%s' cannot be used together"), options[0], options[1]);
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +}
>
> Generally, this is good. I wonder whether we have to expect compiler
> warnings about unreachable break statements after the die() calls.
>
> A bit of code duplication could be avoided if die_if_incompatible_opt3()
> forwarded with an additional pair 0, "" to die_if_incompatible_opt4().

I wondered if a single varargs function

    void die_if_incompatible_optN(const char *name1, int opt1, ...);

that takes a name=NULL terminated sequence of <name, opt> would work, but

 (1) it would require the caller to always spell out the terminating
     NULL, which may be ugly.

 (2) it would tempt people to programatically generate message for N
     options, which leads to sentence lego, which is the exact
     opposite of what this series wants to achieve.

In any case, I do agree with you that the callers of _opt3() variants can just pass (0, "unused") in the 4-th slot. If this were made varargs, then it only needs to pass NULL at the end, so that might be an improvement, though.

Also, isn't "if" in the name of the function misleading? as far as I can tell, this function is not "check if these options are compatible and die if some are incompatible with each other", but the caller is convinced that these options are incompatible before it decides to call this function and there is no "if" in what this function can do.

void die_for_incompatible_opts(const char *name1, int opt1, ...)
{
	const char *name[4];
	int count = 0;
        va_list params;

        va_start(params, name1);

        if (opt1)
	        name[count++] = name1;
        while (count < ARRAY_SIZE(name)) {
                const char *n = va_arg(params, const char *);

                if (!n)
			break;
                if (va_arg(params, int))
	                name[count++] = n;
        }
        va_end(params);

        switch (count) {
	default:
		BUG("die-for-incompatible-opts can only take up to %d args",
		    ARRAY_SIZE(name));
	case 4:
		die(_("options '%s', '%s', '%s', and '%s'"
		      " cannot be used together"),
		    name[0], name[1], name[2], name[3]);
		break;
	case 3:
		die(_("options '%s', '%s', and '%s'"
		      " cannot be used together"),
		    name[0], name[1], name[2]);
		break;
	...
	}
}

might be easier to extend when somebody discovers there needs the "opt5" variant.