From: Victoria Dye <vdye@github.com>
To: Glen Choo <chooglen@google.com>,
Victoria Dye via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org, Jonathan Tan <jonathantanmy@google.com>
Cc: derrickstolee@github.com, gitster@pobox.com
Subject: Re: [PATCH v2 3/3] repository: move 'repository_format_worktree_config' to repo scope
Date: Wed, 7 Jun 2023 15:29:24 -0700 [thread overview]
Message-ID: <49509708-c0a1-2439-a551-cab05d944b66@github.com> (raw)
In-Reply-To: <kl6lr0qwno2q.fsf@chooglen-macbookpro.roam.corp.google.com>
Glen Choo wrote:
> 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...
>
...
>
> 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".
Thanks for pointing this out & sharing your findings!
I agree with the desire to reduce code duplication, but the reason I avoided
that refactor when putting these patches together is because of the subtle
differences across the different repository format assignment blocks.
For example, in addition to what you mentioned here w.r.t. '.hash_algo',
there are also differences in how 'repository_format_partial_clone' is
assigned: it's deep-copied in 'check_repository_format', but shallow-copied
(then subsequently NULL'd in the 'struct repository_format' to avoid freeing
the pointer when the struct is disposed of) in 'discover_git_directory()' &
'setup_git_directory_gently()'.
If we were to settle on a single "copy repository format settings" function,
it's not obvious what the "right" approach is. We could change
'check_repository_format()' to the shallow-copy-then-null like the others:
its two callers (in 'init-db.c' and 'path.c') don't use the value of
'repository_format_partial_clone' in 'struct repository_format' after
calling 'check_repository_format()'. But, if we did that, it'd introduce a
side effect to the input 'struct repository_format', which IMO would be
surprising behavior for a function called 'check_<something>()'. Conversely,
unifying on a deep copy or adding a flag to toggle deep vs. shallow copy
feels like unnecessary complexity if we don't actually need a deep copy.
Beyond the smaller subtleties, there's the larger question (that you sort of
get at with the questions around 'discover_git_directory()') as to whether
we should more heavily refactor or consolidate these setup functions. The
similar code implies "yes", but such a refactor feels firmly out-of-scope
for this series. A smaller change (e.g. just moving the assignments into
their own function) could be less of a diversion, but any benefit seems like
it'd be outweighed by the added churn/complexity of a new function.
In any case, sorry for the long-winded response. I'd initially tried to
implement your feedback, but every time I did I'd get stopped up on the
things I mentioned above. So, rather than continue to put off responding to
this thread, I tried to capture what kept stopping me from moving forward -
hopefully it makes (at least a little bit of) sense!
next prev parent reply other threads:[~2023-06-07 22:30 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
2023-06-12 21:37 ` Glen Choo
2023-06-07 22:29 ` Victoria Dye [this message]
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=49509708-c0a1-2439-a551-cab05d944b66@github.com \
--to=vdye@github.com \
--cc=chooglen@google.com \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=jonathantanmy@google.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).