Re: [PATCH v3 4/6] setup: defer object database creation

Patrick Steinhardt <[email protected]> writes:

> In a subsequent commit we'll make the creation of the on-disk data
> structures of an object database pluggable. This will lead to an
> in-between state where we have already configured the repository's
> object database, but it's not usable yet until we eventually call
> `create_object_directory()`.
>
> Defer the object database creation so that we handle both steps in the
> same function.
>
> Signed-off-by: Patrick Steinhardt <[email protected]>
> ---
>  setup.c | 24 +++++++++++++++---------
>  setup.h |  9 +++++++++
>  2 files changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index 5dfab3e79e..d85171f3b6 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1765,8 +1765,6 @@ int apply_repository_format(struct repository *repo,
>  			    enum apply_repository_format_flags flags,
>  			    struct strbuf *err)
>  {
> -	enum odb_new_flags odb_new_flags = 0;
> -
>  	if (verify_repository_format(format, err) < 0)
>  		return -1;
>  
> @@ -1801,9 +1799,12 @@ int apply_repository_format(struct repository *repo,
>  	repo->repository_format_precious_objects =
>  		format->precious_objects;
>  
> -	if (flags & APPLY_REPOSITORY_FORMAT_HONOR_ENV)
> -		odb_new_flags |= ODB_NEW_HONOR_ENV;
> -	repo->objects = odb_new(repo, odb_new_flags);
> +	if (!(flags & APPLY_REPOSITORY_FORMAT_SKIP_ODB_CREATION)) {
> +		enum odb_new_flags odb_new_flags = 0;
> +		if (flags & APPLY_REPOSITORY_FORMAT_HONOR_ENV)
> +			odb_new_flags |= ODB_NEW_HONOR_ENV;
> +		repo->objects = odb_new(repo, odb_new_flags);
> +	}
>  
>  	return 0;
>  }
> @@ -2651,11 +2652,13 @@ static int create_default_files(struct repository *repo,
>  	return reinit;
>  }
>  
> -static void create_object_directory(struct repository *repo)
> +static void create_object_database(struct repository *repo)
>  {
>  	struct strbuf path = STRBUF_INIT;
>  	size_t baselen;
>  
> +	repo->objects = odb_new(repo, ODB_NEW_HONOR_ENV);
> +
>  	strbuf_addstr(&path, repo_get_object_directory(repo));
>  	baselen = path.len;
>  
> @@ -2864,9 +2867,10 @@ int init_db(struct repository *repo,
>  	 */
>  	read_and_verify_repository_format(&repo_fmt, repo_get_git_dir(repo), NULL);
>  	repository_format_configure(&repo_fmt, hash, ref_storage_format);
> -	if (apply_repository_format(repo, &repo_fmt, APPLY_REPOSITORY_FORMAT_HONOR_ENV, &err) < 0)
> +	if (apply_repository_format(repo, &repo_fmt,
> +				    APPLY_REPOSITORY_FORMAT_HONOR_ENV |
> +				    APPLY_REPOSITORY_FORMAT_SKIP_ODB_CREATION, &err) < 0)
>  		die("%s", err.buf);
> -	startup_info->have_repository = 1;
>  
>  	/*
>  	 * Ensure `core.hidedotfiles` is processed. This must happen after we
> @@ -2882,7 +2886,9 @@ int init_db(struct repository *repo,
>  
>  	if (!(flags & INIT_DB_SKIP_REFDB))
>  		create_reference_database(repo, initial_branch, flags & INIT_DB_QUIET);
> -	create_object_directory(repo);
> +	create_object_database(repo);
> +
> +	startup_info->have_repository = 1;
>  
>  	if (repo_settings_get_shared_repository(repo)) {
>  		char buf[10];
> diff --git a/setup.h b/setup.h
> index 654f10e059..e55d647b70 100644
> --- a/setup.h
> +++ b/setup.h
> @@ -241,6 +241,15 @@ enum apply_repository_format_flags {
>  	 * relate to the object database.
>  	 */
>  	APPLY_REPOSITORY_FORMAT_HONOR_ENV = (1 << 0),
> +
> +	/*
> +	 * Usually, the object database is created after the repository format
> +	 * was applied. This step is skipped if this flag is set, which leaves
> +	 * us with a partially-working repository.
> +	 *
> +	 * This is useful when initializing a new repository.
> +	 */
> +	APPLY_REPOSITORY_FORMAT_SKIP_ODB_CREATION = (1 << 1),
>  };
>  
>  /*
>
> -- 
> 2.55.0.679.g6767b8d81c.dirty
>

With [PATCH v3 3/6], Justin's objection[1] is stronger now:

> Naive question: would it be simpler to just require invoking `odb_new()`
> explicitly after `apply_repository_format()` in all cases? There doesn't
> appear to be too many callsites.

As a matter of fact, I've given this a try and see these changes on top
of this series below.

[1]: <amkXcmwzbBYsMgjc@denethor>

--- >8 ---

diff --git a/repository.c b/repository.c
index 6d633002b4..9eee74113c 100644
--- a/repository.c
+++ b/repository.c
@@ -295,6 +295,8 @@ int repo_init(struct repository *repo,
 		goto error;
 	}
 
+	repo->objects = odb_new(repo, 0);
+
 	if (worktree)
 		repo_set_worktree(repo, worktree);
 
diff --git a/setup.c b/setup.c
index af02cd965c..1106f38bb0 100644
--- a/setup.c
+++ b/setup.c
@@ -1799,13 +1799,6 @@ int apply_repository_format(struct repository *repo,
 	repo->repository_format_precious_objects =
 		format->precious_objects;
 
-	if (!(flags & APPLY_REPOSITORY_FORMAT_SKIP_ODB_CREATION)) {
-		enum odb_new_flags odb_new_flags = 0;
-		if (flags & APPLY_REPOSITORY_FORMAT_HONOR_ENV)
-			odb_new_flags |= ODB_NEW_HONOR_ENV;
-		repo->objects = odb_new(repo, odb_new_flags);
-	}
-
 	return 0;
 }
 
@@ -1889,6 +1882,7 @@ const char *enter_repo(struct repository *repo, const char *path, unsigned flags
 		read_and_verify_repository_format(&fmt, ".", NULL);
 		if (apply_repository_format(repo, &fmt, APPLY_REPOSITORY_FORMAT_HONOR_ENV, &err) < 0)
 			die("%s", err.buf);
+		repo->objects = odb_new(repo, ODB_NEW_HONOR_ENV);
 		startup_info->have_repository = 1;
 
 		clear_repository_format(&fmt);
@@ -2092,6 +2086,8 @@ const char *setup_git_directory_gently(struct repository *repo, int *nongit_ok)
 						    APPLY_REPOSITORY_FORMAT_HONOR_ENV, &err) < 0)
 				die("%s", err.buf);
 
+			repo->objects = odb_new(repo, ODB_NEW_HONOR_ENV);
+
 			clear_repository_format(&discovery.format);
 			strbuf_release(&err);
 		}
@@ -2870,8 +2866,7 @@ int init_db(struct repository *repo,
 	read_and_verify_repository_format(&repo_fmt, repo_get_git_dir(repo), NULL);
 	repository_format_configure(&repo_fmt, hash, ref_storage_format);
 	if (apply_repository_format(repo, &repo_fmt,
-				    APPLY_REPOSITORY_FORMAT_HONOR_ENV |
-				    APPLY_REPOSITORY_FORMAT_SKIP_ODB_CREATION, &err) < 0)
+				    APPLY_REPOSITORY_FORMAT_HONOR_ENV, &err) < 0)
 		die("%s", err.buf);
 
 	/*
diff --git a/setup.h b/setup.h
index e55d647b70..654f10e059 100644
--- a/setup.h
+++ b/setup.h
@@ -241,15 +241,6 @@ enum apply_repository_format_flags {
 	 * relate to the object database.
 	 */
 	APPLY_REPOSITORY_FORMAT_HONOR_ENV = (1 << 0),
-
-	/*
-	 * Usually, the object database is created after the repository format
-	 * was applied. This step is skipped if this flag is set, which leaves
-	 * us with a partially-working repository.
-	 *
-	 * This is useful when initializing a new repository.
-	 */
-	APPLY_REPOSITORY_FORMAT_SKIP_ODB_CREATION = (1 << 1),
 };
 
 /*