From: Elijah Newren <newren@gmail.com>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, vdye@github.com, me@ttaylorr.com,
gitster@pobox.com, Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH 1/3] repository: create disable_replace_refs()
Date: Tue, 30 May 2023 21:41:33 -0700 [thread overview]
Message-ID: <CABPp-BFzA0yVecHK1DEGMpAhewm7oyqEim7BCw7-DTKpUzWnpw@mail.gmail.com> (raw)
In-Reply-To: <56544abc15d1fce6fb4a0946e709470af9225395.1685126618.git.gitgitgadget@gmail.com>
On Fri, May 26, 2023 at 11:43 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <derrickstolee@github.com>
>
> Several builtins depend on being able to disable the replace references
> so we actually operate on each object individually. These currently do
> so by directly mutating the 'read_replace_refs' global.
>
> A future change will move this global into a different place, so it will
> be necessary to change all of these lines. However, we can simplify that
> transition by abstracting the purpose of these global assignments with a
> method call.
>
> We will never scope this to an in-memory repository as we want to make
> sure that we never use replace refs throughout the life of the process
> if this method is called.
I'm confused; doesn't the 3rd patch do exactly what this paragraph
says you'll never do?
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
> builtin/cat-file.c | 2 +-
> builtin/commit-graph.c | 5 ++---
> builtin/fsck.c | 2 +-
> builtin/index-pack.c | 2 +-
> builtin/pack-objects.c | 3 +--
> builtin/prune.c | 3 ++-
> builtin/replace.c | 3 ++-
> builtin/unpack-objects.c | 3 +--
> builtin/upload-pack.c | 3 ++-
> environment.c | 2 +-
> git.c | 2 +-
> replace-object.c | 5 +++++
> replace-object.h | 8 ++++++++
> repo-settings.c | 1 +
> 14 files changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 0bafc14e6c0..27f070267a4 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -805,7 +805,7 @@ static int batch_objects(struct batch_options *opt)
> if (repo_has_promisor_remote(the_repository))
> warning("This repository uses promisor remotes. Some objects may not be loaded.");
>
> - read_replace_refs = 0;
> + disable_replace_refs();
>
> cb.opt = opt;
> cb.expand = &data;
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index a3d00fa232b..639c9ca8b91 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -122,7 +122,6 @@ static int graph_verify(int argc, const char **argv, const char *prefix)
> return ret;
> }
>
> -extern int read_replace_refs;
> static struct commit_graph_opts write_opts;
>
> static int write_option_parse_split(const struct option *opt, const char *arg,
> @@ -323,13 +322,13 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)
> struct option *options = parse_options_concat(builtin_commit_graph_options, common_opts);
>
> git_config(git_default_config, NULL);
> -
> - read_replace_refs = 0;
> save_commit_buffer = 0;
>
> argc = parse_options(argc, argv, prefix, options,
> builtin_commit_graph_usage, 0);
> FREE_AND_NULL(options);
>
> + disable_replace_refs();
> +
In this place and several others in the file, you opt to not just
replace the assignment with a function call, but move the action line
to later in the file. In some cases, much later.
I don't think it hurts things, but it certainly makes me wonder why it
was moved. Did it break for some reason when called earlier? (Is
there something trickier about this patch than I expected?)
> return fn(argc, argv, prefix);
> }
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 2cd461b84c1..8a2d7afc83a 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -927,7 +927,6 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
> fetch_if_missing = 0;
>
> errors_found = 0;
> - read_replace_refs = 0;
> save_commit_buffer = 0;
>
> argc = parse_options(argc, argv, prefix, fsck_opts, fsck_usage, 0);
> @@ -953,6 +952,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
>
> git_config(git_fsck_config, &fsck_obj_options);
> prepare_repo_settings(the_repository);
> + disable_replace_refs();
>
> if (connectivity_only) {
> for_each_loose_object(mark_loose_for_connectivity, NULL, 0);
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index bb67e166559..d0d8067510b 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1752,7 +1752,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
> if (argc == 2 && !strcmp(argv[1], "-h"))
> usage(index_pack_usage);
>
> - read_replace_refs = 0;
> + disable_replace_refs();
> fsck_options.walk = mark_link;
>
> reset_pack_idx_option(&opts);
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index a5b466839ba..55635bdf4b4 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -4284,9 +4284,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
> if (DFS_NUM_STATES > (1 << OE_DFS_STATE_BITS))
> BUG("too many dfs states, increase OE_DFS_STATE_BITS");
>
> - read_replace_refs = 0;
> -
> sparse = git_env_bool("GIT_TEST_PACK_SPARSE", -1);
> + disable_replace_refs();
> if (the_repository->gitdir) {
> prepare_repo_settings(the_repository);
> if (sparse < 0)
> diff --git a/builtin/prune.c b/builtin/prune.c
> index 5dc9b207200..a8f3848c3a3 100644
> --- a/builtin/prune.c
> +++ b/builtin/prune.c
> @@ -164,7 +164,6 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
>
> expire = TIME_MAX;
> save_commit_buffer = 0;
> - read_replace_refs = 0;
> repo_init_revisions(the_repository, &revs, prefix);
>
> argc = parse_options(argc, argv, prefix, options, prune_usage, 0);
> @@ -172,6 +171,8 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
> if (repository_format_precious_objects)
> die(_("cannot prune in a precious-objects repo"));
>
> + disable_replace_refs();
> +
> while (argc--) {
> struct object_id oid;
> const char *name = *argv++;
> diff --git a/builtin/replace.c b/builtin/replace.c
> index 981f1894436..6c6f0b3ed01 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -566,11 +566,12 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
> OPT_END()
> };
>
> - read_replace_refs = 0;
> git_config(git_default_config, NULL);
>
> argc = parse_options(argc, argv, prefix, options, git_replace_usage, 0);
>
> + disable_replace_refs();
> +
> if (!cmdmode)
> cmdmode = argc ? MODE_REPLACE : MODE_LIST;
>
> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
> index 2c52c3a741f..3f5f6719405 100644
> --- a/builtin/unpack-objects.c
> +++ b/builtin/unpack-objects.c
> @@ -609,9 +609,8 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)
> int i;
> struct object_id oid;
>
> - read_replace_refs = 0;
> -
> git_config(git_default_config, NULL);
> + disable_replace_refs();
>
> quiet = !isatty(2);
>
> diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
> index beb9dd08610..6fc9a8feab0 100644
> --- a/builtin/upload-pack.c
> +++ b/builtin/upload-pack.c
> @@ -36,7 +36,6 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
> };
>
> packet_trace_identity("upload-pack");
> - read_replace_refs = 0;
>
> argc = parse_options(argc, argv, prefix, options, upload_pack_usage, 0);
>
> @@ -50,6 +49,8 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
> if (!enter_repo(dir, strict))
> die("'%s' does not appear to be a git repository", dir);
>
> + disable_replace_refs();
> +
> switch (determine_protocol_version_server()) {
> case protocol_v2:
> if (advertise_refs)
> diff --git a/environment.c b/environment.c
> index 8a96997539a..3b4d87c322f 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -185,7 +185,7 @@ void setup_git_env(const char *git_dir)
> strvec_clear(&to_free);
>
> if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))
> - read_replace_refs = 0;
> + disable_replace_refs();
> replace_ref_base = getenv(GIT_REPLACE_REF_BASE_ENVIRONMENT);
> git_replace_ref_base = xstrdup(replace_ref_base ? replace_ref_base
> : "refs/replace/");
> diff --git a/git.c b/git.c
> index 45899be8265..3252d4c7661 100644
> --- a/git.c
> +++ b/git.c
> @@ -185,7 +185,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
> if (envchanged)
> *envchanged = 1;
> } else if (!strcmp(cmd, "--no-replace-objects")) {
> - read_replace_refs = 0;
> + disable_replace_refs();
> setenv(NO_REPLACE_OBJECTS_ENVIRONMENT, "1", 1);
> if (envchanged)
> *envchanged = 1;
> diff --git a/replace-object.c b/replace-object.c
> index e98825d5852..ceec81c940c 100644
> --- a/replace-object.c
> +++ b/replace-object.c
> @@ -84,3 +84,8 @@ const struct object_id *do_lookup_replace_object(struct repository *r,
> }
> die(_("replace depth too high for object %s"), oid_to_hex(oid));
> }
> +
> +void disable_replace_refs(void)
> +{
> + read_replace_refs = 0;
> +}
> diff --git a/replace-object.h b/replace-object.h
> index 500482b02b3..7786d4152b0 100644
> --- a/replace-object.h
> +++ b/replace-object.h
> @@ -48,4 +48,12 @@ static inline const struct object_id *lookup_replace_object(struct repository *r
> return do_lookup_replace_object(r, oid);
> }
>
> +/*
> + * Some commands override config and environment settings for using
> + * replace references. Use this method to disable the setting and ensure
> + * those other settings will not override this choice. This applies
> + * globally to all in-process repositories.
> + */
> +void disable_replace_refs(void);
> +
> #endif /* REPLACE_OBJECT_H */
> diff --git a/repo-settings.c b/repo-settings.c
> index 7b566d729d0..1df0320bf33 100644
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -3,6 +3,7 @@
> #include "repository.h"
> #include "midx.h"
> #include "compat/fsmonitor/fsm-listen.h"
> +#include "environment.h"
Why? There are no other changes in this file, so I don't see why
you'd need another include.
>
> static void repo_cfg_bool(struct repository *r, const char *key, int *dest,
> int def)
> --
> gitgitgadget
I think the patch is probably fine, but I saw a few things that made
me wonder if I had missed something important, highlighted above.
next prev parent reply other threads:[~2023-05-31 4:41 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 [this message]
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
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=CABPp-BFzA0yVecHK1DEGMpAhewm7oyqEim7BCw7-DTKpUzWnpw@mail.gmail.com \
--to=newren@gmail.com \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=me@ttaylorr.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).