Re: [PATCH 06/17] odb/source: make `free()` function pluggable

[Justin Tobler <jltobler@gmail.com>]

On 26/02/23 05:17PM, Patrick Steinhardt wrote:
> Introduce a new callback function in `struct odb_source` to make the
> function pluggable.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  odb/source-files.c | 7 ++++---
>  odb/source-files.h | 3 ---
>  odb/source.c       | 4 +---
>  odb/source.h       | 6 ++++++
>  4 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/odb/source-files.c b/odb/source-files.c
> index 7496e1d9f8..65d7805c5a 100644
> --- a/odb/source-files.c
> +++ b/odb/source-files.c
> @@ -18,10 +18,9 @@ static void odb_source_files_reparent(const char *name UNUSED,
>  	files->base.path = path;
>  }
>  
> -void odb_source_files_free(struct odb_source_files *files)
> +static void odb_source_files_free(struct odb_source *source)
>  {
> -	if (!files)
> -		return;
> +	struct odb_source_files *files = odb_source_files_downcast(source);

Now each callback will be responsible for downcasting to the concrete
type. Looks good.

>  	chdir_notify_unregister(NULL, odb_source_files_reparent, files);
>  	odb_source_loose_free(files->loose);
>  	packfile_store_free(files->packed);
> @@ -40,6 +39,8 @@ struct odb_source_files *odb_source_files_new(struct object_database *odb,
>  	files->loose = odb_source_loose_new(&files->base);
>  	files->packed = packfile_store_new(&files->base);
>  
> +	files->base.free = odb_source_files_free;

Callback is registered.

> +
>  	/*
>  	 * Ideally, we would only ever store absolute paths in the source. This
>  	 * is not (yet) possible though because we access and assume relative
> diff --git a/odb/source-files.h b/odb/source-files.h
> index 803fa995fb..23a3b4e04b 100644
> --- a/odb/source-files.h
> +++ b/odb/source-files.h
> @@ -21,9 +21,6 @@ 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);

The forward header is no longer needed as this function becomes an
internal detail of how the "files" source is impelmented.

> -
>  /*
>   * Cast the given object database source to the files backend. This will cause
>   * a BUG in case the source doesn't use this backend.
> diff --git a/odb/source.c b/odb/source.c
> index c7dcc528f6..7993dcbd65 100644
> --- a/odb/source.c
> +++ b/odb/source.c
> @@ -25,11 +25,9 @@ void odb_source_init(struct odb_source *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);
> +	source->free(source);

Freeing the source can now be gone generically for different sources.
Nice. :)

>  }
>  
>  void odb_source_release(struct odb_source *source)
> diff --git a/odb/source.h b/odb/source.h
> index a1f2f8fdb1..f84da59ef0 100644
> --- a/odb/source.h
> +++ b/odb/source.h
> @@ -52,6 +52,12 @@ struct odb_source {
>  	 * the current working directory.
>  	 */
>  	char *path;
> +
> +	/*
> +	 * This callback is expected to free the underlying object database source and
> +	 * all associated resources. The function will never be called with a NULL pointer.
> +	 */
> +	void (*free)(struct odb_source *source);

Looks good.

-Justin