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: Mon, 12 Jun 2023 12:45:26 -0700 [thread overview]
Message-ID: <dd21767c-7c66-cf42-1a64-954a069dc466@github.com> (raw)
In-Reply-To: <kl6lttvcft59.fsf@chooglen-macbookpro.roam.corp.google.com>
Glen Choo wrote:
> Victoria Dye <vdye@github.com> writes:
>> 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.
>
> I don't agree that this refactor is out of scope. I think we agree that
> the refactor is desirable, but if we apply the same heuristics in the
> future, the next author to copy a member from 'repository_format' to
> 'repository' could do the same and we'd never end up with the refactor
> we wanted. I strongly feel that if we don't put in a concerted effort
> into such refactors along the way, we end up creating more of the churn
> that made our lives harder in the first place.
I don't actually agree that *this* refactor (that is, moving all the
"repository_format -> repository" assignments into a helper function) is
desirable, though, even if we extrapolate to future updates.
Even if we updated the only other 'repository_format' value
('repository_format_precious_objects') to be copied the same way, the
benefit we'd get from eliminating a couple of lines of code duplication
wouldn't necessarily outweigh the the extra complexity of a new abstraction
- which may or may not need special-casing based on who's calling it -
and/or the risk associated with changing behavior if we want to eliminate
those special cases. IOW, I don't feel it's a definitive net improvement in
this situation.
What I do feel is desirable (although, to be honest, not particularly
strongly) is a larger refactor of 'setup.c' - including a deeper analysis
into the all the different setup functions we have, how they're used, why
they differ, etc. - to create a more unified setup process across all of
Git. That might include a helper function like what you've described, but it
might not need one at all, hence my comment about "added churn" (it's just
another thing a larger refactor would need to work around).
*That* refactor is what I was referring to as "out-of-scope" for this series
(sorry I wasn't clearer earlier!), which - considering the goal is "make
submodules work with worktree config" - doesn't seem unreasonable to me.
> E.g. we could support both deep and shallow copying, like:
>
> /*
> * Copy members from a repository_format to repository.
> *
> * If 'src' will no longer be read after copying (e.g. it will be
> * cleared soon), pass a nonzero value so that pointer members will be
> * moved to 'dest' (NULL-ed and shallow copied) instead of being deep
> * copied.
> */
> void copy_repository_format(struct repository *dest,
> struct repository_format *src,
> int take_ownership);
Unless we find that we *need* to support both, this approach would be more
harmful than helpful. If it doesn't matter whether the copy is shallow or
deep, this design proliferates that meaningless distinction in a way that
can easily confuse developers (or at least create more work for them as try
to try to understand it) if they ever want to change or use the function.
next prev parent reply other threads:[~2023-06-12 19:45 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
2023-06-12 18:10 ` Glen Choo
2023-06-12 19:45 ` Victoria Dye [this message]
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=dd21767c-7c66-cf42-1a64-954a069dc466@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).