The changes that replace repository_format_worktree_config with "struct
repository".worktree_config look trivially good. The "struct
repository_format" bits track ebaf3bcf1ae (repository: move global
r_f_p_c to repo struct, 2021-06-17) so it preserves the status quo, but
I have some questions about ebaf3bcf1ae. Cc-ing the author (Jonathan
Tan) for context.
Rearranging the hunks for clarity,
"Victoria Dye via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> @@ -1560,6 +1562,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
> }
> if (startup_info->have_repository) {
> repo_set_hash_algo(the_repository, repo_fmt.hash_algo);
> + the_repository->repository_format_worktree_config =
> + repo_fmt.worktree_config;
> /* take ownership of repo_fmt.partial_clone */
> the_repository->repository_format_partial_clone =
> repo_fmt.partial_clone;
[snip]
> @@ -1651,6 +1655,8 @@ void check_repository_format(struct repository_format *fmt)
> check_repository_format_gently(get_git_dir(), fmt, NULL);
> startup_info->have_repository = 1;
> repo_set_hash_algo(the_repository, fmt->hash_algo);
> + the_repository->repository_format_worktree_config =
> + fmt->worktree_config;
> the_repository->repository_format_partial_clone =
> xstrdup_or_null(fmt->partial_clone);
> clear_repository_format(&repo_fmt);
[snip]
> diff --git a/repository.c b/repository.c
> index c53e480e326..104960f8f59 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -182,6 +182,7 @@ int repo_init(struct repository *repo,
> goto error;
>
> repo_set_hash_algo(repo, format.hash_algo);
> + repo->repository_format_worktree_config = format.worktree_config;
>
> /* take ownership of format.partial_clone */
> repo->repository_format_partial_clone = format.partial_clone;
This patch adds another instance of copying fields from "struct
repository_format" to "struct repository", so I think that we should
start doing this with a helper function instead of copy-pasting the
logic.
As for what should be in the helper function, the above hunks suggest
that we should copy .hash_algo, .partial_clone, and .worktree_config.
However...
> @@ -1423,6 +1422,9 @@ int discover_git_directory(struct strbuf *commondir,
> return -1;
> }
>
> + the_repository->repository_format_worktree_config =
> + candidate.worktree_config;
> +
> /* take ownership of candidate.partial_clone */
> the_repository->repository_format_partial_clone =
> candidate.partial_clone;
This hunk does not copy .hash_algo. I initially wondered if it is safe
to just copy .hash_algo here too, but I now suspect that we shouldn't
have done the_repository setup in discover_git_directory() in the first
place. It isn't used by the setup.c machinery - its one caller in "git"
(it's used by "scalar") is read_early_config(), which is supposed to
work without a fully set up repository, and bears a comment saying that
"no global state is changed" by calling discover_git_directory() (which
stopped being true in ebaf3bcf1ae). It looks like
discover_git_directory() is just a lightweight entrypoint into the
setup.c machinery. 16ac8b8db6 (setup: introduce the
discover_git_directory() function, 2017-03-13)) says "Let's just provide
a convenient wrapper function with an easier signature that *just*
discovers the .git/ directory. We will use it in a subsequent patch to
fix the early config."
If I'm wrong and we _should_ be doing the_repository setup, then I'm
guessing it's safe to copy .hash_algo here too. So either way, I think
we should introduce a helper function to do the copying, especially
because we will probably need to repeat this process yet again for
"repository_format_precious_objects".
