From: Justin Tobler Date: Thu, 15 Aug 2024 21:37:26 GMT Subject: Re: [PATCH 3/5] setup: merge configuration of repository formats Message-ID: In-Reply-To: <16f52b75d8972343776adb269da305e7406ff385.1723708417.git.ps@pks.im> 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 > --- [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