From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 05 Mar 2026 13:23:20 GMT
Subject: Re: [PATCH 03/17] odb: embed base source in the "files" backend
Message-ID: <aamDyLxTYQdh9igw@pks.im>
In-Reply-To: <aahkh1ICViKjP6Il@denethor>

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.

> >  	struct odb_source_loose *loose;
> >  	struct packfile_store *packed;
> >  };
> >  
> >  /* Allocate and initialize a new object source. */
> > -struct odb_source_files *odb_source_files_new(struct odb_source *source);
> > +struct odb_source_files *odb_source_files_new(struct object_database *odb,
> > +					      const char *path,
> > +					      bool local);
> >  
> >  /* Free the object source and release all associated resources. */
> >  void odb_source_files_free(struct odb_source_files *files);
> >  
> > +/*
> > + * Cast the given object database source to the files backend. This will cause
> > + * a BUG in case the source doesn't use this backend.
> > + */
> 
> In the commit message you mention that eventually
> `odb_source_files_downcast()` will BUG() if the source doesn't use the
> backend. But, it doesn't appear to do this yet. Should we still have
> this comment?

Good point, let me move this into the patch that introduces this.

> > diff --git a/odb/source.h b/odb/source.h
> > index 1c34265189..e6698b73a3 100644
> > --- a/odb/source.h
> > +++ b/odb/source.h
> > @@ -53,7 +48,31 @@ struct odb_source *odb_source_new(struct object_database *odb,
> >  				  const char *path,
> >  				  bool local);
> >  
> > -/* Free the object database source, releasing all associated resources. */
> > +/*
> > + * Initialize the source for the given object database located at `path`.
> > + * `local` indicates whether or not the source is the local and thus primary
> > + * object source of the object database.
> > + *
> > + * This function is only supposed to be called by specific object source
> > + * implementations.
> > + */
> > +void odb_source_init(struct odb_source *source,
> > +		     struct object_database *odb,
> > +		     const char *path,
> > +		     bool local);
> > +
> > +/*
> > + * Free the object database source, releasing all associated resources and
> > + * freeing the structure itself.
> > + */
> >  void odb_source_free(struct odb_source *source);
> >  
> > +/*
> > + * Release the object database source, releasing all associated resources.
> > + *
> > + * This function is only supposed to be called by specific object source
> > + * implementations.
> > + */
> > +void odb_source_release(struct odb_source *source);
> 
> 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.

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.

Patrick