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

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