Re: [PATCH v2 00/17] odb: make object database sources pluggable

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

Patrick Steinhardt <ps@pks.im> writes:

> To set expectations: this is only a start, there is still functionality
> missing that needs to be made pluggable. Most importantly:
>
>   - Counting of objects.
>
>   - Abbreviating object IDs and finding ambiguous objects.
>
>   - Consistency checks.
>
>   - Optimizing the object database.
>
>   - Generating packfiles.
>
> These will all happen in later patch series. That being said, with this
> patch series one already gets a lot of the basic functionality, and it's
> almost possible to do local workflows. Only "almost" though because we
> rely on abbreviating object IDs in a lot of places, but once that part
> is implemented in a subsequent patch series you can indeed work locally
> with an alternate backend.

I've been looking over this series, and the transition to a pluggable
interface for ODB sources is very clean and follows the patterns we've
established for refs and streams quite well.

One thing I am puzzled on the design, specifically starting with
patch 09 and onward, is the lack of documentation regarding which of
the new callbacks in `struct odb_source` are mandatory and which are
optional.

In `odb/source.h`, the static inline wrapper functions dereference the
backend's function pointers directly. For example:

+static inline int odb_source_read_object_info(struct odb_source *source,
+					      const struct object_id *oid,
+					      struct object_info *oi,
+					      enum object_info_flags flags)
+{
+	return source->read_object_info(source, oid, oi, flags);
+}

If a future backend (say, a read-only network proxy) doesn't implement
some of the write-related functions or the iteration functions, the
current wrappers will cause a segmentation fault.

Do we want to

  - Document in `struct odb_source` which callbacks must be implemented
    by every backend.

  - Have the wrapper functions check for NULL. If a mandatory function
    is missing, a `BUG()` would be appropriate. If it's truly optional,
    the wrapper could return a suitable error code (like -1 or
    `GIT_ENOTSUP`).

Given that the "files" backend implements the full set, it's easy to
miss, but as we add more specialized backends, a clearly defined
interface contract may become important.

What are your thoughts on which of these should be considered the
"minimal viable" set for an ODB source?