Re: [PATCH 5/5] odb: make creation of on-disk structures pluggable

On 26/07/24 05:48AM, Patrick Steinhardt wrote:
> When creating a new "files" object database source we have to create a
> couple of directories. These directories are of course specific to this
> particular backend, and a different backend may require a setup that is
> completely different.
> 
> Make the creation of on-disk structures pluggable to accommodate for
> this.

Ok.

> Note that there is one exception though: the "objects" directory must
> exist in a repository regardless of which backend is in use. If it
> doesn't exist then the repository is not treated as a Git repository at
> all. Consequently, we create this directory regardless of the backend.

Makes sense.

> Signed-off-by: Patrick Steinhardt <[email protected]>
> ---
>  odb/source-files.c | 19 +++++++++++++++++++
>  odb/source.h       | 23 +++++++++++++++++++++++
>  setup.c            | 35 ++++++++++++++++++++---------------
>  3 files changed, 62 insertions(+), 15 deletions(-)
> 
> diff --git a/odb/source-files.c b/odb/source-files.c
> index 4138758511..0db6e681fe 100644
> --- a/odb/source-files.c
> +++ b/odb/source-files.c
> @@ -9,6 +9,7 @@
>  #include "odb/source-files.h"
>  #include "odb/source-loose.h"
>  #include "packfile.h"
> +#include "path.h"
>  #include "strbuf.h"
>  #include "write-or-die.h"
>  
> @@ -41,6 +42,23 @@ static void odb_source_files_close(struct odb_source *source)
>  	odb_source_close(&files->packed->base);
>  }
>  
> +static int odb_source_files_create_on_disk(struct odb_source *source)
> +{
> +	struct strbuf path = STRBUF_INIT;
> +
> +	safe_create_dir(source->odb->repo, source->path, 1);
> +
> +	strbuf_addf(&path, "%s/pack", source->path);
> +	safe_create_dir(source->odb->repo, path.buf, 1);
> +
> +	strbuf_reset(&path);
> +	strbuf_addf(&path, "%s/info", source->path);
> +	safe_create_dir(source->odb->repo, path.buf, 1);
> +
> +	strbuf_release(&path);
> +	return 0;
> +}

This is the callback to create on-disk state specific to the "files"
source and matches the current set of created files.

> +
>  static void odb_source_files_prepare(struct odb_source *source,
>  				     enum odb_prepare_flags flags)
>  {
> @@ -271,6 +289,7 @@ struct odb_source_files *odb_source_files_new(struct object_database *odb,
>  
>  	files->base.free = odb_source_files_free;
>  	files->base.close = odb_source_files_close;
> +	files->base.create_on_disk = odb_source_files_create_on_disk;
>  	files->base.prepare = odb_source_files_prepare;
>  	files->base.read_object_info = odb_source_files_read_object_info;
>  	files->base.read_object_stream = odb_source_files_read_object_stream;
> diff --git a/odb/source.h b/odb/source.h
> index ab16d152f4..4abc418bdd 100644
> --- a/odb/source.h
> +++ b/odb/source.h
> @@ -89,6 +89,18 @@ struct odb_source {
>  	 */
>  	void (*close)(struct odb_source *source);
>  
> +	/*
> +	 * This callback is expected to create on-disk data structures that are
> +	 * required for this source to operate.
> +	 *
> +	 * The callback is expected to return 0 on success, a negative error
> +	 * code otherwise.
> +	 *
> +	 * This callback may be NULL in case the source does not need any
> +	 * on-disk setup.
> +	 */
> +	int (*create_on_disk)(struct odb_source *source);
> +
>  	/*
>  	 * This callback is expected to prepare the source so that it becomes
>  	 * ready for use. It optionally clears underlying caches of the object
> @@ -316,6 +328,17 @@ static inline void odb_source_close(struct odb_source *source)
>  	source->close(source);
>  }
>  
> +/*
> + * Create on-disk data structures that are required for this source to operate
> + * correctly. Returns 0 on success, a negative error code otherwise.
> + */
> +static inline int odb_source_create_on_disk(struct odb_source *source)
> +{
> +	if (!source->create_on_disk)
> +		return 0;
> +	return source->create_on_disk(source);
> +}
> +
>  /*
>   * Prepare the object database source and clear any caches. Depending on the
>   * backend used this may have the effect that concurrently-written objects
> diff --git a/setup.c b/setup.c
> index a7b1b9eaef..14ef119cb7 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -2666,29 +2666,34 @@ static int create_default_files(struct repository *repo,
>  static void create_object_database(struct repository *repo)
>  {
>  	char *object_directory, *alternate_object_directories;
> -	struct strbuf path = STRBUF_INIT;
> -	size_t baselen;
>  
>  	get_object_directories(&object_directory, &alternate_object_directories);
> -	repo->objects = odb_new(repo, object_directory,
> -				alternate_object_directories);
>  
> -	strbuf_addstr(&path, repo_get_object_directory(repo));
> -	baselen = path.len;
> -
> -	safe_create_dir(repo, path.buf, 1);
> +	/*
> +	 * Create the "objects" directory in the common directory. This is done
> +	 * so that the repository can be discovered regardless of the backend
> +	 * used.
> +	 *
> +	 * Note that we only do this in case the object directory wasn't
> +	 * overwritten via an environment variable. If it _is_ being overridden
> +	 * then we skip this step, as the repository won't be discoverable
> +	 * anyway without the environment variable.
> +	 */
> +	if (!object_directory) {
> +		struct strbuf objects_dir = STRBUF_INIT;
> +		repo_common_path_append(repo, &objects_dir, "objects");
> +		safe_create_dir(repo, objects_dir.buf, 1);
> +		strbuf_release(&objects_dir);
> +	}

Here we always create the objects directory regardless of the backend.
Looks good.

> -	strbuf_setlen(&path, baselen);
> -	strbuf_addstr(&path, "/pack");
> -	safe_create_dir(repo, path.buf, 1);
> +	repo->objects = odb_new(repo, object_directory,
> +				alternate_object_directories);
>  
> -	strbuf_setlen(&path, baselen);
> -	strbuf_addstr(&path, "/info");
> -	safe_create_dir(repo, path.buf, 1);
> +	if (odb_source_create_on_disk(repo->objects->sources) < 0)
> +		die("failed creating object database");

Here we invoke the pluggable callback to create source specific on-disk
state. Part of me does wonder if this would be better to include this
inside of `odb_new()` and enable it with a specific flag, but having it
as a explicit separate step is probably fine too.

-Justin