Re: [PATCH v2 03/12] refs: refactor logic to look up storage backends

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 28, 2023, 20:42 UTC
Message-ID: <xmqq4jg2gitt.fsf@gitster.g>
In-Reply-To: <ZY3Wcua6dtzO2jG4@framework>

Patrick Steinhardt <ps@pks.im> writes:

Show 11 quoted lines

> Yeah, we do not really discern those two cases for now and instead just
> return `NULL` both for any unknown ref storage format. All callers know
> to handle `NULL`, but the error handling will only report a generic
> "unknown" backend error.
>
> The easiest way to discern those cases would be to `BUG()` when being
> passed an invalid ref storage format smaller than 0 or larger than the
> number of known backends. Because ultimately it is just that, a bug that
> shouldn't ever occur.
>
> Not sure whether this is worth a reroll?

By using an unsigned type, you no longer have to worry about getting handed a negative index, as the "must be smaller than ARRAY_SIZE()" check will be sufficient to catch anybody who passes "-1" (casted to unsigned by parameter passing). So I would say that would be a good enough reason to reroll, whether we differentiate 0 and an index that is larger than refs_backends[] (or a negative one) with an explicit BUG(), or just leave it to the caller by returning NULL. As to the error handling, I suspect it is sufficient to return NULL and let the caller handle it.

Thanks.

Show 13 quoted lines

>
> Patrick
>
>> > +static const struct ref_storage_be *refs_backends[] = {
>> > +	[REF_STORAGE_FORMAT_FILES] = &refs_be_files,
>> > +};
>> > ...
>> > +static const struct ref_storage_be *find_ref_storage_backend(int ref_storage_format)
>> >  {
>> > +	if (ref_storage_format < ARRAY_SIZE(refs_backends))
>> > +		return refs_backends[ref_storage_format];
>> >  	return NULL;
>> >  }