On Thu, Dec 28, 2023 at 09:25:55AM -0800, Junio C Hamano wrote:

Patrick Steinhardt [off-list ref] writes:

quoted

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

quoted

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