Re: [PATCH 03/17] odb: embed base source in the "files" backend

[Justin Tobler <jltobler@gmail.com>]

On 26/02/23 05:17PM, Patrick Steinhardt wrote:
> 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.
> 
>   - 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.
> 
> Refactor our "files" object database source to do the same and embed the
> `struct odb_source` in the `struct odb_source_files`.

Makes sense.
 
> 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.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
[snip]
> diff --git a/odb/source-files.c b/odb/source-files.c
> index cbdaa6850f..a43a197157 100644
> --- a/odb/source-files.c
> +++ b/odb/source-files.c
> @@ -1,5 +1,6 @@
>  #include "git-compat-util.h"
>  #include "object-file.h"
> +#include "odb/source.h"
>  #include "odb/source-files.h"
>  #include "packfile.h"
>  
> @@ -9,15 +10,20 @@ void odb_source_files_free(struct odb_source_files *files)
>  		return;
>  	odb_source_loose_free(files->loose);
>  	packfile_store_free(files->packed);
> +	odb_source_release(&files->base);
>  	free(files);
>  }
>  
> -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)
>  {
>  	struct odb_source_files *files;
> +
>  	CALLOC_ARRAY(files, 1);
> -	files->source = source;
> -	files->loose = odb_source_loose_new(source);
> -	files->packed = packfile_store_new(source);
> +	odb_source_init(&files->base, odb, path, local);
> +	files->loose = odb_source_loose_new(&files->base);
> +	files->packed = packfile_store_new(&files->base);

When creating the files ODB source, it is now responsible for also
creating the embedded base ODB souce. Makes sense.

> +
>  	return files;
>  }
> 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
> @@ -1,8 +1,9 @@
>  #ifndef ODB_SOURCE_FILES_H
>  #define ODB_SOURCE_FILES_H
>  
> +#include "odb/source.h"
> +
>  struct odb_source_loose;
> -struct odb_source;
>  struct packfile_store;
>  
>  /*
> @@ -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.

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

> +static inline struct odb_source_files *odb_source_files_downcast(struct odb_source *source)
> +{
> +	return container_of(source, struct odb_source_files, base);
> +}
> +
>  #endif
[snip]
> diff --git a/odb/source.h b/odb/source.h
> index 1c34265189..e6698b73a3 100644
> --- a/odb/source.h
> +++ b/odb/source.h
> @@ -1,8 +1,6 @@
>  #ifndef ODB_SOURCE_H
>  #define ODB_SOURCE_H
>  
> -#include "odb/source-files.h"
> -
>  /*
>   * The source is the part of the object database that stores the actual
>   * objects. It thus encapsulates the logic to read and write the specific
> @@ -21,9 +19,6 @@ struct odb_source {
>  	/* Object database that owns this object source. */
>  	struct object_database *odb;
>  
> -	/* The backend used to store objects. */
> -	struct odb_source_files *files;

Now that the base ODB source is embedded in `struct odb_source_files`,
it is accessed via downcasting and the direct reference is no longer
needed. This is responsible for most of the structural change fallout in
this patch.

> -
>  	/*
>  	 * Figure out whether this is the local source of the owning
>  	 * repository, which would typically be its ".git/objects" directory.
> @@ -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()?

> +
>  #endif
> diff --git a/odb/streaming.c b/odb/streaming.c
> index 26b0a1a0f5..19cda9407d 100644
> --- a/odb/streaming.c
> +++ b/odb/streaming.c
> @@ -187,7 +187,8 @@ static int istream_source(struct odb_read_stream **out,
>  
>  	odb_prepare_alternates(odb);
>  	for (source = odb->sources; source; source = source->next) {
> -		if (!packfile_store_read_object_stream(out, source->files->packed, oid) ||
> +		struct odb_source_files *files = odb_source_files_downcast(source);
> +		if (!packfile_store_read_object_stream(out, files->packed, oid) ||
>  		    !odb_source_loose_read_object_stream(out, source, oid))
>  			return 0;
>  	}

Overall this patch looks good.

-Justin