[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH 0/3] Create stronger guard rails on replace refs
From:       Elijah Newren <newren () gmail ! com>
Date:       2023-05-31 5:11:57
Message-ID: CABPp-BFj0+3y7C4FKy9qzVjHWP2r6-=azW8g0j3WGX-zYpaCQQ () mail ! gmail ! com
[Download RAW message or body]

On Fri, May 26, 2023 at 11:43 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> 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@gmail.com/
>
> 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.
[prev in list] [next in list] [prev in thread] [next in thread]