From: Glen Choo <chooglen@google.com>
To: Victoria Dye via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org, Jonathan Tan <jonathantanmy@google.com>
Cc: derrickstolee@github.com, gitster@pobox.com,
Victoria Dye <vdye@github.com>
Subject: Re: [PATCH v2 3/3] repository: move 'repository_format_worktree_config' to repo scope
Date: Wed, 31 May 2023 15:17:01 -0700 [thread overview]
Message-ID: <kl6lr0qwno2q.fsf@chooglen-macbookpro.roam.corp.google.com> (raw)
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:
> @@ -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".
next prev parent reply other threads:[~2023-05-31 22:17 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-23 23:17 [PATCH 0/2] Fix behavior of worktree config in submodules Victoria Dye via GitGitGadget
2023-05-23 23:17 ` [PATCH 1/2] config: use gitdir to get worktree config Victoria Dye via GitGitGadget
2023-05-25 1:05 ` Glen Choo
2023-05-25 20:05 ` Derrick Stolee
2023-05-23 23:17 ` [PATCH 2/2] repository: move 'repository_format_worktree_config' to repo scope Victoria Dye via GitGitGadget
2023-05-25 1:29 ` Glen Choo
2023-05-25 16:09 ` Glen Choo
2023-05-25 20:02 ` Victoria Dye
2023-05-25 20:13 ` Derrick Stolee
2023-05-24 10:25 ` [PATCH 0/2] Fix behavior of worktree config in submodules Junio C Hamano
2023-05-25 19:56 ` Glen Choo
2023-05-26 1:32 ` [PATCH v2 0/3] " Victoria Dye via GitGitGadget
2023-05-26 1:32 ` [PATCH v2 1/3] config: use gitdir to get worktree config Victoria Dye via GitGitGadget
2023-05-26 1:32 ` [PATCH v2 2/3] config: pass 'repo' directly to 'config_with_options()' Victoria Dye via GitGitGadget
2023-05-26 1:33 ` [PATCH v2 3/3] repository: move 'repository_format_worktree_config' to repo scope Victoria Dye via GitGitGadget
2023-05-31 22:17 ` Glen Choo [this message]
2023-06-01 4:43 ` Junio C Hamano
2023-06-12 21:37 ` Glen Choo
2023-06-07 22:29 ` Victoria Dye
2023-06-12 18:10 ` Glen Choo
2023-06-12 19:45 ` Victoria Dye
2023-06-12 20:23 ` Glen Choo
2023-06-12 23:04 ` [PATCH] setup: copy repository_format using helper Glen Choo
2023-06-13 0:03 ` Victoria Dye
2023-06-13 18:25 ` Glen Choo
2023-06-13 19:45 ` Junio C Hamano
2023-05-26 15:48 ` [PATCH v2 0/3] Fix behavior of worktree config in submodules Derrick Stolee
2023-06-13 22:09 ` Glen Choo
2023-06-13 22:17 ` Victoria Dye
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=kl6lr0qwno2q.fsf@chooglen-macbookpro.roam.corp.google.com \
--to=chooglen@google.com \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=jonathantanmy@google.com \
--cc=vdye@github.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).