| Message ID | 5fc2f923d9e6aa13781d7d6567c9bd38a9dd1f0e.1685126618.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Create stronger guard rails on replace refs | expand |
Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <derrickstolee@github.com> > > 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. > > Signed-off-by: Derrick Stolee <derrickstolee@github.com> > --- > 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(-) > > diff --git a/commit-graph.c b/commit-graph.c > index 43558b4d9b0..95873317bf7 100644 > --- a/commit-graph.c > +++ b/commit-graph.c > @@ -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; 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. > diff --git a/replace-object.c b/replace-object.c > index ceec81c940c..cf91c3ba456 100644 > --- a/replace-object.c > +++ b/replace-object.c > @@ -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; > +} > diff --git a/replace-object.h b/replace-object.h > index 7786d4152b0..b141075023e 100644 > --- a/replace-object.h > +++ b/replace-object.h > @@ -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); 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.
On 6/1/2023 12:35 PM, Victoria Dye wrote: > Derrick Stolee via GitGitGadget wrote: >> From: Derrick Stolee <derrickstolee@github.com> >> diff --git a/replace-object.h b/replace-object.h >> index 7786d4152b0..b141075023e 100644 >> --- a/replace-object.h >> +++ b/replace-object.h >> @@ -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); > > 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
On Thu, Jun 1, 2023 at 12:50 PM Derrick Stolee <derrickstolee@github.com> wrote: > > On 6/1/2023 12:35 PM, Victoria Dye wrote: > > Derrick Stolee via GitGitGadget wrote: > >> From: Derrick Stolee <derrickstolee@github.com> > > >> diff --git a/replace-object.h b/replace-object.h > >> index 7786d4152b0..b141075023e 100644 > >> --- a/replace-object.h > >> +++ b/replace-object.h > >> @@ -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); > > > > 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. 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. :-)
On 6/2/2023 9:47 PM, Elijah Newren wrote: > On Thu, Jun 1, 2023 at 12:50 PM Derrick Stolee <derrickstolee@github.com> wrote: >> >> On 6/1/2023 12:35 PM, Victoria Dye wrote: >>> Derrick Stolee via GitGitGadget wrote: >>>> From: Derrick Stolee <derrickstolee@github.com> >> >>>> diff --git a/replace-object.h b/replace-object.h >>>> index 7786d4152b0..b141075023e 100644 >>>> --- a/replace-object.h >>>> +++ b/replace-object.h >>>> @@ -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); >>> >>> 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. > > 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. :-) 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
diff --git a/commit-graph.c b/commit-graph.c index 43558b4d9b0..95873317bf7 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -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; diff --git a/environment.c b/environment.c index 3b4d87c322f..e198b48081a 100644 --- a/environment.c +++ b/environment.c @@ -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"; diff --git a/log-tree.c b/log-tree.c index 143b86fecf9..86212af3626 100644 --- a/log-tree.c +++ b/log-tree.c @@ -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)) { diff --git a/replace-object.c b/replace-object.c index ceec81c940c..cf91c3ba456 100644 --- a/replace-object.c +++ b/replace-object.c @@ -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; +} diff --git a/replace-object.h b/replace-object.h index 7786d4152b0..b141075023e 100644 --- a/replace-object.h +++ b/replace-object.h @@ -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;