On Fri, May 26, 2023 at 11:43 AM Derrick Stolee via GitGitGadget <gitgitgadget@xxxxxxxxx> wrote: > > (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/pull.1530.git.1683745654800.gitgitgadget@xxxxxxxxx/ > > 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 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.
