From: Junio C Hamano <gitster@pobox.com>
To: Glen Choo <chooglen@google.com>
Cc: Victoria Dye via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org, Jonathan Tan <jonathantanmy@google.com>,
derrickstolee@github.com, Victoria Dye <vdye@github.com>
Subject: Re: [PATCH v2 3/3] repository: move 'repository_format_worktree_config' to repo scope
Date: Thu, 01 Jun 2023 13:43:25 +0900 [thread overview]
Message-ID: <xmqqedmveqs2.fsf@gitster.g> (raw)
In-Reply-To: <kl6lr0qwno2q.fsf@chooglen-macbookpro.roam.corp.google.com> (Glen Choo's message of "Wed, 31 May 2023 15:17:01 -0700")
Glen Choo <chooglen@google.com> writes:
>> @@ -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.
That's quite a departure from the established practice, isn't it?
Due to recent and not so recent header shuffling (moving everything
out of cache.h, dropping "extern", etc.), "git blame" is a bit hard
to follow, but ever since 16ac8b8d (setup: introduce the
discover_git_directory() function, 2017-03-13) added the function,
we do execute the "setup" when we know we are in a repository.
It would probably be worth mentioning that the "global state" Dscho
refers to in that commit is primarily about the current directory of
the Git process. During the discovery, we used to go up one level
at a time and tried to see if the current directory is either the
top of the working tree (i.e. has ".git/" that is a git repository)
or the top of a GIT_DIR-looking directory. That was changed in
ce9b8aab (setup_git_directory_1(): avoid changing global state,
2017-03-13) in the same series and discusses what "global state" the
series addresses.
If a relatively recent and oddball caller calls the function when it
does not want any of the setup donw after finding out that we could
use the directory as a repository, a new early "pure discovery" part
should be split out of the function, and both the function itself
and the oddball caller should be taught to call that pure-discovery
helper, I think.
> 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".
I do not know (or care in the context of this thread) about the
"precious objects" bit, but .worktree-config is the third one on top
of .hash_algo and .partial_clone, and it generally is a good time to
refactor when you find yourself adding the third instance of
repetitive code. So I agree with you that it is time to introduce a
helper function to copy from a "struct repository_format" to the
repository instance.
next prev parent reply other threads:[~2023-06-01 4:43 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
2023-06-01 4:43 ` Junio C Hamano [this message]
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=xmqqedmveqs2.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=chooglen@google.com \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.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).