Create stronger guard rails on replace refs
cover Create stronger guard rails on replace refs
(This series is based on tb/pack-bitmap-traversal-with-boundary due to wanting to modify prepare_repo_settings() in a similar way.)
The replace-refs can be ignored via the core.useReplaceRefs=false config setting. This setting is possible to miss in some Git commands if they do not load default config at the appropriate time. See [1] for a recent example of this.
[1]
https://lore.kernel.org/git/[email protected]/
This series aims to avoid this kind of error from happening in the future. The idea is to encapsulate the setting in such a way that we can guarantee that config has been checked before using the in-memory value.
Further, we must be careful that some Git commands want to disable replace refs unconditionally, as if GIT_NO_REPLACE_REFS was enabled in the environment.
The approach taken here is to split the global into two different sources. First, read_replace_refs is kept (but moved to replace-objects.c scope) and reflects whether or not the feature is permitted by the environment and the current command. Second, a new value is added to repo-settings and this is checked after using prepare_repo_settings() to guarantee the config has been read.
This presents a potential behavior change, in that now core.useReplaceRefs is specific to each in-memory repository instead of applying the superproject value to all submodules. I could not find a Git command that has multiple in-memory repositories and follows OIDs to object contents, so I'm not sure how to demonstrate it in a test.
Here is the breakdown of the series:
* Patch 1 creates disable_replace_refs() to encapsulate the global disabling of the feature.
* Patch 2 creates replace_refs_enabled() to check if the feature is enabled (with respect to a given repository). This is a thin wrapper of the global at this point, but does allow us to remove it from environment.h.
* Patch 3 creates the value in repo-settings as well as ensures that the repo settings have been prepared before accessing the value within replace_refs_enabled().
Thanks, -Stolee
Derrick Stolee (3):
repository: create disable_replace_refs()
replace-objects: create wrapper around setting
repository: create read_replace_refs setting
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 ++- commit-graph.c | 4 +--- config.c | 5 ----- environment.c | 3 +-- git.c | 2 +- log-tree.c | 2 +- replace-object.c | 23 ++++++++++++++++++++++- replace-object.h | 31 ++++++++++++++++++++++--------- repo-settings.c | 2 ++ repository.h | 8 ++++++++ 18 files changed, 71 insertions(+), 35 deletions(-)
base-commit: b0afdce5dab61f224fd66c13768facc36a7f8705
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1537%2Fderrickstolee%2Freplace-refs-safety-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1537/derrickstolee/replace-refs-safety-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1537
Signature · 1 line
gitgitgadget
PATCH 1/3 repository: create disable_replace_refs()
From: Derrick Stolee <[email protected]>
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.
--- 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(-)
builtin/cat-file.c +1 -1
@@ -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;
builtin/commit-graph.c +2 -3
@@ -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); }
builtin/fsck.c +1 -1
@@ -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 );
builtin/index-pack.c +1 -1
@@ -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);
builtin/pack-objects.c +1 -2
@@ -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 )
builtin/prune.c +2 -1
@@ -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++;
builtin/replace.c +2 -1
@@ -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 ;
builtin/unpack-objects.c +1 -2
@@ -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 );
builtin/upload-pack.c +2 -1
@@ -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)
environment.c +1 -1
@@ -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/" );
git.c +1 -1
@@ -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 ;
replace-object.c +5 -0
@@ -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 ; +}
replace-object.h +8 -0
@@ -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 */
repo-settings.c +1 -0
@@ -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)
Signature · 1 line
gitgitgadget
Re: [PATCH 1/3] repository: create disable_replace_refs()
Quoted 7 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
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.
Quoted 8 lines
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?
Quoted 47 lines
Signed-off-by Derrick Stolee <[email protected]>--- 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(-)builtin/cat-file.c +1
-1@@ -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;builtin/commit-graph.c +0
-1@@ -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 );
builtin/commit-graph.c int cmd_commit_graph(int argc, const char **argv, const char *prefix) · line 332
Quoted 10 lines
builtin/commit-graph.c +2
-2- - 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?)
Quoted 186 lines
builtin/commit-graph.c +0
-0return fn (argc, argv, prefix); }builtin/fsck.c +1
-1@@ -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 );builtin/index-pack.c +1
-1@@ -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);builtin/pack-objects.c +1
-2@@ -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 )builtin/prune.c +2
-1@@ -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++;builtin/replace.c +2
-1@@ -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 ;builtin/unpack-objects.c +1
-2@@ -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 );builtin/upload-pack.c +2
-1@@ -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)environment.c +1
-1@@ -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/" );git.c +1
-1@@ -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 ;replace-object.c +5
-0@@ -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 ; +}replace-object.h +8
-0@@ -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 */ repo-settings.c +0
-0
repo-settings.c line 6
Quoted 4 lines
repo-settings.c +1
-0@@ -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.
Quoted 5 lines
repo-settings.c +0
-1static 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.
Re: [PATCH 1/3] repository: create disable_replace_refs()
Quoted 9 lines · Elijah Newren
Quoted 3 lines · Derrick Stolee via GitGitGadget
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.
Quoted 19 lines
Quoted 4 lines
builtin/commit-graph.c +0
-0@@ -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 );builtin/commit-graph.c int cmd_commit_graph(int argc, const char **argv, const char *prefix) · line 332
Quoted 10 lines
builtin/commit-graph.c +2
-2- - 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.
Quoted 3 lines
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!
Quoted 4 lines
Quoted 4 lines
repo-settings.c +0
-0
Quoted 8 lines
repo-settings.c line 6
Quoted 5 lines
repo-settings.c +1
-0@@ -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.
Quoted 8 lines
Quoted 5 lines
repo-settings.c +0
-1static 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
Re: [PATCH 1/3] repository: create disable_replace_refs()
Quoted 10 lines · Derrick Stolee
Quoted 7 lines
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.
Quoted 6 lines
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
Re: [PATCH 1/3] repository: create disable_replace_refs()
Quoted 10 lines · Jeff King
Quoted 2 lines
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.
Re: [PATCH 1/3] repository: create disable_replace_refs()
Quoted 8 lines · Junio C Hamano
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.
Quoted 5 lines
Indeed, I think "git branch --no-merged 0369cf" resolves the object name down to a commit object in
parse_opt_merge_filter()beforeparse_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
Re: [PATCH 1/3] repository: create disable_replace_refs()
In reply to Derrick Stolee via GitGitGadgetQuoted 11 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
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.
Quoted 6 lines
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 <[email protected]>---
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.
Re: [PATCH 1/3] repository: create disable_replace_refs()
In reply to Derrick Stolee via GitGitGadgetQuoted 6 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
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.
Quoted 9 lines
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/[email protected]/
replace-object.h static inline const struct object_id *lookup_replace_object(struct repository *r · line 58
Quoted 8 lines
replace-object.h +8
-0+/* +* 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.
Quoted 1 line
#endif /*
REPLACE_OBJECT_H*/
PATCH 2/3 replace-objects: create wrapper around setting
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
The 'read_replace_objects' constant is initialized by git_default_config (if core.useReplaceRefs is disabled) and within setup_git_env (if GIT_NO_REPLACE_OBJECTS) is set. To ensure that this variable cannot be set accidentally in other places, wrap it in a replace_refs_enabled() method.
This allows us to remove the global from being recognized outside of replace-objects.c.
Further, a future change will help prevent the variable from being read before it is initialized. Centralizing its access is an important first step.
--- commit-graph.c | 4 +--- environment.c | 1 - log-tree.c | 2 +- replace-object.c | 7 +++++++ replace-object.h | 15 ++++++++++++++- 5 files changed, 23 insertions(+), 6 deletions(-)
commit-graph.c +1 -3
@@ -203,14 +203,12 @@ static struct commit_graph *alloc_commit_graph(void)return g; } -extern int read_replace_refs; -static int commit_graph_compatible (struct repository *r) {if (!r->gitdir)return 0 ; -if (read_replace_refs) { +if (replace_refs_enabled (r)) {prepare_replace_object (r);if (hashmap_get_size (&r->objects->replace_map->map))return 0 ;
environment.c +0 -1
@@ -63,7 +63,6 @@ const char *editor_program;const char *askpass_program;const char *excludes_file;enum auto_crlf auto_crlf =AUTO_CRLF_FALSE ; -int read_replace_refs =1 ;enum eol core_eol =EOL_UNSET ;int global_conv_flags_eol =CONV_EOL_RNDTRP_WARN ;char *check_roundtrip_encoding ="SHIFT-JIS" ;
log-tree.c +1 -1
@@ -155,7 +155,7 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,if (starts_with (refname, git_replace_ref_base)) {struct object_id original_oid; -if (!read_replace_refs) +if (!replace_refs_enabled (the_repository))return 0 ;if (get_oid_hex (refname +strlen (git_replace_ref_base), &original_oid)) {
replace-object.c +7 -0
@@ -85,7 +85,14 @@ const struct object_id *do_lookup_replace_object(struct repository *r,die (_ ("replace depth too high for object %s" ),oid_to_hex (oid)); } +static int read_replace_refs =1 ; +void disable_replace_refs (void ) { read_replace_refs =0 ; } + +int replace_refs_enabled (struct repository *r) +{ +return read_replace_refs; +}
replace-object.h +14 -1
@@ -27,6 +27,19 @@ void prepare_replace_object(struct repository *r);const struct object_id *do_lookup_replace_object (struct repository *r,const struct object_id *oid); + +/* +* Some commands disable replace-refs unconditionally, and otherwise each +* repository could alter the core.useReplaceRefs config value. +* +* Return 1 if and only if all of the following are true: +* +* a. disable_replace_refs() has not been called. +* b. GIT_NO_REPLACE_OBJECTS is unset or zero. +* c. the given repository does not have core.useReplaceRefs=false. +*/ +int replace_refs_enabled (struct repository *r); +/* * If object sha1 should be replaced, return the replacement object's * name (replaced recursively, if necessary). The return value is
@@ -41,7 +54,7 @@ const struct object_id *do_lookup_replace_object(struct repository *r,static inline const struct object_id *lookup_replace_object (struct repository *r,const struct object_id *oid) { -if (!read_replace_refs || +if (!replace_refs_enabled (r) || (r->objects->replace_map_initialized && r->objects->replace_map->map.tablesize ==0 ))return oid;
Signature · 1 line
gitgitgadget
Re: [PATCH 2/3] replace-objects: create wrapper around setting
Quoted 34 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
The '
read_replace_objects' constant is initialized bygit_default_config(if core.useReplaceRefs is disabled) and withinsetup_git_env(ifGIT_NO_REPLACE_OBJECTS) is set. To ensure that this variable cannot be set accidentally in other places, wrap it in areplace_refs_enabled()method.This allows us to remove the global from being recognized outside of
replace-objects.c.Further, a future change will help prevent the variable from being read before it is initialized. Centralizing its access is an important first step.
Signed-off-by Derrick Stolee <[email protected]>--- commit-graph.c | 4 +--- environment.c | 1 - log-tree.c | 2 +- replace-object.c | 7 +++++++ replace-object.h | 15 ++++++++++++++- 5 files changed, 23 insertions(+), 6 deletions(-)commit-graph.c +0
-2@@ -203,14 +203,12 @@ static struct commit_graph *alloc_commit_graph(void)return g; } -extern int read_replace_refs; -
commit-graph.c static struct commit_graph *alloc_commit_graph(void) · line 214
Quoted 10 lines
commit-graph.c +1
-1static int commit_graph_compatible (struct repository *r) {if (!r->gitdir)return 0 ; -if (read_replace_refs) { +if (replace_refs_enabled (r)) {prepare_replace_object (r);if (hashmap_get_size (&r->objects->replace_map->map))return 0 ;
This and the other 'read_replace_refs' -> 'replace_refs_enabled()' replacements all look good. Although we're not using the 'struct repository' argument yet, I see that it'll be used in patch 3 - adding the (unused) arg here helps avoid the extra churn there.
Quoted 29 lines
replace-object.c +7
-0@@ -85,7 +85,14 @@ const struct object_id *do_lookup_replace_object(struct repository *r,die (_ ("replace depth too high for object %s" ),oid_to_hex (oid)); } +static int read_replace_refs =1 ; +void disable_replace_refs (void ) { read_replace_refs =0 ; } + +int replace_refs_enabled (struct repository *r) +{ +return read_replace_refs; +}replace-object.h +2
-0@@ -27,6 +27,19 @@ void prepare_replace_object(struct repository *r);const struct object_id *do_lookup_replace_object (struct repository *r,const struct object_id *oid); + +/*
replace-object.h void prepare_replace_object(struct repository *r); · line 41
Quoted 10 lines
replace-object.h +10
-0+* Some commands disable replace-refs unconditionally, and otherwise each +* repository could alter the core.useReplaceRefs config value. +* +* Return 1 if and only if all of the following are true: +* +* a. disable_replace_refs() has not been called. +* b. GIT_NO_REPLACE_OBJECTS is unset or zero. +* c. the given repository does not have core.useReplaceRefs=false. +*/ +int replace_refs_enabled (struct repository *r);
Since the purpose of this function is to access global state, would 'environment.[c|h]' be a more appropriate place for it (and 'disable_replace_refs()', for that matter)? There's also some precedent; 'set_shared_repository()' and 'get_shared_repository()' have a very similar design to what you've added here.
Re: [PATCH 2/3] replace-objects: create wrapper around setting
Quoted 21 lines · Victoria Dye
Quoted 20 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
replace-object.h +10
-0@@ -27,6 +27,19 @@ void prepare_replace_object(struct repository *r);const struct object_id *do_lookup_replace_object (struct repository *r,const struct object_id *oid); + +/* +* Some commands disable replace-refs unconditionally, and otherwise each +* repository could alter the core.useReplaceRefs config value. +* +* Return 1 if and only if all of the following are true: +* +* a. disable_replace_refs() has not been called. +* b. GIT_NO_REPLACE_OBJECTS is unset or zero. +* c. the given repository does not have core.useReplaceRefs=false.
Quoted 8 lines
replace-object.h void prepare_replace_object(struct repository *r); · line 41
Quoted 2 lines
replace-object.h +2
-0+*/ +int replace_refs_enabled (struct repository *r);Since the purpose of this function is to access global state, would 'environment.[c|h]' be a more appropriate place for it (and '
disable_replace_refs()', for that matter)? There's also some precedent; 'set_shared_repository()' and 'get_shared_repository()' have a very similar design to what you've added here.
That's an interesting idea that I had not considered. My vague sense is that it is worth isolating the functionality to this header instead of lumping it into the giant environment.h header, but I've CC'd Elijah (who is leading a lot of this header organization stuff) to see if he has an opinion on this matter.
Thanks,
-Stolee
Re: [PATCH 2/3] replace-objects: create wrapper around setting
Quoted 32 lines · Derrick Stolee
Quoted 21 lines · Victoria Dye
Quoted 20 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
replace-object.h +10
-0@@ -27,6 +27,19 @@ void prepare_replace_object(struct repository *r);const struct object_id *do_lookup_replace_object (struct repository *r,const struct object_id *oid); + +/* +* Some commands disable replace-refs unconditionally, and otherwise each +* repository could alter the core.useReplaceRefs config value. +* +* Return 1 if and only if all of the following are true: +* +* a. disable_replace_refs() has not been called. +* b. GIT_NO_REPLACE_OBJECTS is unset or zero. +* c. the given repository does not have core.useReplaceRefs=false. Quoted 8 lines
replace-object.h void prepare_replace_object(struct repository *r); · line 41
Quoted 2 lines
replace-object.h +2
-0+*/ +int replace_refs_enabled (struct repository *r);Since the purpose of this function is to access global state, would 'environment.[c|h]' be a more appropriate place for it (and '
disable_replace_refs()', for that matter)? There's also some precedent; 'set_shared_repository()' and 'get_shared_repository()' have a very similar design to what you've added here.
Quoted 5 lines
That's an interesting idea that I had not considered. My vague sense is that it is worth isolating the functionality to this header instead of lumping it into the giant
environment.hheader, but I've CC'd Elijah (who is leading a lot of this header organization stuff) to see if he has an opinion on this matter.
I haven't really formed much of an opinion on the sea of globals in environment.h and elsewhere beyond "I sure wish we didn't have so many globals". Maybe I should have an opinion on it, but there was plenty to clean up without worrying about all of those. :-)
Re: [PATCH 2/3] replace-objects: create wrapper around setting
Quoted 39 lines · Elijah Newren
Quoted 32 lines · Derrick Stolee
Quoted 21 lines · Victoria Dye
Quoted 20 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
replace-object.h +10
-0@@ -27,6 +27,19 @@ void prepare_replace_object(struct repository *r);const struct object_id *do_lookup_replace_object (struct repository *r,const struct object_id *oid); + +/* +* Some commands disable replace-refs unconditionally, and otherwise each +* repository could alter the core.useReplaceRefs config value. +* +* Return 1 if and only if all of the following are true: +* +* a. disable_replace_refs() has not been called. +* b. GIT_NO_REPLACE_OBJECTS is unset or zero. +* c. the given repository does not have core.useReplaceRefs=false. Quoted 8 lines
replace-object.h void prepare_replace_object(struct repository *r); · line 41
Quoted 2 lines
replace-object.h +2
-0+*/ +int replace_refs_enabled (struct repository *r);Since the purpose of this function is to access global state, would 'environment.[c|h]' be a more appropriate place for it (and '
disable_replace_refs()', for that matter)? There's also some precedent; 'set_shared_repository()' and 'get_shared_repository()' have a very similar design to what you've added here.Quoted 5 lines
That's an interesting idea that I had not considered. My vague sense is that it is worth isolating the functionality to this header instead of lumping it into the giant
environment.hheader, but I've CC'd Elijah (who is leading a lot of this header organization stuff) to see if he has an opinion on this matter.
Quoted 4 lines
I haven't really formed much of an opinion on the sea of globals in
environment.hand elsewhere beyond "I sure wish we didn't have so many globals". Maybe I should have an opinion on it, but there was plenty to clean up without worrying about all of those. :-)
Thanks for chiming in (even with "I haven't thought about it too much").
Thinking back on this with some time since the initial question, I think the grouping "global state" into environment.h isn't the right goal. Using replace-object.h collects all _functionality related to the feature_ in a single place, and it just so happens to include some global state due to the needs of the feature.
I plan to keep these methods in replace-object.h. With that, we have only 20 files that include that, as opposed to 141 including environment.h.
Thanks,
-Stolee
PATCH 3/3 repository: create read_replace_refs setting
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
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.
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.
--- config.c | 5 ----- replace-object.c | 13 +++++++++++-- replace-object.h | 8 -------- repo-settings.c | 1 + repository.h | 8 ++++++++ 5 files changed, 20 insertions(+), 15 deletions(-)
config.c +0 -5
@@ -1838,11 +1838,6 @@ static int git_default_core_config(const char *var, const char *value, void *cb)return 0 ; } -if (!strcmp (var,"core.usereplacerefs" )) { - read_replace_refs =git_config_bool (var, value); -return 0 ; - } -/* Add other config variables here and to Documentation/config.txt. */ return platform_core_config (var, value, cb); }
replace-object.c +11 -2
@@ -64,7 +64,7 @@ void prepare_replace_object(struct repository *r)* replacement object's name (replaced recursively, if necessary). * The return value is either oid or a pointer to a * permanently-allocated value. This function always respects replace -* references, regardless of the value of read_replace_refs. +* references, regardless of the value of r->settings.read_replace_refs. */ const struct object_id *do_lookup_replace_object (struct repository *r,const struct object_id *oid)
@@ -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 ; }
replace-object.h +0 -8
@@ -5,14 +5,6 @@#include "repository.h" #include "object-store.h" -/* -* 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. -*/ -extern int read_replace_refs; -struct replace_object {struct oidmap_entry original;struct object_id replacement;
repo-settings.c +1 -0
@@ -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 );/* * The GIT_TEST_MULTI_PACK_INDEX variable is special in that
repository.h +8 -0
@@ -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; +struct fsmonitor_settings *fsmonitor;/* lazily loaded */ int index_version;
Signature · 1 line
gitgitgadget
Re: [PATCH 3/3] repository: create read_replace_refs setting
Quoted 11 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
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 inb6551feadfd(merge-tree:
load default git config, 2023-05-10).To make this more robust, move its config-based initialization out of
git_default_configand intoprepare_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/[email protected]/
Quoted 16 lines
The global still has its purpose: it is disabled process-wide by the
GIT_NO_REPLACE_OBJECTSenvironment variable or by a call todisable_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.
Quoted 4 lines
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?
Quoted 6 lines
replace-object.c +1
-1@@ -94,5 +94,14 @@ void disable_replace_refs(void)int replace_refs_enabled (struct repository *r) { -return read_replace_refs; +if (!read_replace_refs)
replace-object.c void disable_replace_refs(void) · line 107
Quoted 10 lines
replace-object.c +9
-0+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.
repo-settings.c void prepare_repo_settings(struct repository *r) · line 71
Quoted 9 lines
repo-settings.c +1
-0@@ -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!
Quoted 8 lines
repo-settings.c +0
-0/* * The GIT_TEST_MULTI_PACK_INDEX variable is special in that repository.h +0
-0
repository.h struct repo_settings { · line 48
Quoted 10 lines
repository.h +7
-0@@ -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').
Quoted 4 lines
repository.h +1
-0+struct fsmonitor_settings *fsmonitor;/* lazily loaded */ int index_version;
Re: [PATCH 3/3] repository: create read_replace_refs setting
Quoted 8 lines · Victoria Dye
Quoted 6 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
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.
Quoted 5 lines
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?
You're right. I was laser-focused on 'ls-files', but it shouldn't be hard to construct an example where 'git grep --recurse-submodules' would show different behavior when this config is toggled.
Quoted 4 lines
Quoted 4 lines
repository.h +4
-0+/* +* 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
Quoted 8 lines
repository.h struct repo_settings { · line 48
Quoted 3 lines
repository.h +3
-0+* 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').
Thank you for catching my mistake here. I'll be sure to update it in v2.
Thanks,
-Stolee
Quoted 35 lines · Derrick Stolee via GitGitGadget
(This series is based on tb/pack-bitmap-traversal-with-boundary due to wanting to modify
prepare_repo_settings()in a similar way.)The replace-refs can be ignored via the core.useReplaceRefs=false config setting. This setting is possible to miss in some Git commands if they do not load default config at the appropriate time. See [1] for a recent example of this.
[1]
https://lore.kernel.org/git/[email protected]/This series aims to avoid this kind of error from happening in the future. The idea is to encapsulate the setting in such a way that we can guarantee that config has been checked before using the in-memory value.
Further, we must be careful that some Git commands want to disable replace refs unconditionally, as if
GIT_NO_REPLACE_REFSwas enabled in the environment.The approach taken here is to split the global into two different sources. First,
read_replace_refsis kept (but moved toreplace-objects.cscope) and reflects whether or not the feature is permitted by the environment and the current command. Second, a new value is added to repo-settings and this is checked after usingprepare_repo_settings()to guarantee the config has been read.This presents a potential behavior change, in that now core.useReplaceRefs is specific to each in-memory repository instead of applying the superproject value to all submodules. I could not find a Git command that has multiple in-memory repositories and follows OIDs to object contents, so I'm not sure how to demonstrate it in a test.
Here is the breakdown of the series:
Quoted 8 lines
* Patch 1 creates
disable_replace_refs()to encapsulate the global disabling of the feature.
* Patch 2 createsreplace_refs_enabled()to check if the feature is enabled (with respect to a given repository). This is a thin wrapper of the global at this point, but does allow us to remove it fromenvironment.h.
* Patch 3 creates the value in repo-settings as well as ensures that the repo settings have been prepared before accessing the value withinreplace_refs_enabled().
Thanks for implementing this. I had a few questions on the first patch (though I think one of them was answered by noting that you have both a global and a repository setting for the flag), but otherwise it looks good.
v2 cover Create stronger guard rails on replace refs
In reply to Derrick Stolee via GitGitGadget(This series is based on tb/pack-bitmap-traversal-with-boundary due to wanting to modify prepare_repo_settings() in a similar way.)
The replace-refs can be ignored via the core.useReplaceRefs=false config setting. This setting is possible to miss in some Git commands if they do not load default config at the appropriate time. See [1] for a recent example of this.
[1]
https://lore.kernel.org/git/[email protected]/
This series aims to avoid this kind of error from happening in the future. The idea is to encapsulate the setting in such a way that we can guarantee that config has been checked before using the in-memory value.
Further, we must be careful that some Git commands want to disable replace refs unconditionally, as if GIT_NO_REPLACE_REFS was enabled in the environment.
The approach taken here is to split the global into two different sources. First, read_replace_refs is kept (but moved to replace-objects.c scope) and reflects whether or not the feature is permitted by the environment and the current command. Second, a new value is added to repo-settings and this is checked after using prepare_repo_settings() to guarantee the config has been read.
This presents a potential behavior change, in that now core.useReplaceRefs is specific to each in-memory repository instead of applying the superproject value to all submodules. I could not find a Git command that has multiple in-memory repositories and follows OIDs to object contents, so I'm not sure how to demonstrate it in a test.
Here is the breakdown of the series:
* Patch 1 creates disable_replace_refs() to encapsulate the global disabling of the feature.
* Patch 2 creates replace_refs_enabled() to check if the feature is enabled (with respect to a given repository). This is a thin wrapper of the global at this point, but does allow us to remove it from environment.h.
* Patch 3 creates the value in repo-settings as well as ensures that the repo settings have been prepared before accessing the value within replace_refs_enabled(). A test is added to demonstrate how the config value is now scoped on a per-repository basis.
Updates in v2
=============
Thanks for the careful review on v1!
* disable_replace_refs() now replaces "read_replace_refs = 0" in the exact same line to avoid possible behavior change.
* Stale comments, include headers, and commit messages are updated to include the latest status.
* Patch 3 contains a test of the repo-scoped value using 'git grep'.
Thanks, -Stolee
Derrick Stolee (3):
repository: create disable_replace_refs()
replace-objects: create wrapper around setting
repository: create read_replace_refs setting
builtin/cat-file.c | 2 +- builtin/commit-graph.c | 2 +- builtin/fsck.c | 2 +- builtin/index-pack.c | 2 +- builtin/pack-objects.c | 2 +- builtin/prune.c | 2 +- builtin/replace.c | 2 +- builtin/unpack-objects.c | 2 +- builtin/upload-pack.c | 2 +- commit-graph.c | 4 +-- config.c | 5 ---- environment.c | 3 +-- git.c | 2 +- log-tree.c | 2 +- replace-object.c | 23 ++++++++++++++++- replace-object.h | 31 ++++++++++++++++------- repo-settings.c | 1 + repository.h | 9 +++++++ t/t7814-grep-recurse-submodules.sh | 40 ++++++++++++++++++++++++++++++ 19 files changed, 107 insertions(+), 31 deletions(-)
base-commit: b0afdce5dab61f224fd66c13768facc36a7f8705
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1537%2Fderrickstolee%2Freplace-refs-safety-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1537/derrickstolee/replace-refs-safety-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1537
Range-diff vs v1:
1: 56544abc15d ! 1: 0616fdbf303 repository: create disable_replace_refs()
@@ Commit message
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.
+ We will need to keep this read_replace_refs global forever, as we want
+ to make sure that we never use replace refs throughout the life of the
+ process if this method is called. Future changes may present a
+ repository-scoped version of the variable to represent that repository's
+ core.useReplaceRefs config value, but a zero-valued read_replace_refs
+ will always override such a setting.
@@ builtin/cat-file.c: static int batch_objects(struct batch_options *opt)
cb.expand = &data;
## builtin/commit-graph.c ##
-@@ builtin/commit-graph.c: 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,
@@ builtin/commit-graph.c: 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;
++ disable_replace_refs();
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);
- }
## builtin/fsck.c ##
@@ builtin/fsck.c: int cmd_fsck(int argc, const char **argv, const char *prefix)
@@ builtin/fsck.c: int cmd_fsck(int argc, const char **argv, const char *prefix)
errors_found = 0;
- read_replace_refs = 0;
++ disable_replace_refs();
save_commit_buffer = 0;
argc = parse_options(argc, argv, prefix, fsck_opts, fsck_usage, 0);
-@@ builtin/fsck.c: 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);
## builtin/index-pack.c ##
@@ builtin/index-pack.c: int cmd_index_pack(int argc, const char **argv, const char *prefix)
@@ builtin/pack-objects.c: int cmd_pack_objects(int argc, const char **argv, const
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();
+
+ sparse = git_env_bool("GIT_TEST_PACK_SPARSE", -1);
if (the_repository->gitdir) {
- prepare_repo_settings(the_repository);
- if (sparse < 0)
## builtin/prune.c ##
@@ builtin/prune.c: int cmd_prune(int argc, const char **argv, const char *prefix)
@@ builtin/prune.c: int cmd_prune(int argc, const char **argv, const char *prefix)
expire = TIME_MAX;
save_commit_buffer = 0;
- read_replace_refs = 0;
++ disable_replace_refs();
repo_init_revisions(the_repository, &revs, prefix);
argc = parse_options(argc, argv, prefix, options, prune_usage, 0);
-@@ builtin/prune.c: 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++;
## builtin/replace.c ##
@@ builtin/replace.c: int cmd_replace(int argc, const char **argv, const char *prefix)
@@ builtin/replace.c: int cmd_replace(int argc, const char **argv, const char *pref
};
- read_replace_refs = 0;
++ disable_replace_refs();
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;
-
## builtin/unpack-objects.c ##
@@ builtin/unpack-objects.c: int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)
@@ builtin/unpack-objects.c: int cmd_unpack_objects(int argc, const char **argv, co
struct object_id oid;
- read_replace_refs = 0;
--
- git_config(git_default_config, NULL);
+ disable_replace_refs();
- quiet = !isatty(2);
+ git_config(git_default_config, NULL);
## builtin/upload-pack.c ##
@@ builtin/upload-pack.c: int cmd_upload_pack(int argc, const char **argv, const ch
packet_trace_identity("upload-pack");
- read_replace_refs = 0;
++ disable_replace_refs();
argc = parse_options(argc, argv, prefix, options, upload_pack_usage, 0);
-@@ builtin/upload-pack.c: 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)
## environment.c ##
@@ environment.c: void setup_git_env(const char *git_dir)
@@ replace-object.h: static inline const struct object_id *lookup_replace_object(st
+void disable_replace_refs(void);
+
#endif /* REPLACE_OBJECT_H */
-
- ## repo-settings.c ##
-@@
- #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)
2: 5fc2f923d9e = 2: 0831e7f8b5e replace-objects: create wrapper around setting
3: 481a81a515e ! 3: 4c7dbeb8c6d repository: create read_replace_refs setting
@@ Commit message
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.
+ 'git grep --recurse-submodules' is such a command that recurses into
+ submodules in-process. We can demonstrate the granularity of this config
+ value via a test in t7814.
Signed-off-by: Derrick Stolee <[email protected]>
@@ repository.h: struct repo_settings {
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.
++ * Has this repository have core.useReplaceRefs=true (on by
++ * default)? This provides a repository-scoped version of this
++ * config, though it could be disabled process-wide via some Git
++ * builtins or the --no-replace-objects option. See
++ * replace_refs_enabled() for more details.
+ */
+ int read_replace_refs;
+
struct fsmonitor_settings *fsmonitor; /* lazily loaded */
int index_version;
+
+ ## t/t7814-grep-recurse-submodules.sh ##
+@@ t/t7814-grep-recurse-submodules.sh: test_expect_success 'grep partially-cloned submodule' '
+ )
+ '
+
++test_expect_success 'check scope of core.useReplaceRefs' '
++ git init base &&
++ git init base/sub &&
++
++ echo A >base/a &&
++ echo B >base/b &&
++ echo C >base/sub/c &&
++ echo D >base/sub/d &&
++
++ git -C base/sub add c d &&
++ git -C base/sub commit -m "Add files" &&
++
++ git -C base submodule add ./sub &&
++ git -C base add a b sub &&
++ git -C base commit -m "Add files and submodule" &&
++
++ A=$(git -C base rev-parse HEAD:a) &&
++ B=$(git -C base rev-parse HEAD:b) &&
++ C=$(git -C base/sub rev-parse HEAD:c) &&
++ D=$(git -C base/sub rev-parse HEAD:d) &&
++
++ git -C base replace $A $B &&
++ git -C base/sub replace $C $D &&
++
++ test_must_fail git -C base grep --cached --recurse-submodules A &&
++ test_must_fail git -C base grep --cached --recurse-submodules C &&
++
++ git -C base config core.useReplaceRefs false &&
++ git -C base grep --recurse-submodules A &&
++ test_must_fail git -C base grep --cached --recurse-submodules C &&
++
++ git -C base/sub config core.useReplaceRefs false &&
++ git -C base grep --cached --recurse-submodules A &&
++ git -C base grep --cached --recurse-submodules C &&
++
++ git -C base config --unset core.useReplaceRefs &&
++ test_must_fail git -C base grep --cached --recurse-submodules A &&
++ git -C base grep --cached --recurse-submodules C
++'
++
+ test_doneSignature · 1 line
gitgitgadget
PATCH v2 1/3 repository: create disable_replace_refs()
From: Derrick Stolee <[email protected]>
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 need to keep this read_replace_refs global forever, as we want to make sure that we never use replace refs throughout the life of the process if this method is called. Future changes may present a repository-scoped version of the variable to represent that repository's core.useReplaceRefs config value, but a zero-valued read_replace_refs will always override such a setting.
--- builtin/cat-file.c | 2 +- builtin/commit-graph.c | 2 +- builtin/fsck.c | 2 +- builtin/index-pack.c | 2 +- builtin/pack-objects.c | 2 +- builtin/prune.c | 2 +- builtin/replace.c | 2 +- builtin/unpack-objects.c | 2 +- builtin/upload-pack.c | 2 +- environment.c | 2 +- git.c | 2 +- replace-object.c | 5 +++++ replace-object.h | 8 ++++++++ 13 files changed, 24 insertions(+), 11 deletions(-)
builtin/cat-file.c +1 -1
@@ -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;
builtin/commit-graph.c +1 -1
@@ -324,7 +324,7 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)git_config (git_default_config,NULL ); - read_replace_refs =0 ; +disable_replace_refs (); save_commit_buffer =0 ; argc = parse_options(argc, argv, prefix, options,
builtin/fsck.c +1 -1
@@ -927,7 +927,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix) fetch_if_missing =0 ; errors_found =0 ; - read_replace_refs =0 ; +disable_replace_refs (); save_commit_buffer =0 ; argc =parse_options (argc, argv, prefix, fsck_opts, fsck_usage,0 );
builtin/index-pack.c +1 -1
@@ -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);
builtin/pack-objects.c +1 -1
@@ -4284,7 +4284,7 @@ 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 ; +disable_replace_refs (); sparse =git_env_bool ("GIT_TEST_PACK_SPARSE" ,-1 );if (the_repository->gitdir) {
builtin/prune.c +1 -1
@@ -164,7 +164,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix) expire =TIME_MAX ; save_commit_buffer =0 ; - read_replace_refs =0 ; +disable_replace_refs ();repo_init_revisions (the_repository, &revs, prefix); argc =parse_options (argc, argv, prefix, options, prune_usage,0 );
builtin/replace.c +1 -1
@@ -566,7 +566,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix) OPT_END() }; - read_replace_refs =0 ; +disable_replace_refs ();git_config (git_default_config,NULL ); argc =parse_options (argc, argv, prefix, options, git_replace_usage,0 );
builtin/unpack-objects.c +1 -1
@@ -609,7 +609,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)int i;struct object_id oid; - read_replace_refs =0 ; +disable_replace_refs ();git_config (git_default_config,NULL );
builtin/upload-pack.c +1 -1
@@ -36,7 +36,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix) };packet_trace_identity ("upload-pack" ); - read_replace_refs =0 ; +disable_replace_refs (); argc =parse_options (argc, argv, prefix, options, upload_pack_usage,0 );
environment.c +1 -1
@@ -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/" );
git.c +1 -1
@@ -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 ;
replace-object.c +5 -0
@@ -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 ; +}
replace-object.h +8 -0
@@ -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 */
Signature · 1 line
gitgitgadget
PATCH v2 2/3 replace-objects: create wrapper around setting
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
The 'read_replace_objects' constant is initialized by git_default_config (if core.useReplaceRefs is disabled) and within setup_git_env (if GIT_NO_REPLACE_OBJECTS) is set. To ensure that this variable cannot be set accidentally in other places, wrap it in a replace_refs_enabled() method.
This allows us to remove the global from being recognized outside of replace-objects.c.
Further, a future change will help prevent the variable from being read before it is initialized. Centralizing its access is an important first step.
--- commit-graph.c | 4 +--- environment.c | 1 - log-tree.c | 2 +- replace-object.c | 7 +++++++ replace-object.h | 15 ++++++++++++++- 5 files changed, 23 insertions(+), 6 deletions(-)
commit-graph.c +1 -3
@@ -203,14 +203,12 @@ static struct commit_graph *alloc_commit_graph(void)return g; } -extern int read_replace_refs; -static int commit_graph_compatible (struct repository *r) {if (!r->gitdir)return 0 ; -if (read_replace_refs) { +if (replace_refs_enabled (r)) {prepare_replace_object (r);if (hashmap_get_size (&r->objects->replace_map->map))return 0 ;
environment.c +0 -1
@@ -63,7 +63,6 @@ const char *editor_program;const char *askpass_program;const char *excludes_file;enum auto_crlf auto_crlf =AUTO_CRLF_FALSE ; -int read_replace_refs =1 ;enum eol core_eol =EOL_UNSET ;int global_conv_flags_eol =CONV_EOL_RNDTRP_WARN ;char *check_roundtrip_encoding ="SHIFT-JIS" ;
log-tree.c +1 -1
@@ -155,7 +155,7 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,if (starts_with (refname, git_replace_ref_base)) {struct object_id original_oid; -if (!read_replace_refs) +if (!replace_refs_enabled (the_repository))return 0 ;if (get_oid_hex (refname +strlen (git_replace_ref_base), &original_oid)) {
replace-object.c +7 -0
@@ -85,7 +85,14 @@ const struct object_id *do_lookup_replace_object(struct repository *r,die (_ ("replace depth too high for object %s" ),oid_to_hex (oid)); } +static int read_replace_refs =1 ; +void disable_replace_refs (void ) { read_replace_refs =0 ; } + +int replace_refs_enabled (struct repository *r) +{ +return read_replace_refs; +}
replace-object.h +14 -1
@@ -27,6 +27,19 @@ void prepare_replace_object(struct repository *r);const struct object_id *do_lookup_replace_object (struct repository *r,const struct object_id *oid); + +/* +* Some commands disable replace-refs unconditionally, and otherwise each +* repository could alter the core.useReplaceRefs config value. +* +* Return 1 if and only if all of the following are true: +* +* a. disable_replace_refs() has not been called. +* b. GIT_NO_REPLACE_OBJECTS is unset or zero. +* c. the given repository does not have core.useReplaceRefs=false. +*/ +int replace_refs_enabled (struct repository *r); +/* * If object sha1 should be replaced, return the replacement object's * name (replaced recursively, if necessary). The return value is
@@ -41,7 +54,7 @@ const struct object_id *do_lookup_replace_object(struct repository *r,static inline const struct object_id *lookup_replace_object (struct repository *r,const struct object_id *oid) { -if (!read_replace_refs || +if (!replace_refs_enabled (r) || (r->objects->replace_map_initialized && r->objects->replace_map->map.tablesize ==0 ))return oid;
Signature · 1 line
gitgitgadget
Re: [PATCH v2 2/3] replace-objects: create wrapper around setting
Am 02.06.23 um 16:29 schrieb Derrick Stolee via GitGitGadget:
replace-object.c const struct object_id *do_lookup_replace_object(struct repository *r, · line 89
Quoted 10 lines
replace-object.c +2
-0@@ -85,7 +85,14 @@ const struct object_id *do_lookup_replace_object(struct repository *r,die (_ ("replace depth too high for object %s" ),oid_to_hex (oid)); } +static int read_replace_refs =1 ; +
This breaks compilation:
replace-object.c:88:12: error: static declaration of 'read_replace_refs' follows non-static declaration
static int read_replace_refs = 1;
^
./replace-object.h:14:12: note: previous declaration is here
extern int read_replace_refs;
^And this variable is still referenced in two more places outside this file, which won't work now that it has become static (file-scoped):
$ git grep read_replace_refs builtin/commit-graph.c:extern int read_replace_refs; config.c: read_replace_refs = git_config_bool(var, value); replace-object.c: * references, regardless of the value of read_replace_refs. replace-object.c:static int read_replace_refs = 1; replace-object.c: read_replace_refs = 0; replace-object.c: return read_replace_refs; replace-object.h:extern int read_replace_refs;
Perhaps postpone adding "static" to patch 3?
Quoted 42 lines
replace-object.c +5
-0void disable_replace_refs (void ) { read_replace_refs =0 ; } + +int replace_refs_enabled (struct repository *r) +{ +return read_replace_refs; +}replace-object.h +14
-1@@ -27,6 +27,19 @@ void prepare_replace_object(struct repository *r);const struct object_id *do_lookup_replace_object (struct repository *r,const struct object_id *oid); + +/* +* Some commands disable replace-refs unconditionally, and otherwise each +* repository could alter the core.useReplaceRefs config value. +* +* Return 1 if and only if all of the following are true: +* +* a. disable_replace_refs() has not been called. +* b. GIT_NO_REPLACE_OBJECTS is unset or zero. +* c. the given repository does not have core.useReplaceRefs=false. +*/ +int replace_refs_enabled (struct repository *r); +/* * If object sha1 should be replaced, return the replacement object's * name (replaced recursively, if necessary). The return value is @@ -41,7 +54,7 @@ const struct object_id *do_lookup_replace_object(struct repository *r,static inline const struct object_id *lookup_replace_object (struct repository *r,const struct object_id *oid) { -if (!read_replace_refs || +if (!replace_refs_enabled (r) || (r->objects->replace_map_initialized && r->objects->replace_map->map.tablesize ==0 ))return oid;
Re: [PATCH v2 2/3] replace-objects: create wrapper around setting
Quoted 14 lines · René Scharfe
Am 02.06.23 um 16:29 schrieb Derrick Stolee via GitGitGadget:
replace-object.c const struct object_id *do_lookup_replace_object(struct repository *r, · line 89
Quoted 10 lines
replace-object.c +2
-0@@ -85,7 +85,14 @@ const struct object_id *do_lookup_replace_object(struct repository *r,die (_ ("replace depth too high for object %s" ),oid_to_hex (oid)); } +static int read_replace_refs =1 ; +This breaks compilation:
Quoted 6 lines
replace-object.c:88:12: error: static declaration of 'read_replace_refs' follows non-static declaration
static int read_replace_refs = 1;
^
./replace-object.h:14:12: note: previous declaration is here
extern int read_replace_refs;
^Thanks for finding this issue within the patch. The removal of the global should have happened in this patch, but I missed it when adjusting things for this version.
Quoted 12 lines
And this variable is still referenced in two more places outside this file, which won't work now that it has become static (file-scoped):
$ git grep read_replace_refs builtin/commit-graph.c:extern int read_replace_refs; config.c: read_replace_refs = git_config_bool(var, value); replace-object.c: * references, regardless of the value of read_replace_refs. replace-object.c:static int read_replace_refs = 1; replace-object.c: read_replace_refs = 0; replace-object.c: return read_replace_refs; replace-object.h:extern int read_replace_refs;
Quoted 1 line
Perhaps postpone adding "static" to patch 3?
You're right that this won't work unless we fix the config-parsing code already here, so I should move the introduction of the static global until patch 3, as you suggest.
Thanks,
-Stolee
PATCH v2 3/3 repository: create read_replace_refs setting
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
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.
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.
'git grep --recurse-submodules' is such a command that recurses into submodules in-process. We can demonstrate the granularity of this config value via a test in t7814.
--- config.c | 5 ---- replace-object.c | 13 ++++++++-- replace-object.h | 8 ------ repo-settings.c | 1 + repository.h | 9 +++++++ t/t7814-grep-recurse-submodules.sh | 40 ++++++++++++++++++++++++++++++ 6 files changed, 61 insertions(+), 15 deletions(-)
config.c +0 -5
@@ -1838,11 +1838,6 @@ static int git_default_core_config(const char *var, const char *value, void *cb)return 0 ; } -if (!strcmp (var,"core.usereplacerefs" )) { - read_replace_refs =git_config_bool (var, value); -return 0 ; - } -/* Add other config variables here and to Documentation/config.txt. */ return platform_core_config (var, value, cb); }
replace-object.c +11 -2
@@ -64,7 +64,7 @@ void prepare_replace_object(struct repository *r)* replacement object's name (replaced recursively, if necessary). * The return value is either oid or a pointer to a * permanently-allocated value. This function always respects replace -* references, regardless of the value of read_replace_refs. +* references, regardless of the value of r->settings.read_replace_refs. */ const struct object_id *do_lookup_replace_object (struct repository *r,const struct object_id *oid)
@@ -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 ; }
replace-object.h +0 -8
@@ -5,14 +5,6 @@#include "repository.h" #include "object-store.h" -/* -* 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. -*/ -extern int read_replace_refs; -struct replace_object {struct oidmap_entry original;struct object_id replacement;
repo-settings.c +1 -0
@@ -67,6 +67,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 );/* * The GIT_TEST_MULTI_PACK_INDEX variable is special in that
repository.h +9 -0
@@ -39,6 +39,15 @@ struct repo_settings {int pack_read_reverse_index;int pack_use_bitmap_boundary_traversal; +/* +* Has this repository have core.useReplaceRefs=true (on by +* default)? This provides a repository-scoped version of this +* config, though it could be disabled process-wide via some Git +* builtins or the --no-replace-objects option. See +* replace_refs_enabled() for more details. +*/ +int read_replace_refs; +struct fsmonitor_settings *fsmonitor;/* lazily loaded */ int index_version;
t/t7814-grep-recurse-submodules.sh +40 -0
@@ -594,4 +594,44 @@ test_expect_success 'grep partially-cloned submodule' ' ) ' +test_expect_success 'check scope of core.useReplaceRefs' ' + git init base && + git init base/sub && + + echo A >base/a && + echo B >base/b && + echo C >base/sub/c && + echo D >base/sub/d && + + git -C base/sub add c d && + git -C base/sub commit -m "Add files" && + + git -C base submodule add ./sub && + git -C base add a b sub && + git -C base commit -m "Add files and submodule" && + + A=$(git -C base rev-parse HEAD:a) && + B=$(git -C base rev-parse HEAD:b) && + C=$(git -C base/sub rev-parse HEAD:c) && + D=$(git -C base/sub rev-parse HEAD:d) && + + git -C base replace $A $B && + git -C base/sub replace $C $D && + + test_must_fail git -C base grep --cached --recurse-submodules A && + test_must_fail git -C base grep --cached --recurse-submodules C && + + git -C base config core.useReplaceRefs false && + git -C base grep --recurse-submodules A && + test_must_fail git -C base grep --cached --recurse-submodules C && + + git -C base/sub config core.useReplaceRefs false && + git -C base grep --cached --recurse-submodules A && + git -C base grep --cached --recurse-submodules C && + + git -C base config --unset core.useReplaceRefs && + test_must_fail git -C base grep --cached --recurse-submodules A && + git -C base grep --cached --recurse-submodules C +' + test_done
Signature · 1 line
gitgitgadget
Re: [PATCH v2 3/3] repository: create read_replace_refs setting
repository.h struct repo_settings { · line 43
Quoted 10 lines · Derrick Stolee via GitGitGadget
repository.h +2
-0@@ -39,6 +39,15 @@ struct repo_settings {int pack_read_reverse_index;int pack_use_bitmap_boundary_traversal; +/* +* Has this repository have core.useReplaceRefs=true (on by
s/Has/Does(?)
It's not a huge deal, but if you were planning to re-roll anyway due to [1], this should be easy enough to include.
Quoted 34 lines
repository.h +7
-0+* default)? This provides a repository-scoped version of this +* config, though it could be disabled process-wide via some Git +* builtins or the --no-replace-objects option. See +* replace_refs_enabled() for more details. +*/ +int read_replace_refs; +struct fsmonitor_settings *fsmonitor;/* lazily loaded */ int index_version;t/t7814-grep-recurse-submodules.sh +16
-0@@ -594,4 +594,44 @@ test_expect_success 'grep partially-cloned submodule' ' ) ' +test_expect_success 'check scope of core.useReplaceRefs' ' + git init base && + git init base/sub && + + echo A >base/a && + echo B >base/b && + echo C >base/sub/c && + echo D >base/sub/d && + + git -C base/sub add c d && + git -C base/sub commit -m "Add files" && + + git -C base submodule add ./sub && + git -C base add a b sub && + git -C base commit -m "Add files and submodule" && +
t/t7814-grep-recurse-submodules.sh test_expect_success 'grep partially-cloned submodule' ' · line 622
Quoted 10 lines
t/t7814-grep-recurse-submodules.sh +10
-0+ A=$(git -C base rev-parse HEAD:a) && + B=$(git -C base rev-parse HEAD:b) && + C=$(git -C base/sub rev-parse HEAD:c) && + D=$(git -C base/sub rev-parse HEAD:d) && + + git -C base replace $A $B && + git -C base/sub replace $C $D && + + test_must_fail git -C base grep --cached --recurse-submodules A && + test_must_fail git -C base grep --cached --recurse-submodules C &&
First, we verify that replace refs are enabled on both the superproject and submodule by default (if they are, 'a' has been replaced with 'b' and 'c' has been replaced with 'd')...
t/t7814-grep-recurse-submodules.sh test_expect_success 'grep partially-cloned submodule' ' · line 626
Quoted 4 lines
t/t7814-grep-recurse-submodules.sh +4
-0+ + git -C base config core.useReplaceRefs false && + git -C base grep --recurse-submodules A && + test_must_fail git -C base grep --cached --recurse-submodules C &&
...then, if replace refs are disabled in the superproject (but not the submodule), 'b' no longer replaces 'a' but 'd' still replaces 'c'...
t/t7814-grep-recurse-submodules.sh test_expect_success 'grep partially-cloned submodule' ' · line 630
Quoted 4 lines
t/t7814-grep-recurse-submodules.sh +4
-0+ + git -C base/sub config core.useReplaceRefs false && + git -C base grep --cached --recurse-submodules A && + git -C base grep --cached --recurse-submodules C &&
...but once replace refs are disabled in the submodule, 'd' no longer replaces 'c', and 'b' still doesn't replace 'a'...
t/t7814-grep-recurse-submodules.sh test_expect_success 'grep partially-cloned submodule' ' · line 634
Quoted 4 lines
t/t7814-grep-recurse-submodules.sh +4
-0+ + git -C base config --unset core.useReplaceRefs && + test_must_fail git -C base grep --cached --recurse-submodules A && + git -C base grep --cached --recurse-submodules C
...and, finally, if we restore the default state in the superproject (replace refs enabled) but not the submodule, 'b' once again replaces 'a', but 'd' still doesn't replace 'c'.
Thanks for adding this test! It clearly and thoroughly exercises the updated config behavior.
Quoted 3 lines
t/t7814-grep-recurse-submodules.sh +2
-0+' + test_done
Hi,
On Fri, Jun 2, 2023 at 7:29 AM Derrick Stolee via GitGitGadget <[email protected]> wrote:
[...]
Quoted 10 lines
Updates in v2
=============Thanks for the careful review on v1!
*
disable_replace_refs()now replaces "read_replace_refs= 0" in the exact same line to avoid possible behavior change.
* Stale comments, include headers, and commit messages are updated to include the latest status.
* Patch 3 contains a test of the repo-scoped value using 'git grep'.
This version looks good to me; thanks for working on it!
(As a sidenote, it might be nice to call out Victoria's questions about possibly moving stuff to environment.h[1], so that if others have opinions on the matter they can comment on it. I don't have one at this time.)
[1] Last paragraph of
https://lore.kernel.org/git/[email protected]/
v3 cover Create stronger guard rails on replace refs
In reply to Derrick Stolee via GitGitGadget(This series is based on tb/pack-bitmap-traversal-with-boundary due to wanting to modify prepare_repo_settings() in a similar way.)
The replace-refs can be ignored via the core.useReplaceRefs=false config setting. This setting is possible to miss in some Git commands if they do not load default config at the appropriate time. See [1] for a recent example of this.
[1]
https://lore.kernel.org/git/[email protected]/
This series aims to avoid this kind of error from happening in the future. The idea is to encapsulate the setting in such a way that we can guarantee that config has been checked before using the in-memory value.
Further, we must be careful that some Git commands want to disable replace refs unconditionally, as if GIT_NO_REPLACE_REFS was enabled in the environment.
The approach taken here is to split the global into two different sources. First, read_replace_refs is kept (but moved to replace-objects.c scope) and reflects whether or not the feature is permitted by the environment and the current command. Second, a new value is added to repo-settings and this is checked after using prepare_repo_settings() to guarantee the config has been read.
This presents a potential behavior change, in that now core.useReplaceRefs is specific to each in-memory repository instead of applying the superproject value to all submodules. I could not find a Git command that has multiple in-memory repositories and follows OIDs to object contents, so I'm not sure how to demonstrate it in a test.
Here is the breakdown of the series:
* Patch 1 creates disable_replace_refs() to encapsulate the global disabling of the feature.
* Patch 2 creates replace_refs_enabled() to check if the feature is enabled (with respect to a given repository).
* Patch 3 creates the value in repo-settings as well as ensures that the repo settings have been prepared before accessing the value within replace_refs_enabled(). A test is added to demonstrate how the config value is now scoped on a per-repository basis.
Updates in v3
=============
Thanks for the review on v2!
* The removal of the global from environment.c is delayed to patch 3 because config.c still assigns the value in patch 2.
* The comment for the member in the repo_settings struct is modified for better grammar.
Updates in v2
=============
Thanks for the careful review on v1!
* disable_replace_refs() now replaces "read_replace_refs = 0" in the exact same line to avoid possible behavior change.
* Stale comments, include headers, and commit messages are updated to include the latest status.
* Patch 3 contains a test of the repo-scoped value using 'git grep'.
Thanks, -Stolee
Derrick Stolee (3):
repository: create disable_replace_refs()
replace-objects: create wrapper around setting
repository: create read_replace_refs setting
builtin/cat-file.c | 2 +- builtin/commit-graph.c | 2 +- builtin/fsck.c | 2 +- builtin/index-pack.c | 2 +- builtin/pack-objects.c | 2 +- builtin/prune.c | 2 +- builtin/replace.c | 2 +- builtin/unpack-objects.c | 2 +- builtin/upload-pack.c | 2 +- commit-graph.c | 4 +-- config.c | 5 ---- environment.c | 3 +-- git.c | 2 +- log-tree.c | 2 +- replace-object.c | 28 ++++++++++++++++++++- replace-object.h | 30 +++++++++++++++------- repo-settings.c | 1 + repository.h | 9 +++++++ t/t7814-grep-recurse-submodules.sh | 40 ++++++++++++++++++++++++++++++ 19 files changed, 111 insertions(+), 31 deletions(-)
base-commit: b0afdce5dab61f224fd66c13768facc36a7f8705
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1537%2Fderrickstolee%2Freplace-refs-safety-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1537/derrickstolee/replace-refs-safety-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1537
Range-diff vs v2:
1: 0616fdbf303 = 1: 0616fdbf303 repository: create disable_replace_refs() 2: 0831e7f8b5e ! 2: 4e75a76f5dd replace-objects: create wrapper around setting
@@ Commit message
set accidentally in other places, wrap it in a replace_refs_enabled() method.
- This allows us to remove the global from being recognized outside of
- replace-objects.c.
+ Since we still assign this global in config.c, we are not able to remove
+ the global scope of this variable and make it a static within
+ replace-object.c. This will happen in a later change which will also
+ prevent the variable from being read before it is initialized.
- Further, a future change will help prevent the variable from being read
- before it is initialized. Centralizing its access is an important first
- step.
+ Centralizing read access to the variable is an important first step.
@@ commit-graph.c: static struct commit_graph *alloc_commit_graph(void)
if (hashmap_get_size(&r->objects->replace_map->map))
return 0;
- ## environment.c ##
-@@ environment.c: const char *editor_program;
- const char *askpass_program;
- const char *excludes_file;
- enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
--int read_replace_refs = 1;
- enum eol core_eol = EOL_UNSET;
- int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
- char *check_roundtrip_encoding = "SHIFT-JIS";
-
## log-tree.c ##
@@ log-tree.c: static int add_ref_decoration(const char *refname, const struct object_id *oid,
@@ log-tree.c: static int add_ref_decoration(const char *refname, const struct obje
&original_oid)) {
## replace-object.c ##
-@@ replace-object.c: const struct object_id *do_lookup_replace_object(struct repository *r,
- die(_("replace depth too high for object %s"), oid_to_hex(oid));
- }
-
-+static int read_replace_refs = 1;
-+
- void disable_replace_refs(void)
+@@ replace-object.c: void disable_replace_refs(void)
{
read_replace_refs = 0;
}
@@ replace-object.h: void prepare_replace_object(struct repository *r);
const struct object_id *do_lookup_replace_object(struct repository *r,
const struct object_id *oid);
-+
+/*
+ * Some commands disable replace-refs unconditionally, and otherwise each
+ * repository could alter the core.useReplaceRefs config value.
3: 4c7dbeb8c6d ! 3: 8b7c7714c8c repository: create read_replace_refs setting
@@ Commit message
method, if necessary. This solves the problem of forgetting to check the config, as we will check it before returning this value.
+ Due to this encapsulation, the global can move to be static within
+ replace-object.c.
+
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,
@@ config.c: static int git_default_core_config(const char *var, const char *value,
return platform_core_config(var, value, cb);
}
+ ## environment.c ##
+@@ environment.c: const char *editor_program;
+ const char *askpass_program;
+ const char *excludes_file;
+ enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
+-int read_replace_refs = 1;
+ enum eol core_eol = EOL_UNSET;
+ int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
+ char *check_roundtrip_encoding = "SHIFT-JIS";
+
## replace-object.c ##
@@ replace-object.c: void prepare_replace_object(struct repository *r)
* replacement object's name (replaced recursively, if necessary).
@@ replace-object.c: void prepare_replace_object(struct repository *r)
*/
const struct object_id *do_lookup_replace_object(struct repository *r,
const struct object_id *oid)
+@@ replace-object.c: const struct object_id *do_lookup_replace_object(struct repository *r,
+ die(_("replace depth too high for object %s"), oid_to_hex(oid));
+ }
+
++/*
++ * This indicator determines whether replace references should be
++ * respected process-wide, regardless of which repository is being
++ * using at the time.
++ */
++static int read_replace_refs = 1;
++
+ void disable_replace_refs(void)
+ {
+ read_replace_refs = 0;
@@ replace-object.c: void disable_replace_refs(void)
int replace_refs_enabled(struct repository *r)
@@ repository.h: struct repo_settings {
int pack_use_bitmap_boundary_traversal;
+ /*
-+ * Has this repository have core.useReplaceRefs=true (on by
++ * Does this repository have core.useReplaceRefs=true (on by
+ * default)? This provides a repository-scoped version of this
+ * config, though it could be disabled process-wide via some Git
+ * builtins or the --no-replace-objects option. See
Signature · 1 line
gitgitgadget
PATCH v3 1/3 repository: create disable_replace_refs()
From: Derrick Stolee <[email protected]>
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 need to keep this read_replace_refs global forever, as we want to make sure that we never use replace refs throughout the life of the process if this method is called. Future changes may present a repository-scoped version of the variable to represent that repository's core.useReplaceRefs config value, but a zero-valued read_replace_refs will always override such a setting.
--- builtin/cat-file.c | 2 +- builtin/commit-graph.c | 2 +- builtin/fsck.c | 2 +- builtin/index-pack.c | 2 +- builtin/pack-objects.c | 2 +- builtin/prune.c | 2 +- builtin/replace.c | 2 +- builtin/unpack-objects.c | 2 +- builtin/upload-pack.c | 2 +- environment.c | 2 +- git.c | 2 +- replace-object.c | 5 +++++ replace-object.h | 8 ++++++++ 13 files changed, 24 insertions(+), 11 deletions(-)
builtin/cat-file.c +1 -1
@@ -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;
builtin/commit-graph.c +1 -1
@@ -324,7 +324,7 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)git_config (git_default_config,NULL ); - read_replace_refs =0 ; +disable_replace_refs (); save_commit_buffer =0 ; argc = parse_options(argc, argv, prefix, options,
builtin/fsck.c +1 -1
@@ -927,7 +927,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix) fetch_if_missing =0 ; errors_found =0 ; - read_replace_refs =0 ; +disable_replace_refs (); save_commit_buffer =0 ; argc =parse_options (argc, argv, prefix, fsck_opts, fsck_usage,0 );
builtin/index-pack.c +1 -1
@@ -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);
builtin/pack-objects.c +1 -1
@@ -4284,7 +4284,7 @@ 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 ; +disable_replace_refs (); sparse =git_env_bool ("GIT_TEST_PACK_SPARSE" ,-1 );if (the_repository->gitdir) {
builtin/prune.c +1 -1
@@ -164,7 +164,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix) expire =TIME_MAX ; save_commit_buffer =0 ; - read_replace_refs =0 ; +disable_replace_refs ();repo_init_revisions (the_repository, &revs, prefix); argc =parse_options (argc, argv, prefix, options, prune_usage,0 );
builtin/replace.c +1 -1
@@ -566,7 +566,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix) OPT_END() }; - read_replace_refs =0 ; +disable_replace_refs ();git_config (git_default_config,NULL ); argc =parse_options (argc, argv, prefix, options, git_replace_usage,0 );
builtin/unpack-objects.c +1 -1
@@ -609,7 +609,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)int i;struct object_id oid; - read_replace_refs =0 ; +disable_replace_refs ();git_config (git_default_config,NULL );
builtin/upload-pack.c +1 -1
@@ -36,7 +36,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix) };packet_trace_identity ("upload-pack" ); - read_replace_refs =0 ; +disable_replace_refs (); argc =parse_options (argc, argv, prefix, options, upload_pack_usage,0 );
environment.c +1 -1
@@ -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/" );
git.c +1 -1
@@ -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 ;
replace-object.c +5 -0
@@ -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 ; +}
replace-object.h +8 -0
@@ -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 */
Signature · 1 line
gitgitgadget
PATCH v3 2/3 replace-objects: create wrapper around setting
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
The 'read_replace_objects' constant is initialized by git_default_config (if core.useReplaceRefs is disabled) and within setup_git_env (if GIT_NO_REPLACE_OBJECTS) is set. To ensure that this variable cannot be set accidentally in other places, wrap it in a replace_refs_enabled() method.
Since we still assign this global in config.c, we are not able to remove the global scope of this variable and make it a static within replace-object.c. This will happen in a later change which will also prevent the variable from being read before it is initialized.
Centralizing read access to the variable is an important first step.
--- commit-graph.c | 4 +--- log-tree.c | 2 +- replace-object.c | 5 +++++ replace-object.h | 14 +++++++++++++- 4 files changed, 20 insertions(+), 5 deletions(-)
commit-graph.c +1 -3
@@ -203,14 +203,12 @@ static struct commit_graph *alloc_commit_graph(void)return g; } -extern int read_replace_refs; -static int commit_graph_compatible (struct repository *r) {if (!r->gitdir)return 0 ; -if (read_replace_refs) { +if (replace_refs_enabled (r)) {prepare_replace_object (r);if (hashmap_get_size (&r->objects->replace_map->map))return 0 ;
log-tree.c +1 -1
@@ -155,7 +155,7 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,if (starts_with (refname, git_replace_ref_base)) {struct object_id original_oid; -if (!read_replace_refs) +if (!replace_refs_enabled (the_repository))return 0 ;if (get_oid_hex (refname +strlen (git_replace_ref_base), &original_oid)) {
replace-object.c +5 -0
@@ -89,3 +89,8 @@ void disable_replace_refs(void) { read_replace_refs =0 ; } + +int replace_refs_enabled (struct repository *r) +{ +return read_replace_refs; +}
replace-object.h +13 -1
@@ -27,6 +27,18 @@ void prepare_replace_object(struct repository *r);const struct object_id *do_lookup_replace_object (struct repository *r,const struct object_id *oid); +/* +* Some commands disable replace-refs unconditionally, and otherwise each +* repository could alter the core.useReplaceRefs config value. +* +* Return 1 if and only if all of the following are true: +* +* a. disable_replace_refs() has not been called. +* b. GIT_NO_REPLACE_OBJECTS is unset or zero. +* c. the given repository does not have core.useReplaceRefs=false. +*/ +int replace_refs_enabled (struct repository *r); +/* * If object sha1 should be replaced, return the replacement object's * name (replaced recursively, if necessary). The return value is
@@ -41,7 +53,7 @@ const struct object_id *do_lookup_replace_object(struct repository *r,static inline const struct object_id *lookup_replace_object (struct repository *r,const struct object_id *oid) { -if (!read_replace_refs || +if (!replace_refs_enabled (r) || (r->objects->replace_map_initialized && r->objects->replace_map->map.tablesize ==0 ))return oid;
Signature · 1 line
gitgitgadget
PATCH v3 3/3 repository: create read_replace_refs setting
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
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.
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.
Due to this encapsulation, the global can move to be static within replace-object.c.
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.
'git grep --recurse-submodules' is such a command that recurses into submodules in-process. We can demonstrate the granularity of this config value via a test in t7814.
--- config.c | 5 ---- environment.c | 1 - replace-object.c | 20 +++++++++++++-- replace-object.h | 8 ------ repo-settings.c | 1 + repository.h | 9 +++++++ t/t7814-grep-recurse-submodules.sh | 40 ++++++++++++++++++++++++++++++ 7 files changed, 68 insertions(+), 16 deletions(-)
config.c +0 -5
@@ -1838,11 +1838,6 @@ static int git_default_core_config(const char *var, const char *value, void *cb)return 0 ; } -if (!strcmp (var,"core.usereplacerefs" )) { - read_replace_refs =git_config_bool (var, value); -return 0 ; - } -/* Add other config variables here and to Documentation/config.txt. */ return platform_core_config (var, value, cb); }
environment.c +0 -1
@@ -63,7 +63,6 @@ const char *editor_program;const char *askpass_program;const char *excludes_file;enum auto_crlf auto_crlf =AUTO_CRLF_FALSE ; -int read_replace_refs =1 ;enum eol core_eol =EOL_UNSET ;int global_conv_flags_eol =CONV_EOL_RNDTRP_WARN ;char *check_roundtrip_encoding ="SHIFT-JIS" ;
replace-object.c +18 -2
@@ -64,7 +64,7 @@ void prepare_replace_object(struct repository *r)* replacement object's name (replaced recursively, if necessary). * The return value is either oid or a pointer to a * permanently-allocated value. This function always respects replace -* references, regardless of the value of read_replace_refs. +* references, regardless of the value of r->settings.read_replace_refs. */ const struct object_id *do_lookup_replace_object (struct repository *r,const struct object_id *oid)
@@ -85,6 +85,13 @@ const struct object_id *do_lookup_replace_object(struct repository *r,die (_ ("replace depth too high for object %s" ),oid_to_hex (oid)); } +/* +* This indicator determines whether replace references should be +* respected process-wide, regardless of which repository is being +* using at the time. +*/ +static int read_replace_refs =1 ; +void disable_replace_refs (void ) { read_replace_refs =0 ;
@@ -92,5 +99,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 ; }
replace-object.h +0 -8
@@ -5,14 +5,6 @@#include "repository.h" #include "object-store.h" -/* -* 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. -*/ -extern int read_replace_refs; -struct replace_object {struct oidmap_entry original;struct object_id replacement;
repo-settings.c +1 -0
@@ -67,6 +67,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 );/* * The GIT_TEST_MULTI_PACK_INDEX variable is special in that
repository.h +9 -0
@@ -39,6 +39,15 @@ struct repo_settings {int pack_read_reverse_index;int pack_use_bitmap_boundary_traversal; +/* +* Does this repository have core.useReplaceRefs=true (on by +* default)? This provides a repository-scoped version of this +* config, though it could be disabled process-wide via some Git +* builtins or the --no-replace-objects option. See +* replace_refs_enabled() for more details. +*/ +int read_replace_refs; +struct fsmonitor_settings *fsmonitor;/* lazily loaded */ int index_version;
t/t7814-grep-recurse-submodules.sh +40 -0
@@ -594,4 +594,44 @@ test_expect_success 'grep partially-cloned submodule' ' ) ' +test_expect_success 'check scope of core.useReplaceRefs' ' + git init base && + git init base/sub && + + echo A >base/a && + echo B >base/b && + echo C >base/sub/c && + echo D >base/sub/d && + + git -C base/sub add c d && + git -C base/sub commit -m "Add files" && + + git -C base submodule add ./sub && + git -C base add a b sub && + git -C base commit -m "Add files and submodule" && + + A=$(git -C base rev-parse HEAD:a) && + B=$(git -C base rev-parse HEAD:b) && + C=$(git -C base/sub rev-parse HEAD:c) && + D=$(git -C base/sub rev-parse HEAD:d) && + + git -C base replace $A $B && + git -C base/sub replace $C $D && + + test_must_fail git -C base grep --cached --recurse-submodules A && + test_must_fail git -C base grep --cached --recurse-submodules C && + + git -C base config core.useReplaceRefs false && + git -C base grep --recurse-submodules A && + test_must_fail git -C base grep --cached --recurse-submodules C && + + git -C base/sub config core.useReplaceRefs false && + git -C base grep --cached --recurse-submodules A && + git -C base grep --cached --recurse-submodules C && + + git -C base config --unset core.useReplaceRefs && + test_must_fail git -C base grep --cached --recurse-submodules A && + git -C base grep --cached --recurse-submodules C +' + test_done
Signature · 1 line
gitgitgadget
Quoted 10 lines · Derrick Stolee via GitGitGadget
Updates in v3
=============Thanks for the review on v2!
* The removal of the global from
environment.cis delayed to patch 3 becauseconfig.cstill assigns the value in patch 2.
* The comment for the member in therepo_settingsstruct is modified for better grammar.
The changes in this version were small (and easy to review via range-diff); everything looks good to me. Thanks for the updates!
Quoted 9 lines · Derrick Stolee via GitGitGadget
Updates in v3
=============Thanks for the review on v2!
* The removal of the global from
environment.cis delayed to patch 3 becauseconfig.cstill assigns the value in patch 2.
* The comment for the member in therepo_settingsstruct is modified for better grammar.
Thanks. Will queue. Let's merge it down to 'next'.