[0/3] Create stronger guard rails on replace refs

Message

Derrick Stolee via GitGitGadget May 26, 2023, 6:43 p.m. UTC

(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/[e-mail]/

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

Comments

Elijah Newren May 31, 2023, 5:11 a.m. UTC | #1

On Fri, May 26, 2023 at 11:43 AM Derrick Stolee via GitGitGadget
<[e-mail]> 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/[e-mail]/
>
> 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.