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@xxxxxx> > --- [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
