From db2f2285fe1d48f5f8e4baf6d48fd9e6c26de537 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 18 Jun 2026 08:54:30 +0200 Subject: [PATCH 1/8] setup: inline `check_and_apply_repository_format()` We have two callsites of `check_and_apply_repository_format()`. In a subsequent commit we'll want to adapt one of those callsites to change the order in which we read and apply the repository format, at which point the helper function will not really be a good fit for us anymore. Inline the function to both of the callsites. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- setup.c | 47 ++++++++++++++++------------------------------- 1 file changed, 16 insertions(+), 31 deletions(-) diff --git a/setup.c b/setup.c index b4652651df..a9db1f2c23 100644 --- a/setup.c +++ b/setup.c @@ -1788,32 +1788,6 @@ int apply_repository_format(struct repository *repo, return 0; } -/* - * Check the repository format version in the path found in repo_get_git_dir(repo), - * and die if it is a version we don't understand. Generally one would - * set_git_dir() before calling this, and use it only for "are we in a valid - * repo?". - * - * If successful and fmt is not NULL, fill fmt with data. - */ -static void check_and_apply_repository_format(struct repository *repo, - struct repository_format *fmt, - enum apply_repository_format_flags flags) -{ - struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT; - struct strbuf err = STRBUF_INIT; - - if (!fmt) - fmt = &repo_fmt; - - check_repository_format_gently(repo_get_git_dir(repo), fmt, NULL); - if (apply_repository_format(repo, fmt, flags, &err) < 0) - die("%s", err.buf); - startup_info->have_repository = 1; - - clear_repository_format(&repo_fmt); -} - const char *enter_repo(struct repository *repo, const char *path, unsigned flags) { static struct strbuf validated_path = STRBUF_INIT; @@ -1887,9 +1861,17 @@ const char *enter_repo(struct repository *repo, const char *path, unsigned flags } if (is_git_directory(".")) { + struct repository_format fmt = REPOSITORY_FORMAT_INIT; + struct strbuf err = STRBUF_INIT; + set_git_dir(repo, ".", 0); - check_and_apply_repository_format(repo, NULL, - APPLY_REPOSITORY_FORMAT_HONOR_ENV); + check_repository_format_gently(".", &fmt, NULL); + if (apply_repository_format(repo, &fmt, APPLY_REPOSITORY_FORMAT_HONOR_ENV, &err) < 0) + die("%s", err.buf); + startup_info->have_repository = 1; + + clear_repository_format(&fmt); + strbuf_release(&err); return path; } @@ -2820,6 +2802,7 @@ int init_db(struct repository *repo, int exist_ok = flags & INIT_DB_EXIST_OK; char *original_git_dir = real_pathdup(git_dir, 1); struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT; + struct strbuf err = STRBUF_INIT; if (real_git_dir) { struct stat st; @@ -2846,9 +2829,10 @@ int init_db(struct repository *repo, * config file, so this will not fail. What we are catching * is an attempt to reinitialize new repository with an old tool. */ - check_and_apply_repository_format(repo, &repo_fmt, - APPLY_REPOSITORY_FORMAT_HONOR_ENV); - + check_repository_format_gently(repo_get_git_dir(repo), &repo_fmt, NULL); + if (apply_repository_format(repo, &repo_fmt, APPLY_REPOSITORY_FORMAT_HONOR_ENV, &err) < 0) + die("%s", err.buf); + startup_info->have_repository = 1; repository_format_configure(repo, &repo_fmt, hash, ref_storage_format); /* @@ -2904,6 +2888,7 @@ int init_db(struct repository *repo, } clear_repository_format(&repo_fmt); + strbuf_release(&err); free(original_git_dir); return 0; } From 700c4bc29869f0757528d3a980f93a52121c6e25 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 18 Jun 2026 08:54:31 +0200 Subject: [PATCH 2/8] setup: stop applying repository format twice When discovering the repository in "setup.c" we apply the final repository format multiple times: - Once via `repository_format_configure()`, where we apply the hash algorithm and ref storage format to both `struct repository_format` and `struct repository`. - And once via `apply_repository_format()`, where we apply these two settings from `struct repository_format` to `struct repository`. With the current flow both of these are in fact necessary. But this is only because we call `repository_format_configure()` after we have called `apply_repository_format()`. Consequently, if we only changed the repository format in `repository_format_configure()` it would never propagate to the repository. Refactor the code so that we first configure the repository format before applying it to the repository so that we can stop setting the hash and reference storage format multiple times. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- setup.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/setup.c b/setup.c index a9db1f2c23..2748155964 100644 --- a/setup.c +++ b/setup.c @@ -2710,8 +2710,7 @@ out: return ret; } -static void repository_format_configure(struct repository *repo, - struct repository_format *repo_fmt, +static void repository_format_configure(struct repository_format *repo_fmt, int hash, enum ref_storage_format ref_format) { struct default_format_config cfg = { @@ -2748,7 +2747,6 @@ static void repository_format_configure(struct repository *repo, } else if (cfg.hash != GIT_HASH_UNKNOWN) { repo_fmt->hash_algo = cfg.hash; } - repo_set_hash_algo(repo, repo_fmt->hash_algo); env = getenv("GIT_DEFAULT_REF_FORMAT"); if (repo_fmt->version >= 0 && @@ -2786,9 +2784,6 @@ static void repository_format_configure(struct repository *repo, free(backend); } - - repo_set_ref_storage_format(repo, repo_fmt->ref_storage_format, - repo_fmt->ref_storage_payload); } int init_db(struct repository *repo, @@ -2830,10 +2825,10 @@ int init_db(struct repository *repo, * is an attempt to reinitialize new repository with an old tool. */ check_repository_format_gently(repo_get_git_dir(repo), &repo_fmt, NULL); + repository_format_configure(&repo_fmt, hash, ref_storage_format); if (apply_repository_format(repo, &repo_fmt, APPLY_REPOSITORY_FORMAT_HONOR_ENV, &err) < 0) die("%s", err.buf); startup_info->have_repository = 1; - repository_format_configure(repo, &repo_fmt, hash, ref_storage_format); /* * Ensure `core.hidedotfiles` is processed. This must happen after we From 8b937b633794158884c25070fc508d1a72cf8223 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 18 Jun 2026 08:54:32 +0200 Subject: [PATCH 3/8] setup: don't apply "GIT_REFERENCE_BACKEND" without a repository When discovering a repository we eventually also apply the "GIT_REFERENCE_BACKEND" environment variable to the repository. There's two problems with that: - We do this unconditionally, which is rather pointless: we really only have to configure the repository when we have found one. - We have already applied the repository format at that point in time, so we need to manually reapply it. Move the logic around so that we only apply the environment variable when a repository was discovered. This also allows us to drop the explcit call to `repo_set_ref_storage_format()` because we now adjust the format before we apply it via `apply_repository_format()`. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- setup.c | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/setup.c b/setup.c index 2748155964..79125db565 100644 --- a/setup.c +++ b/setup.c @@ -1906,7 +1906,6 @@ const char *setup_git_directory_gently(struct repository *repo, int *nongit_ok) static struct strbuf cwd = STRBUF_INIT; struct strbuf dir = STRBUF_INIT, gitdir = STRBUF_INIT, report = STRBUF_INIT; const char *prefix = NULL; - const char *ref_backend_uri; struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT; /* @@ -2032,6 +2031,25 @@ const char *setup_git_directory_gently(struct repository *repo, int *nongit_ok) if (startup_info->have_repository) { struct strbuf err = STRBUF_INIT; + const char *ref_backend_uri; + + /* + * The env variable should override the repository config + * for 'extensions.refStorage'. + */ + ref_backend_uri = getenv(GIT_REFERENCE_BACKEND_ENVIRONMENT); + if (ref_backend_uri) { + char *format; + + free(repo_fmt.ref_storage_payload); + + parse_reference_uri(ref_backend_uri, &format, &repo_fmt.ref_storage_payload); + repo_fmt.ref_storage_format = ref_storage_format_by_name(format); + if (repo_fmt.ref_storage_format == REF_STORAGE_FORMAT_UNKNOWN) + die(_("unknown ref storage format: '%s'"), format); + + free(format); + } if (apply_repository_format(repo, &repo_fmt, APPLY_REPOSITORY_FORMAT_HONOR_ENV, &err) < 0) @@ -2057,25 +2075,6 @@ const char *setup_git_directory_gently(struct repository *repo, int *nongit_ok) setenv(GIT_PREFIX_ENVIRONMENT, "", 1); } - /* - * The env variable should override the repository config - * for 'extensions.refStorage'. - */ - ref_backend_uri = getenv(GIT_REFERENCE_BACKEND_ENVIRONMENT); - if (ref_backend_uri) { - char *backend, *payload; - enum ref_storage_format format; - - parse_reference_uri(ref_backend_uri, &backend, &payload); - format = ref_storage_format_by_name(backend); - if (format == REF_STORAGE_FORMAT_UNKNOWN) - die(_("unknown ref storage format: '%s'"), backend); - repo_set_ref_storage_format(repo, format, payload); - - free(backend); - free(payload); - } - setup_original_cwd(repo); strbuf_release(&dir); From 88a2361bd6e68dd78cb889e0486e5e6c192bb740 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 18 Jun 2026 08:54:33 +0200 Subject: [PATCH 4/8] refs: unregister reference stores from "chdir_notify" When creating reference stores we register them with the "chdir_notify" subsystem. This is required because some of the paths we track may be relative paths, so we have to reparent them in case the current working directory changes. But while we register the reference stores, we never unregister them. This can have multiple outcomes: - For a repository's main reference database we essentially keep the pointer alive. We never free that database, either, and our leak checker doesn't notice because it's still registered. - For submodule and worktree reference databases we do eventually free them in `repo_clear()`, so we may keep pointers to free'd memory registered. We never notice though as we don't tend to chdir around in the middle of the process. We never noticed either of these symptoms, but they are obviously bad. Partially fix those issues by unregistering the reference stores when releasing them. The leak of the main reference database will be fixed in a subsequent commit. Note that this requires us to use `chdir_notify_register()` instead of `chdir_notify_reparent()`, as there is no infrastructure to unregister the latter. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs/files-backend.c | 22 +++++++++++++++++++--- refs/packed-backend.c | 16 +++++++++++++++- refs/reftable-backend.c | 16 +++++++++++++++- 3 files changed, 49 insertions(+), 5 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index a4c7858787..296981584b 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -100,6 +100,23 @@ static void clear_loose_ref_cache(struct files_ref_store *refs) } } +static void files_ref_store_reparent(const char *name UNUSED, + const char *old_cwd, + const char *new_cwd, + void *payload) +{ + struct files_ref_store *refs = payload; + char *tmp; + + tmp = reparent_relative_path(old_cwd, new_cwd, refs->base.gitdir); + free(refs->base.gitdir); + refs->base.gitdir = tmp; + + tmp = reparent_relative_path(old_cwd, new_cwd, refs->gitcommondir); + free(refs->gitcommondir); + refs->gitcommondir = tmp; +} + /* * Create a new submodule ref cache and add it to the internal * set of caches. @@ -128,9 +145,7 @@ static struct ref_store *files_ref_store_init(struct repository *repo, repo_config_get_bool(repo, "core.prefersymlinkrefs", &refs->prefer_symlink_refs); - chdir_notify_reparent("files-backend $GIT_DIR", &refs->base.gitdir); - chdir_notify_reparent("files-backend $GIT_COMMONDIR", - &refs->gitcommondir); + chdir_notify_register(NULL, files_ref_store_reparent, refs); strbuf_release(&refdir); @@ -182,6 +197,7 @@ static void files_ref_store_release(struct ref_store *ref_store) free(refs->gitcommondir); ref_store_release(refs->packed_ref_store); free(refs->packed_ref_store); + chdir_notify_unregister(NULL, files_ref_store_reparent, refs); } static void files_reflog_path(struct files_ref_store *refs, diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 0acde48c45..499cb55dfa 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -211,6 +211,19 @@ static size_t snapshot_hexsz(const struct snapshot *snapshot) return snapshot->refs->base.repo->hash_algo->hexsz; } +static void packed_ref_store_reparent(const char *name UNUSED, + const char *old_cwd, + const char *new_cwd, + void *payload) +{ + struct packed_ref_store *refs = payload; + char *tmp; + + tmp = reparent_relative_path(old_cwd, new_cwd, refs->path); + free(refs->path); + refs->path = tmp; +} + /* * Since packed-refs is only stored in the common dir, don't parse the * payload and rely on the files-backend to set 'gitdir' correctly. @@ -229,7 +242,7 @@ struct ref_store *packed_ref_store_init(struct repository *repo, strbuf_addf(&sb, "%s/packed-refs", gitdir); refs->path = strbuf_detach(&sb, NULL); - chdir_notify_reparent("packed-refs", &refs->path); + chdir_notify_register(NULL, packed_ref_store_reparent, refs); return ref_store; } @@ -274,6 +287,7 @@ static void packed_ref_store_release(struct ref_store *ref_store) clear_snapshot(refs); rollback_lock_file(&refs->lock); delete_tempfile(&refs->tempfile); + chdir_notify_unregister(NULL, packed_ref_store_reparent, refs); free(refs->path); } diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 4ae22922de..8c93070677 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -365,6 +365,19 @@ static int reftable_be_config(const char *var, const char *value, return 0; } +static void reftable_be_reparent(const char *name UNUSED, + const char *old_cwd, + const char *new_cwd, + void *payload) +{ + struct reftable_ref_store *refs = payload; + char *tmp; + + tmp = reparent_relative_path(old_cwd, new_cwd, refs->base.gitdir); + free(refs->base.gitdir); + refs->base.gitdir = tmp; +} + static struct ref_store *reftable_be_init(struct repository *repo, const char *payload, const char *gitdir, @@ -447,7 +460,7 @@ static struct ref_store *reftable_be_init(struct repository *repo, goto done; } - chdir_notify_reparent("reftables-backend $GIT_DIR", &refs->base.gitdir); + chdir_notify_register(NULL, reftable_be_reparent, refs); done: assert(refs->err != REFTABLE_API_ERROR); @@ -474,6 +487,7 @@ static void reftable_be_release(struct ref_store *ref_store) free(be); } strmap_clear(&refs->worktree_backends, 0); + chdir_notify_unregister(NULL, reftable_be_reparent, refs); } static int reftable_be_create_on_disk(struct ref_store *ref_store, From 825f7eb80ea14845593906f861bdb9181961eb18 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 18 Jun 2026 08:54:34 +0200 Subject: [PATCH 5/8] chdir-notify: drop unused `chdir_notify_reparent()` With the preceding commit we've removed all callers of `chdir_notify_reparent()`, so the function is unused now. Drop it. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- chdir-notify.c | 26 -------------------------- chdir-notify.h | 6 +----- 2 files changed, 1 insertion(+), 31 deletions(-) diff --git a/chdir-notify.c b/chdir-notify.c index f8bfe3cbef..1237a45e2e 100644 --- a/chdir-notify.c +++ b/chdir-notify.c @@ -43,32 +43,6 @@ void chdir_notify_unregister(const char *name, chdir_notify_callback cb, } } -static void reparent_cb(const char *name, - const char *old_cwd, - const char *new_cwd, - void *data) -{ - char **path = data; - char *tmp = *path; - - if (!tmp) - return; - - *path = reparent_relative_path(old_cwd, new_cwd, tmp); - free(tmp); - - if (name) { - trace_printf_key(&trace_setup_key, - "setup: reparent %s to '%s'", - name, *path); - } -} - -void chdir_notify_reparent(const char *name, char **path) -{ - chdir_notify_register(name, reparent_cb, path); -} - int chdir_notify(const char *new_cwd) { struct strbuf old_cwd = STRBUF_INIT; diff --git a/chdir-notify.h b/chdir-notify.h index 81eb69d846..36b4114472 100644 --- a/chdir-notify.h +++ b/chdir-notify.h @@ -19,10 +19,7 @@ * chdir_notify_register("description", foo, data); * * In practice most callers will want to move a relative path to the new root; - * they can use the reparent_relative_path() helper for that. If that's all - * you're doing, you can also use the convenience function: - * - * chdir_notify_reparent("description", &my_path); + * they can use the reparent_relative_path() helper for that. * * Whenever a chdir event occurs, that will update my_path (if it's relative) * to adjust for the new cwd by freeing any existing string and allocating a @@ -43,7 +40,6 @@ typedef void (*chdir_notify_callback)(const char *name, void chdir_notify_register(const char *name, chdir_notify_callback cb, void *data); void chdir_notify_unregister(const char *name, chdir_notify_callback cb, void *data); -void chdir_notify_reparent(const char *name, char **path); /* * From e022c82d0f8523c3200da2a98e44f4d49b3210f0 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 18 Jun 2026 08:54:35 +0200 Subject: [PATCH 6/8] repository: free main reference database While we release worktree and submodule reference databases when clearing a repository, we don't ever release the main reference database. This memory leak went unnoticed because its pointer is kept alive by the "chdir_notify" subsystem. Fix the memory leak. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- repository.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/repository.c b/repository.c index 187dd471c4..e2b5c6712b 100644 --- a/repository.c +++ b/repository.c @@ -421,6 +421,11 @@ void repo_clear(struct repository *repo) FREE_AND_NULL(repo->remote_state); } + if (repo->refs_private) { + ref_store_release(repo->refs_private); + FREE_AND_NULL(repo->refs_private); + } + strmap_for_each_entry(&repo->submodule_ref_stores, &iter, e) ref_store_release(e->value); strmap_clear(&repo->submodule_ref_stores, 1); From 2eb9eb727d8c2df456bef84812db904d9715ab2f Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 18 Jun 2026 08:54:36 +0200 Subject: [PATCH 7/8] refs: fix recursing `get_main_ref_store()` with "onbranch" config When we have an "onbranch" condition we need to ask the reference database whether HEAD currently points at the configured branch. This unfortunately creates a chicken-and-egg problem: - The reference database needs to read the configuration so that it can configure itself. - The configuration needs to construct a reference database to fully parse all of its conditionals. The way we handle this is by simply excluding "onbranch" conditionals when we haven't yet configured the reference database. The mechanism for this is broken though: to verify whether or not we have configured the reference database we check whether its format is set to `REF_STORAGE_UNKNOWN` in `include_by_branch()`. But typically, the format _is_ already known at that time because we set it up during repository discovery in "setup.c". The consequence is that we have recursion: 1. We call `get_main_ref_store()`. 2. We don't yet have a reference store, so we call `ref_store_init()`. 3. We parse the configuration required for the reference store. 4. We eventually end up in `include_by_branch()`. 5. We have already configured the reference storage format, so we end up calling `get_main_ref_store()` again. We still haven't finished (1) though, so `get_main_ref_store()` will now call `ref_store_init()` a second time. The end result is that we have constructed the same reference store twice. Of course, as both reference stores would be assigned to `refs_private`, we leak one of those two instances. This never surfaced as an actual leak though because the pointer is kept alive by the "chdir_notify" subsystem. For now, we can fix the issue by explicitly unsetting the reference storage format before constructing it. This makes the mentioned check trigger as expected, and consequently we won't end up constructing a second reference database at all. Ultimately, this means that we consistently stop evaluating "onbranch" conditions when constructing the main reference database. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/refs.c b/refs.c index d3caa9a633..e69b9b8ac8 100644 --- a/refs.c +++ b/refs.c @@ -2351,15 +2351,31 @@ void ref_store_release(struct ref_store *ref_store) struct ref_store *get_main_ref_store(struct repository *r) { + enum ref_storage_format format; + if (r->refs_private) return r->refs_private; if (!r->gitdir) BUG("attempting to get main_ref_store outside of repository"); - r->refs_private = ref_store_init(r, r->ref_storage_format, - r->gitdir, REF_STORE_ALL_CAPS); + /* + * When constructing the reference backend we'll end up reading the Git + * configuration. This means we'll also try to evaluate "onbranch" + * conditions. + * + * We cannot read branches when constructing the refdb, so it is not + * possible to evaluate those conditions in the first place. To gate + * their evaluation we check whether or not the reference storage + * format has been configured -- we thus have to temporarily set it to + * UNKNOWN here so that we don't end up recursing. + */ + format = r->ref_storage_format; + r->ref_storage_format = REF_STORAGE_FORMAT_UNKNOWN; + r->refs_private = ref_store_init(r, format, r->gitdir, REF_STORE_ALL_CAPS); r->refs_private = maybe_debug_wrap_ref_store(r->gitdir, r->refs_private); + r->ref_storage_format = format; + return r->refs_private; } From ff3b66bd6306a5a46c68a8f062283f71db493408 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 18 Jun 2026 08:54:37 +0200 Subject: [PATCH 8/8] refs: drop local buffer in `refs_compute_filesystem_location()` We're using a local buffer in `refs_compute_filesystem_location()` that is only used so that we can fill it and then call `strbuf_realpath()` on its result. This roundtrip isn't necessary though: `strbuf_realpath()` already knows to use a single buffer as both input and output at the same time. So all this does is to add a bit of confusion and an extra memory allocation. Drop the local buffer. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/refs.c b/refs.c index e69b9b8ac8..4912510590 100644 --- a/refs.c +++ b/refs.c @@ -3571,8 +3571,6 @@ void refs_compute_filesystem_location(const char *gitdir, const char *payload, bool *is_worktree, struct strbuf *refdir, struct strbuf *ref_common_dir) { - struct strbuf sb = STRBUF_INIT; - *is_worktree = get_common_dir_noenv(ref_common_dir, gitdir); if (!payload) { @@ -3586,8 +3584,8 @@ void refs_compute_filesystem_location(const char *gitdir, const char *payload, } if (!is_absolute_path(payload)) { - strbuf_addf(&sb, "%s/%s", ref_common_dir->buf, payload); - strbuf_realpath(ref_common_dir, sb.buf, 1); + strbuf_addf(ref_common_dir, "/%s", payload); + strbuf_realpath(ref_common_dir, ref_common_dir->buf, 1); } else { strbuf_realpath(ref_common_dir, payload, 1); } @@ -3600,6 +3598,4 @@ void refs_compute_filesystem_location(const char *gitdir, const char *payload, BUG("worktree path does not contain slash"); strbuf_addf(refdir, "/worktrees/%s", wt_id + 1); } - - strbuf_release(&sb); }