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

From: Justin Tobler <jltobler@gmail.com>
Date: Aug 15, 2024, 21:37 UTC
Message-ID: <hxnibmbakpvh72kfrycxnvo5a24f6atcqa73yys4zthbqpwikq@s7435n3nqmlz>
In-Reply-To: <16f52b75d8972343776adb269da305e7406ff385.1723708417.git.ps@pks.im>

On 24/08/15 10:00AM, Patrick Steinhardt wrote:

Show 7 quoted lines

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

Show 5 quoted lines

> 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