On 6/2/2023 9:47 PM, Elijah Newren wrote: > On Thu, Jun 1, 2023 at 12:50 PM Derrick Stolee <derrickstolee@xxxxxxxxxx> wrote: >> >> On 6/1/2023 12:35 PM, Victoria Dye wrote: >>> Derrick Stolee via GitGitGadget wrote: >>>> From: Derrick Stolee <derrickstolee@xxxxxxxxxx> >> >>>> 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
