From: Victoria Dye <vdye@github.com>
Date: Thu, 01 Jun 2023 16:35:25 GMT
Subject: Re: [PATCH 1/3] repository: create disable_replace_refs()
Message-ID: <caa3540c-0e7c-f428-e635-0cf54fc04eff@github.com>
In-Reply-To: <56544abc15d1fce6fb4a0946e709470af9225395.1685126618.git.gitgitgadget@gmail.com>

Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
> 
> Several builtins depend on being able to disable the replace references
> so we actually operate on each object individually. These currently do
> so by directly mutating the 'read_replace_refs' global.
> 
> A future change will move this global into a different place, so it will
> be necessary to change all of these lines. However, we can simplify that
> transition by abstracting the purpose of these global assignments with a
> method call.
> 
> We will never scope this to an in-memory repository as we want to make
> sure that we never use replace refs throughout the life of the process
> if this method is called.
> 

Although unfortunate (it would be nice to remove the global), this makes
sense. Disabling replace refs needs to be process-wide, and manually
propagating a repository setting to other repositories would be awkward
and prone to error.

All of my questions on this patch ("why were the 'disable_replace_refs()'
calls added later in the function than the original 'read_replace_refs =
0'?" and "why was '#include "environment.h"' added in 'repo-settings.c'?")
were asked [1] and answered [2] already. Beyond those two points, this patch
looks good!

[1] https://lore.kernel.org/git/CABPp-BFzA0yVecHK1DEGMpAhewm7oyqEim7BCw7-DTKpUzWnpw@mail.gmail.com/
[2] https://lore.kernel.org/git/ae89feda-0a76-29d7-14ce-662214414638@github.com/

> +/*
> + * Some commands override config and environment settings for using
> + * replace references. Use this method to disable the setting and ensure
> + * those other settings will not override this choice. This applies
> + * globally to all in-process repositories.
> + */
> +void disable_replace_refs(void);
> +

Thanks for including the function documentation. It concisely explains the
purpose of 'disable_replace_refs()' and helps clarify how replace refs are
treated in Git. 

>  #endif /* REPLACE_OBJECT_H */