Subject: Re: [PATCH 3/3] repository: create read_replace_refs setting

From: Victoria Dye <[email-masked]>
Date: Thu, 1 Jun 2023 09:36:12 -0700

Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <[email-masked]>
> 
> The 'read_replace_refs' global specifies whether or not we should
> respect the references of the form 'refs/replace/<oid>' to replace which
> object we look up when asking for '<oid>'. This global has caused issues
> when it is not initialized properly, such as in b6551feadfd (merge-tree:
> load default git config, 2023-05-10).
> 
> To make this more robust, move its config-based initialization out of
> git_default_config and into prepare_repo_settings(). This provides a
> repository-scoped version of the 'read_replace_refs' global.

As you noted in [1], this could be clearer. I think the most confusing part
is referring to it as "a repository-scoped version of the...global" because
it implies that the global and repo-scoped setting do the same thing/take
the same precedence (when, in reality, if replace refs are disabled
globally, the config doesn't do anything). Maybe something like this would
make that clearer?

"This provides a repository-scoped configuration that's only used if replace
refs are not already disabled process-wide with the global
'read_replace_refs'."

[1] https://lore.kernel.org/git/[email-masked]/

> 
> The global still has its purpose: it is disabled process-wide by the
> GIT_NO_REPLACE_OBJECTS environment variable or by a call to
> disable_replace_refs() in some specific Git commands.
> 
> Since we already encapsulated the use of the constant inside
> replace_refs_enabled(), we can perform the initialization inside that
> method, if necessary. This solves the problem of forgetting to check the
> config, as we will check it before returning this value.
> 
> There is an interesting behavior change possible here: we now have a
> repository-scoped understanding of this config value. Thus, if there was
> a command that recurses into submodules and might follow replace refs,
> then it would now respect the core.useReplaceRefs config value in each
> repository.
> 
> Unfortunately, the existing processes that recurse into submodules do
> not appear to follow object IDs to their contents, so this behavior
> change is not visible in the current implementation. It is something
> valuable for future behavior changes.

AFAIK, the only '--recurse-submodules' commands that recurse in-process are
'ls-files' and 'grep'. However, 'grep' does call 'parse_object_or_die()',
which (further down in the call stack) calls 'lookup_replace_object()'.
Maybe I'm misreading and the replaced object isn't actually used, but could
'git grep --recurse-submodules' be used to test this?

> @@ -94,5 +94,14 @@ void disable_replace_refs(void)
>  
>  int replace_refs_enabled(struct repository *r)
>  {
> -	return read_replace_refs;
> +	if (!read_replace_refs)
> +		return 0;
> +
> +	if (r->gitdir) {
> +		prepare_repo_settings(r);
> +		return r->settings.read_replace_refs;
> +	}
> +
> +	/* repository has no objects or refs. */
> +	return 0;
>  }

This implementation matches the intent outlined in this patch/the cover
letter:

- if replace refs are disabled process-wide, always return 0
- if the gitdir is present, return the value of 'core.usereplacerefs'
- if there's no gitdir, there's no repository set up (and therefore no
  config to read/objects to replace), so return 0

I was a bit unsure about whether 'r->gitdir' was the right check to make,
but it's consistent with other gates to 'prepare_repo_settings()' (e.g.
those added in 059fda19021 (checkout/fetch/pull/pack-objects: allow `-h`
outside a repository, 2022-02-08)), so I'm happy with it.

> diff --git a/repo-settings.c b/repo-settings.c
> index 1df0320bf33..5a7c990300d 100644
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -68,6 +68,7 @@ void prepare_repo_settings(struct repository *r)
>  	repo_cfg_bool(r, "pack.usebitmapboundarytraversal",
>  		      &r->settings.pack_use_bitmap_boundary_traversal,
>  		      r->settings.pack_use_bitmap_boundary_traversal);
> +	repo_cfg_bool(r, "core.usereplacerefs", &r->settings.read_replace_refs, 1);

This defaults to enabling replace refs, consistent with the (intended)
behavior prior to this series. Good!

>  
>  	/*
>  	 * The GIT_TEST_MULTI_PACK_INDEX variable is special in that
> diff --git a/repository.h b/repository.h
> index c42f7ab6bdc..13fefa540bc 100644
> --- a/repository.h
> +++ b/repository.h
> @@ -39,6 +39,14 @@ struct repo_settings {
>  	int pack_read_reverse_index;
>  	int pack_use_bitmap_boundary_traversal;
>  
> +	/*
> +	 * Do replace refs need to be checked this run?  This variable is
> +	 * initialized to true unless --no-replace-object is used or
> +	 * $GIT_NO_REPLACE_OBJECTS is set, but is set to false by some
> +	 * commands that do not want replace references to be active.
> +	 */
> +	int read_replace_refs;

I don't think this comment is accurate anymore, since the repo-scoped
'read_replace_refs' value is determined *only* by the 'core.usereplacerefs'
config. It's 'replace_refs_enabled()' that makes the overall determination
(taking into account 'GIT_NO_REPLACE_OBJECTS'/'--no-replace-object').

> +
>  	struct fsmonitor_settings *fsmonitor; /* lazily loaded */
>  
>  	int index_version;