From: Victoria Dye <vdye@github.com>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org
Cc: me@ttaylorr.com, newren@gmail.com, gitster@pobox.com,
Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH 3/3] repository: create read_replace_refs setting
Date: Thu, 1 Jun 2023 09:36:12 -0700 [thread overview]
Message-ID: <68fd5ca2-a6c2-d742-5d74-ba93c1497169@github.com> (raw)
In-Reply-To: <481a81a515efb29bc4eb0b1a09b7d1df3f3c074b.1685126618.git.gitgitgadget@gmail.com>
Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
>
> The 'read_replace_refs' global specifies whether or not we should
> respect the references of the form 'refs/replace/<oid>' to replace which
> object we look up when asking for '<oid>'. This global has caused issues
> when it is not initialized properly, such as in b6551feadfd (merge-tree:
> load default git config, 2023-05-10).
>
> To make this more robust, move its config-based initialization out of
> git_default_config and into prepare_repo_settings(). This provides a
> repository-scoped version of the 'read_replace_refs' global.
As you noted in [1], this could be clearer. I think the most confusing part
is referring to it as "a repository-scoped version of the...global" because
it implies that the global and repo-scoped setting do the same thing/take
the same precedence (when, in reality, if replace refs are disabled
globally, the config doesn't do anything). Maybe something like this would
make that clearer?
"This provides a repository-scoped configuration that's only used if replace
refs are not already disabled process-wide with the global
'read_replace_refs'."
[1] https://lore.kernel.org/git/ae89feda-0a76-29d7-14ce-662214414638@github.com/
>
> The global still has its purpose: it is disabled process-wide by the
> GIT_NO_REPLACE_OBJECTS environment variable or by a call to
> disable_replace_refs() in some specific Git commands.
>
> Since we already encapsulated the use of the constant inside
> replace_refs_enabled(), we can perform the initialization inside that
> method, if necessary. This solves the problem of forgetting to check the
> config, as we will check it before returning this value.
>
> There is an interesting behavior change possible here: we now have a
> repository-scoped understanding of this config value. Thus, if there was
> a command that recurses into submodules and might follow replace refs,
> then it would now respect the core.useReplaceRefs config value in each
> repository.
>
> Unfortunately, the existing processes that recurse into submodules do
> not appear to follow object IDs to their contents, so this behavior
> change is not visible in the current implementation. It is something
> valuable for future behavior changes.
AFAIK, the only '--recurse-submodules' commands that recurse in-process are
'ls-files' and 'grep'. However, 'grep' does call 'parse_object_or_die()',
which (further down in the call stack) calls 'lookup_replace_object()'.
Maybe I'm misreading and the replaced object isn't actually used, but could
'git grep --recurse-submodules' be used to test this?
> @@ -94,5 +94,14 @@ void disable_replace_refs(void)
>
> int replace_refs_enabled(struct repository *r)
> {
> - return read_replace_refs;
> + if (!read_replace_refs)
> + return 0;
> +
> + if (r->gitdir) {
> + prepare_repo_settings(r);
> + return r->settings.read_replace_refs;
> + }
> +
> + /* repository has no objects or refs. */
> + return 0;
> }
This implementation matches the intent outlined in this patch/the cover
letter:
- if replace refs are disabled process-wide, always return 0
- if the gitdir is present, return the value of 'core.usereplacerefs'
- if there's no gitdir, there's no repository set up (and therefore no
config to read/objects to replace), so return 0
I was a bit unsure about whether 'r->gitdir' was the right check to make,
but it's consistent with other gates to 'prepare_repo_settings()' (e.g.
those added in 059fda19021 (checkout/fetch/pull/pack-objects: allow `-h`
outside a repository, 2022-02-08)), so I'm happy with it.
> diff --git a/repo-settings.c b/repo-settings.c
> index 1df0320bf33..5a7c990300d 100644
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -68,6 +68,7 @@ void prepare_repo_settings(struct repository *r)
> repo_cfg_bool(r, "pack.usebitmapboundarytraversal",
> &r->settings.pack_use_bitmap_boundary_traversal,
> r->settings.pack_use_bitmap_boundary_traversal);
> + repo_cfg_bool(r, "core.usereplacerefs", &r->settings.read_replace_refs, 1);
This defaults to enabling replace refs, consistent with the (intended)
behavior prior to this series. Good!
>
> /*
> * The GIT_TEST_MULTI_PACK_INDEX variable is special in that
> diff --git a/repository.h b/repository.h
> index c42f7ab6bdc..13fefa540bc 100644
> --- a/repository.h
> +++ b/repository.h
> @@ -39,6 +39,14 @@ struct repo_settings {
> int pack_read_reverse_index;
> int pack_use_bitmap_boundary_traversal;
>
> + /*
> + * Do replace refs need to be checked this run? This variable is
> + * initialized to true unless --no-replace-object is used or
> + * $GIT_NO_REPLACE_OBJECTS is set, but is set to false by some
> + * commands that do not want replace references to be active.
> + */
> + int read_replace_refs;
I don't think this comment is accurate anymore, since the repo-scoped
'read_replace_refs' value is determined *only* by the 'core.usereplacerefs'
config. It's 'replace_refs_enabled()' that makes the overall determination
(taking into account 'GIT_NO_REPLACE_OBJECTS'/'--no-replace-object').
> +
> struct fsmonitor_settings *fsmonitor; /* lazily loaded */
>
> int index_version;
next prev parent reply other threads:[~2023-06-01 16:36 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-26 18:43 [PATCH 0/3] Create stronger guard rails on replace refs Derrick Stolee via GitGitGadget
2023-05-26 18:43 ` [PATCH 1/3] repository: create disable_replace_refs() Derrick Stolee via GitGitGadget
2023-05-31 4:41 ` Elijah Newren
2023-05-31 13:37 ` Derrick Stolee
2023-06-01 17:47 ` Jeff King
2023-06-03 0:28 ` Junio C Hamano
2023-06-04 6:32 ` Jeff King
2023-06-01 5:23 ` Junio C Hamano
2023-06-01 16:35 ` Victoria Dye
2023-05-26 18:43 ` [PATCH 2/3] replace-objects: create wrapper around setting Derrick Stolee via GitGitGadget
2023-06-01 16:35 ` Victoria Dye
2023-06-01 19:50 ` Derrick Stolee
2023-06-03 1:47 ` Elijah Newren
2023-06-05 15:44 ` Derrick Stolee
2023-05-26 18:43 ` [PATCH 3/3] repository: create read_replace_refs setting Derrick Stolee via GitGitGadget
2023-06-01 16:36 ` Victoria Dye [this message]
2023-06-01 19:52 ` Derrick Stolee
2023-05-31 5:11 ` [PATCH 0/3] Create stronger guard rails on replace refs Elijah Newren
2023-06-02 14:29 ` [PATCH v2 " Derrick Stolee via GitGitGadget
2023-06-02 14:29 ` [PATCH v2 1/3] repository: create disable_replace_refs() Derrick Stolee via GitGitGadget
2023-06-02 14:29 ` [PATCH v2 2/3] replace-objects: create wrapper around setting Derrick Stolee via GitGitGadget
2023-06-03 6:22 ` René Scharfe
2023-06-05 13:22 ` Derrick Stolee
2023-06-02 14:29 ` [PATCH v2 3/3] repository: create read_replace_refs setting Derrick Stolee via GitGitGadget
2023-06-05 19:32 ` Victoria Dye
2023-06-03 1:52 ` [PATCH v2 0/3] Create stronger guard rails on replace refs Elijah Newren
2023-06-06 13:24 ` [PATCH v3 " Derrick Stolee via GitGitGadget
2023-06-06 13:24 ` [PATCH v3 1/3] repository: create disable_replace_refs() Derrick Stolee via GitGitGadget
2023-06-06 13:24 ` [PATCH v3 2/3] replace-objects: create wrapper around setting Derrick Stolee via GitGitGadget
2023-06-06 13:24 ` [PATCH v3 3/3] repository: create read_replace_refs setting Derrick Stolee via GitGitGadget
2023-06-06 16:15 ` [PATCH v3 0/3] Create stronger guard rails on replace refs Victoria Dye
2023-06-12 20:45 ` Junio C Hamano
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=68fd5ca2-a6c2-d742-5d74-ba93c1497169@github.com \
--to=vdye@github.com \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=me@ttaylorr.com \
--cc=newren@gmail.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).