From: Justin Tobler <jltobler@gmail.com>
Date: Thu, 05 Mar 2026 17:06:10 GMT
Subject: Re: [PATCH 03/17] odb: embed base source in the "files" backend
Message-ID: <aam2f4NBwOEor-Qc@denethor>
In-Reply-To: <aamDyLxTYQdh9igw@pks.im>

On 26/03/05 02:23PM, Patrick Steinhardt wrote:
> On Wed, Mar 04, 2026 at 11:40:47AM -0600, Justin Tobler wrote:
> > On 26/02/23 05:17PM, Patrick Steinhardt wrote:
> > > diff --git a/odb/source-files.h b/odb/source-files.h
> > > index 0b8bf773ca..58753d40de 100644
> > > --- a/odb/source-files.h
> > > +++ b/odb/source-files.h
> > > @@ -10,15 +11,26 @@ struct packfile_store;
> > >   * packfiles. It is the default backend used by Git to store objects.
> > >   */
> > >  struct odb_source_files {
> > > -	struct odb_source *source;
> > > +	struct odb_source base;
> > 
> > Out of curiousity, was there any reason to the reference ODB source in
> > the prior patch? Seems like we could have just added it here.
> 
> Good question. The reason why I stored this pointer in the preceding
> commit is mostly to demonstrate that we're actually using the source
> that's passed to `db_source_files_new()`. I didn't want to have to
> change the signature of that function in this commit again.
> 
> So the field was unused indeed, but intentionally so.

That's fair. I did find it mildly confusing to see its introduction
without any uses, only to be renamed here. But it's not really a big
deal either way.

> > From a naming perspective, I do find the odb_source_new() vs
> > odb_source_init() and odb_source_free() vs odb_source_release()
> > interfaces to be tad bit confusing. I understand that odb_source_init()
> > and odb_source_release() and only intended for use by the concrete ODB
> > source implementations to facilitate initializing/freeing the base ODB
> > source. The comments also do help clarify this, but I think it is still
> > rather easy to get them mixed up when reading.
> > 
> > Maybe we could rename them to odb_base_source_init() and
> > odb_base_source_free()?
> 
> I think for `odb_source_free()` it's a definitive no. This will be the
> way to free any source, not only the base, and this will become clear in
> a subsequent patch.

Fair.

> For `odb_source_init()` you have a better point though, as it really
> only cares about initializing the base object. But I think it's still
> sensible to keep the name as it _does_ act on `struct odb_source`, and
> it would be the only instance where we have the "base" infix.

Ya it does still act on the `struct odb_source`, but IMO the name fails
to properly differentiant it's usecase which it a tad bit confusing.
Naming is hard though and I don't have really a better suggestion so it
is probably fine as-is. At least the comments do a reasonable job of
explaining the intent here. :)

-Justin