From: Karthik Nayak <karthik.188@gmail.com>
Date: Thu, 05 Mar 2026 10:45:07 GMT
Subject: Re: [PATCH 03/17] odb: embed base source in the "files" backend
Message-ID: <CAOLa=ZSY8WE_BiWF0TZpV1-bf6p3z8zV4F_o4xo-V1ZC5ZiQLA@mail.gmail.com>
In-Reply-To: <20260223-b4-pks-odb-source-pluggable-v1-3-253bac1db598@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> The "files" backend is implemented as a pointer in the `struct
> odb_source`. This contradicts our typical pattern for pluggable backends
> like we use it for example in the ref store or for object database
> streams, where we typically embed the generic base structure in the
> specialized implementation. This pattern has a couple of small benefits:
>
>   - We avoid an extra allocation.
>

Because currently we allocate `obd_source` and also its `files` variable
independently. With the change, the `odb_source_files` will embed the
`obd_source` and be allocated together in one call. Makes sense.

>   - We hide implementation details in the generic structure.
>
>   - We can easily downcast from a generic backend to the specialized
>     structure and vice versa because the offsets are known at compile
>     time.
>
>   - It becomes trivial to identify locations where we depend on backend
>     specific logic because the cast needs to be explicit.
>

Indeed, also makes it easier to move generic logic out of individual
backends into the generic layer.

> Refactor our "files" object database source to do the same and embed the
> `struct odb_source` in the `struct odb_source_files`.
>
> There are still a bunch of sites in our code base where we do have to
> access internals of the "files" backend. The intent is that those will
> go away over time, but this will certainly take a while. Meanwhile,
> provide a `odb_source_files_downcast()` function that can convert a
> generic source into a "files" source.
>
> As we only have a single source the downcast succeeds unconditionally
> for now. Eventually though the intent is to make the cast `BUG()` in
> case the caller requests to downcast a non-"files" backend to a "files"
> backend.
>

Do we also plan to add read/write permissions check within the downcast
logic? Similar to the refs DB? Doesn't have to be in this patch, just
curious if that is something we plan to include.

> diff --git a/odb/source.c b/odb/source.c
> index 9d7fd19f45..d8b2176a94 100644
> --- a/odb/source.c
> +++ b/odb/source.c
> @@ -1,5 +1,6 @@
>  #include "git-compat-util.h"
>  #include "object-file.h"
> +#include "odb/source-files.h"
>  #include "odb/source.h"
>  #include "packfile.h"
>
> @@ -7,20 +8,31 @@ struct odb_source *odb_source_new(struct object_database *odb,
>  				  const char *path,
>  				  bool local)
>  {
> -	struct odb_source *source;
> +	return &odb_source_files_new(odb, path, local)->base;
> +}
>

Since we only have one source right now (files), we directly call the
internals of that source, I guess once we add more this would be more
modular.

> -	CALLOC_ARRAY(source, 1);
> +void odb_source_init(struct odb_source *source,
> +		     struct object_database *odb,
> +		     const char *path,
> +		     bool local)
> +{
>  	source->odb = odb;
>  	source->local = local;
>  	source->path = xstrdup(path);
> -	source->files = odb_source_files_new(source);
> -
> -	return source;
>  }
>
>  void odb_source_free(struct odb_source *source)
>  {
> +	struct odb_source_files *files;
> +	if (!source)
> +		return;
> +	files = odb_source_files_downcast(source);
> +	odb_source_files_free(files);
> +}
> +
> +void odb_source_release(struct odb_source *source)
> +{
> +	if (!source)
> +		return;
>  	free(source->path);
> -	odb_source_files_free(source->files);
> -	free(source);
>  }

The patch looks good.