On Thu, Dec 28, 2023 at 09:25:55AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@xxxxxx> writes:
>
> > In order to look up ref storage backends, we're currently using a linked
> > list of backends, where each backend is expected to set up its `next`
> > pointer to the next ref storage backend. This is kind of a weird setup
> > as backends need to be aware of other backends without much of a reason.
> >
> > Refactor the code so that the array of backends is centrally defined in
> > "refs.c", where each backend is now identified by an integer constant.
> > Expose functions to translate from those integer constants to the name
> > and vice versa, which will be required by subsequent patches.
>
> A small question. Does this have to be "int", or is "unsigned" (or
> even an enum, rewrittenfrom the "REF_STORAGE_FORMAT_*" family of CPP
> macro constants) good enough? I am only wondering what happens when
> you clal find_ref_storage_backend() with a negative index.
No, it does not have to be an `int`, and handling a negative index would
be a bug. I tried to stick to what we have with `GIT_HASH_UNKNOWN`,
`GIT_HASH_SHA1` etc, which is exactly similar in spirit. Whether it's
the perfect way to handle this... probably not. Without the context I
would've used an `enum`, but instead I opted for consistency.
> For that matter, how REF_STORAGE_FORMAT_UNKNOWN (whose value is 0)
> is handled by the function also gets curious. The caller may have
> to find that the backend hasn't been specified by receiving an
> element in the refs_backends[] array that corresponds to it, but the
> error behaviour of this function is also to return NULL, so it has
> to be prepared to handle both cases?
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?
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;
> > }
Attachment:
signature.asc
Description: PGP signature
