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

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

Patrick Steinhardt <ps@pks.im> writes:

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


>
> 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;
>> >  }