diff mbox series

[3/5] setup: merge configuration of repository formats

Message ID 16f52b75d8972343776adb269da305e7406ff385.1723708417.git.ps@pks.im (mailing list archive)
State Superseded
Headers show
Series Introduce configs for default repo format | expand

Commit Message

Patrick Steinhardt Aug. 15, 2024, 8 a.m. UTC
The configuration of repository formats is split up across two functions
`validate_hash_algorithm()` and `validate_ref_storage_format()`. This is
fine as-is, but we are about to extend the logic to also read default
values from the config. With the logic split across two functions, we
would either have to pass in additional parameters read from the config,
or read the config multiple times. Both of these options feel a bit
unwieldy.

Merge the code into a new a new function `repository_format_configure()`
that is responsible for configuring the whole repository's format. Like
this, we can easily read the config in a single place, only.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 setup.c | 47 ++++++++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 27 deletions(-)

Comments

Justin Tobler Aug. 15, 2024, 9:37 p.m. UTC | #1
On 24/08/15 10:00AM, Patrick Steinhardt wrote:
> The configuration of repository formats is split up across two functions
> `validate_hash_algorithm()` and `validate_ref_storage_format()`. This is
> fine as-is, but we are about to extend the logic to also read default
> values from the config. With the logic split across two functions, we
> would either have to pass in additional parameters read from the config,
> or read the config multiple times. Both of these options feel a bit
> unwieldy.

Combining the repository format configuration logic into a single
function seems like a good option. It looks like go we even further
though since we also include setting the hash and ref format for
`the_repository`. Might be nice to also mention this.

> Merge the code into a new a new function `repository_format_configure()`

s/new a new/new/

> that is responsible for configuring the whole repository's format. Like
> this, we can easily read the config in a single place, only.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
[snip]
> -	/*
> -	 * Now that we have set up both the hash algorithm and the ref storage
> -	 * format we can update the repository's settings accordingly.
> -	 */

The above comment somewhat made it sound like the repository format had
to be configured for both the hash algo and ref storage before updating
`the_repository`, but that does not seem to be a requirement in
actuality.

> -	repo_set_hash_algo(the_repository, repo_fmt.hash_algo);
> -	repo_set_ref_storage_format(the_repository, repo_fmt.ref_storage_format);
> +	repository_format_configure(&repo_fmt, hash, ref_storage_format);

Overall, I like that this repostory format configuration is under
`repository_format_configure()`. The `validate_*()` functions names
confused me slightly initially because I assumed they were only
validating, but they also configure the repo format. Looking good :)

-Justin
Patrick Steinhardt Aug. 16, 2024, 8:06 a.m. UTC | #2
On Thu, Aug 15, 2024 at 04:37:26PM -0500, Justin Tobler wrote:
> On 24/08/15 10:00AM, Patrick Steinhardt wrote:
> > -	repo_set_hash_algo(the_repository, repo_fmt.hash_algo);
> > -	repo_set_ref_storage_format(the_repository, repo_fmt.ref_storage_format);
> > +	repository_format_configure(&repo_fmt, hash, ref_storage_format);
> 
> Overall, I like that this repostory format configuration is under
> `repository_format_configure()`. The `validate_*()` functions names
> confused me slightly initially because I assumed they were only
> validating, but they also configure the repo format. Looking good :)

Yeah, agreed. We _did_ validate whether the new configuration makes
sense, but that wasn't really the main thing those functions did.

Patrick
diff mbox series

Patch

diff --git a/setup.c b/setup.c
index d458edcc02..5dfcdc99dd 100644
--- a/setup.c
+++ b/setup.c
@@ -2284,14 +2284,17 @@  static void separate_git_dir(const char *git_dir, const char *git_link)
 	write_file(git_link, "gitdir: %s", git_dir);
 }
 
-static void validate_hash_algorithm(struct repository_format *repo_fmt, int hash)
+static void repository_format_configure(struct repository_format *repo_fmt,
+					int hash, enum ref_storage_format ref_format)
 {
-	const char *env = getenv(GIT_DEFAULT_HASH_ENVIRONMENT);
+	const char *env;
+
 	/*
 	 * If we already have an initialized repo, don't allow the user to
 	 * specify a different algorithm, as that could cause corruption.
 	 * Otherwise, if the user has specified one on the command line, use it.
 	 */
+	env = getenv(GIT_DEFAULT_HASH_ENVIRONMENT);
 	if (repo_fmt->version >= 0 && hash != GIT_HASH_UNKNOWN && hash != repo_fmt->hash_algo)
 		die(_("attempt to reinitialize repository with different hash"));
 	else if (hash != GIT_HASH_UNKNOWN)
@@ -2302,25 +2305,22 @@  static void validate_hash_algorithm(struct repository_format *repo_fmt, int hash
 			die(_("unknown hash algorithm '%s'"), env);
 		repo_fmt->hash_algo = env_algo;
 	}
-}
-
-static void validate_ref_storage_format(struct repository_format *repo_fmt,
-					enum ref_storage_format format)
-{
-	const char *name = getenv("GIT_DEFAULT_REF_FORMAT");
+	repo_set_hash_algo(the_repository, repo_fmt->hash_algo);
 
+	env = getenv("GIT_DEFAULT_REF_FORMAT");
 	if (repo_fmt->version >= 0 &&
-	    format != REF_STORAGE_FORMAT_UNKNOWN &&
-	    format != repo_fmt->ref_storage_format) {
+	    ref_format != REF_STORAGE_FORMAT_UNKNOWN &&
+	    ref_format != repo_fmt->ref_storage_format) {
 		die(_("attempt to reinitialize repository with different reference storage format"));
-	} else if (format != REF_STORAGE_FORMAT_UNKNOWN) {
-		repo_fmt->ref_storage_format = format;
-	} else if (name) {
-		format = ref_storage_format_by_name(name);
-		if (format == REF_STORAGE_FORMAT_UNKNOWN)
-			die(_("unknown ref storage format '%s'"), name);
-		repo_fmt->ref_storage_format = format;
+	} else if (ref_format != REF_STORAGE_FORMAT_UNKNOWN) {
+		repo_fmt->ref_storage_format = ref_format;
+	} else if (env) {
+		ref_format = ref_storage_format_by_name(env);
+		if (ref_format == REF_STORAGE_FORMAT_UNKNOWN)
+			die(_("unknown ref storage format '%s'"), env);
+		repo_fmt->ref_storage_format = ref_format;
 	}
+	repo_set_ref_storage_format(the_repository, repo_fmt->ref_storage_format);
 }
 
 int init_db(const char *git_dir, const char *real_git_dir,
@@ -2353,22 +2353,15 @@  int init_db(const char *git_dir, const char *real_git_dir,
 	}
 	startup_info->have_repository = 1;
 
-	/* Check to see if the repository version is right.
+	/*
+	 * Check to see if the repository version is right.
 	 * Note that a newly created repository does not have
 	 * config file, so this will not fail.  What we are catching
 	 * is an attempt to reinitialize new repository with an old tool.
 	 */
 	check_repository_format(&repo_fmt);
 
-	validate_hash_algorithm(&repo_fmt, hash);
-	validate_ref_storage_format(&repo_fmt, ref_storage_format);
-
-	/*
-	 * Now that we have set up both the hash algorithm and the ref storage
-	 * format we can update the repository's settings accordingly.
-	 */
-	repo_set_hash_algo(the_repository, repo_fmt.hash_algo);
-	repo_set_ref_storage_format(the_repository, repo_fmt.ref_storage_format);
+	repository_format_configure(&repo_fmt, hash, ref_storage_format);
 
 	/*
 	 * Ensure `core.hidedotfiles` is processed. This must happen after we