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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 5, 2026, 20:42 UTC
Message-ID: <xmqq4imu2el0.fsf@gitster.g>
In-Reply-To: <20260305-b4-pks-odb-source-pluggable-v2-0-3290bfd1f444@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 19 quoted lines

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