Re: [PATCH v5 07/11] refs: move parsing of "core.logAllRefUpdates" back into ref stores

[Justin Tobler <jltobler@gmail.com>]

On 26/06/22 10:28AM, Patrick Steinhardt wrote:
> In cc42c88945 (refs: extract out reflog config to generic layer,
> 2026-05-04) we have refactored how we parse "core.logAllRefUpdates" so
> that it happens in the generic layer. Unfortunately, this has worsened a
> preexisting issue where we may recurse when creating the reference store
> because of a chicken-and-egg problem between parsing the configuration
> and evaluating "onbranch" conditions.

Ok so IIUC, parsing "core.logAllRefUpdates" in the generic layer forces
us to read the config earlier. This is problematic though since the
refstore has not been initialized yet which we need to evaluate
"onbranch" conditions.

> Prepare for a fix by essentially reverting that change so that we handle
> this setting in the respective backends again. The backends are already
> parsing other configuration anyway, so by moving the logic back in there
> we can ensure that all backend configuration is parsed the same way.

Makes sense.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/checkout.c      |  7 +++++--
>  refs.c                  | 10 +++++++++-
>  refs.h                  |  9 +++++++++
>  refs/files-backend.c    | 20 +++++++++++++++++---
>  refs/refs-internal.h    |  6 ------
>  refs/reftable-backend.c | 20 +++++++++++---------
>  repo-settings.c         | 16 ----------------
>  repo-settings.h         |  9 ---------
>  setup.c                 |  7 ++++++-
>  9 files changed, 57 insertions(+), 47 deletions(-)
> 
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index b78b3a1d16..aee84ca897 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -952,10 +952,13 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
>  	const char *old_desc, *reflog_msg;
>  	if (opts->new_branch) {
>  		if (opts->new_orphan_branch) {
> -			enum log_refs_config log_all_ref_updates =
> -				repo_settings_get_log_all_ref_updates(the_repository);
> +			enum log_refs_config log_all_ref_updates = LOG_REFS_UNSET;
> +			const char *value;
>  			char *refname;
>  
> +			if (!repo_config_get_string_tmp(the_repository, "core.logallrefupdates", &value))
> +				log_all_ref_updates = refs_parse_log_all_ref_updates_config(value);
> +
>  			refname = mkpathdup("refs/heads/%s", opts->new_orphan_branch);
>  			if (opts->new_branch_log &&
>  			    !should_autocreate_reflog(log_all_ref_updates, refname)) {
> diff --git a/refs.c b/refs.c
> index d3caa9a633..5b773b1c15 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1053,6 +1053,15 @@ static char *normalize_reflog_message(const char *msg)
>  	return strbuf_detach(&sb, NULL);
>  }
>  
> +enum log_refs_config refs_parse_log_all_ref_updates_config(const char *value)
> +{
> +	if (value && !strcasecmp(value, "always"))
> +		return LOG_REFS_ALWAYS;
> +	else if (git_config_bool("core.logallrefupdates", value))
> +		return LOG_REFS_NORMAL;
> +	return LOG_REFS_NONE;
> +}

This function replaces `repo_settings_get_log_all_ref_updates()`. I
assume we just wanted a slightly more simple function where the only
concern was parsing the `core.logallrefupdates` value.

> +
>  int should_autocreate_reflog(enum log_refs_config log_all_ref_updates,
>  			     const char *refname)
>  {
> @@ -2327,7 +2336,6 @@ static struct ref_store *ref_store_init(struct repository *repo,
>  	struct ref_store *refs;
>  	struct ref_store_init_options opts = {
>  		.access_flags = flags,
> -		.log_all_ref_updates = repo_settings_get_log_all_ref_updates(repo),

This config is no longer handled in the generic layer.

[snip]
> diff --git a/setup.c b/setup.c
> index 79125db565..0c6efb0560 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -2584,10 +2584,15 @@ static int create_default_files(struct repository *repo,
>  	if (is_bare_repository())
>  		repo_config_set(repo, "core.bare", "true");
>  	else {
> +		const char *value;
> +
>  		repo_config_set(repo, "core.bare", "false");
> +
>  		/* allow template config file to override the default */
> -		if (repo_settings_get_log_all_ref_updates(repo) == LOG_REFS_UNSET)
> +		if (repo_config_get_string_tmp(repo, "core.logallrefupdates", &value) ||
> +		    refs_parse_log_all_ref_updates_config(value) == LOG_REFS_UNSET)

Huh, can `refs_parse_log_all_ref_updates_config()` even return
LOG_REFS_UNSET?

-Justin