| Message ID | 56544abc15d1fce6fb4a0946e709470af9225395.1685126618.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Create stronger guard rails on replace refs | expand |
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.
On 5/31/2023 12:41 AM, Elijah Newren wrote: > On Fri, May 26, 2023 at 11:43 AM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> 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? You mentioned in another reply that you figured it out, but for the sake of anyone reading here: we _add_ a repo-scoped version for the config, but we need this globally-scoped one for process-wide disabling the feature. This could be said more clearly. >> @@ -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?) Generally, I decided to move it after option-parsing, so it wouldn't be called if we are hitting an option-parse error. However, these moves were only important for a draft version where I had not separated the global and local scopes, so calling the method would also load config. In this version of the patch, this is not needed at all, and I could do an in-line replacement. Thanks! >> 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. Thanks. This is a leftover from a previous version of the patch. >> >> 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. Thank you for pointing them out, as the things that brought you confusion are cruft from an earlier version but are no longer valuable in this version. Thanks, -Stolee
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > 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. > > Signed-off-by: Derrick Stolee <derrickstolee@github.com> > --- It will naturally be outside the scope of the series, but this change will allow us to add a sanity check to make sure that nobody has read objects that would have been affected by these replace refs before the disable call was made, which is another reason to welcome this change.
Derrick Stolee via GitGitGadget 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. > Although unfortunate (it would be nice to remove the global), this makes sense. Disabling replace refs needs to be process-wide, and manually propagating a repository setting to other repositories would be awkward and prone to error. All of my questions on this patch ("why were the 'disable_replace_refs()' calls added later in the function than the original 'read_replace_refs = 0'?" and "why was '#include "environment.h"' added in 'repo-settings.c'?") were asked [1] and answered [2] already. Beyond those two points, this patch looks good! [1] https://lore.kernel.org/git/CABPp-BFzA0yVecHK1DEGMpAhewm7oyqEim7BCw7-DTKpUzWnpw@mail.gmail.com/ [2] https://lore.kernel.org/git/ae89feda-0a76-29d7-14ce-662214414638@github.com/ > +/* > + * 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); > + Thanks for including the function documentation. It concisely explains the purpose of 'disable_replace_refs()' and helps clarify how replace refs are treated in Git. > #endif /* REPLACE_OBJECT_H */
On Wed, May 31, 2023 at 09:37:10AM -0400, Derrick Stolee wrote: > > 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?) > > Generally, I decided to move it after option-parsing, so it wouldn't > be called if we are hitting an option-parse error. Playing devil's advocate: would option parsing ever access an object? I think in most cases the answer is no, but I could imagine it happening for some special cases (e.g., update-index uses callbacks to act on options as we parse them, since order is important). So I think as a general principle it makes sense for commands to set this flag as early as possible. > However, these moves were only important for a draft version where > I had not separated the global and local scopes, so calling the method > would also load config. > > In this version of the patch, this is not needed at all, and I could > do an in-line replacement. Thanks! It sounds like you were going to switch the locations back anyway, but maybe the above gives an extra motivation. :) -Peff
Jeff King <peff@peff.net> writes: >> Generally, I decided to move it after option-parsing, so it wouldn't >> be called if we are hitting an option-parse error. > > Playing devil's advocate: would option parsing ever access an object? I > think in most cases the answer is no, but I could imagine it happening > for some special cases (e.g., update-index uses callbacks to act on > options as we parse them, since order is important). > > So I think as a general principle it makes sense for commands to set > this flag as early as possible. I agree with the "devil's advocate" above; indeed my suggestion to follow-on work that is enabled by introducing this function, i.e. we can ensure that the objects already instantiated when the call is made are not affected by the replace mechanism, was exactly for such a "we already accessed some objects via the replace mechanism and then try to close the door of the barn afterwards with this call" case. Indeed, I think "git branch --no-merged 0369cf" resolves the object name down to a commit object in parse_opt_merge_filter() before parse_options() call returns. Yes.
On Sat, Jun 03, 2023 at 09:28:13AM +0900, Junio C Hamano wrote: > I agree with the "devil's advocate" above; indeed my suggestion to > follow-on work that is enabled by introducing this function, i.e. we > can ensure that the objects already instantiated when the call is > made are not affected by the replace mechanism, was exactly for such > a "we already accessed some objects via the replace mechanism and > then try to close the door of the barn afterwards with this call" > case. > > Indeed, I think "git branch --no-merged 0369cf" resolves the object > name down to a commit object in parse_opt_merge_filter() before > parse_options() call returns. > > Yes. Ah, very good example. Yes, I'd think it would be appropriate to BUG() if disable_replace_refs() is called and anybody has looked up an object already. -Peff
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(); + 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" static void repo_cfg_bool(struct repository *r, const char *key, int *dest, int def)