Re: [PATCH 03/17] odb: embed base source in the "files" backend
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Mar 5, 2026, 10:45 UTC
- 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:
Show 8 quoted lines
> 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.
Show 9 quoted lines
> - 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.
Show 14 quoted lines
> 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.
Show 19 quoted lines
> 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.
Show 31 quoted lines
> - 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.