Re: [PATCH v2 3/3] repository: move 'repository_format_worktree_config' to repo scope
- From
Glen Choo <chooglen@google.com>
- Date
- May 31, 2023, 22:17 UTC
- Message-ID
- <kl6lr0qwno2q.fsf@chooglen-macbookpro.roam.corp.google.com>
- In-Reply-To
- <506a2cf8c73549bc8f9761b56532ef08ed220da4.1685064781.git.gitgitgadget@gmail.com>
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@gmail.com> writes:
Show 9 quoted lines
> @@ -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]
Show 9 quoted lines
> @@ -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]
Show 12 quoted lines
> 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...
Show 10 quoted lines
> @@ -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".