[PATCH 0/9] refs: stop using `chdir_notify_reparent()`

[PATCH 0/9] refs: stop using `chdir_notify_reparent()`

[Patrick Steinhardt]

Hi,

this patch series is a follow-up of the discussion at [1]. It converts
the reference backends to always use absolute paths internally, which
then allows us to drop the calls to `chdir_notify_reparent()`.

Unfortunately, the series has grown quite a bit larger than anticipated.
This is due to a couple of weirdnesses in how the reference database is
constructed with an "onbranch" condition. We essentially construct the
refdb twice and loose one, but we never noticed because the chdir
notification subsystem kept the pointer to it reachable.

Note that the first couple patches that touch "setup.c" aren't strictly
required. They are a remnant of a previous iteration where I tried to
solve the issue in a different way. But I ultimately figured that these
changes are worth it by themselves as they simplify "setup.c" a bit.

This series is built on top of 1ff279f340 (The 13th batch, 2026-06-09)
with ps/setup-centralize-odb-creation at 42b9d3dc9d (setup: construct
object database in `apply_repository_format()`, 2026-06-04) merged into
it.

Thanks!

Patrick

[1]: <aifAVpxanV31KUpC@pks.im>

---
Patrick Steinhardt (9):
      setup: inline `check_and_apply_repository_format()`
      setup: stop applying repository format twice
      setup: don't apply "GIT_REFERENCE_BACKEND" without a repository
      refs: unregister reference stores from "chdir_notify"
      chdir-notify: drop unused `chdir_notify_reparent()`
      repository: free main reference database
      refs: fix recursing `get_main_ref_store()` with "onbranch" config
      refs: drop local buffer in `refs_compute_filesystem_location()`
      refs: always use absolute paths for reference stores

 chdir-notify.c              | 26 ------------
 chdir-notify.h              |  6 +--
 refs.c                      | 35 ++++++++++++-----
 refs/files-backend.c        |  6 ---
 refs/packed-backend.c       |  4 +-
 refs/reftable-backend.c     |  3 --
 repository.c                |  5 +++
 setup.c                     | 96 ++++++++++++++++++---------------------------
 t/pack-refs-tests.sh        |  6 +--
 t/t0600-reffiles-backend.sh |  4 +-
 t/t1423-ref-backend.sh      |  9 +++--
 t/t5510-fetch.sh            |  2 +-
 12 files changed, 83 insertions(+), 119 deletions(-)


---
base-commit: 255322df35357168daefec8523a3cdc849edd6c1
change-id: 20260609-b4-pks-refs-avoid-chdir-notify-reparent-a4eaf1edbcab

[PATCH 1/9] setup: inline `check_and_apply_repository_format()`

[Patrick Steinhardt]

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 <ps@pks.im>
---
 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;
 }

-- 
2.54.0.1189.g8c84645362.dirty

[PATCH 2/9] setup: stop applying repository format twice

[Patrick Steinhardt]

When discovering the repository in "setup.c" we apply the final
repository format multiple times:

  - Once via `repository_format_configure()`, where we configure the
    repository format for both `struct repository_format` and `struct
    repository`.

  - And once via `apply_repository_format()`, where we then apply the
    `struct repository_format` to the `struct repository` again.

As the format will be applied to the repository when applying the format
it's thus somewhat unnecessary to also apply it to the repository when
adapting the discovered format. The only reason we have to do this is
because we call `repository_format_configure()` after we have already
applied it.

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 <ps@pks.im>
---
 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 @@ static int read_default_format_config(const char *key, const char *value,
 	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

-- 
2.54.0.1189.g8c84645362.dirty

[PATCH 3/9] setup: don't apply "GIT_REFERENCE_BACKEND" without a repository

[Patrick Steinhardt]

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 <ps@pks.im>
---
 setup.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/setup.c b/setup.c
index 2748155964..7b2e50a8c5 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;
 
 	/*
@@ -2023,6 +2022,8 @@ const char *setup_git_directory_gently(struct repository *repo, int *nongit_ok)
 	    startup_info->have_repository ||
 	    /* GIT_DIR_EXPLICIT */
 	    getenv(GIT_DIR_ENVIRONMENT)) {
+		const char *ref_backend_uri;
+
 		if (!repo->gitdir) {
 			const char *gitdir = getenv(GIT_DIR_ENVIRONMENT);
 			if (!gitdir)
@@ -2030,6 +2031,24 @@ const char *setup_git_directory_gently(struct repository *repo, int *nongit_ok)
 			setup_git_env_internal(repo, gitdir);
 		}
 
+		/*
+		 * 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 (startup_info->have_repository) {
 			struct strbuf err = STRBUF_INIT;
 
@@ -2057,25 +2076,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);

-- 
2.54.0.1189.g8c84645362.dirty

[PATCH 4/9] refs: unregister reference stores from "chdir_notify"

[Patrick Steinhardt]

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_parent()`, as there is no infrastructure to unregister the
latter. It ultimately doesn't matter much though: in a subsequent commit
we'll drop this infrastructure completely. We merely require this step
here so that we can fix the memory leaks ahead of time.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 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,

-- 
2.54.0.1189.g8c84645362.dirty

[PATCH 5/9] chdir-notify: drop unused `chdir_notify_reparent()`

[Patrick Steinhardt]

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 <ps@pks.im>
---
 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);
 
 /*
  *

-- 
2.54.0.1189.g8c84645362.dirty

[PATCH 6/9] repository: free main reference database

[Patrick Steinhardt]

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 <ps@pks.im>
---
 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);

-- 
2.54.0.1189.g8c84645362.dirty

[PATCH 7/9] refs: fix recursing `get_main_ref_store()` with "onbranch" config

[Patrick Steinhardt]

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 <ps@pks.im>
---
 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;
 }
 

-- 
2.54.0.1189.g8c84645362.dirty

[PATCH 8/9] refs: drop local buffer in `refs_compute_filesystem_location()`

[Patrick Steinhardt]

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 <ps@pks.im>
---
 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);
 }

-- 
2.54.0.1189.g8c84645362.dirty

[PATCH 9/9] refs: always use absolute paths for reference stores

[Patrick Steinhardt]

Both the "files" and "reftable" backends use
`refs_compute_filesystem_location()` to figure out the location of both
the git and common directories. Depending on how the function is called
we may or may not return an absolute path.

There isn't really a good reason to use relative paths though. Quite on
the contrary, because we sometimes use relative paths we are forced to
register for chdir(3p) notifications via `chdir_notify_reparent()`.

Adapt the function to always return absolute paths. This results in a
user-visible change in behaviour where we now unconditionally print
absolute paths in error messages. But arguably, that change in behaviour
is acceptable and may even be good in cases where a Git command may end
up accessing references across multiple different repositories.

Furthermore, drop the calls to `chdir_notify_reparent()`, which aren't
required anymore now that the paths are always absolute.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.c                      | 11 ++++++++---
 refs/files-backend.c        | 22 ----------------------
 refs/packed-backend.c       | 18 +-----------------
 refs/reftable-backend.c     | 17 -----------------
 t/pack-refs-tests.sh        |  6 +++---
 t/t0600-reffiles-backend.sh |  4 ++--
 t/t1423-ref-backend.sh      |  9 ++++++---
 t/t5510-fetch.sh            |  2 +-
 8 files changed, 21 insertions(+), 68 deletions(-)

diff --git a/refs.c b/refs.c
index 4912510590..8679677bf7 100644
--- a/refs.c
+++ b/refs.c
@@ -3579,15 +3579,16 @@ void refs_compute_filesystem_location(const char *gitdir, const char *payload,
 		 * worktree path, as the 'gitdir' here is already the worktree
 		 * path and is different from 'commondir' denoted by 'ref_common_dir'.
 		 */
+		strbuf_reset(refdir);
 		strbuf_addstr(refdir, gitdir);
-		return;
+		goto out;
 	}
 
 	if (!is_absolute_path(payload)) {
 		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);
+		strbuf_reset(ref_common_dir);
+		strbuf_addstr(ref_common_dir, payload);
 	}
 
 	strbuf_addbuf(refdir, ref_common_dir);
@@ -3598,4 +3599,8 @@ 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);
 	}
+
+out:
+	strbuf_realpath(ref_common_dir, ref_common_dir->buf, 1);
+	strbuf_realpath(refdir, refdir->buf, 1);
 }
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 296981584b..762f392e67 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -21,7 +21,6 @@
 #include "../lockfile.h"
 #include "../path.h"
 #include "../dir.h"
-#include "../chdir-notify.h"
 #include "../setup.h"
 #include "../worktree.h"
 #include "../wrapper.h"
@@ -100,23 +99,6 @@ 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.
@@ -145,10 +127,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_register(NULL, files_ref_store_reparent, refs);
-
 	strbuf_release(&refdir);
-
 	return ref_store;
 }
 
@@ -197,7 +176,6 @@ 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 499cb55dfa..89e41a35a3 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -13,7 +13,6 @@
 #include "packed-backend.h"
 #include "../iterator.h"
 #include "../lockfile.h"
-#include "../chdir-notify.h"
 #include "../statinfo.h"
 #include "../worktree.h"
 #include "../wrapper.h"
@@ -211,19 +210,6 @@ 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.
@@ -239,10 +225,9 @@ struct ref_store *packed_ref_store_init(struct repository *repo,
 
 	base_ref_store_init(ref_store, repo, gitdir, &refs_be_packed);
 	refs->store_flags = opts->access_flags;
-
 	strbuf_addf(&sb, "%s/packed-refs", gitdir);
 	refs->path = strbuf_detach(&sb, NULL);
-	chdir_notify_register(NULL, packed_ref_store_reparent, refs);
+
 	return ref_store;
 }
 
@@ -287,7 +272,6 @@ 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 8c93070677..8cc1dbbbdd 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -2,7 +2,6 @@
 
 #include "../git-compat-util.h"
 #include "../abspath.h"
-#include "../chdir-notify.h"
 #include "../config.h"
 #include "../dir.h"
 #include "../environment.h"
@@ -365,19 +364,6 @@ 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,
@@ -460,8 +446,6 @@ static struct ref_store *reftable_be_init(struct repository *repo,
 			goto done;
 	}
 
-	chdir_notify_register(NULL, reftable_be_reparent, refs);
-
 done:
 	assert(refs->err != REFTABLE_API_ERROR);
 	strbuf_release(&ref_common_dir);
@@ -487,7 +471,6 @@ 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,
diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
index d76b087b09..357413ba3c 100644
--- a/t/pack-refs-tests.sh
+++ b/t/pack-refs-tests.sh
@@ -237,7 +237,7 @@ test_expect_success 'reject packed-refs with unterminated line' '
 	cp .git/packed-refs .git/packed-refs.bak &&
 	test_when_finished "mv .git/packed-refs.bak .git/packed-refs" &&
 	printf "%s" "$HEAD refs/zzzzz" >>.git/packed-refs &&
-	echo "fatal: unterminated line in .git/packed-refs: $HEAD refs/zzzzz" >expected_err &&
+	echo "fatal: unterminated line in $(pwd)/.git/packed-refs: $HEAD refs/zzzzz" >expected_err &&
 	test_must_fail git for-each-ref >out 2>err &&
 	test_cmp expected_err err
 '
@@ -246,7 +246,7 @@ test_expect_success 'reject packed-refs containing junk' '
 	cp .git/packed-refs .git/packed-refs.bak &&
 	test_when_finished "mv .git/packed-refs.bak .git/packed-refs" &&
 	printf "%s\n" "bogus content" >>.git/packed-refs &&
-	echo "fatal: unexpected line in .git/packed-refs: bogus content" >expected_err &&
+	echo "fatal: unexpected line in $(pwd)/.git/packed-refs: bogus content" >expected_err &&
 	test_must_fail git for-each-ref >out 2>err &&
 	test_cmp expected_err err
 '
@@ -255,7 +255,7 @@ test_expect_success 'reject packed-refs with a short SHA-1' '
 	cp .git/packed-refs .git/packed-refs.bak &&
 	test_when_finished "mv .git/packed-refs.bak .git/packed-refs" &&
 	printf "%.7s %s\n" $HEAD refs/zzzzz >>.git/packed-refs &&
-	printf "fatal: unexpected line in .git/packed-refs: %.7s %s\n" $HEAD refs/zzzzz >expected_err &&
+	printf "fatal: unexpected line in $(pwd)/.git/packed-refs: %.7s %s\n" $HEAD refs/zzzzz >expected_err &&
 	test_must_fail git for-each-ref >out 2>err &&
 	test_cmp expected_err err
 '
diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
index 74bfa2e9ba..b17f0940c2 100755
--- a/t/t0600-reffiles-backend.sh
+++ b/t/t0600-reffiles-backend.sh
@@ -96,7 +96,7 @@ test_expect_success 'non-empty directory blocks create' - <<\EOT
 	: >.git/$prefix/foo/bar/baz.lock &&
 	test_when_finished "rm -f .git/$prefix/foo/bar/baz.lock" &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref '$prefix/foo': there is a non-empty directory '.git/$prefix/foo' blocking reference '$prefix/foo'
+	fatal: cannot lock ref '$prefix/foo': there is a non-empty directory '$(pwd)/.git/$prefix/foo' blocking reference '$prefix/foo'
 	EOF
 	printf "%s\n" "update $prefix/foo $C" |
 	test_must_fail git update-ref --stdin 2>output.err &&
@@ -135,7 +135,7 @@ test_expect_success 'non-empty directory blocks indirect create' - <<\EOT
 	: >.git/$prefix/foo/bar/baz.lock &&
 	test_when_finished "rm -f .git/$prefix/foo/bar/baz.lock" &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref '$prefix/symref': there is a non-empty directory '.git/$prefix/foo' blocking reference '$prefix/foo'
+	fatal: cannot lock ref '$prefix/symref': there is a non-empty directory '$(pwd)/.git/$prefix/foo' blocking reference '$prefix/foo'
 	EOF
 	printf "%s\n" "update $prefix/symref $C" |
 	test_must_fail git update-ref --stdin 2>output.err &&
diff --git a/t/t1423-ref-backend.sh b/t/t1423-ref-backend.sh
index fd47d77e8e..875857f2d0 100755
--- a/t/t1423-ref-backend.sh
+++ b/t/t1423-ref-backend.sh
@@ -145,7 +145,8 @@ do
 				test_commit 3 &&
 
 				git refs migrate --dry-run --ref-format=$to_format >out &&
-				BACKEND_PATH="$dir/$(sed "s/.* ${SQ}.git\/\(.*\)${SQ}/\1/" out)" &&
+				BACKEND_PATH=$(sed "s/.* the result can be found at ${SQ}\(.*\)${SQ}$/\1/" out) &&
+				test_path_is_dir "$BACKEND_PATH" &&
 				test_refs_backend . $from_format "$to_format://$BACKEND_PATH" "$method"
 			)
 		'
@@ -160,7 +161,8 @@ do
 				test_commit 3 &&
 
 				git refs migrate --dry-run --ref-format=$to_format >out &&
-				BACKEND_PATH="$dir/$(sed "s/.* ${SQ}.git\/\(.*\)${SQ}/\1/" out)" &&
+				BACKEND_PATH=$(sed "s/.* the result can be found at ${SQ}\(.*\)${SQ}$/\1/" out) &&
+				test_path_is_dir "$BACKEND_PATH" &&
 
 				test_refs_backend . $from_format "$to_format://$BACKEND_PATH" "$method" &&
 
@@ -187,7 +189,8 @@ do
 				test_commit 3 &&
 
 				git refs migrate --dry-run --ref-format=$to_format >out &&
-				BACKEND_PATH="$dir/$(sed "s/.* ${SQ}.git\/\(.*\)${SQ}/\1/" out)" &&
+				BACKEND_PATH=$(sed "s/.* the result can be found at ${SQ}\(.*\)${SQ}$/\1/" out) &&
+				test_path_is_dir "$BACKEND_PATH" &&
 
 				run_with_uri . "$from_format" "$to_format://$BACKEND_PATH" \
 					"worktree add ../wt 2" "$method" &&
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index eca9a973b5..d5f84d99df 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -1741,7 +1741,7 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'D/F conflict on case insensiti
 		cd case_insensitive &&
 		git remote add origin -- ../case_sensitive_df &&
 		test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err &&
-		test_grep "cannot lock ref ${SQ}refs/remotes/origin/foo${SQ}: there is a non-empty directory ${SQ}./refs/remotes/origin/foo${SQ} blocking reference ${SQ}refs/remotes/origin/foo${SQ}" err &&
+		test_grep "cannot lock ref ${SQ}refs/remotes/origin/foo${SQ}: there is a non-empty directory ${SQ}$(pwd)/refs/remotes/origin/foo${SQ} blocking reference ${SQ}refs/remotes/origin/foo${SQ}" err &&
 		git rev-parse refs/heads/main >expect &&
 		git rev-parse refs/heads/Foo/bar >actual &&
 		test_cmp expect actual

-- 
2.54.0.1189.g8c84645362.dirty

Re: [PATCH 3/9] setup: don't apply "GIT_REFERENCE_BACKEND" without a repository

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> 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.

Does the second point have a small typo, i.e., "if we have a
repository, we have already applied the ref backend to it when we
discovered it, so NO need to manually reapply"?

> 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()`.

Makes sense.

Re: [PATCH 0/9] refs: stop using `chdir_notify_reparent()`

[Jeff King]

On Wed, Jun 10, 2026 at 04:57:06PM +0200, Patrick Steinhardt wrote:

> this patch series is a follow-up of the discussion at [1]. It converts
> the reference backends to always use absolute paths internally, which
> then allows us to drop the calls to `chdir_notify_reparent()`.

We added chdir-notify to suport set_work_tree(). Commit 8500e0de3f
(set_work_tree: use chdir_notify, 2018-03-30) mentions an optimization
from 044bbbcb63 (Make git_dir a path relative to work_tree in
setup_work_tree(), 2008-06-19). That commit demonstrates some measurable
speedup from using relative versus absolute paths.

If we move to a world of all absolute paths where chdir-notify is not
necessary, will we lose that optimization?

I'm not sure how much it matters in practice these days, or if those
timings could be repeated. And they weren't all _that_ big to start
with. I guess it may depend on how deep your repo is within your
filesystem, too.

-Peff

Re: [PATCH 3/9] setup: don't apply "GIT_REFERENCE_BACKEND" without a repository

[Patrick Steinhardt]

On Wed, Jun 10, 2026 at 10:32:46AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > 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.
> 
> Does the second point have a small typo, i.e., "if we have a
> repository, we have already applied the ref backend to it when we
> discovered it, so NO need to manually reapply"?

No, this is correct as-is. At the point in time where we handle
GIT_REFERENCE_BACKEND we have already discovered the repository format,
applied it to the repository, configured the reference database format
et al. So because we handle GIT_REFERENCE_BACKEND _after_ that whole
dance we basically have to re-configure the reference database format,
which is awkward.

Patrick

Re: [PATCH 0/9] refs: stop using `chdir_notify_reparent()`

[Patrick Steinhardt]

On Thu, Jun 11, 2026 at 02:53:46AM -0400, Jeff King wrote:
> On Wed, Jun 10, 2026 at 04:57:06PM +0200, Patrick Steinhardt wrote:
> 
> > this patch series is a follow-up of the discussion at [1]. It converts
> > the reference backends to always use absolute paths internally, which
> > then allows us to drop the calls to `chdir_notify_reparent()`.
> 
> We added chdir-notify to suport set_work_tree(). Commit 8500e0de3f
> (set_work_tree: use chdir_notify, 2018-03-30) mentions an optimization
> from 044bbbcb63 (Make git_dir a path relative to work_tree in
> setup_work_tree(), 2008-06-19). That commit demonstrates some measurable
> speedup from using relative versus absolute paths.

Oh, that is context I wasn't aware of. Not much of a surprise though,
given that this is from 2008 :) So thanks a lot for the pointer!

> If we move to a world of all absolute paths where chdir-notify is not
> necessary, will we lose that optimization?

Probably. Unfortunately, the commit doesn't have any repeatable
benchmarks in there, so it's hard to say whether we could still
reproduce those issues or not.

> I'm not sure how much it matters in practice these days, or if those
> timings could be repeated. And they weren't all _that_ big to start
> with. I guess it may depend on how deep your repo is within your
> filesystem, too.

Ideally, we'd have the best of both worlds: absolute paths everywhere
without the performance hit. A while back I had a discussion with
Torvalds on the securiy mailing list around this issue, and ultimately
the conclusion was that the best way forward would be to use openat(3p).

This wouldn't only allow us to optimize cases like this, but it also has
the added benefit that we're much less prone to TOCTOU-style issues and
we might even be able to use flags like O_BENEATH. So it would basically
be win-win. The only problem is of course that Windows doesn't have
openat(3p), so we'd have to emulate it, and that's where I always lost
the desire to do this.

When waking up this morning though I had the thought that we shouldn't
try to emulate openat(3p) directly, but instead create a higher-level
interface.

    struct fsroot;

    /*
     * Open a new filesystem root at the given directory. All subsequent
     * calls to open will be relative to this fsroot.
     */
    struct fsroot *fsroot_new(const char *dir);

    /*
     * Create a new fsroot from a subdirectory relative to the given
     * root directory.
     */
    struct fsroot *fsroot_new_subdir(struct fsroot *r, const char *dir);

    /*
     * Open a new file relative to the given fsroot. This will use the
     * equivalent of O_BENEATH so that we only ever open files that are
     * located below the fsroot.
     */
    int fsroot_open(struct fsroot *r, const char *path, int oflag, ...);

This is of course heavily inspired by similar interfaces that exist in
Go [1]. By having such a higher-level abstraction it should also be way
easier to port this to different platforms, where we can then add safety
features like O_BENEATH when available on any given platform.

The idea here would be that we can then convert some subsystems to use
those structures instead of tracking paths. I'd for example love for the
repository's working tree to use this mechanism so that we can squash a
whole class of potential security issues when checking out files that
end in locations we didn't intend to.

Thanks!

Patrick

[1]: https://pkg.go.dev/io/fs#FS

Re: [PATCH 2/9] setup: stop applying repository format twice

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 3140 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> When discovering the repository in "setup.c" we apply the final
> repository format multiple times:
>
>   - Once via `repository_format_configure()`, where we configure the
>     repository format for both `struct repository_format` and `struct
>     repository`.
>
>   - And once via `apply_repository_format()`, where we then apply the
>     `struct repository_format` to the `struct repository` again.
>

Okay so we're talking applying the repository format to the `struct
repository` specifically.

> As the format will be applied to the repository when applying the format
> it's thus somewhat unnecessary to also apply it to the repository when
> adapting the discovered format.

This was a bit confusing to read at first. Okay since we already apply
the format in the second step, the first is not necessary.

> The only reason we have to do this is
> because we call `repository_format_configure()` after we have already
> applied it.

Right, so there is a need to do this.

>
> 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.
>

Makse sense.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  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 @@ static int read_default_format_config(const char *key, const char *value,
>  	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
>
> --
> 2.54.0.1189.g8c84645362.dirty

The patch looks good.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 4/9] refs: unregister reference stores from "chdir_notify"

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 6726 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> 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.
>

So `ref_store_release()` is what is called to release a ref_store. So
in the former's case, we never release the ref_store even if the
repository is closed, wow.

> 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_parent()`, as there is no infrastructure to unregister the

Shouldn't this be s/chdir_notify_parent/chdir_notify_reparent ?

> latter. It ultimately doesn't matter much though: in a subsequent commit
> we'll drop this infrastructure completely. We merely require this step
> here so that we can fix the memory leaks ahead of time.

Right, we can't unregister when using `chdir_notify_reparent()` because
it internally calls `chdir_notify_register()` with a private cb
function, and we need to supply the callback function during
un-registering. Makes sense.

>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  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;
> +}
> +

Looks similar to `void reparent_cb()` but for both the directories.

>  /*
>   * 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,
>
> --
> 2.54.0.1189.g8c84645362.dirty

The changes here look good.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 6/9] repository: free main reference database

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1131 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> 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.
>

Funny, cause long ago I looked into this and thought I was clearly
missing something and eventually forgot about it. Good to know that I
was correct :)

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  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);
>
> --
> 2.54.0.1189.g8c84645362.dirty

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 9/9] refs: always use absolute paths for reference stores

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 7719 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> Both the "files" and "reftable" backends use
> `refs_compute_filesystem_location()` to figure out the location of both
> the git and common directories. Depending on how the function is called
> we may or may not return an absolute path.
>
> There isn't really a good reason to use relative paths though. Quite on
> the contrary, because we sometimes use relative paths we are forced to
> register for chdir(3p) notifications via `chdir_notify_reparent()`.
>

With the previous changes added, we register via
`chdir_notify_register()`

> Adapt the function to always return absolute paths. This results in a
> user-visible change in behaviour where we now unconditionally print
> absolute paths in error messages. But arguably, that change in behaviour
> is acceptable and may even be good in cases where a Git command may end
> up accessing references across multiple different repositories.
>
> Furthermore, drop the calls to `chdir_notify_reparent()`, which aren't
> required anymore now that the paths are always absolute.
>

Same here, should be `chdir_notify_register()`

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs.c                      | 11 ++++++++---
>  refs/files-backend.c        | 22 ----------------------
>  refs/packed-backend.c       | 18 +-----------------
>  refs/reftable-backend.c     | 17 -----------------
>  t/pack-refs-tests.sh        |  6 +++---
>  t/t0600-reffiles-backend.sh |  4 ++--
>  t/t1423-ref-backend.sh      |  9 ++++++---
>  t/t5510-fetch.sh            |  2 +-
>  8 files changed, 21 insertions(+), 68 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 4912510590..8679677bf7 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3579,15 +3579,16 @@ void refs_compute_filesystem_location(const char *gitdir, const char *payload,
>  		 * worktree path, as the 'gitdir' here is already the worktree
>  		 * path and is different from 'commondir' denoted by 'ref_common_dir'.
>  		 */
> +		strbuf_reset(refdir);
>  		strbuf_addstr(refdir, gitdir);
> -		return;
> +		goto out;
>  	}
>
>  	if (!is_absolute_path(payload)) {
>  		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);
> +		strbuf_reset(ref_common_dir);
> +		strbuf_addstr(ref_common_dir, payload);
>  	}
>
>  	strbuf_addbuf(refdir, ref_common_dir);
> @@ -3598,4 +3599,8 @@ 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);
>  	}
> +
> +out:
> +	strbuf_realpath(ref_common_dir, ref_common_dir->buf, 1);
> +	strbuf_realpath(refdir, refdir->buf, 1);
>  }
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 296981584b..762f392e67 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -21,7 +21,6 @@
>  #include "../lockfile.h"
>  #include "../path.h"
>  #include "../dir.h"
> -#include "../chdir-notify.h"
>  #include "../setup.h"
>  #include "../worktree.h"
>  #include "../wrapper.h"
> @@ -100,23 +99,6 @@ 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.
> @@ -145,10 +127,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_register(NULL, files_ref_store_reparent, refs);
> -
>  	strbuf_release(&refdir);
> -
>  	return ref_store;
>  }
>
> @@ -197,7 +176,6 @@ 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 499cb55dfa..89e41a35a3 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -13,7 +13,6 @@
>  #include "packed-backend.h"
>  #include "../iterator.h"
>  #include "../lockfile.h"
> -#include "../chdir-notify.h"
>  #include "../statinfo.h"
>  #include "../worktree.h"
>  #include "../wrapper.h"
> @@ -211,19 +210,6 @@ 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.
> @@ -239,10 +225,9 @@ struct ref_store *packed_ref_store_init(struct repository *repo,
>
>  	base_ref_store_init(ref_store, repo, gitdir, &refs_be_packed);
>  	refs->store_flags = opts->access_flags;
> -
>  	strbuf_addf(&sb, "%s/packed-refs", gitdir);
>  	refs->path = strbuf_detach(&sb, NULL);
> -	chdir_notify_register(NULL, packed_ref_store_reparent, refs);
> +
>  	return ref_store;
>  }
>
> @@ -287,7 +272,6 @@ 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 8c93070677..8cc1dbbbdd 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -2,7 +2,6 @@
>
>  #include "../git-compat-util.h"
>  #include "../abspath.h"
> -#include "../chdir-notify.h"
>  #include "../config.h"
>  #include "../dir.h"
>  #include "../environment.h"
> @@ -365,19 +364,6 @@ 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,
> @@ -460,8 +446,6 @@ static struct ref_store *reftable_be_init(struct repository *repo,
>  			goto done;
>  	}
>
> -	chdir_notify_register(NULL, reftable_be_reparent, refs);
> -
>  done:
>  	assert(refs->err != REFTABLE_API_ERROR);
>  	strbuf_release(&ref_common_dir);
> @@ -487,7 +471,6 @@ 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,
>

The changes look good to me.

[snip]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]