{"thread":{"id":"62911","subject":"[PATCH 00/16] path: remove dependency on `the_repository`","startedAt":"2025-02-06T07:58:05Z","lastAt":"2025-02-07T10:49:23Z","messageCount":36,"participants":["Patrick Steinhardt","Karthik Nayak","shejialuo"],"isPatch":true,"patchVersion":1,"patchTotal":16},"messages":[{"id":"511967","messageId":"20250206-b4-pks-path-drop-the-repository-v1-0-4e77f0313206@pks.im","threadId":"62911","inReplyTo":null,"subject":"[PATCH 00/16] path: remove dependency on `the_repository`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-06T07:57:56Z","receivedAt":"2025-02-06T07:58:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch series removes the dependency on `the_repository` from the\n\"path\" subsystem. The series is structured as follows:\n\n - Patches 1 to 5 unifyf the interfaces that can be used to retrieve\n repository paths (gitdir, commondir, workdir and submodule paths)\n and adapts callers accodringly.\n\n - Patches 6 to 12 drop repository path functions that had an implicit\n dependency on `the_repository`.\n\n - Patches 13 to 16 refactor \"path.c\"-internal code to stop depending\n on `the_repository`.\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (16):\n path: refactor `repo_common_path()` family of functions\n path: refactor `repo_git_path()` family of functions\n path: refactor `repo_worktree_path()` family of functions\n submodule: refactor `submodule_to_gitdir()` to accept a repo\n path: refactor `repo_submodule_path()` family of functions\n path: drop unused `strbuf_git_path()` function\n path: drop `git_pathdup()` in favor of `repo_git_path()`\n path: drop `git_path_buf()` in favor of `repo_git_path_replace()`\n worktree: return allocated string from `get_worktree_git_dir()`\n path: drop `git_common_path()` in favor of `repo_common_path()`\n rerere: let `rerere_path()` write paths into a caller-provided buffer\n path: drop `git_path()` in favor of `repo_git_path()`\n repo-settings: introduce function to clear struct\n environment: move access to \"core.hooksPath\" into repo settings\n environment: move access to \"core.sharedRepository\" into repo settings\n path: adjust last remaining users of `the_repository`\n\n bisect.c | 2 +-\n branch.c | 7 +-\n builtin/am.c | 2 +-\n builtin/clone.c | 4 +-\n builtin/commit.c | 8 +-\n builtin/config.c | 8 +-\n builtin/fast-import.c | 4 +-\n builtin/fsck.c | 10 ++-\n builtin/gc.c | 25 ++++--\n builtin/init-db.c | 8 +-\n builtin/log.c | 6 +-\n builtin/notes.c | 9 ++-\n builtin/rebase.c | 2 +-\n builtin/receive-pack.c | 4 +-\n builtin/remote.c | 6 +-\n builtin/replace.c | 2 +-\n builtin/rerere.c | 11 ++-\n builtin/rev-parse.c | 6 +-\n builtin/submodule--helper.c | 4 +-\n builtin/tag.c | 2 +-\n builtin/worktree.c | 43 +++++++---\n commit-graph.c | 2 +-\n compat/precompose_utf8.c | 6 +-\n config.c | 5 --\n copy.c | 4 +-\n dir.c | 2 +-\n editor.c | 6 +-\n environment.c | 27 -------\n environment.h | 11 ---\n hook.c | 3 +-\n http-backend.c | 2 +-\n loose.c | 8 +-\n midx-write.c | 2 +-\n notes-merge.c | 26 +++---\n object-file.c | 14 ++--\n pack-bitmap-write.c | 2 +-\n pack-write.c | 10 ++-\n path.c | 166 +++++++++++++++++++++++++++-----------\n path.h | 191 ++++++++++++--------------------------------\n reachable.c | 6 +-\n read-cache.c | 26 ++++--\n refs.c | 6 +-\n refs/files-backend.c | 10 +--\n refs/reftable-backend.c | 10 +--\n remote.c | 21 ++---\n repo-settings.c | 44 +++++++++-\n repo-settings.h | 14 ++++\n repository.c | 6 +-\n rerere.c | 101 ++++++++++++++++-------\n rerere.h | 3 +-\n revision.c | 7 +-\n server-info.c | 2 +-\n setup.c | 70 ++++++++--------\n shallow.c | 4 +-\n submodule.c | 15 ++--\n submodule.h | 3 +-\n t/helper/test-ref-store.c | 7 +-\n tempfile.c | 4 +-\n tmp-objdir.c | 17 ++--\n worktree.c | 49 ++++++++----\n worktree.h | 2 +-\n wt-status.c | 42 ++++++----\n 62 files changed, 639 insertions(+), 490 deletions(-)\n\n\n---\nbase-commit: bc204b742735ae06f65bb20291c95985c9633b7f\nchange-id: 20250204-b4-pks-path-drop-the-repository-9ed0f9956361\n\n"},{"id":"511968","messageId":"20250206-b4-pks-path-drop-the-repository-v1-1-4e77f0313206@pks.im","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-0-4e77f0313206@pks.im","subject":"[PATCH 01/16] path: refactor `repo_common_path()` family of functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-06T07:57:57Z","receivedAt":"2025-02-06T07:58:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The functions provided by the \"path\" subsystem to derive repository\npaths for the commondir, gitdir, worktrees and submodules are quite\ninconsistent. Some functions have a `strbuf_` prefix, others have\ndifferent return values, some don't provide a variant working on top of\n`strbuf`s.\n\nWe're thus about to refactor all of these family of functions so that\nthey follow a common pattern:\n\n - `repo_*_path()` returns an allocated string.\n\n - `repo_*_path_append()` appends the path to the caller-provided\n buffer while returning a constant pointer to the buffer. This\n clarifies whether the buffer is being appended to or rewritten,\n which otherwise wasn't immediately obvious.\n\n - `repo_*_path_replace()` replaces contents of the buffer with the\n computed path, again returning a pointer to the buffer contents.\n\nRefactor the commondir family of functions accordingly and adapt all\ncallers.\n\nSigned-off-by: Patrick Steinhardt \n---\n loose.c | 6 +++---\n path.c | 32 ++++++++++++++++++++++++++++----\n path.h | 30 +++++++++++++++++-------------\n refs.c | 4 ++--\n setup.c | 4 ++--\n worktree.c | 5 ++---\n 6 files changed, 54 insertions(+), 27 deletions(-)\n\ndiff --git a/loose.c b/loose.c\nindex 897ba389da..51ef490f93 100644\n--- a/loose.c\n+++ b/loose.c\n@@ -75,7 +75,7 @@ static int load_one_loose_object_map(struct repository *repo, struct object_dire\n \tinsert_loose_map(dir, repo->hash_algo->empty_blob, repo->compat_hash_algo->empty_blob);\n \tinsert_loose_map(dir, repo->hash_algo->null_oid, repo->compat_hash_algo->null_oid);\n \n-\tstrbuf_git_common_path(&path, repo, \"objects/loose-object-idx\");\n+\trepo_common_path_replace(repo, &path, \"objects/loose-object-idx\");\n \tfp = fopen(path.buf, \"rb\");\n \tif (!fp) {\n \t\tstrbuf_release(&path);\n@@ -133,7 +133,7 @@ int repo_write_loose_object_map(struct repository *repo)\n \tif (!should_use_loose_object_map(repo))\n \t\treturn 0;\n \n-\tstrbuf_git_common_path(&path, repo, \"objects/loose-object-idx\");\n+\trepo_common_path_replace(repo, &path, \"objects/loose-object-idx\");\n \tfd = hold_lock_file_for_update_timeout(&lock, path.buf, LOCK_DIE_ON_ERROR, -1);\n \titer = kh_begin(map);\n \tif (write_in_full(fd, loose_object_header, strlen(loose_object_header)) < 0)\n@@ -174,7 +174,7 @@ static int write_one_object(struct repository *repo, const struct object_id *oid\n \tstruct stat st;\n \tstruct strbuf buf = STRBUF_INIT, path = STRBUF_INIT;\n \n-\tstrbuf_git_common_path(&path, repo, \"objects/loose-object-idx\");\n+\trepo_common_path_replace(repo, &path, \"objects/loose-object-idx\");\n \thold_lock_file_for_update_timeout(&lock, path.buf, LOCK_DIE_ON_ERROR, -1);\n \n \tfd = open(path.buf, O_WRONLY | O_CREAT | O_APPEND, 0666);\ndiff --git a/path.c b/path.c\nindex 07964f5d32..273b649e00 100644\n--- a/path.c\n+++ b/path.c\n@@ -414,7 +414,7 @@ static void strbuf_worktree_gitdir(struct strbuf *buf,\n \telse if (!wt->id)\n \t\tstrbuf_addstr(buf, repo->commondir);\n \telse\n-\t\tstrbuf_git_common_path(buf, repo, \"worktrees/%s\", wt->id);\n+\t\trepo_common_path_append(repo, buf, \"worktrees/%s\", wt->id);\n }\n \n void repo_git_pathv(const struct repository *repo,\n@@ -596,14 +596,38 @@ void repo_common_pathv(const struct repository *repo,\n \tstrbuf_cleanup_path(sb);\n }\n \n-void strbuf_git_common_path(struct strbuf *sb,\n-\t\t\t const struct repository *repo,\n-\t\t\t const char *fmt, ...)\n+char *repo_common_path(const struct repository *repo,\n+\t\t const char *fmt, ...)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tva_list args;\n+\tva_start(args, fmt);\n+\trepo_common_pathv(repo, &sb, fmt, args);\n+\tva_end(args);\n+\treturn strbuf_detach(&sb, NULL);\n+}\n+\n+const char *repo_common_path_append(const struct repository *repo,\n+\t\t\t\t struct strbuf *sb,\n+\t\t\t\t const char *fmt, ...)\n+{\n+\tva_list args;\n+\tva_start(args, fmt);\n+\trepo_common_pathv(repo, sb, fmt, args);\n+\tva_end(args);\n+\treturn sb->buf;\n+}\n+\n+const char *repo_common_path_replace(const struct repository *repo,\n+\t\t\t\t struct strbuf *sb,\n+\t\t\t\t const char *fmt, ...)\n {\n \tva_list args;\n+\tstrbuf_reset(sb);\n \tva_start(args, fmt);\n \trepo_common_pathv(repo, sb, fmt, args);\n \tva_end(args);\n+\treturn sb->buf;\n }\n \n static struct passwd *getpw_str(const char *username, size_t len)\ndiff --git a/path.h b/path.h\nindex 5f6c85e5f8..3c75495e1a 100644\n--- a/path.h\n+++ b/path.h\n@@ -25,22 +25,20 @@ char *mkpathdup(const char *fmt, ...)\n \t__attribute__((format (printf, 1, 2)));\n \n /*\n- * The `strbuf_git_common_path` family of functions will construct a path into a\n+ * The `repo_common_path` family of functions will construct a path into a\n * repository's common git directory, which is shared by all worktrees.\n */\n-\n-/*\n- * Constructs a path into the common git directory of repository `repo` and\n- * append it in the provided buffer `sb`.\n- */\n-void strbuf_git_common_path(struct strbuf *sb,\n-\t\t\t const struct repository *repo,\n-\t\t\t const char *fmt, ...)\n+char *repo_common_path(const struct repository *repo,\n+\t\t const char *fmt, ...)\n+\t__attribute__((format (printf, 2, 3)));\n+const char *repo_common_path_append(const struct repository *repo,\n+\t\t\t\t struct strbuf *sb,\n+\t\t\t\t const char *fmt, ...)\n+\t__attribute__((format (printf, 3, 4)));\n+const char *repo_common_path_replace(const struct repository *repo,\n+\t\t\t\t struct strbuf *sb,\n+\t\t\t\t const char *fmt, ...)\n \t__attribute__((format (printf, 3, 4)));\n-void repo_common_pathv(const struct repository *repo,\n-\t\t struct strbuf *buf,\n-\t\t const char *fmt,\n-\t\t va_list args);\n \n /*\n * The `repo_git_path` family of functions will construct a path into a repository's\n@@ -243,6 +241,12 @@ struct strbuf *get_pathname(void);\n # include \"strbuf.h\"\n # include \"repository.h\"\n \n+/* Internal implementation detail that should not be used. */\n+void repo_common_pathv(const struct repository *repo,\n+\t\t struct strbuf *buf,\n+\t\t const char *fmt,\n+\t\t va_list args);\n+\n /*\n * Return a statically allocated path into the main repository's\n * (the_repository) common git directory.\ndiff --git a/refs.c b/refs.c\nindex f4094a326a..daf6a84205 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2184,8 +2184,8 @@ struct ref_store *get_worktree_ref_store(const struct worktree *wt)\n \n \tif (wt->id) {\n \t\tstruct strbuf common_path = STRBUF_INIT;\n-\t\tstrbuf_git_common_path(&common_path, wt->repo,\n-\t\t\t\t \"worktrees/%s\", wt->id);\n+\t\trepo_common_path_append(wt->repo, &common_path,\n+\t\t\t\t\t\"worktrees/%s\", wt->id);\n \t\trefs = ref_store_init(wt->repo, wt->repo->ref_storage_format,\n \t\t\t\t common_path.buf, REF_STORE_ALL_CAPS);\n \t\tstrbuf_release(&common_path);\ndiff --git a/setup.c b/setup.c\nindex 8a488f3e7c..74b5ba5325 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -792,7 +792,7 @@ int upgrade_repository_format(int target_version)\n \tstruct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n \tint ret;\n \n-\tstrbuf_git_common_path(&sb, the_repository, \"config\");\n+\trepo_common_path_append(the_repository, &sb, \"config\");\n \tread_repository_format(&repo_fmt, sb.buf);\n \tstrbuf_release(&sb);\n \n@@ -2242,7 +2242,7 @@ void initialize_repository_version(int hash_algo,\n \t\tstruct strbuf config = STRBUF_INIT;\n \t\tstruct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n \n-\t\tstrbuf_git_common_path(&config, the_repository, \"config\");\n+\t\trepo_common_path_append(the_repository, &config, \"config\");\n \t\tread_repository_format(&repo_fmt, config.buf);\n \n \t\tif (repo_fmt.v1_only_extensions.nr)\ndiff --git a/worktree.c b/worktree.c\nindex 248bbb39d4..f8d6e7127f 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -104,7 +104,7 @@ struct worktree *get_linked_worktree(const char *id,\n \tif (!id)\n \t\tdie(\"Missing linked worktree name\");\n \n-\tstrbuf_git_common_path(&path, the_repository, \"worktrees/%s/gitdir\", id);\n+\trepo_common_path_append(the_repository, &path, \"worktrees/%s/gitdir\", id);\n \tif (strbuf_read_file(&worktree_path, path.buf, 0) <= 0)\n \t\t/* invalid gitdir file */\n \t\tgoto done;\n@@ -731,8 +731,7 @@ static ssize_t infer_backlink(const char *gitfile, struct strbuf *inferred)\n \tid++; /* advance past '/' to point at */\n \tif (!*id)\n \t\tgoto error;\n-\tstrbuf_reset(inferred);\n-\tstrbuf_git_common_path(inferred, the_repository, \"worktrees/%s\", id);\n+\trepo_common_path_replace(the_repository, inferred, \"worktrees/%s\", id);\n \tif (!is_directory(inferred->buf))\n \t\tgoto error;\n \n\n-- \n2.48.1.538.gc4cfc42d60.dirty\n\n"},{"id":"511969","messageId":"20250206-b4-pks-path-drop-the-repository-v1-2-4e77f0313206@pks.im","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-0-4e77f0313206@pks.im","subject":"[PATCH 02/16] path: refactor `repo_git_path()` family of functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-06T07:57:58Z","receivedAt":"2025-02-06T07:58:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"As explained in an earlier commit, we're refactoring path-related\nfunctions to provide a consistent interface for computing paths into the\ncommondir, gitdir and worktree. Refactor the \"gitdir\" family of\nfunctions accordingly.\n\nNote that the `repo_git_pathv()` function is converted into an internal\nimplementation detail. It is only used to implement `the_repository`\ncompatibility shims and will eventually be removed from the public\ninterface.\n\nSigned-off-by: Patrick Steinhardt \n---\n editor.c | 6 ++----\n hook.c | 3 +--\n path.c | 23 ++++++++++++++++++-----\n path.h | 34 ++++++++++++----------------------\n submodule.c | 4 ++--\n 5 files changed, 35 insertions(+), 35 deletions(-)\n\ndiff --git a/editor.c b/editor.c\nindex 6b9ce81d5f..b79d97b0e7 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -142,10 +142,8 @@ int strbuf_edit_interactively(struct repository *r,\n \tstruct strbuf sb = STRBUF_INIT;\n \tint fd, res = 0;\n \n-\tif (!is_absolute_path(path)) {\n-\t\tstrbuf_repo_git_path(&sb, r, \"%s\", path);\n-\t\tpath = sb.buf;\n-\t}\n+\tif (!is_absolute_path(path))\n+\t\tpath = repo_git_path_append(r, &sb, \"%s\", path);\n \n \tfd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);\n \tif (fd < 0)\ndiff --git a/hook.c b/hook.c\nindex 9ddbdee06d..b3de1048bf 100644\n--- a/hook.c\n+++ b/hook.c\n@@ -16,8 +16,7 @@ const char *find_hook(struct repository *r, const char *name)\n \n \tint found_hook;\n \n-\tstrbuf_reset(&path);\n-\tstrbuf_repo_git_path(&path, r, \"hooks/%s\", name);\n+\trepo_git_path_replace(r, &path, \"hooks/%s\", name);\n \tfound_hook = access(path.buf, X_OK) >= 0;\n #ifdef STRIP_EXTENSION\n \tif (!found_hook) {\ndiff --git a/path.c b/path.c\nindex 273b649e00..f386a5ff52 100644\n--- a/path.c\n+++ b/path.c\n@@ -417,7 +417,7 @@ static void strbuf_worktree_gitdir(struct strbuf *buf,\n \t\trepo_common_path_append(repo, buf, \"worktrees/%s\", wt->id);\n }\n \n-void repo_git_pathv(const struct repository *repo,\n+void repo_git_pathv(struct repository *repo,\n \t\t const struct worktree *wt, struct strbuf *buf,\n \t\t const char *fmt, va_list args)\n {\n@@ -432,7 +432,7 @@ void repo_git_pathv(const struct repository *repo,\n \tstrbuf_cleanup_path(buf);\n }\n \n-char *repo_git_path(const struct repository *repo,\n+char *repo_git_path(struct repository *repo,\n \t\t const char *fmt, ...)\n {\n \tstruct strbuf path = STRBUF_INIT;\n@@ -443,14 +443,27 @@ char *repo_git_path(const struct repository *repo,\n \treturn strbuf_detach(&path, NULL);\n }\n \n-void strbuf_repo_git_path(struct strbuf *sb,\n-\t\t\t const struct repository *repo,\n-\t\t\t const char *fmt, ...)\n+const char *repo_git_path_append(struct repository *repo,\n+\t\t\t\t struct strbuf *sb,\n+\t\t\t\t const char *fmt, ...)\n {\n \tva_list args;\n \tva_start(args, fmt);\n \trepo_git_pathv(repo, NULL, sb, fmt, args);\n \tva_end(args);\n+\treturn sb->buf;\n+}\n+\n+const char *repo_git_path_replace(struct repository *repo,\n+\t\t\t\t struct strbuf *sb,\n+\t\t\t\t const char *fmt, ...)\n+{\n+\tva_list args;\n+\tstrbuf_reset(sb);\n+\tva_start(args, fmt);\n+\trepo_git_pathv(repo, NULL, sb, fmt, args);\n+\tva_end(args);\n+\treturn sb->buf;\n }\n \n char *mkpathdup(const char *fmt, ...)\ndiff --git a/path.h b/path.h\nindex 3c75495e1a..c1cb3ce3be 100644\n--- a/path.h\n+++ b/path.h\n@@ -52,29 +52,16 @@ const char *repo_common_path_replace(const struct repository *repo,\n * For an exhaustive list of the adjustments made look at `common_list` and\n * `adjust_git_path` in path.c.\n */\n-\n-/*\n- * Return a path into the git directory of repository `repo`.\n- */\n-char *repo_git_path(const struct repository *repo,\n+char *repo_git_path(struct repository *repo,\n \t\t const char *fmt, ...)\n \t__attribute__((format (printf, 2, 3)));\n-\n-/*\n- * Print a path into the git directory of repository `repo` into the provided\n- * buffer.\n- */\n-void repo_git_pathv(const struct repository *repo,\n-\t\t const struct worktree *wt, struct strbuf *buf,\n-\t\t const char *fmt, va_list args);\n-\n-/*\n- * Construct a path into the git directory of repository `repo` and append it\n- * to the provided buffer `sb`.\n- */\n-void strbuf_repo_git_path(struct strbuf *sb,\n-\t\t\t const struct repository *repo,\n-\t\t\t const char *fmt, ...)\n+const char *repo_git_path_append(struct repository *repo,\n+\t\t\t\t struct strbuf *sb,\n+\t\t\t\t const char *fmt, ...)\n+\t__attribute__((format (printf, 3, 4)));\n+const char *repo_git_path_replace(struct repository *repo,\n+\t\t\t\t struct strbuf *sb,\n+\t\t\t\t const char *fmt, ...)\n \t__attribute__((format (printf, 3, 4)));\n \n /*\n@@ -241,11 +228,14 @@ struct strbuf *get_pathname(void);\n # include \"strbuf.h\"\n # include \"repository.h\"\n \n-/* Internal implementation detail that should not be used. */\n+/* Internal implementation details that should not be used. */\n void repo_common_pathv(const struct repository *repo,\n \t\t struct strbuf *buf,\n \t\t const char *fmt,\n \t\t va_list args);\n+void repo_git_pathv(struct repository *repo,\n+\t\t const struct worktree *wt, struct strbuf *buf,\n+\t\t const char *fmt, va_list args);\n \n /*\n * Return a statically allocated path into the main repository's\ndiff --git a/submodule.c b/submodule.c\nindex b361076c5b..211ead54a0 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1315,7 +1315,7 @@ static int repo_has_absorbed_submodules(struct repository *r)\n \tint ret;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tstrbuf_repo_git_path(&buf, r, \"modules/\");\n+\trepo_git_path_append(r, &buf, \"modules/\");\n \tret = file_exists(buf.buf) && !is_empty_dir(buf.buf);\n \tstrbuf_release(&buf);\n \treturn ret;\n@@ -2629,6 +2629,6 @@ void submodule_name_to_gitdir(struct strbuf *buf, struct repository *r,\n \t * administrators can explicitly set. Nothing has been decided,\n \t * so for now, just append the name at the end of the path.\n \t */\n-\tstrbuf_repo_git_path(buf, r, \"modules/\");\n+\trepo_git_path_append(r, buf, \"modules/\");\n \tstrbuf_addstr(buf, submodule_name);\n }\n\n-- \n2.48.1.538.gc4cfc42d60.dirty\n\n"},{"id":"511970","messageId":"20250206-b4-pks-path-drop-the-repository-v1-3-4e77f0313206@pks.im","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-0-4e77f0313206@pks.im","subject":"[PATCH 03/16] path: refactor `repo_worktree_path()` family of functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-06T07:57:59Z","receivedAt":"2025-02-06T07:58:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"As explained in an earlier commit, we're refactoring path-related\nfunctions to provide a consistent interface for computing paths into the\ncommondir, gitdir and worktree. Refactor the \"worktree\" family of\nfunctions accordingly.\n\nSigned-off-by: Patrick Steinhardt \n---\n path.c | 28 ++++++++++++++++++++++------\n path.h | 20 +++++++++-----------\n repository.c | 4 ++--\n 3 files changed, 33 insertions(+), 19 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex f386a5ff52..be75808acc 100644\n--- a/path.c\n+++ b/path.c\n@@ -519,28 +519,44 @@ char *repo_worktree_path(const struct repository *repo, const char *fmt, ...)\n \tstruct strbuf path = STRBUF_INIT;\n \tva_list args;\n \n+\tva_start(args, fmt);\n+\tdo_worktree_path(repo, &path, fmt, args);\n+\tva_end(args);\n+\n+\treturn strbuf_detach(&path, NULL);\n+}\n+\n+const char *repo_worktree_path_append(const struct repository *repo,\n+\t\t\t\t struct strbuf *sb,\n+\t\t\t\t const char *fmt, ...)\n+{\n+\tva_list args;\n+\n \tif (!repo->worktree)\n \t\treturn NULL;\n \n \tva_start(args, fmt);\n-\tdo_worktree_path(repo, &path, fmt, args);\n+\tdo_worktree_path(repo, sb, fmt, args);\n \tva_end(args);\n \n-\treturn strbuf_detach(&path, NULL);\n+\treturn sb->buf;\n }\n \n-void strbuf_repo_worktree_path(struct strbuf *sb,\n-\t\t\t const struct repository *repo,\n-\t\t\t const char *fmt, ...)\n+const char *repo_worktree_path_replace(const struct repository *repo,\n+\t\t\t\t struct strbuf *sb,\n+\t\t\t\t const char *fmt, ...)\n {\n \tva_list args;\n \n+\tstrbuf_reset(sb);\n \tif (!repo->worktree)\n-\t\treturn;\n+\t\treturn NULL;\n \n \tva_start(args, fmt);\n \tdo_worktree_path(repo, sb, fmt, args);\n \tva_end(args);\n+\n+\treturn sb->buf;\n }\n \n /* Returns 0 on success, negative on failure. */\ndiff --git a/path.h b/path.h\nindex c1cb3ce3be..8761c4c660 100644\n--- a/path.h\n+++ b/path.h\n@@ -75,24 +75,22 @@ const char *worktree_git_path(struct repository *r,\n \t__attribute__((format (printf, 3, 4)));\n \n /*\n- * Return a path into the worktree of repository `repo`.\n+ * The `repo_worktree_path` family of functions will construct a path into a\n+ * repository's worktree.\n *\n- * If the repository doesn't have a worktree NULL is returned.\n+ * Returns a `NULL` pointer in case the repository has no worktree.\n */\n char *repo_worktree_path(const struct repository *repo,\n \t\t\t\tconst char *fmt, ...)\n \t__attribute__((format (printf, 2, 3)));\n-\n-/*\n- * Construct a path into the worktree of repository `repo` and append it\n- * to the provided buffer `sb`.\n- *\n- * If the repository doesn't have a worktree nothing will be appended to `sb`.\n- */\n-void strbuf_repo_worktree_path(struct strbuf *sb,\n-\t\t\t\t const struct repository *repo,\n+const char *repo_worktree_path_append(const struct repository *repo,\n+\t\t\t\t struct strbuf *sb,\n \t\t\t\t const char *fmt, ...)\n \t__attribute__((format (printf, 3, 4)));\n+const char *repo_worktree_path_replace(const struct repository *repo,\n+\t\t\t\t struct strbuf *sb,\n+\t\t\t\t const char *fmt, ...)\n+\t__attribute__((format (printf, 3, 4)));\n \n /*\n * Return a path into a submodule's git directory located at `path`. `path`\ndiff --git a/repository.c b/repository.c\nindex 1a6a62bbd0..648cd88474 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -312,8 +312,8 @@ int repo_submodule_init(struct repository *subrepo,\n \tstruct strbuf worktree = STRBUF_INIT;\n \tint ret = 0;\n \n-\tstrbuf_repo_worktree_path(&gitdir, superproject, \"%s/.git\", path);\n-\tstrbuf_repo_worktree_path(&worktree, superproject, \"%s\", path);\n+\trepo_worktree_path_append(superproject, &gitdir, \"%s/.git\", path);\n+\trepo_worktree_path_append(superproject, &worktree, \"%s\", path);\n \n \tif (repo_init(subrepo, gitdir.buf, worktree.buf)) {\n \t\t/*\n\n-- \n2.48.1.538.gc4cfc42d60.dirty\n\n"},{"id":"511971","messageId":"20250206-b4-pks-path-drop-the-repository-v1-4-4e77f0313206@pks.im","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-0-4e77f0313206@pks.im","subject":"[PATCH 04/16] submodule: refactor `submodule_to_gitdir()` to accept a repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-06T07:58:00Z","receivedAt":"2025-02-06T07:58:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `submodule_to_gitdir()` function implicitly uses `the_repository` to\nresolve submodule paths. Refactor the function to instead accept a repo\nas parameter to remove the dependency on global state.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/submodule--helper.c | 2 +-\n path.c | 2 +-\n refs.c | 2 +-\n submodule.c | 11 ++++++-----\n submodule.h | 3 ++-\n 5 files changed, 11 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f9b970f8a6..3a64f7e605 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1301,7 +1301,7 @@ static void sync_submodule(const char *path, const char *prefix,\n \tremote_key = xstrfmt(\"remote.%s.url\", default_remote);\n \tfree(default_remote);\n \n-\tsubmodule_to_gitdir(&sb, path);\n+\tsubmodule_to_gitdir(the_repository, &sb, path);\n \tstrbuf_addstr(&sb, \"/config\");\n \n \tif (git_config_set_in_file_gently(sb.buf, remote_key, NULL, sub_origin_url))\ndiff --git a/path.c b/path.c\nindex be75808acc..d918d0409e 100644\n--- a/path.c\n+++ b/path.c\n@@ -567,7 +567,7 @@ static int do_submodule_path(struct strbuf *buf, const char *path,\n \tstruct strbuf git_submodule_dir = STRBUF_INIT;\n \tint ret;\n \n-\tret = submodule_to_gitdir(&git_submodule_dir, path);\n+\tret = submodule_to_gitdir(the_repository, &git_submodule_dir, path);\n \tif (ret)\n \t\tgoto cleanup;\n \ndiff --git a/refs.c b/refs.c\nindex daf6a84205..e1293e53aa 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2146,7 +2146,7 @@ struct ref_store *repo_get_submodule_ref_store(struct repository *repo,\n \tif (!is_nonbare_repository_dir(&submodule_sb))\n \t\tgoto done;\n \n-\tif (submodule_to_gitdir(&submodule_sb, submodule))\n+\tif (submodule_to_gitdir(repo, &submodule_sb, submodule))\n \t\tgoto done;\n \n \tsubrepo = xmalloc(sizeof(*subrepo));\ndiff --git a/submodule.c b/submodule.c\nindex 211ead54a0..0530e8cf24 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -536,7 +536,8 @@ static struct repository *open_submodule(const char *path)\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct repository *out = xmalloc(sizeof(*out));\n \n-\tif (submodule_to_gitdir(&sb, path) || repo_init(out, sb.buf, NULL)) {\n+\tif (submodule_to_gitdir(the_repository, &sb, path) ||\n+\t repo_init(out, sb.buf, NULL)) {\n \t\tstrbuf_release(&sb);\n \t\tfree(out);\n \t\treturn NULL;\n@@ -2572,7 +2573,8 @@ int get_superproject_working_tree(struct strbuf *buf)\n * Put the gitdir for a submodule (given relative to the main\n * repository worktree) into `buf`, or return -1 on error.\n */\n-int submodule_to_gitdir(struct strbuf *buf, const char *submodule)\n+int submodule_to_gitdir(struct repository *repo,\n+\t\t\tstruct strbuf *buf, const char *submodule)\n {\n \tconst struct submodule *sub;\n \tconst char *git_dir;\n@@ -2592,14 +2594,13 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule)\n \t\tstrbuf_addstr(buf, git_dir);\n \t}\n \tif (!is_git_directory(buf->buf)) {\n-\t\tsub = submodule_from_path(the_repository, null_oid(),\n-\t\t\t\t\t submodule);\n+\t\tsub = submodule_from_path(repo, null_oid(), submodule);\n \t\tif (!sub) {\n \t\t\tret = -1;\n \t\t\tgoto cleanup;\n \t\t}\n \t\tstrbuf_reset(buf);\n-\t\tsubmodule_name_to_gitdir(buf, the_repository, sub->name);\n+\t\tsubmodule_name_to_gitdir(buf, repo, sub->name);\n \t}\n \n cleanup:\ndiff --git a/submodule.h b/submodule.h\nindex 4deb1b5f84..db980c1d08 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -136,7 +136,8 @@ int push_unpushed_submodules(struct repository *r,\n * path of that submodule in 'buf'. Return -1 on error or when the\n * submodule is not initialized.\n */\n-int submodule_to_gitdir(struct strbuf *buf, const char *submodule);\n+int submodule_to_gitdir(struct repository *repo,\n+\t\t\tstruct strbuf *buf, const char *submodule);\n \n /*\n * Given a submodule name, create a path to where the submodule's gitdir lives\n\n-- \n2.48.1.538.gc4cfc42d60.dirty\n\n"},{"id":"511972","messageId":"20250206-b4-pks-path-drop-the-repository-v1-5-4e77f0313206@pks.im","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-0-4e77f0313206@pks.im","subject":"[PATCH 05/16] path: refactor `repo_submodule_path()` family of functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-06T07:58:01Z","receivedAt":"2025-02-06T07:58:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"As explained in an earlier commit, we're refactoring path-related\nfunctions to provide a consistent interface for computing paths into the\ncommondir, gitdir and worktree. Refactor the \"submodule\" family of\nfunctions accordingly.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/submodule--helper.c | 2 +-\n path.c | 37 +++++++++++++++++++++++++++++--------\n path.h | 30 ++++++++++++++++++------------\n t/helper/test-ref-store.c | 7 +++----\n worktree.c | 3 ++-\n 5 files changed, 53 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 3a64f7e605..c1a8029714 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1826,7 +1826,7 @@ static int clone_submodule(const struct module_clone_data *clone_data,\n \n \tconnect_work_tree_and_git_dir(clone_data_path, sm_gitdir, 0);\n \n-\tp = git_pathdup_submodule(clone_data_path, \"config\");\n+\tp = repo_submodule_path(the_repository, clone_data_path, \"config\");\n \tif (!p)\n \t\tdie(_(\"could not get submodule directory for '%s'\"), clone_data_path);\n \ndiff --git a/path.c b/path.c\nindex d918d0409e..d721507be8 100644\n--- a/path.c\n+++ b/path.c\n@@ -560,14 +560,15 @@ const char *repo_worktree_path_replace(const struct repository *repo,\n }\n \n /* Returns 0 on success, negative on failure. */\n-static int do_submodule_path(struct strbuf *buf, const char *path,\n+static int do_submodule_path(struct repository *repo,\n+\t\t\t struct strbuf *buf, const char *path,\n \t\t\t const char *fmt, va_list args)\n {\n \tstruct strbuf git_submodule_common_dir = STRBUF_INIT;\n \tstruct strbuf git_submodule_dir = STRBUF_INIT;\n \tint ret;\n \n-\tret = submodule_to_gitdir(the_repository, &git_submodule_dir, path);\n+\tret = submodule_to_gitdir(repo, &git_submodule_dir, path);\n \tif (ret)\n \t\tgoto cleanup;\n \n@@ -586,13 +587,14 @@ static int do_submodule_path(struct strbuf *buf, const char *path,\n \treturn ret;\n }\n \n-char *git_pathdup_submodule(const char *path, const char *fmt, ...)\n+char *repo_submodule_path(struct repository *repo,\n+\t\t\t const char *path, const char *fmt, ...)\n {\n \tint err;\n \tva_list args;\n \tstruct strbuf buf = STRBUF_INIT;\n \tva_start(args, fmt);\n-\terr = do_submodule_path(&buf, path, fmt, args);\n+\terr = do_submodule_path(repo, &buf, path, fmt, args);\n \tva_end(args);\n \tif (err) {\n \t\tstrbuf_release(&buf);\n@@ -601,16 +603,35 @@ char *git_pathdup_submodule(const char *path, const char *fmt, ...)\n \treturn strbuf_detach(&buf, NULL);\n }\n \n-int strbuf_git_path_submodule(struct strbuf *buf, const char *path,\n-\t\t\t const char *fmt, ...)\n+const char *repo_submodule_path_append(struct repository *repo,\n+\t\t\t\t struct strbuf *buf,\n+\t\t\t\t const char *path,\n+\t\t\t\t const char *fmt, ...)\n {\n \tint err;\n \tva_list args;\n \tva_start(args, fmt);\n-\terr = do_submodule_path(buf, path, fmt, args);\n+\terr = do_submodule_path(repo, buf, path, fmt, args);\n \tva_end(args);\n+\tif (err)\n+\t\treturn NULL;\n+\treturn buf->buf;\n+}\n \n-\treturn err;\n+const char *repo_submodule_path_replace(struct repository *repo,\n+\t\t\t\t\tstruct strbuf *buf,\n+\t\t\t\t\tconst char *path,\n+\t\t\t\t\tconst char *fmt, ...)\n+{\n+\tint err;\n+\tva_list args;\n+\tstrbuf_reset(buf);\n+\tva_start(args, fmt);\n+\terr = do_submodule_path(repo, buf, path, fmt, args);\n+\tva_end(args);\n+\tif (err)\n+\t\treturn NULL;\n+\treturn buf->buf;\n }\n \n void repo_common_pathv(const struct repository *repo,\ndiff --git a/path.h b/path.h\nindex 8761c4c660..63a8f91947 100644\n--- a/path.h\n+++ b/path.h\n@@ -93,20 +93,26 @@ const char *repo_worktree_path_replace(const struct repository *repo,\n \t__attribute__((format (printf, 3, 4)));\n \n /*\n- * Return a path into a submodule's git directory located at `path`. `path`\n- * must only reference a submodule of the main repository (the_repository).\n- */\n-char *git_pathdup_submodule(const char *path, const char *fmt, ...)\n-\t__attribute__((format (printf, 2, 3)));\n-\n-/*\n- * Construct a path into a submodule's git directory located at `path` and\n- * append it to the provided buffer `sb`. `path` must only reference a\n- * submodule of the main repository (the_repository).\n+ * The `repo_submodule_path` family of functions will construct a path into a\n+ * submodule's git directory located at `path`. `path` must be a submodule path\n+ * as found in the index and must be part of the given repository.\n+ *\n+ * Returns a `NULL` pointer in case the submodule cannot be found.\n */\n-int strbuf_git_path_submodule(struct strbuf *sb, const char *path,\n-\t\t\t\t const char *fmt, ...)\n+char *repo_submodule_path(struct repository *repo,\n+\t\t\t const char *path,\n+\t\t\t const char *fmt, ...)\n \t__attribute__((format (printf, 3, 4)));\n+const char *repo_submodule_path_append(struct repository *repo,\n+\t\t\t\t struct strbuf *sb,\n+\t\t\t\t const char *path,\n+\t\t\t\t const char *fmt, ...)\n+\t__attribute__((format (printf, 4, 5)));\n+const char *repo_submodule_path_replace(struct repository *repo,\n+\t\t\t\t\tstruct strbuf *sb,\n+\t\t\t\t\tconst char *path,\n+\t\t\t\t\tconst char *fmt, ...)\n+\t__attribute__((format (printf, 4, 5)));\n \n void report_linked_checkout_garbage(struct repository *r);\n \ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex 1cc05f043a..e00fce592b 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -75,11 +75,10 @@ static const char **get_store(const char **argv, struct ref_store **refs)\n \t\t*refs = get_main_ref_store(the_repository);\n \t} else if (skip_prefix(argv[0], \"submodule:\", &gitdir)) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n-\t\tint ret;\n \n-\t\tret = strbuf_git_path_submodule(&sb, gitdir, \"objects/\");\n-\t\tif (ret)\n-\t\t\tdie(\"strbuf_git_path_submodule failed: %d\", ret);\n+\t\tif (!repo_submodule_path_append(the_repository,\n+\t\t\t\t\t\t&sb, gitdir, \"objects/\"))\n+\t\t\tdie(\"computing submodule path failed\");\n \t\tadd_to_alternates_memory(sb.buf);\n \t\tstrbuf_release(&sb);\n \ndiff --git a/worktree.c b/worktree.c\nindex f8d6e7127f..8f4fc10c44 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -487,7 +487,8 @@ int submodule_uses_worktrees(const char *path)\n \tint ret = 0;\n \tstruct repository_format format = REPOSITORY_FORMAT_INIT;\n \n-\tsubmodule_gitdir = git_pathdup_submodule(path, \"%s\", \"\");\n+\tsubmodule_gitdir = repo_submodule_path(the_repository,\n+\t\t\t\t\t path, \"%s\", \"\");\n \tif (!submodule_gitdir)\n \t\treturn 0;\n \n\n-- \n2.48.1.538.gc4cfc42d60.dirty\n\n"},{"id":"511973","messageId":"20250206-b4-pks-path-drop-the-repository-v1-6-4e77f0313206@pks.im","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-0-4e77f0313206@pks.im","subject":"[PATCH 06/16] path: drop unused `strbuf_git_path()` function","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-06T07:58:02Z","receivedAt":"2025-02-06T07:58:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `strbuf_git_path()` function isn't used anywhere, and neither should\nit grow any callers because it depends on `the_repository`. Remove it.\n\nSigned-off-by: Patrick Steinhardt \n---\n path.h | 13 -------------\n 1 file changed, 13 deletions(-)\n\ndiff --git a/path.h b/path.h\nindex 63a8f91947..cebc93cb0a 100644\n--- a/path.h\n+++ b/path.h\n@@ -272,19 +272,6 @@ static inline char *git_path_buf(struct strbuf *buf, const char *fmt, ...)\n \treturn buf->buf;\n }\n \n-/*\n- * Construct a path into the main repository's (the_repository) git directory\n- * and append it to the provided buffer `sb`.\n- */\n-__attribute__((format (printf, 2, 3)))\n-static inline void strbuf_git_path(struct strbuf *sb, const char *fmt, ...)\n-{\n-\tva_list args;\n-\tva_start(args, fmt);\n-\trepo_git_pathv(the_repository, NULL, sb, fmt, args);\n-\tva_end(args);\n-}\n-\n /*\n * Return a statically allocated path into the main repository's\n * (the_repository) git directory.\n\n-- \n2.48.1.538.gc4cfc42d60.dirty\n\n"},{"id":"511974","messageId":"20250206-b4-pks-path-drop-the-repository-v1-7-4e77f0313206@pks.im","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-0-4e77f0313206@pks.im","subject":"[PATCH 07/16] path: drop `git_pathdup()` in favor of `repo_git_path()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-06T07:58:03Z","receivedAt":"2025-02-06T07:58:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Remove `git_pathdup()` in favor of `repo_git_path()`. The latter does\nessentially the same, with the only exception that it does not rely on\n`the_repository` but takes the repo as separate parameter.\n\nSigned-off-by: Patrick Steinhardt \n---\n bisect.c | 2 +-\n builtin/am.c | 2 +-\n builtin/clone.c | 2 +-\n builtin/config.c | 8 ++++----\n builtin/fast-import.c | 4 ++--\n builtin/fsck.c | 2 +-\n builtin/gc.c | 4 ++--\n builtin/notes.c | 2 +-\n builtin/replace.c | 2 +-\n builtin/tag.c | 2 +-\n builtin/worktree.c | 4 ++--\n dir.c | 2 +-\n http-backend.c | 2 +-\n notes-merge.c | 2 +-\n object-file.c | 2 +-\n path.h | 16 +---------------\n 16 files changed, 22 insertions(+), 36 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 7a3c77c6d8..269a98bf97 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -930,7 +930,7 @@ static enum bisect_error check_good_are_ancestors_of_bad(struct repository *r,\n \tif (!current_bad_oid)\n \t\treturn error(_(\"a %s revision is needed\"), term_bad);\n \n-\tfilename = git_pathdup(\"BISECT_ANCESTORS_OK\");\n+\tfilename = repo_git_path(the_repository, \"BISECT_ANCESTORS_OK\");\n \n \t/* Check if file BISECT_ANCESTORS_OK exists. */\n \tif (!stat(filename, &st) && S_ISREG(st.st_mode))\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 390b463144..2921bb89ef 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -158,7 +158,7 @@ static void am_state_init(struct am_state *state)\n \n \tmemset(state, 0, sizeof(*state));\n \n-\tstate->dir = git_pathdup(\"rebase-apply\");\n+\tstate->dir = repo_git_path(the_repository, \"rebase-apply\");\n \n \tstate->prec = 4;\n \ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex fd001d800c..5ae6ee9db9 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -938,7 +938,7 @@ static void write_refspec_config(const char *src_ref_prefix,\n \n static void dissociate_from_references(void)\n {\n-\tchar *alternates = git_pathdup(\"objects/info/alternates\");\n+\tchar *alternates = repo_git_path(the_repository, \"objects/info/alternates\");\n \n \tif (!access(alternates, F_OK)) {\n \t\tstruct child_process cmd = CHILD_PROCESS_INIT;\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 16e6e30555..53a90094e3 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -775,13 +775,13 @@ static void location_options_init(struct config_location_options *opts,\n \t\topts->source.file = opts->file_to_free = git_system_config();\n \t\topts->source.scope = CONFIG_SCOPE_SYSTEM;\n \t} else if (opts->use_local_config) {\n-\t\topts->source.file = opts->file_to_free = git_pathdup(\"config\");\n+\t\topts->source.file = opts->file_to_free = repo_git_path(the_repository, \"config\");\n \t\topts->source.scope = CONFIG_SCOPE_LOCAL;\n \t} else if (opts->use_worktree_config) {\n \t\tstruct worktree **worktrees = get_worktrees();\n \t\tif (the_repository->repository_format_worktree_config)\n \t\t\topts->source.file = opts->file_to_free =\n-\t\t\t\tgit_pathdup(\"config.worktree\");\n+\t\t\t\trepo_git_path(the_repository, \"config.worktree\");\n \t\telse if (worktrees[0] && worktrees[1])\n \t\t\tdie(_(\"--worktree cannot be used with multiple \"\n \t\t\t \"working trees unless the config\\n\"\n@@ -790,7 +790,7 @@ static void location_options_init(struct config_location_options *opts,\n \t\t\t \"section in \\\"git help worktree\\\" for details\"));\n \t\telse\n \t\t\topts->source.file = opts->file_to_free =\n-\t\t\t\tgit_pathdup(\"config\");\n+\t\t\t\trepo_git_path(the_repository, \"config\");\n \t\topts->source.scope = CONFIG_SCOPE_LOCAL;\n \t\tfree_worktrees(worktrees);\n \t} else if (opts->source.file) {\n@@ -1087,7 +1087,7 @@ static int show_editor(struct config_location_options *opts)\n \tgit_config(git_default_config, NULL);\n \tconfig_file = opts->source.file ?\n \t\t\txstrdup(opts->source.file) :\n-\t\t\tgit_pathdup(\"config\");\n+\t\t\trepo_git_path(the_repository, \"config\");\n \tif (opts->use_global_config) {\n \t\tint fd = open(config_file, O_CREAT | O_EXCL | O_WRONLY, 0666);\n \t\tif (fd >= 0) {\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex a6a84058cd..c6f5147e8b 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -328,7 +328,7 @@ static void write_branch_report(FILE *rpt, struct branch *b)\n \n static void write_crash_report(const char *err)\n {\n-\tchar *loc = git_pathdup(\"fast_import_crash_%\"PRIuMAX, (uintmax_t) getpid());\n+\tchar *loc = repo_git_path(the_repository, \"fast_import_crash_%\"PRIuMAX, (uintmax_t) getpid());\n \tFILE *rpt = fopen(loc, \"w\");\n \tstruct branch *b;\n \tunsigned long lu;\n@@ -3280,7 +3280,7 @@ static char* make_fast_import_path(const char *path)\n {\n \tif (!relative_marks_paths || is_absolute_path(path))\n \t\treturn prefix_filename(global_prefix, path);\n-\treturn git_pathdup(\"info/fast-import/%s\", path);\n+\treturn repo_git_path(the_repository, \"info/fast-import/%s\", path);\n }\n \n static void option_import_marks(const char *marks,\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 7a4dcb0716..c12203e012 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -326,7 +326,7 @@ static void check_unreachable_object(struct object *obj)\n \t\t\t\t printable_type(&obj->oid, obj->type),\n \t\t\t\t describe_object(&obj->oid));\n \t\tif (write_lost_and_found) {\n-\t\t\tchar *filename = git_pathdup(\"lost-found/%s/%s\",\n+\t\t\tchar *filename = repo_git_path(the_repository, \"lost-found/%s/%s\",\n \t\t\t\tobj->type == OBJ_COMMIT ? \"commit\" : \"other\",\n \t\t\t\tdescribe_object(&obj->oid));\n \t\t\tFILE *f;\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 0bf3533494..57f6aee174 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -546,7 +546,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \tif (xgethostname(my_host, sizeof(my_host)))\n \t\txsnprintf(my_host, sizeof(my_host), \"unknown\");\n \n-\tpidfile_path = git_pathdup(\"gc.pid\");\n+\tpidfile_path = repo_git_path(the_repository, \"gc.pid\");\n \tfd = hold_lock_file_for_update(&lock, pidfile_path,\n \t\t\t\t LOCK_DIE_ON_ERROR);\n \tif (!force) {\n@@ -607,7 +607,7 @@ static int report_last_gc_error(void)\n \tint ret = 0;\n \tssize_t len;\n \tstruct stat st;\n-\tchar *gc_log_path = git_pathdup(\"gc.log\");\n+\tchar *gc_log_path = repo_git_path(the_repository, \"gc.log\");\n \n \tif (stat(gc_log_path, &st)) {\n \t\tif (errno == ENOENT)\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex d051abf6df..18bcbb2f91 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -197,7 +197,7 @@ static void prepare_note_data(const struct object_id *object, struct note_data *\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n \t\t/* write the template message before editing: */\n-\t\td->edit_path = git_pathdup(\"NOTES_EDITMSG\");\n+\t\td->edit_path = repo_git_path(the_repository, \"NOTES_EDITMSG\");\n \t\tfd = xopen(d->edit_path, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n \n \t\tif (d->msg_nr)\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex a4eaadff91..15ec0922ce 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -345,7 +345,7 @@ static int edit_and_replace(const char *object_ref, int force, int raw)\n \t}\n \tstrbuf_release(&ref);\n \n-\ttmpfile = git_pathdup(\"REPLACE_EDITOBJ\");\n+\ttmpfile = repo_git_path(the_repository, \"REPLACE_EDITOBJ\");\n \tif (export_object(&old_oid, type, raw, tmpfile)) {\n \t\tfree(tmpfile);\n \t\treturn -1;\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex e8a344b926..d3e0943b73 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -667,7 +667,7 @@ int cmd_tag(int argc,\n \tif (create_tag_object) {\n \t\tif (force_sign_annotate && !annotate)\n \t\t\topt.sign = 1;\n-\t\tpath = git_pathdup(\"TAG_EDITMSG\");\n+\t\tpath = repo_git_path(the_repository, \"TAG_EDITMSG\");\n \t\tcreate_tag(&object, object_ref, tag, &buf, &opt, &prev, &object,\n \t\t\t &trailer_args, path);\n \t}\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex c043d4d523..c84e6aa2cb 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -337,7 +337,7 @@ static void check_candidate_path(const char *path,\n \n static void copy_sparse_checkout(const char *worktree_git_dir)\n {\n-\tchar *from_file = git_pathdup(\"info/sparse-checkout\");\n+\tchar *from_file = repo_git_path(the_repository, \"info/sparse-checkout\");\n \tchar *to_file = xstrfmt(\"%s/info/sparse-checkout\", worktree_git_dir);\n \n \tif (file_exists(from_file)) {\n@@ -353,7 +353,7 @@ static void copy_sparse_checkout(const char *worktree_git_dir)\n \n static void copy_filtered_worktree_config(const char *worktree_git_dir)\n {\n-\tchar *from_file = git_pathdup(\"config.worktree\");\n+\tchar *from_file = repo_git_path(the_repository, \"config.worktree\");\n \tchar *to_file = xstrfmt(\"%s/config.worktree\", worktree_git_dir);\n \n \tif (file_exists(from_file)) {\ndiff --git a/dir.c b/dir.c\nindex 5b2181e589..4122f6513d 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -3455,7 +3455,7 @@ void setup_standard_excludes(struct dir_struct *dir)\n \n char *get_sparse_checkout_filename(void)\n {\n-\treturn git_pathdup(\"info/sparse-checkout\");\n+\treturn repo_git_path(the_repository, \"info/sparse-checkout\");\n }\n \n int get_sparse_checkout_patterns(struct pattern_list *pl)\ndiff --git a/http-backend.c b/http-backend.c\nindex 33cf378282..50b2858fad 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -183,7 +183,7 @@ static void send_strbuf(struct strbuf *hdr,\n static void send_local_file(struct strbuf *hdr, const char *the_type,\n \t\t\t\tconst char *name)\n {\n-\tchar *p = git_pathdup(\"%s\", name);\n+\tchar *p = repo_git_path(the_repository, \"%s\", name);\n \tsize_t buf_alloc = 8192;\n \tchar *buf = xmalloc(buf_alloc);\n \tint fd;\ndiff --git a/notes-merge.c b/notes-merge.c\nindex 8d701ed428..c997c0c1e3 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -309,7 +309,7 @@ static void write_buf_to_worktree(const struct object_id *obj,\n \t\t\t\t const char *buf, unsigned long size)\n {\n \tint fd;\n-\tchar *path = git_pathdup(NOTES_MERGE_WORKTREE \"/%s\", oid_to_hex(obj));\n+\tchar *path = repo_git_path(the_repository, NOTES_MERGE_WORKTREE \"/%s\", oid_to_hex(obj));\n \tif (safe_create_leading_directories_const(path))\n \t\tdie_errno(\"unable to create directory for '%s'\", path);\n \ndiff --git a/object-file.c b/object-file.c\nindex 6ce1caacae..335cc2a5da 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -717,7 +717,7 @@ static void read_info_alternates(struct repository *r,\n void add_to_alternates_file(const char *reference)\n {\n \tstruct lock_file lock = LOCK_INIT;\n-\tchar *alts = git_pathdup(\"objects/info/alternates\");\n+\tchar *alts = repo_git_path(the_repository, \"objects/info/alternates\");\n \tFILE *in, *out;\n \tint found = 0;\n \ndiff --git a/path.h b/path.h\nindex cebc93cb0a..df16dd23bd 100644\n--- a/path.h\n+++ b/path.h\n@@ -292,24 +292,10 @@ static inline const char *git_path(const char *fmt, ...)\n \t{ \\\n \t\tstatic char *ret; \\\n \t\tif (!ret) \\\n-\t\t\tret = git_pathdup(filename); \\\n+\t\t\tret = repo_git_path(the_repository, filename); \\\n \t\treturn ret; \\\n \t}\n \n-/*\n- * Return a path into the main repository's (the_repository) git directory.\n- */\n-__attribute__((format (printf, 1, 2)))\n-static inline char *git_pathdup(const char *fmt, ...)\n-{\n-\tstruct strbuf path = STRBUF_INIT;\n-\tva_list args;\n-\tva_start(args, fmt);\n-\trepo_git_pathv(the_repository, NULL, &path, fmt, args);\n-\tva_end(args);\n-\treturn strbuf_detach(&path, NULL);\n-}\n-\n # endif /* USE_THE_REPOSITORY_VARIABLE */\n \n #endif /* PATH_H */\n\n-- \n2.48.1.538.gc4cfc42d60.dirty\n\n"},{"id":"511975","messageId":"20250206-b4-pks-path-drop-the-repository-v1-8-4e77f0313206@pks.im","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-0-4e77f0313206@pks.im","subject":"[PATCH 08/16] path: drop `git_path_buf()` in favor of `repo_git_path_replace()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-06T07:58:04Z","receivedAt":"2025-02-06T07:58:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Remove `git_path_buf()` in favor of `repo_git_path_replace()`. The\nlatter does essentially the same, with the only exception that it does\nnot rely on `the_repository` but takes the repo as separate parameter.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/worktree.c | 2 +-\n compat/precompose_utf8.c | 6 +++---\n notes-merge.c | 4 ++--\n object-file.c | 4 ++--\n path.h | 16 ----------------\n setup.c | 33 ++++++++++++++++-----------------\n 6 files changed, 24 insertions(+), 41 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex c84e6aa2cb..7959b10d26 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -457,7 +457,7 @@ static int add_worktree(const char *path, const char *refname,\n \t\tBUG(\"How come '%s' becomes empty after sanitization?\", sb.buf);\n \tstrbuf_reset(&sb);\n \tname = sb_name.buf;\n-\tgit_path_buf(&sb_repo, \"worktrees/%s\", name);\n+\trepo_git_path_replace(the_repository, &sb_repo, \"worktrees/%s\", name);\n \tlen = sb_repo.len;\n \tif (safe_create_leading_directories_const(sb_repo.buf))\n \t\tdie_errno(_(\"could not create leading directories of '%s'\"),\ndiff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c\nindex f7cc7b3be5..12e38e0ea3 100644\n--- a/compat/precompose_utf8.c\n+++ b/compat/precompose_utf8.c\n@@ -50,15 +50,15 @@ void probe_utf8_pathname_composition(void)\n \tint output_fd;\n \tif (precomposed_unicode != -1)\n \t\treturn; /* We found it defined in the global config, respect it */\n-\tgit_path_buf(&path, \"%s\", auml_nfc);\n+\trepo_git_path_replace(the_repository, &path, \"%s\", auml_nfc);\n \toutput_fd = open(path.buf, O_CREAT|O_EXCL|O_RDWR, 0600);\n \tif (output_fd >= 0) {\n \t\tclose(output_fd);\n-\t\tgit_path_buf(&path, \"%s\", auml_nfd);\n+\t\trepo_git_path_replace(the_repository, &path, \"%s\", auml_nfd);\n \t\tprecomposed_unicode = access(path.buf, R_OK) ? 0 : 1;\n \t\tgit_config_set(\"core.precomposeunicode\",\n \t\t\t precomposed_unicode ? \"true\" : \"false\");\n-\t\tgit_path_buf(&path, \"%s\", auml_nfc);\n+\t\trepo_git_path_replace(the_repository, &path, \"%s\", auml_nfc);\n \t\tif (unlink(path.buf))\n \t\t\tdie_errno(_(\"failed to unlink '%s'\"), path.buf);\n \t}\ndiff --git a/notes-merge.c b/notes-merge.c\nindex c997c0c1e3..8c22a171c1 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -695,7 +695,7 @@ int notes_merge_commit(struct notes_merge_options *o,\n \tconst char *msg = strstr(buffer, \"\\n\\n\");\n \tint baselen;\n \n-\tgit_path_buf(&path, NOTES_MERGE_WORKTREE);\n+\trepo_git_path_replace(the_repository, &path, NOTES_MERGE_WORKTREE);\n \tif (o->verbosity >= 3)\n \t\tprintf(\"Committing notes in notes merge worktree at %s\\n\",\n \t\t\tpath.buf);\n@@ -757,7 +757,7 @@ int notes_merge_abort(struct notes_merge_options *o)\n \tstruct strbuf buf = STRBUF_INIT;\n \tint ret;\n \n-\tgit_path_buf(&buf, NOTES_MERGE_WORKTREE);\n+\trepo_git_path_replace(the_repository, &buf, NOTES_MERGE_WORKTREE);\n \tif (o->verbosity >= 3)\n \t\tprintf(\"Removing notes merge worktree at %s/*\\n\", buf.buf);\n \tret = remove_dir_recursively(&buf, REMOVE_DIR_KEEP_TOPLEVEL);\ndiff --git a/object-file.c b/object-file.c\nindex 335cc2a5da..dc9fcaf3e9 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -476,14 +476,14 @@ int odb_mkstemp(struct strbuf *temp_filename, const char *pattern)\n \t * restrictive except to remove write permission.\n \t */\n \tint mode = 0444;\n-\tgit_path_buf(temp_filename, \"objects/%s\", pattern);\n+\trepo_git_path_replace(the_repository, temp_filename, \"objects/%s\", pattern);\n \tfd = git_mkstemp_mode(temp_filename->buf, mode);\n \tif (0 <= fd)\n \t\treturn fd;\n \n \t/* slow path */\n \t/* some mkstemp implementations erase temp_filename on failure */\n-\tgit_path_buf(temp_filename, \"objects/%s\", pattern);\n+\trepo_git_path_replace(the_repository, temp_filename, \"objects/%s\", pattern);\n \tsafe_create_leading_directories(temp_filename->buf);\n \treturn xmkstemp_mode(temp_filename->buf, mode);\n }\ndiff --git a/path.h b/path.h\nindex df16dd23bd..904eeac068 100644\n--- a/path.h\n+++ b/path.h\n@@ -256,22 +256,6 @@ static inline const char *git_common_path(const char *fmt, ...)\n \treturn pathname->buf;\n }\n \n-/*\n- * Construct a path into the main repository's (the_repository) git directory\n- * and place it in the provided buffer `buf`, the contents of the buffer will\n- * be overridden.\n- */\n-__attribute__((format (printf, 2, 3)))\n-static inline char *git_path_buf(struct strbuf *buf, const char *fmt, ...)\n-{\n-\tva_list args;\n-\tstrbuf_reset(buf);\n-\tva_start(args, fmt);\n-\trepo_git_pathv(the_repository, NULL, buf, fmt, args);\n-\tva_end(args);\n-\treturn buf->buf;\n-}\n-\n /*\n * Return a statically allocated path into the main repository's\n * (the_repository) git directory.\ndiff --git a/setup.c b/setup.c\nindex 74b5ba5325..30889386f7 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -2264,7 +2264,7 @@ static int is_reinit(void)\n \tchar junk[2];\n \tint ret;\n \n-\tgit_path_buf(&buf, \"HEAD\");\n+\trepo_git_path_replace(the_repository, &buf, \"HEAD\");\n \tret = !access(buf.buf, R_OK) || readlink(buf.buf, junk, sizeof(junk) - 1) != -1;\n \tstrbuf_release(&buf);\n \treturn ret;\n@@ -2316,8 +2316,7 @@ static int create_default_files(const char *template_path,\n \t\t\t\tint init_shared_repository)\n {\n \tstruct stat st1;\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tchar *path;\n+\tstruct strbuf path = STRBUF_INIT;\n \tint reinit;\n \tint filemode;\n \tconst char *work_tree = repo_get_work_tree(the_repository);\n@@ -2358,14 +2357,14 @@ static int create_default_files(const char *template_path,\n \tinitialize_repository_version(fmt->hash_algo, fmt->ref_storage_format, reinit);\n \n \t/* Check filemode trustability */\n-\tpath = git_path_buf(&buf, \"config\");\n+\trepo_git_path_replace(the_repository, &path, \"config\");\n \tfilemode = TEST_FILEMODE;\n-\tif (TEST_FILEMODE && !lstat(path, &st1)) {\n+\tif (TEST_FILEMODE && !lstat(path.buf, &st1)) {\n \t\tstruct stat st2;\n-\t\tfilemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&\n-\t\t\t\t!lstat(path, &st2) &&\n+\t\tfilemode = (!chmod(path.buf, st1.st_mode ^ S_IXUSR) &&\n+\t\t\t\t!lstat(path.buf, &st2) &&\n \t\t\t\tst1.st_mode != st2.st_mode &&\n-\t\t\t\t!chmod(path, st1.st_mode));\n+\t\t\t\t!chmod(path.buf, st1.st_mode));\n \t\tif (filemode && !reinit && (st1.st_mode & S_IXUSR))\n \t\t\tfilemode = 0;\n \t}\n@@ -2384,24 +2383,24 @@ static int create_default_files(const char *template_path,\n \n \tif (!reinit) {\n \t\t/* Check if symlink is supported in the work tree */\n-\t\tpath = git_path_buf(&buf, \"tXXXXXX\");\n-\t\tif (!close(xmkstemp(path)) &&\n-\t\t !unlink(path) &&\n-\t\t !symlink(\"testing\", path) &&\n-\t\t !lstat(path, &st1) &&\n+\t\trepo_git_path_replace(the_repository, &path, \"tXXXXXX\");\n+\t\tif (!close(xmkstemp(path.buf)) &&\n+\t\t !unlink(path.buf) &&\n+\t\t !symlink(\"testing\", path.buf) &&\n+\t\t !lstat(path.buf, &st1) &&\n \t\t S_ISLNK(st1.st_mode))\n-\t\t\tunlink(path); /* good */\n+\t\t\tunlink(path.buf); /* good */\n \t\telse\n \t\t\tgit_config_set(\"core.symlinks\", \"false\");\n \n \t\t/* Check if the filesystem is case-insensitive */\n-\t\tpath = git_path_buf(&buf, \"CoNfIg\");\n-\t\tif (!access(path, F_OK))\n+\t\trepo_git_path_replace(the_repository, &path, \"CoNfIg\");\n+\t\tif (!access(path.buf, F_OK))\n \t\t\tgit_config_set(\"core.ignorecase\", \"true\");\n \t\tprobe_utf8_pathname_composition();\n \t}\n \n-\tstrbuf_release(&buf);\n+\tstrbuf_release(&path);\n \treturn reinit;\n }\n \n\n-- \n2.48.1.538.gc4cfc42d60.dirty\n\n"},{"id":"511976","messageId":"20250206-b4-pks-path-drop-the-repository-v1-9-4e77f0313206@pks.im","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-0-4e77f0313206@pks.im","subject":"[PATCH 09/16] worktree: return allocated string from `get_worktree_git_dir()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-06T07:58:05Z","receivedAt":"2025-02-06T07:58:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `get_worktree_git_dir()` function returns a string constant that\ndoes not need to be free'd by the caller. For `repo_get_git_dir()` and\n`repo_get_common_dir()` this is because we return strings owned by\n`the_repository`.\n\nBut for `git_common_path()` it's a bit less obvious though, because that\nfunction does end up allocating memory. This doesn't result in a memory\nleak either because we write into a buffer returned by `get_pathname()`,\nwhich returns one out of four static buffers.\n\nWe're about to drop `git_common_path()` in favor of `repo_common_path()`,\nwhich doesn't use the same mechanism but instead returns an allocated\nstring owned by the caller. While we could adapt `get_worktree_git_dir()`\nto also use `get_pathname()` and print the derived common path into that\nbuffer, the whole schema feels a lot like premature optimization in this\ncontext. There are some callsites where we call `get_worktree_git_dir()`\nin a loop that iterates through all worktrees. But none of these loops\nseem to be even remotely in the hot path, so saving a single allocation\nthere does not feel worth it.\n\nRefactor the function to instead consistently return an allocated path\nso that we can start using `repo_common_path()` in a subsequent commit.\n\nSigned-off-by: Patrick Steinhardt \n---\n branch.c | 7 +++++--\n builtin/fsck.c | 8 ++++++--\n builtin/receive-pack.c | 4 +++-\n builtin/worktree.c | 10 ++++++++--\n reachable.c | 6 +++++-\n revision.c | 7 ++++++-\n worktree.c | 11 ++++++-----\n worktree.h | 2 +-\n 8 files changed, 40 insertions(+), 15 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 77716966fe..91297d55ac 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -397,7 +397,7 @@ static void prepare_checked_out_branches(void)\n \tworktrees = get_worktrees();\n \n \twhile (worktrees[i]) {\n-\t\tchar *old;\n+\t\tchar *old, *wt_gitdir;\n \t\tstruct wt_status_state state = { 0 };\n \t\tstruct worktree *wt = worktrees[i++];\n \t\tstruct string_list update_refs = STRING_LIST_INIT_DUP;\n@@ -437,7 +437,8 @@ static void prepare_checked_out_branches(void)\n \t\t}\n \t\twt_status_state_free_buffers(&state);\n \n-\t\tif (!sequencer_get_update_refs_state(get_worktree_git_dir(wt),\n+\t\twt_gitdir = get_worktree_git_dir(wt);\n+\t\tif (!sequencer_get_update_refs_state(wt_gitdir,\n \t\t\t\t\t\t &update_refs)) {\n \t\t\tstruct string_list_item *item;\n \t\t\tfor_each_string_list_item(item, &update_refs) {\n@@ -448,6 +449,8 @@ static void prepare_checked_out_branches(void)\n \t\t\t}\n \t\t\tstring_list_clear(&update_refs, 1);\n \t\t}\n+\n+\t\tfree(wt_gitdir);\n \t}\n \n \tfree_worktrees(worktrees);\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex c12203e012..eea1d43647 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -1057,7 +1057,7 @@ int cmd_fsck(int argc,\n \t\t\tstruct worktree *wt = *p;\n \t\t\tstruct index_state istate =\n \t\t\t\tINDEX_STATE_INIT(the_repository);\n-\t\t\tchar *path;\n+\t\t\tchar *path, *wt_gitdir;\n \n \t\t\t/*\n \t\t\t * Make a copy since the buffer is reusable\n@@ -1065,9 +1065,13 @@ int cmd_fsck(int argc,\n \t\t\t * while we're examining the index.\n \t\t\t */\n \t\t\tpath = xstrdup(worktree_git_path(the_repository, wt, \"index\"));\n-\t\t\tread_index_from(&istate, path, get_worktree_git_dir(wt));\n+\t\t\twt_gitdir = get_worktree_git_dir(wt);\n+\n+\t\t\tread_index_from(&istate, path, wt_gitdir);\n \t\t\tfsck_index(&istate, path, wt->is_current);\n+\n \t\t\tdiscard_index(&istate);\n+\t\t\tfree(wt_gitdir);\n \t\t\tfree(path);\n \t\t}\n \t\tfree_worktrees(worktrees);\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex b7ea774609..d65a441f32 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1435,7 +1435,8 @@ static const char *push_to_checkout(unsigned char *hash,\n \n static const char *update_worktree(unsigned char *sha1, const struct worktree *worktree)\n {\n-\tconst char *retval, *git_dir;\n+\tconst char *retval;\n+\tchar *git_dir;\n \tstruct strvec env = STRVEC_INIT;\n \tint invoked_hook;\n \n@@ -1453,6 +1454,7 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w\n \t\tretval = push_to_deploy(sha1, &env, worktree->path);\n \n \tstrvec_clear(&env);\n+\tfree(git_dir);\n \treturn retval;\n }\n \ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 7959b10d26..2cea9441a6 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -657,8 +657,9 @@ static int can_use_local_refs(const struct add_opts *opts)\n \t\tif (!opts->quiet) {\n \t\t\tstruct strbuf path = STRBUF_INIT;\n \t\t\tstruct strbuf contents = STRBUF_INIT;\n+\t\t\tchar *wt_gitdir = get_worktree_git_dir(NULL);\n \n-\t\t\tstrbuf_add_real_path(&path, get_worktree_git_dir(NULL));\n+\t\t\tstrbuf_add_real_path(&path, wt_gitdir);\n \t\t\tstrbuf_addstr(&path, \"/HEAD\");\n \t\t\tstrbuf_read_file(&contents, path.buf, 64);\n \t\t\tstrbuf_stripspace(&contents, NULL);\n@@ -670,6 +671,7 @@ static int can_use_local_refs(const struct add_opts *opts)\n \t\t\t\t path.buf, contents.buf);\n \t\t\tstrbuf_release(&path);\n \t\t\tstrbuf_release(&contents);\n+\t\t\tfree(wt_gitdir);\n \t\t}\n \t\treturn 1;\n \t}\n@@ -1157,6 +1159,9 @@ static void validate_no_submodules(const struct worktree *wt)\n \tstruct index_state istate = INDEX_STATE_INIT(the_repository);\n \tstruct strbuf path = STRBUF_INIT;\n \tint i, found_submodules = 0;\n+\tchar *wt_gitdir;\n+\n+\twt_gitdir = get_worktree_git_dir(wt);\n \n \tif (is_directory(worktree_git_path(the_repository, wt, \"modules\"))) {\n \t\t/*\n@@ -1166,7 +1171,7 @@ static void validate_no_submodules(const struct worktree *wt)\n \t\t */\n \t\tfound_submodules = 1;\n \t} else if (read_index_from(&istate, worktree_git_path(the_repository, wt, \"index\"),\n-\t\t\t\t get_worktree_git_dir(wt)) > 0) {\n+\t\t\t\t wt_gitdir) > 0) {\n \t\tfor (i = 0; i < istate.cache_nr; i++) {\n \t\t\tstruct cache_entry *ce = istate.cache[i];\n \t\t\tint err;\n@@ -1185,6 +1190,7 @@ static void validate_no_submodules(const struct worktree *wt)\n \t}\n \tdiscard_index(&istate);\n \tstrbuf_release(&path);\n+\tfree(wt_gitdir);\n \n \tif (found_submodules)\n \t\tdie(_(\"working trees containing submodules cannot be moved or removed\"));\ndiff --git a/reachable.c b/reachable.c\nindex ecf7ccf504..9ee04c89ec 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -65,8 +65,10 @@ static void add_rebase_files(struct rev_info *revs)\n \tstruct worktree **worktrees = get_worktrees();\n \n \tfor (struct worktree **wt = worktrees; *wt; wt++) {\n+\t\tchar *wt_gitdir = get_worktree_git_dir(*wt);\n+\n \t\tstrbuf_reset(&buf);\n-\t\tstrbuf_addstr(&buf, get_worktree_git_dir(*wt));\n+\t\tstrbuf_addstr(&buf, wt_gitdir);\n \t\tstrbuf_complete(&buf, '/');\n \t\tlen = buf.len;\n \t\tfor (size_t i = 0; i < ARRAY_SIZE(path); i++) {\n@@ -74,6 +76,8 @@ static void add_rebase_files(struct rev_info *revs)\n \t\t\tstrbuf_addstr(&buf, path[i]);\n \t\t\tadd_one_file(buf.buf, revs);\n \t\t}\n+\n+\t\tfree(wt_gitdir);\n \t}\n \tstrbuf_release(&buf);\n \tfree_worktrees(worktrees);\ndiff --git a/revision.c b/revision.c\nindex 474fa1e767..be72f226f3 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1874,15 +1874,20 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)\n \tfor (p = worktrees; *p; p++) {\n \t\tstruct worktree *wt = *p;\n \t\tstruct index_state istate = INDEX_STATE_INIT(revs->repo);\n+\t\tchar *gitdir;\n \n \t\tif (wt->is_current)\n \t\t\tcontinue; /* current index already taken care of */\n \n+\t\tgitdir = get_worktree_git_dir(wt);\n+\n \t\tif (read_index_from(&istate,\n \t\t\t\t worktree_git_path(the_repository, wt, \"index\"),\n-\t\t\t\t get_worktree_git_dir(wt)) > 0)\n+\t\t\t\t gitdir) > 0)\n \t\t\tdo_add_index_objects_to_pending(revs, &istate, flags);\n+\n \t\tdiscard_index(&istate);\n+\t\tfree(gitdir);\n \t}\n \tfree_worktrees(worktrees);\n }\ndiff --git a/worktree.c b/worktree.c\nindex 8f4fc10c44..3b94535963 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -59,8 +59,9 @@ static void add_head_info(struct worktree *wt)\n static int is_current_worktree(struct worktree *wt)\n {\n \tchar *git_dir = absolute_pathdup(repo_get_git_dir(the_repository));\n-\tconst char *wt_git_dir = get_worktree_git_dir(wt);\n+\tchar *wt_git_dir = get_worktree_git_dir(wt);\n \tint is_current = !fspathcmp(git_dir, absolute_path(wt_git_dir));\n+\tfree(wt_git_dir);\n \tfree(git_dir);\n \treturn is_current;\n }\n@@ -175,14 +176,14 @@ struct worktree **get_worktrees(void)\n \treturn get_worktrees_internal(0);\n }\n \n-const char *get_worktree_git_dir(const struct worktree *wt)\n+char *get_worktree_git_dir(const struct worktree *wt)\n {\n \tif (!wt)\n-\t\treturn repo_get_git_dir(the_repository);\n+\t\treturn xstrdup(repo_get_git_dir(the_repository));\n \telse if (!wt->id)\n-\t\treturn repo_get_common_dir(the_repository);\n+\t\treturn xstrdup(repo_get_common_dir(the_repository));\n \telse\n-\t\treturn git_common_path(\"worktrees/%s\", wt->id);\n+\t\treturn xstrdup(git_common_path(\"worktrees/%s\", wt->id));\n }\n \n static struct worktree *find_worktree_by_suffix(struct worktree **list,\ndiff --git a/worktree.h b/worktree.h\nindex 38145df80f..16368588a0 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -39,7 +39,7 @@ int submodule_uses_worktrees(const char *path);\n * Return git dir of the worktree. Note that the path may be relative.\n * If wt is NULL, git dir of current worktree is returned.\n */\n-const char *get_worktree_git_dir(const struct worktree *wt);\n+char *get_worktree_git_dir(const struct worktree *wt);\n \n /*\n * Search for the worktree identified unambiguously by `arg` -- typically\n\n-- \n2.48.1.538.gc4cfc42d60.dirty\n\n"},{"id":"511977","messageId":"20250206-b4-pks-path-drop-the-repository-v1-10-4e77f0313206@pks.im","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-0-4e77f0313206@pks.im","subject":"[PATCH 10/16] path: drop `git_common_path()` in favor of `repo_common_path()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-06T07:58:06Z","receivedAt":"2025-02-06T07:58:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Remove `git_common_path()` in favor of the `repo_common_path()` family\nof functions, which makes the implicit dependency on `the_repository` go\naway.\n\nNote that `git_common_path()` used to return a string allocated via\n`get_pathname()`, which uses a rotating set of statically allocated\nbuffers. Consequently, callers didn't have to free the returned string.\nThe same isn't true for `repo_common_path()`, so we also have to add\nlogic to free the returned strings.\n\nThis refactoring also allows us to remove `repo_common_pathv()` from the\npublic interface.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/worktree.c | 16 ++++++++++++----\n path.c | 8 ++++----\n path.h | 19 -------------------\n worktree.c | 32 ++++++++++++++++++++++++--------\n 4 files changed, 40 insertions(+), 35 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 2cea9441a6..761e302a36 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -151,7 +151,7 @@ static int delete_git_dir(const char *id)\n \tstruct strbuf sb = STRBUF_INIT;\n \tint ret;\n \n-\tstrbuf_addstr(&sb, git_common_path(\"worktrees/%s\", id));\n+\trepo_common_path_append(the_repository, &sb, \"worktrees/%s\", id);\n \tret = remove_dir_recursively(&sb, 0);\n \tif (ret < 0 && errno == ENOTDIR)\n \t\tret = unlink(sb.buf);\n@@ -1102,6 +1102,7 @@ static int lock_worktree(int ac, const char **av, const char *prefix,\n \t\tOPT_END()\n \t};\n \tstruct worktree **worktrees, *wt;\n+\tchar *path;\n \n \tac = parse_options(ac, av, prefix, options, git_worktree_lock_usage, 0);\n \tif (ac != 1)\n@@ -1122,9 +1123,11 @@ static int lock_worktree(int ac, const char **av, const char *prefix,\n \t\tdie(_(\"'%s' is already locked\"), av[0]);\n \t}\n \n-\twrite_file(git_common_path(\"worktrees/%s/locked\", wt->id),\n-\t\t \"%s\", reason);\n+\tpath = repo_common_path(the_repository, \"worktrees/%s/locked\", wt->id);\n+\twrite_file(path, \"%s\", reason);\n+\n \tfree_worktrees(worktrees);\n+\tfree(path);\n \treturn 0;\n }\n \n@@ -1135,6 +1138,7 @@ static int unlock_worktree(int ac, const char **av, const char *prefix,\n \t\tOPT_END()\n \t};\n \tstruct worktree **worktrees, *wt;\n+\tchar *path;\n \tint ret;\n \n \tac = parse_options(ac, av, prefix, options, git_worktree_unlock_usage, 0);\n@@ -1149,8 +1153,12 @@ static int unlock_worktree(int ac, const char **av, const char *prefix,\n \t\tdie(_(\"The main working tree cannot be locked or unlocked\"));\n \tif (!worktree_lock_reason(wt))\n \t\tdie(_(\"'%s' is not locked\"), av[0]);\n-\tret = unlink_or_warn(git_common_path(\"worktrees/%s/locked\", wt->id));\n+\n+\tpath = repo_common_path(the_repository, \"worktrees/%s/locked\", wt->id);\n+\tret = unlink_or_warn(path);\n+\n \tfree_worktrees(worktrees);\n+\tfree(path);\n \treturn ret;\n }\n \ndiff --git a/path.c b/path.c\nindex d721507be8..f6b795d75f 100644\n--- a/path.c\n+++ b/path.c\n@@ -634,10 +634,10 @@ const char *repo_submodule_path_replace(struct repository *repo,\n \treturn buf->buf;\n }\n \n-void repo_common_pathv(const struct repository *repo,\n-\t\t struct strbuf *sb,\n-\t\t const char *fmt,\n-\t\t va_list args)\n+static void repo_common_pathv(const struct repository *repo,\n+\t\t\t struct strbuf *sb,\n+\t\t\t const char *fmt,\n+\t\t\t va_list args)\n {\n \tstrbuf_addstr(sb, repo->commondir);\n \tif (sb->len && !is_dir_sep(sb->buf[sb->len - 1]))\ndiff --git a/path.h b/path.h\nindex 904eeac068..496f27fdfd 100644\n--- a/path.h\n+++ b/path.h\n@@ -233,29 +233,10 @@ struct strbuf *get_pathname(void);\n # include \"repository.h\"\n \n /* Internal implementation details that should not be used. */\n-void repo_common_pathv(const struct repository *repo,\n-\t\t struct strbuf *buf,\n-\t\t const char *fmt,\n-\t\t va_list args);\n void repo_git_pathv(struct repository *repo,\n \t\t const struct worktree *wt, struct strbuf *buf,\n \t\t const char *fmt, va_list args);\n \n-/*\n- * Return a statically allocated path into the main repository's\n- * (the_repository) common git directory.\n- */\n-__attribute__((format (printf, 1, 2)))\n-static inline const char *git_common_path(const char *fmt, ...)\n-{\n-\tstruct strbuf *pathname = get_pathname();\n-\tva_list args;\n-\tva_start(args, fmt);\n-\trepo_common_pathv(the_repository, pathname, fmt, args);\n-\tva_end(args);\n-\treturn pathname->buf;\n-}\n-\n /*\n * Return a statically allocated path into the main repository's\n * (the_repository) git directory.\ndiff --git a/worktree.c b/worktree.c\nindex 3b94535963..d5d07d7a84 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -183,7 +183,7 @@ char *get_worktree_git_dir(const struct worktree *wt)\n \telse if (!wt->id)\n \t\treturn xstrdup(repo_get_common_dir(the_repository));\n \telse\n-\t\treturn xstrdup(git_common_path(\"worktrees/%s\", wt->id));\n+\t\treturn repo_common_path(the_repository, \"worktrees/%s\", wt->id);\n }\n \n static struct worktree *find_worktree_by_suffix(struct worktree **list,\n@@ -314,6 +314,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,\n {\n \tstruct strbuf wt_path = STRBUF_INIT;\n \tstruct strbuf realpath = STRBUF_INIT;\n+\tstruct strbuf buf = STRBUF_INIT;\n \tchar *path = NULL;\n \tint err, ret = -1;\n \n@@ -343,7 +344,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,\n \tif (!is_absolute_path(wt->path)) {\n \t\tstrbuf_addf_gently(errmsg,\n \t\t\t\t _(\"'%s' file does not contain absolute path to the working tree location\"),\n-\t\t\t\t git_common_path(\"worktrees/%s/gitdir\", wt->id));\n+\t\t\t\t repo_common_path_replace(the_repository, &buf, \"worktrees/%s/gitdir\", wt->id));\n \t\tgoto done;\n \t}\n \n@@ -365,14 +366,16 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,\n \t\tgoto done;\n \t}\n \n-\tstrbuf_realpath(&realpath, git_common_path(\"worktrees/%s\", wt->id), 1);\n+\tstrbuf_realpath(&realpath, repo_common_path_replace(the_repository, &buf, \"worktrees/%s\", wt->id), 1);\n \tret = fspathcmp(path, realpath.buf);\n \n \tif (ret)\n \t\tstrbuf_addf_gently(errmsg, _(\"'%s' does not point back to '%s'\"),\n-\t\t\t\t wt->path, git_common_path(\"worktrees/%s\", wt->id));\n+\t\t\t\t wt->path, repo_common_path_replace(the_repository, &buf,\n+\t\t\t\t\t\t\t\t \"worktrees/%s\", wt->id));\n done:\n \tfree(path);\n+\tstrbuf_release(&buf);\n \tstrbuf_release(&wt_path);\n \tstrbuf_release(&realpath);\n \treturn ret;\n@@ -384,11 +387,13 @@ void update_worktree_location(struct worktree *wt, const char *path_,\n \tstruct strbuf path = STRBUF_INIT;\n \tstruct strbuf dotgit = STRBUF_INIT;\n \tstruct strbuf gitdir = STRBUF_INIT;\n+\tchar *wt_gitdir;\n \n \tif (is_main_worktree(wt))\n \t\tBUG(\"can't relocate main worktree\");\n \n-\tstrbuf_realpath(&gitdir, git_common_path(\"worktrees/%s/gitdir\", wt->id), 1);\n+\twt_gitdir = repo_common_path(the_repository, \"worktrees/%s/gitdir\", wt->id);\n+\tstrbuf_realpath(&gitdir, wt_gitdir, 1);\n \tstrbuf_realpath(&path, path_, 1);\n \tstrbuf_addf(&dotgit, \"%s/.git\", path.buf);\n \tif (fspathcmp(wt->path, path.buf)) {\n@@ -400,6 +405,7 @@ void update_worktree_location(struct worktree *wt, const char *path_,\n \tstrbuf_release(&path);\n \tstrbuf_release(&dotgit);\n \tstrbuf_release(&gitdir);\n+\tfree(wt_gitdir);\n }\n \n int is_worktree_being_rebased(const struct worktree *wt,\n@@ -585,6 +591,7 @@ static void repair_gitfile(struct worktree *wt,\n \tstruct strbuf backlink = STRBUF_INIT;\n \tchar *dotgit_contents = NULL;\n \tconst char *repair = NULL;\n+\tchar *path = NULL;\n \tint err;\n \n \t/* missing worktree can't be repaired */\n@@ -596,7 +603,8 @@ static void repair_gitfile(struct worktree *wt,\n \t\tgoto done;\n \t}\n \n-\tstrbuf_realpath(&repo, git_common_path(\"worktrees/%s\", wt->id), 1);\n+\tpath = repo_common_path(the_repository, \"worktrees/%s\", wt->id);\n+\tstrbuf_realpath(&repo, path, 1);\n \tstrbuf_addf(&dotgit, \"%s/.git\", wt->path);\n \tstrbuf_addf(&gitdir, \"%s/gitdir\", repo.buf);\n \tdotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));\n@@ -626,6 +634,7 @@ static void repair_gitfile(struct worktree *wt,\n \n done:\n \tfree(dotgit_contents);\n+\tfree(path);\n \tstrbuf_release(&repo);\n \tstrbuf_release(&dotgit);\n \tstrbuf_release(&gitdir);\n@@ -657,11 +666,13 @@ void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path\n \tstruct strbuf gitdir = STRBUF_INIT;\n \tstruct strbuf dotgit = STRBUF_INIT;\n \tint is_relative_path;\n+\tchar *path = NULL;\n \n \tif (is_main_worktree(wt))\n \t\tgoto done;\n \n-\tstrbuf_realpath(&gitdir, git_common_path(\"worktrees/%s/gitdir\", wt->id), 1);\n+\tpath = repo_common_path(the_repository, \"worktrees/%s/gitdir\", wt->id);\n+\tstrbuf_realpath(&gitdir, path, 1);\n \n \tif (strbuf_read_file(&dotgit, gitdir.buf, 0) < 0)\n \t\tgoto done;\n@@ -680,6 +691,7 @@ void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path\n done:\n \tstrbuf_release(&gitdir);\n \tstrbuf_release(&dotgit);\n+\tfree(path);\n }\n \n void repair_worktrees_after_gitdir_move(const char *old_path)\n@@ -871,7 +883,11 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,\n \tssize_t read_result;\n \n \t*wtpath = NULL;\n-\tstrbuf_realpath(&repo, git_common_path(\"worktrees/%s\", id), 1);\n+\n+\tpath = repo_common_path(the_repository, \"worktrees/%s\", id);\n+\tstrbuf_realpath(&repo, path, 1);\n+\tFREE_AND_NULL(path);\n+\n \tstrbuf_addf(&gitdir, \"%s/gitdir\", repo.buf);\n \tif (!is_directory(repo.buf)) {\n \t\tstrbuf_addstr(reason, _(\"not a valid directory\"));\n\n-- \n2.48.1.538.gc4cfc42d60.dirty\n\n"},{"id":"511978","messageId":"20250206-b4-pks-path-drop-the-repository-v1-11-4e77f0313206@pks.im","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-0-4e77f0313206@pks.im","subject":"[PATCH 11/16] rerere: let `rerere_path()` write paths into a caller-provided buffer","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-06T07:58:07Z","receivedAt":"2025-02-06T07:58:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Same as with `get_worktree_git_dir()` a couple of commits ago, the\n`rerere_path()` function returns paths that need not be free'd by the\ncaller because `git_path()` internally uses `get_pathname()`.\n\nRefactor the function to instead accept a caller-provided buffer that\nthe path will be written into, passing on ownership to the caller. This\nrefactoring prepares us for the removal of `git_path()`.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/rerere.c | 11 ++++---\n rerere.c | 87 ++++++++++++++++++++++++++++++++++++++------------------\n rerere.h | 3 +-\n 3 files changed, 69 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/rerere.c b/builtin/rerere.c\nindex 41127e24e5..1312e79d89 100644\n--- a/builtin/rerere.c\n+++ b/builtin/rerere.c\n@@ -4,9 +4,9 @@\n #include \"config.h\"\n #include \"gettext.h\"\n #include \"parse-options.h\"\n-\n-#include \"string-list.h\"\n #include \"rerere.h\"\n+#include \"strbuf.h\"\n+#include \"string-list.h\"\n #include \"xdiff/xdiff.h\"\n #include \"xdiff-interface.h\"\n #include \"pathspec.h\"\n@@ -112,15 +112,18 @@ int cmd_rerere(int argc,\n \t\t\t\tmerge_rr.items[i].util = NULL;\n \t\t}\n \t} else if (!strcmp(argv[0], \"diff\")) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n \t\tif (setup_rerere(the_repository, &merge_rr,\n \t\t\t\t flags | RERERE_READONLY) < 0)\n \t\t\treturn 0;\n \t\tfor (size_t i = 0; i < merge_rr.nr; i++) {\n \t\t\tconst char *path = merge_rr.items[i].string;\n \t\t\tconst struct rerere_id *id = merge_rr.items[i].util;\n-\t\t\tif (diff_two(rerere_path(id, \"preimage\"), path, path, path))\n-\t\t\t\tdie(_(\"unable to generate diff for '%s'\"), rerere_path(id, NULL));\n+\t\t\tif (diff_two(rerere_path(&buf, id, \"preimage\"), path, path, path))\n+\t\t\t\tdie(_(\"unable to generate diff for '%s'\"), rerere_path(&buf, id, NULL));\n \t\t}\n+\n+\t\tstrbuf_release(&buf);\n \t} else\n \t\tusage_with_options(rerere_usage, options);\n \ndiff --git a/rerere.c b/rerere.c\nindex e7fa6426b3..763cb715a6 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -91,16 +91,18 @@ static void assign_variant(struct rerere_id *id)\n \tid->variant = variant;\n }\n \n-const char *rerere_path(const struct rerere_id *id, const char *file)\n+const char *rerere_path(struct strbuf *buf, const struct rerere_id *id, const char *file)\n {\n \tif (!file)\n-\t\treturn git_path(\"rr-cache/%s\", rerere_id_hex(id));\n+\t\treturn repo_git_path_replace(the_repository, buf, \"rr-cache/%s\",\n+\t\t\t\t\t rerere_id_hex(id));\n \n \tif (id->variant <= 0)\n-\t\treturn git_path(\"rr-cache/%s/%s\", rerere_id_hex(id), file);\n+\t\treturn repo_git_path_replace(the_repository, buf, \"rr-cache/%s/%s\",\n+\t\t\t\t\t rerere_id_hex(id), file);\n \n-\treturn git_path(\"rr-cache/%s/%s.%d\",\n-\t\t\trerere_id_hex(id), file, id->variant);\n+\treturn repo_git_path_replace(the_repository, buf, \"rr-cache/%s/%s.%d\",\n+\t\t\t\t rerere_id_hex(id), file, id->variant);\n }\n \n static int is_rr_file(const char *name, const char *filename, int *variant)\n@@ -624,9 +626,10 @@ static int try_merge(struct index_state *istate,\n {\n \tenum ll_merge_result ret;\n \tmmfile_t base = {NULL, 0}, other = {NULL, 0};\n+\tstruct strbuf buf = STRBUF_INIT;\n \n-\tif (read_mmfile(&base, rerere_path(id, \"preimage\")) ||\n-\t read_mmfile(&other, rerere_path(id, \"postimage\"))) {\n+\tif (read_mmfile(&base, rerere_path(&buf, id, \"preimage\")) ||\n+\t read_mmfile(&other, rerere_path(&buf, id, \"postimage\"))) {\n \t\tret = LL_MERGE_CONFLICT;\n \t} else {\n \t\t/*\n@@ -637,6 +640,7 @@ static int try_merge(struct index_state *istate,\n \t\t\t istate, NULL);\n \t}\n \n+\tstrbuf_release(&buf);\n \tfree(base.ptr);\n \tfree(other.ptr);\n \n@@ -657,6 +661,7 @@ static int merge(struct index_state *istate, const struct rerere_id *id, const c\n {\n \tFILE *f;\n \tint ret;\n+\tstruct strbuf buf = STRBUF_INIT;\n \tmmfile_t cur = {NULL, 0};\n \tmmbuffer_t result = {NULL, 0};\n \n@@ -664,8 +669,8 @@ static int merge(struct index_state *istate, const struct rerere_id *id, const c\n \t * Normalize the conflicts in path and write it out to\n \t * \"thisimage\" temporary file.\n \t */\n-\tif ((handle_file(istate, path, NULL, rerere_path(id, \"thisimage\")) < 0) ||\n-\t read_mmfile(&cur, rerere_path(id, \"thisimage\"))) {\n+\tif ((handle_file(istate, path, NULL, rerere_path(&buf, id, \"thisimage\")) < 0) ||\n+\t read_mmfile(&cur, rerere_path(&buf, id, \"thisimage\"))) {\n \t\tret = 1;\n \t\tgoto out;\n \t}\n@@ -678,9 +683,9 @@ static int merge(struct index_state *istate, const struct rerere_id *id, const c\n \t * A successful replay of recorded resolution.\n \t * Mark that \"postimage\" was used to help gc.\n \t */\n-\tif (utime(rerere_path(id, \"postimage\"), NULL) < 0)\n+\tif (utime(rerere_path(&buf, id, \"postimage\"), NULL) < 0)\n \t\twarning_errno(_(\"failed utime() on '%s'\"),\n-\t\t\t rerere_path(id, \"postimage\"));\n+\t\t\t rerere_path(&buf, id, \"postimage\"));\n \n \t/* Update \"path\" with the resolution */\n \tf = fopen(path, \"w\");\n@@ -694,6 +699,7 @@ static int merge(struct index_state *istate, const struct rerere_id *id, const c\n out:\n \tfree(cur.ptr);\n \tfree(result.ptr);\n+\tstrbuf_release(&buf);\n \n \treturn ret;\n }\n@@ -720,9 +726,11 @@ static void update_paths(struct repository *r, struct string_list *update)\n \n static void remove_variant(struct rerere_id *id)\n {\n-\tunlink_or_warn(rerere_path(id, \"postimage\"));\n-\tunlink_or_warn(rerere_path(id, \"preimage\"));\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tunlink_or_warn(rerere_path(&buf, id, \"postimage\"));\n+\tunlink_or_warn(rerere_path(&buf, id, \"preimage\"));\n \tid->collection->status[id->variant] = 0;\n+\tstrbuf_release(&buf);\n }\n \n /*\n@@ -739,6 +747,7 @@ static void do_rerere_one_path(struct index_state *istate,\n \tconst char *path = rr_item->string;\n \tstruct rerere_id *id = rr_item->util;\n \tstruct rerere_dir *rr_dir = id->collection;\n+\tstruct strbuf buf = STRBUF_INIT;\n \tint variant;\n \n \tvariant = id->variant;\n@@ -746,12 +755,12 @@ static void do_rerere_one_path(struct index_state *istate,\n \t/* Has the user resolved it already? */\n \tif (variant >= 0) {\n \t\tif (!handle_file(istate, path, NULL, NULL)) {\n-\t\t\tcopy_file(rerere_path(id, \"postimage\"), path, 0666);\n+\t\t\tcopy_file(rerere_path(&buf, id, \"postimage\"), path, 0666);\n \t\t\tid->collection->status[variant] |= RR_HAS_POSTIMAGE;\n \t\t\tfprintf_ln(stderr, _(\"Recorded resolution for '%s'.\"), path);\n \t\t\tfree_rerere_id(rr_item);\n \t\t\trr_item->util = NULL;\n-\t\t\treturn;\n+\t\t\tgoto out;\n \t\t}\n \t\t/*\n \t\t * There may be other variants that can cleanly\n@@ -787,22 +796,25 @@ static void do_rerere_one_path(struct index_state *istate,\n \t\t\t\t path);\n \t\tfree_rerere_id(rr_item);\n \t\trr_item->util = NULL;\n-\t\treturn;\n+\t\tgoto out;\n \t}\n \n \t/* None of the existing one applies; we need a new variant */\n \tassign_variant(id);\n \n \tvariant = id->variant;\n-\thandle_file(istate, path, NULL, rerere_path(id, \"preimage\"));\n+\thandle_file(istate, path, NULL, rerere_path(&buf, id, \"preimage\"));\n \tif (id->collection->status[variant] & RR_HAS_POSTIMAGE) {\n-\t\tconst char *path = rerere_path(id, \"postimage\");\n+\t\tconst char *path = rerere_path(&buf, id, \"postimage\");\n \t\tif (unlink(path))\n \t\t\tdie_errno(_(\"cannot unlink stray '%s'\"), path);\n \t\tid->collection->status[variant] &= ~RR_HAS_POSTIMAGE;\n \t}\n \tid->collection->status[variant] |= RR_HAS_PREIMAGE;\n \tfprintf_ln(stderr, _(\"Recorded preimage for '%s'\"), path);\n+\n+out:\n+\tstrbuf_release(&buf);\n }\n \n static int do_plain_rerere(struct repository *r,\n@@ -810,6 +822,7 @@ static int do_plain_rerere(struct repository *r,\n {\n \tstruct string_list conflict = STRING_LIST_INIT_DUP;\n \tstruct string_list update = STRING_LIST_INIT_DUP;\n+\tstruct strbuf buf = STRBUF_INIT;\n \tint i;\n \n \tfind_conflict(r, &conflict);\n@@ -843,7 +856,7 @@ static int do_plain_rerere(struct repository *r,\n \t\tstring_list_insert(rr, path)->util = id;\n \n \t\t/* Ensure that the directory exists. */\n-\t\tmkdir_in_gitdir(rerere_path(id, NULL));\n+\t\tmkdir_in_gitdir(rerere_path(&buf, id, NULL));\n \t}\n \n \tfor (i = 0; i < rr->nr; i++)\n@@ -854,6 +867,7 @@ static int do_plain_rerere(struct repository *r,\n \n \tstring_list_clear(&conflict, 0);\n \tstring_list_clear(&update, 0);\n+\tstrbuf_release(&buf);\n \treturn write_rr(rr, fd);\n }\n \n@@ -1033,6 +1047,7 @@ static int rerere_forget_one_path(struct index_state *istate,\n \tstruct rerere_id *id;\n \tunsigned char hash[GIT_MAX_RAWSZ];\n \tint ret;\n+\tstruct strbuf buf = STRBUF_INIT;\n \tstruct string_list_item *item;\n \n \t/*\n@@ -1056,8 +1071,8 @@ static int rerere_forget_one_path(struct index_state *istate,\n \t\tif (!has_rerere_resolution(id))\n \t\t\tcontinue;\n \n-\t\thandle_cache(istate, path, hash, rerere_path(id, \"thisimage\"));\n-\t\tif (read_mmfile(&cur, rerere_path(id, \"thisimage\"))) {\n+\t\thandle_cache(istate, path, hash, rerere_path(&buf, id, \"thisimage\"));\n+\t\tif (read_mmfile(&cur, rerere_path(&buf, id, \"thisimage\"))) {\n \t\t\tfree(cur.ptr);\n \t\t\terror(_(\"failed to update conflicted state in '%s'\"), path);\n \t\t\tgoto fail_exit;\n@@ -1074,7 +1089,7 @@ static int rerere_forget_one_path(struct index_state *istate,\n \t\tgoto fail_exit;\n \t}\n \n-\tfilename = rerere_path(id, \"postimage\");\n+\tfilename = rerere_path(&buf, id, \"postimage\");\n \tif (unlink(filename)) {\n \t\tif (errno == ENOENT)\n \t\t\terror(_(\"no remembered resolution for '%s'\"), path);\n@@ -1088,7 +1103,7 @@ static int rerere_forget_one_path(struct index_state *istate,\n \t * conflict in the working tree, run us again to record\n \t * the postimage.\n \t */\n-\thandle_cache(istate, path, hash, rerere_path(id, \"preimage\"));\n+\thandle_cache(istate, path, hash, rerere_path(&buf, id, \"preimage\"));\n \tfprintf_ln(stderr, _(\"Updated preimage for '%s'\"), path);\n \n \t/*\n@@ -1099,9 +1114,11 @@ static int rerere_forget_one_path(struct index_state *istate,\n \tfree_rerere_id(item);\n \titem->util = id;\n \tfprintf(stderr, _(\"Forgot resolution for '%s'\\n\"), path);\n+\tstrbuf_release(&buf);\n \treturn 0;\n \n fail_exit:\n+\tstrbuf_release(&buf);\n \tfree(id);\n \treturn -1;\n }\n@@ -1147,16 +1164,26 @@ int rerere_forget(struct repository *r, struct pathspec *pathspec)\n \n static timestamp_t rerere_created_at(struct rerere_id *id)\n {\n+\tstruct strbuf buf = STRBUF_INIT;\n \tstruct stat st;\n+\tint ret;\n \n-\treturn stat(rerere_path(id, \"preimage\"), &st) ? (time_t) 0 : st.st_mtime;\n+\tret = stat(rerere_path(&buf, id, \"preimage\"), &st) ? (time_t) 0 : st.st_mtime;\n+\n+\tstrbuf_release(&buf);\n+\treturn ret;\n }\n \n static timestamp_t rerere_last_used_at(struct rerere_id *id)\n {\n+\tstruct strbuf buf = STRBUF_INIT;\n \tstruct stat st;\n+\tint ret;\n+\n+\tret = stat(rerere_path(&buf, id, \"postimage\"), &st) ? (time_t) 0 : st.st_mtime;\n \n-\treturn stat(rerere_path(id, \"postimage\"), &st) ? (time_t) 0 : st.st_mtime;\n+\tstrbuf_release(&buf);\n+\treturn ret;\n }\n \n /*\n@@ -1164,9 +1191,11 @@ static timestamp_t rerere_last_used_at(struct rerere_id *id)\n */\n static void unlink_rr_item(struct rerere_id *id)\n {\n-\tunlink_or_warn(rerere_path(id, \"thisimage\"));\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tunlink_or_warn(rerere_path(&buf, id, \"thisimage\"));\n \tremove_variant(id);\n \tid->collection->status[id->variant] = 0;\n+\tstrbuf_release(&buf);\n }\n \n static void prune_one(struct rerere_id *id,\n@@ -1264,10 +1293,14 @@ void rerere_clear(struct repository *r, struct string_list *merge_rr)\n \n \tfor (i = 0; i < merge_rr->nr; i++) {\n \t\tstruct rerere_id *id = merge_rr->items[i].util;\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\n \t\tif (!has_rerere_resolution(id)) {\n \t\t\tunlink_rr_item(id);\n-\t\t\trmdir(rerere_path(id, NULL));\n+\t\t\trmdir(rerere_path(&buf, id, NULL));\n \t\t}\n+\n+\t\tstrbuf_release(&buf);\n \t}\n \tunlink_or_warn(git_path_merge_rr(r));\n \trollback_lock_file(&write_lock);\ndiff --git a/rerere.h b/rerere.h\nindex 5d6cb63879..d4b5f7c932 100644\n--- a/rerere.h\n+++ b/rerere.h\n@@ -32,7 +32,8 @@ int repo_rerere(struct repository *, int);\n * path to that filesystem entity. With \"file\" specified with NULL,\n * return the path to the directory that houses these files.\n */\n-const char *rerere_path(const struct rerere_id *, const char *file);\n+const char *rerere_path(struct strbuf *buf, const struct rerere_id *,\n+\t\t\tconst char *file);\n int rerere_forget(struct repository *, struct pathspec *);\n int rerere_remaining(struct repository *, struct string_list *);\n void rerere_clear(struct repository *, struct string_list *);\n\n-- \n2.48.1.538.gc4cfc42d60.dirty\n\n"},{"id":"511980","messageId":"20250206-b4-pks-path-drop-the-repository-v1-12-4e77f0313206@pks.im","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-0-4e77f0313206@pks.im","subject":"[PATCH 12/16] path: drop `git_path()` in favor of `repo_git_path()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-06T07:58:08Z","receivedAt":"2025-02-06T07:58:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Remove `git_path()` in favor of the `repo_git_path()` family of\nfunctions, which makes the implicit dependency on `the_repository` go\naway.\n\nNote that `git_path()` returned a string allocated via `get_pathname()`,\nwhich uses a rotating set of statically allocated buffers. Consequently,\ncallers didn't have to free the returned string. The same isn't true for\n`repo_common_path()`, so we also have to add logic to free the returned\nstrings.\n\nThis refactoring also allows us to remove `repo_common_pathv()` as well\nas `get_pathname()` from the public interface.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/commit.c | 8 +++++---\n builtin/gc.c | 21 +++++++++++++++------\n builtin/notes.c | 7 ++++++-\n builtin/rebase.c | 2 +-\n builtin/remote.c | 6 ++++--\n builtin/rev-parse.c | 6 +++---\n builtin/worktree.c | 11 +++++++++--\n notes-merge.c | 20 ++++++++++++--------\n path.c | 8 ++++----\n path.h | 27 ---------------------------\n read-cache.c | 24 +++++++++++++++++-------\n remote.c | 21 ++++++++++++---------\n rerere.c | 14 +++++++++++---\n shallow.c | 4 +++-\n wt-status.c | 42 ++++++++++++++++++++++++++----------------\n 15 files changed, 128 insertions(+), 93 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 9fb405dd4a..2f45968222 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -352,6 +352,7 @@ static const char *prepare_index(const char **argv, const char *prefix,\n \tstruct pathspec pathspec;\n \tint refresh_flags = REFRESH_QUIET;\n \tconst char *ret;\n+\tchar *path = NULL;\n \n \tif (is_status)\n \t\trefresh_flags |= REFRESH_UNMERGED;\n@@ -524,9 +525,9 @@ static const char *prepare_index(const char **argv, const char *prefix,\n \tif (write_locked_index(the_repository->index, &index_lock, 0))\n \t\tdie(_(\"unable to write new index file\"));\n \n-\thold_lock_file_for_update(&false_lock,\n-\t\t\t\t git_path(\"next-index-%\"PRIuMAX,\n-\t\t\t\t\t (uintmax_t) getpid()),\n+\tpath = repo_git_path(the_repository, \"next-index-%\"PRIuMAX,\n+\t\t\t (uintmax_t) getpid());\n+\thold_lock_file_for_update(&false_lock, path,\n \t\t\t\t LOCK_DIE_ON_ERROR);\n \n \tcreate_base_index(current_head);\n@@ -542,6 +543,7 @@ static const char *prepare_index(const char **argv, const char *prefix,\n out:\n \tstring_list_clear(&partial, 0);\n \tclear_pathspec(&pathspec);\n+\tfree(path);\n \treturn ret;\n }\n \ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 57f6aee174..5923d9a05b 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -99,9 +99,11 @@ static void process_log_file(void)\n \t\t/* There was some error recorded in the lock file */\n \t\tcommit_lock_file(&log_lock);\n \t} else {\n+\t\tchar *path = repo_git_path(the_repository, \"gc.log\");\n \t\t/* No error, clean up any old gc.log */\n-\t\tunlink(git_path(\"gc.log\"));\n+\t\tunlink(path);\n \t\trollback_lock_file(&log_lock);\n+\t\tfree(path);\n \t}\n }\n \n@@ -299,8 +301,11 @@ static int too_many_loose_objects(struct gc_config *cfg)\n \tint num_loose = 0;\n \tint needed = 0;\n \tconst unsigned hexsz_loose = the_hash_algo->hexsz - 2;\n+\tchar *path;\n \n-\tdir = opendir(git_path(\"objects/17\"));\n+\tpath = repo_git_path(the_repository, \"objects/17\");\n+\tdir = opendir(path);\n+\tfree(path);\n \tif (!dir)\n \t\treturn 0;\n \n@@ -821,11 +826,12 @@ struct repository *repo UNUSED)\n \t}\n \n \tif (daemonized) {\n-\t\thold_lock_file_for_update(&log_lock,\n-\t\t\t\t\t git_path(\"gc.log\"),\n+\t\tchar *path = repo_git_path(the_repository, \"gc.log\");\n+\t\thold_lock_file_for_update(&log_lock, path,\n \t\t\t\t\t LOCK_DIE_ON_ERROR);\n \t\tdup2(get_lock_file_fd(&log_lock), 2);\n \t\tatexit(process_log_file_at_exit);\n+\t\tfree(path);\n \t}\n \n \tgc_before_repack(&opts, &cfg);\n@@ -887,8 +893,11 @@ struct repository *repo UNUSED)\n \t\twarning(_(\"There are too many unreachable loose objects; \"\n \t\t\t\"run 'git prune' to remove them.\"));\n \n-\tif (!daemonized)\n-\t\tunlink(git_path(\"gc.log\"));\n+\tif (!daemonized) {\n+\t\tchar *path = repo_git_path(the_repository, \"gc.log\");\n+\t\tunlink(path);\n+\t\tfree(path);\n+\t}\n \n out:\n \tgc_config_release(&cfg);\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 18bcbb2f91..ff61ec5f2d 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -979,6 +979,8 @@ static int merge(int argc, const char **argv, const char *prefix,\n \telse { /* Merge has unresolved conflicts */\n \t\tstruct worktree **worktrees;\n \t\tconst struct worktree *wt;\n+\t\tchar *path;\n+\n \t\t/* Update .git/NOTES_MERGE_PARTIAL with partial merge result */\n \t\trefs_update_ref(get_main_ref_store(the_repository), msg.buf,\n \t\t\t\t\"NOTES_MERGE_PARTIAL\", &result_oid, NULL,\n@@ -994,10 +996,13 @@ static int merge(int argc, const char **argv, const char *prefix,\n \t\tif (refs_update_symref(get_main_ref_store(the_repository), \"NOTES_MERGE_REF\", notes_ref, NULL))\n \t\t\tdie(_(\"failed to store link to current notes ref (%s)\"),\n \t\t\t notes_ref);\n+\n+\t\tpath = repo_git_path(the_repository, NOTES_MERGE_WORKTREE);\n \t\tfprintf(stderr, _(\"Automatic notes merge failed. Fix conflicts in %s \"\n \t\t\t\t \"and commit the result with 'git notes merge --commit', \"\n \t\t\t\t \"or abort the merge with 'git notes merge --abort'.\\n\"),\n-\t\t\tgit_path(NOTES_MERGE_WORKTREE));\n+\t\t\tpath);\n+\t\tfree(path);\n \t}\n \n \tfree_notes(t);\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 6c9eaf3788..d4715ed35d 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -644,7 +644,7 @@ static int run_am(struct rebase_options *opts)\n \t\treturn run_command(&am);\n \t}\n \n-\trebased_patches = xstrdup(git_path(\"rebased-patches\"));\n+\trebased_patches = repo_git_path(the_repository, \"rebased-patches\");\n \tformat_patch.out = open(rebased_patches,\n \t\t\t\tO_WRONLY | O_CREAT | O_TRUNC, 0666);\n \tif (format_patch.out < 0) {\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 71d84fb3cf..0489fcc8f3 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -644,9 +644,11 @@ static int migrate_file(struct remote *remote)\n \t\tgit_config_set_multivar(buf.buf, remote->fetch.items[i].raw, \"^$\", 0);\n #ifndef WITH_BREAKING_CHANGES\n \tif (remote->origin == REMOTE_REMOTES)\n-\t\tunlink_or_warn(git_path(\"remotes/%s\", remote->name));\n+\t\tunlink_or_warn(repo_git_path_replace(the_repository, &buf,\n+\t\t\t\t\t\t \"remotes/%s\", remote->name));\n \telse if (remote->origin == REMOTE_BRANCHES)\n-\t\tunlink_or_warn(git_path(\"branches/%s\", remote->name));\n+\t\tunlink_or_warn(repo_git_path_replace(the_repository, &buf,\n+\t\t\t\t\t\t \"branches/%s\", remote->name));\n #endif /* WITH_BREAKING_CHANGES */\n \tstrbuf_release(&buf);\n \ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 428c866c05..490da33bec 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -789,8 +789,8 @@ int cmd_rev_parse(int argc,\n \t\t\tif (!strcmp(arg, \"--git-path\")) {\n \t\t\t\tif (!argv[i + 1])\n \t\t\t\t\tdie(_(\"--git-path requires an argument\"));\n-\t\t\t\tstrbuf_reset(&buf);\n-\t\t\t\tprint_path(git_path(\"%s\", argv[i + 1]), prefix,\n+\t\t\t\tprint_path(repo_git_path_replace(the_repository, &buf,\n+\t\t\t\t\t\t\t\t \"%s\", argv[i + 1]), prefix,\n \t\t\t\t\t\tformat,\n \t\t\t\t\t\tDEFAULT_RELATIVE_IF_SHARED);\n \t\t\t\ti++;\n@@ -1083,7 +1083,7 @@ int cmd_rev_parse(int argc,\n \t\t\t\t\tdie(_(\"Could not read the index\"));\n \t\t\t\tif (the_repository->index->split_index) {\n \t\t\t\t\tconst struct object_id *oid = &the_repository->index->split_index->base_oid;\n-\t\t\t\t\tconst char *path = git_path(\"sharedindex.%s\", oid_to_hex(oid));\n+\t\t\t\t\tconst char *path = repo_git_path_replace(the_repository, &buf, \"sharedindex.%s\", oid_to_hex(oid));\n \t\t\t\t\tprint_path(path, prefix, format, DEFAULT_RELATIVE);\n \t\t\t\t}\n \t\t\t\tcontinue;\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 761e302a36..48448a8355 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -163,7 +163,9 @@ static int delete_git_dir(const char *id)\n \n static void delete_worktrees_dir_if_empty(void)\n {\n-\trmdir(git_path(\"worktrees\")); /* ignore failed removal */\n+\tchar *path = repo_git_path(the_repository, \"worktrees\");\n+\trmdir(path); /* ignore failed removal */\n+\tfree(path);\n }\n \n static void prune_worktree(const char *id, const char *reason)\n@@ -212,8 +214,13 @@ static void prune_worktrees(void)\n \tstruct strbuf reason = STRBUF_INIT;\n \tstruct strbuf main_path = STRBUF_INIT;\n \tstruct string_list kept = STRING_LIST_INIT_DUP;\n-\tDIR *dir = opendir(git_path(\"worktrees\"));\n+\tchar *path;\n+\tDIR *dir;\n \tstruct dirent *d;\n+\n+\tpath = repo_git_path(the_repository, \"worktrees\");\n+\tdir = opendir(path);\n+\tfree(path);\n \tif (!dir)\n \t\treturn;\n \twhile ((d = readdir_skip_dot_and_dotdot(dir)) != NULL) {\ndiff --git a/notes-merge.c b/notes-merge.c\nindex 8c22a171c1..67a472020d 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -275,34 +275,38 @@ static void diff_tree_local(struct notes_merge_options *o,\n \n static void check_notes_merge_worktree(struct notes_merge_options *o)\n {\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n \tif (!o->has_worktree) {\n \t\t/*\n \t\t * Must establish NOTES_MERGE_WORKTREE.\n \t\t * Abort if NOTES_MERGE_WORKTREE already exists\n \t\t */\n-\t\tif (file_exists(git_path(NOTES_MERGE_WORKTREE)) &&\n-\t\t !is_empty_dir(git_path(NOTES_MERGE_WORKTREE))) {\n+\t\tif (file_exists(repo_git_path_replace(the_repository, &buf, NOTES_MERGE_WORKTREE)) &&\n+\t\t !is_empty_dir(repo_git_path_replace(the_repository, &buf, NOTES_MERGE_WORKTREE))) {\n \t\t\tif (advice_enabled(ADVICE_RESOLVE_CONFLICT))\n \t\t\t\tdie(_(\"You have not concluded your previous \"\n \t\t\t\t \"notes merge (%s exists).\\nPlease, use \"\n \t\t\t\t \"'git notes merge --commit' or 'git notes \"\n \t\t\t\t \"merge --abort' to commit/abort the \"\n \t\t\t\t \"previous merge before you start a new \"\n-\t\t\t\t \"notes merge.\"), git_path(\"NOTES_MERGE_*\"));\n+\t\t\t\t \"notes merge.\"), repo_git_path_replace(the_repository, &buf, \"NOTES_MERGE_*\"));\n \t\t\telse\n \t\t\t\tdie(_(\"You have not concluded your notes merge \"\n-\t\t\t\t \"(%s exists).\"), git_path(\"NOTES_MERGE_*\"));\n+\t\t\t\t \"(%s exists).\"), repo_git_path_replace(the_repository, &buf, \"NOTES_MERGE_*\"));\n \t\t}\n \n-\t\tif (safe_create_leading_directories_const(git_path(\n+\t\tif (safe_create_leading_directories_const(repo_git_path_replace(the_repository, &buf,\n \t\t\t\tNOTES_MERGE_WORKTREE \"/.test\")))\n \t\t\tdie_errno(\"unable to create directory %s\",\n-\t\t\t\t git_path(NOTES_MERGE_WORKTREE));\n+\t\t\t\t repo_git_path_replace(the_repository, &buf, NOTES_MERGE_WORKTREE));\n \t\to->has_worktree = 1;\n-\t} else if (!file_exists(git_path(NOTES_MERGE_WORKTREE)))\n+\t} else if (!file_exists(repo_git_path_replace(the_repository, &buf, NOTES_MERGE_WORKTREE)))\n \t\t/* NOTES_MERGE_WORKTREE should already be established */\n \t\tdie(\"missing '%s'. This should not happen\",\n-\t\t git_path(NOTES_MERGE_WORKTREE));\n+\t\t repo_git_path_replace(the_repository, &buf, NOTES_MERGE_WORKTREE));\n+\n+\tstrbuf_release(&buf);\n }\n \n static void write_buf_to_worktree(const struct object_id *obj,\ndiff --git a/path.c b/path.c\nindex f6b795d75f..ddfc901661 100644\n--- a/path.c\n+++ b/path.c\n@@ -30,7 +30,7 @@ static int get_st_mode_bits(const char *path, int *mode)\n \treturn 0;\n }\n \n-struct strbuf *get_pathname(void)\n+static struct strbuf *get_pathname(void)\n {\n \tstatic struct strbuf pathname_array[4] = {\n \t\tSTRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT\n@@ -417,9 +417,9 @@ static void strbuf_worktree_gitdir(struct strbuf *buf,\n \t\trepo_common_path_append(repo, buf, \"worktrees/%s\", wt->id);\n }\n \n-void repo_git_pathv(struct repository *repo,\n-\t\t const struct worktree *wt, struct strbuf *buf,\n-\t\t const char *fmt, va_list args)\n+static void repo_git_pathv(struct repository *repo,\n+\t\t\t const struct worktree *wt, struct strbuf *buf,\n+\t\t\t const char *fmt, va_list args)\n {\n \tint gitdir_len;\n \tstrbuf_worktree_gitdir(buf, repo, wt);\ndiff --git a/path.h b/path.h\nindex 496f27fdfd..373404dd9d 100644\n--- a/path.h\n+++ b/path.h\n@@ -221,37 +221,10 @@ char *xdg_cache_home(const char *filename);\n */\n void safe_create_dir(const char *dir, int share);\n \n-/*\n- * Do not use this function. It is only exported to other subsystems until we\n- * can get rid of the below block of functions that implicitly rely on\n- * `the_repository`.\n- */\n-struct strbuf *get_pathname(void);\n-\n # ifdef USE_THE_REPOSITORY_VARIABLE\n # include \"strbuf.h\"\n # include \"repository.h\"\n \n-/* Internal implementation details that should not be used. */\n-void repo_git_pathv(struct repository *repo,\n-\t\t const struct worktree *wt, struct strbuf *buf,\n-\t\t const char *fmt, va_list args);\n-\n-/*\n- * Return a statically allocated path into the main repository's\n- * (the_repository) git directory.\n- */\n-__attribute__((format (printf, 1, 2)))\n-static inline const char *git_path(const char *fmt, ...)\n-{\n-\tstruct strbuf *pathname = get_pathname();\n-\tva_list args;\n-\tva_start(args, fmt);\n-\trepo_git_pathv(the_repository, NULL, pathname, fmt, args);\n-\tva_end(args);\n-\treturn pathname->buf;\n-}\n-\n #define GIT_PATH_FUNC(func, filename) \\\n \tconst char *func(void) \\\n \t{ \\\ndiff --git a/read-cache.c b/read-cache.c\nindex d54be2c172..66ad0015a7 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -3251,15 +3251,18 @@ static int clean_shared_index_files(const char *current_hex)\n \n \twhile ((de = readdir(dir)) != NULL) {\n \t\tconst char *sha1_hex;\n-\t\tconst char *shared_index_path;\n+\t\tchar *shared_index_path;\n \t\tif (!skip_prefix(de->d_name, \"sharedindex.\", &sha1_hex))\n \t\t\tcontinue;\n \t\tif (!strcmp(sha1_hex, current_hex))\n \t\t\tcontinue;\n-\t\tshared_index_path = git_path(\"%s\", de->d_name);\n+\n+\t\tshared_index_path = repo_git_path(the_repository, \"%s\", de->d_name);\n \t\tif (should_delete_shared_index(shared_index_path) > 0 &&\n \t\t unlink(shared_index_path))\n \t\t\twarning_errno(_(\"unable to unlink: %s\"), shared_index_path);\n+\n+\t\tfree(shared_index_path);\n \t}\n \tclosedir(dir);\n \n@@ -3271,6 +3274,7 @@ static int write_shared_index(struct index_state *istate,\n {\n \tstruct split_index *si = istate->split_index;\n \tint ret, was_full = !istate->sparse_index;\n+\tchar *path;\n \n \tmove_cache_to_base_index(istate);\n \tconvert_to_sparse(istate, 0);\n@@ -3291,13 +3295,15 @@ static int write_shared_index(struct index_state *istate,\n \t\terror(_(\"cannot fix permission bits on '%s'\"), get_tempfile_path(*temp));\n \t\treturn ret;\n \t}\n-\tret = rename_tempfile(temp,\n-\t\t\t git_path(\"sharedindex.%s\", oid_to_hex(&si->base->oid)));\n+\n+\tpath = repo_git_path(the_repository, \"sharedindex.%s\", oid_to_hex(&si->base->oid));\n+\tret = rename_tempfile(temp, path);\n \tif (!ret) {\n \t\toidcpy(&si->base_oid, &si->base->oid);\n \t\tclean_shared_index_files(oid_to_hex(&si->base->oid));\n \t}\n \n+\tfree(path);\n \treturn ret;\n }\n \n@@ -3378,9 +3384,12 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \tif (new_shared_index) {\n \t\tstruct tempfile *temp;\n \t\tint saved_errno;\n+\t\tchar *path;\n \n \t\t/* Same initial permissions as the main .git/index file */\n-\t\ttemp = mks_tempfile_sm(git_path(\"sharedindex_XXXXXX\"), 0, 0666);\n+\t\tpath = repo_git_path(the_repository, \"sharedindex_XXXXXX\");\n+\t\ttemp = mks_tempfile_sm(path, 0, 0666);\n+\t\tfree(path);\n \t\tif (!temp) {\n \t\t\tret = do_write_locked_index(istate, lock, flags,\n \t\t\t\t\t\t ~WRITE_SPLIT_INDEX_EXTENSION);\n@@ -3401,9 +3410,10 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \n \t/* Freshen the shared index only if the split-index was written */\n \tif (!ret && !new_shared_index && !is_null_oid(&si->base_oid)) {\n-\t\tconst char *shared_index = git_path(\"sharedindex.%s\",\n-\t\t\t\t\t\t oid_to_hex(&si->base_oid));\n+\t\tchar *shared_index = repo_git_path(the_repository, \"sharedindex.%s\",\n+\t\t\t\t\t\t oid_to_hex(&si->base_oid));\n \t\tfreshen_shared_index(shared_index, 1);\n+\t\tfree(shared_index);\n \t}\n \n out:\ndiff --git a/remote.c b/remote.c\nindex 1779f0e7bb..81d151a507 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -321,10 +321,11 @@ static void read_remotes_file(struct remote_state *remote_state,\n \t\t\t struct remote *remote)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tFILE *f = fopen_or_warn(git_path(\"remotes/%s\", remote->name), \"r\");\n+\tFILE *f = fopen_or_warn(repo_git_path_append(the_repository, &buf,\n+\t\t\t\t\t\t \"remotes/%s\", remote->name), \"r\");\n \n \tif (!f)\n-\t\treturn;\n+\t\tgoto out;\n \n \twarn_about_deprecated_remote_type(\"remotes\", remote);\n \n@@ -343,8 +344,10 @@ static void read_remotes_file(struct remote_state *remote_state,\n \t\telse if (skip_prefix(buf.buf, \"Pull:\", &v))\n \t\t\trefspec_append(&remote->fetch, skip_spaces(v));\n \t}\n-\tstrbuf_release(&buf);\n \tfclose(f);\n+\n+out:\n+\tstrbuf_release(&buf);\n }\n \n static void read_branches_file(struct remote_state *remote_state,\n@@ -352,20 +355,19 @@ static void read_branches_file(struct remote_state *remote_state,\n {\n \tchar *frag, *to_free = NULL;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tFILE *f = fopen_or_warn(git_path(\"branches/%s\", remote->name), \"r\");\n+\tFILE *f = fopen_or_warn(repo_git_path_append(the_repository, &buf,\n+\t\t\t\t\t\t \"branches/%s\", remote->name), \"r\");\n \n \tif (!f)\n-\t\treturn;\n+\t\tgoto out;\n \n \twarn_about_deprecated_remote_type(\"branches\", remote);\n \n \tstrbuf_getline_lf(&buf, f);\n \tfclose(f);\n \tstrbuf_trim(&buf);\n-\tif (!buf.len) {\n-\t\tstrbuf_release(&buf);\n-\t\treturn;\n-\t}\n+\tif (!buf.len)\n+\t\tgoto out;\n \n \tremote->configured_in_repo = 1;\n \tremote->origin = REMOTE_BRANCHES;\n@@ -393,6 +395,7 @@ static void read_branches_file(struct remote_state *remote_state,\n \trefspec_appendf(&remote->push, \"HEAD:refs/heads/%s\", frag);\n \tremote->fetch_tags = 1; /* always auto-follow */\n \n+out:\n \tstrbuf_release(&buf);\n \tfree(to_free);\n }\ndiff --git a/rerere.c b/rerere.c\nindex 763cb715a6..2239c2d775 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -127,8 +127,12 @@ static int is_rr_file(const char *name, const char *filename, int *variant)\n static void scan_rerere_dir(struct rerere_dir *rr_dir)\n {\n \tstruct dirent *de;\n-\tDIR *dir = opendir(git_path(\"rr-cache/%s\", rr_dir->name));\n+\tchar *path;\n+\tDIR *dir;\n \n+\tpath = repo_git_path(the_repository, \"rr-cache/%s\", rr_dir->name);\n+\tdir = opendir(path);\n+\tfree(path);\n \tif (!dir)\n \t\treturn;\n \twhile ((de = readdir(dir)) != NULL) {\n@@ -1234,6 +1238,7 @@ void rerere_gc(struct repository *r, struct string_list *rr)\n \ttimestamp_t now = time(NULL);\n \ttimestamp_t cutoff_noresolve = now - 15 * 86400;\n \ttimestamp_t cutoff_resolve = now - 60 * 86400;\n+\tstruct strbuf buf = STRBUF_INIT;\n \n \tif (setup_rerere(r, rr, 0) < 0)\n \t\treturn;\n@@ -1243,7 +1248,7 @@ void rerere_gc(struct repository *r, struct string_list *rr)\n \trepo_config_get_expiry_in_days(the_repository, \"gc.rerereunresolved\",\n \t\t\t\t &cutoff_noresolve, now);\n \tgit_config(git_default_config, NULL);\n-\tdir = opendir(git_path(\"rr-cache\"));\n+\tdir = opendir(repo_git_path_replace(the_repository, &buf, \"rr-cache\"));\n \tif (!dir)\n \t\tdie_errno(_(\"unable to open rr-cache directory\"));\n \t/* Collect stale conflict IDs ... */\n@@ -1272,9 +1277,12 @@ void rerere_gc(struct repository *r, struct string_list *rr)\n \n \t/* ... and then remove the empty directories */\n \tfor (i = 0; i < to_remove.nr; i++)\n-\t\trmdir(git_path(\"rr-cache/%s\", to_remove.items[i].string));\n+\t\trmdir(repo_git_path_replace(the_repository, &buf,\n+\t\t\t\t\t \"rr-cache/%s\", to_remove.items[i].string));\n+\n \tstring_list_clear(&to_remove, 0);\n \trollback_lock_file(&write_lock);\n+\tstrbuf_release(&buf);\n }\n \n /*\ndiff --git a/shallow.c b/shallow.c\nindex b54244ffa9..4bd9342c9a 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -364,7 +364,9 @@ const char *setup_temporary_shallow(const struct oid_array *extra)\n \tstruct strbuf sb = STRBUF_INIT;\n \n \tif (write_shallow_commits(&sb, 0, extra)) {\n-\t\ttemp = xmks_tempfile(git_path(\"shallow_XXXXXX\"));\n+\t\tchar *path = repo_git_path(the_repository, \"shallow_XXXXXX\");\n+\t\ttemp = xmks_tempfile(path);\n+\t\tfree(path);\n \n \t\tif (write_in_full(temp->fd, sb.buf, sb.len) < 0 ||\n \t\t close_tempfile_gently(temp) < 0)\ndiff --git a/wt-status.c b/wt-status.c\nindex 3ee9181764..1da5732f57 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1289,7 +1289,8 @@ static void show_am_in_progress(struct wt_status *s,\n static char *read_line_from_git_path(const char *filename)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tFILE *fp = fopen_or_warn(git_path(\"%s\", filename), \"r\");\n+\tFILE *fp = fopen_or_warn(repo_git_path_append(the_repository, &buf,\n+\t\t\t\t\t\t \"%s\", filename), \"r\");\n \n \tif (!fp) {\n \t\tstrbuf_release(&buf);\n@@ -1383,27 +1384,33 @@ static void abbrev_oid_in_line(struct strbuf *line)\n \n static int read_rebase_todolist(const char *fname, struct string_list *lines)\n {\n-\tstruct strbuf line = STRBUF_INIT;\n-\tFILE *f = fopen(git_path(\"%s\", fname), \"r\");\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tFILE *f = fopen(repo_git_path_append(the_repository, &buf, \"%s\", fname), \"r\");\n+\tint ret;\n \n \tif (!f) {\n-\t\tif (errno == ENOENT)\n-\t\t\treturn -1;\n+\t\tif (errno == ENOENT) {\n+\t\t\tret = -1;\n+\t\t\tgoto out;\n+\t\t}\n \t\tdie_errno(\"Could not open file %s for reading\",\n-\t\t\t git_path(\"%s\", fname));\n+\t\t\t repo_git_path_replace(the_repository, &buf, \"%s\", fname));\n \t}\n-\twhile (!strbuf_getline_lf(&line, f)) {\n-\t\tif (starts_with(line.buf, comment_line_str))\n+\twhile (!strbuf_getline_lf(&buf, f)) {\n+\t\tif (starts_with(buf.buf, comment_line_str))\n \t\t\tcontinue;\n-\t\tstrbuf_trim(&line);\n-\t\tif (!line.len)\n+\t\tstrbuf_trim(&buf);\n+\t\tif (!buf.len)\n \t\t\tcontinue;\n-\t\tabbrev_oid_in_line(&line);\n-\t\tstring_list_append(lines, line.buf);\n+\t\tabbrev_oid_in_line(&buf);\n+\t\tstring_list_append(lines, buf.buf);\n \t}\n \tfclose(f);\n-\tstrbuf_release(&line);\n-\treturn 0;\n+\n+\tret = 0;\n+out:\n+\tstrbuf_release(&buf);\n+\treturn ret;\n }\n \n static void show_rebase_information(struct wt_status *s,\n@@ -1434,9 +1441,12 @@ static void show_rebase_information(struct wt_status *s,\n \t\t\t\ti < have_done.nr;\n \t\t\t\ti++)\n \t\t\t\tstatus_printf_ln(s, color, \" %s\", have_done.items[i].string);\n-\t\t\tif (have_done.nr > nr_lines_to_show && s->hints)\n+\t\t\tif (have_done.nr > nr_lines_to_show && s->hints) {\n+\t\t\t\tchar *path = repo_git_path(the_repository, \"rebase-merge/done\");\n \t\t\t\tstatus_printf_ln(s, color,\n-\t\t\t\t\t_(\" (see more in file %s)\"), git_path(\"rebase-merge/done\"));\n+\t\t\t\t\t_(\" (see more in file %s)\"), path);\n+\t\t\t\tfree(path);\n+\t\t\t}\n \t\t}\n \n \t\tif (yet_to_do.nr == 0)\n\n-- \n2.48.1.538.gc4cfc42d60.dirty\n\n"},{"id":"511979","messageId":"20250206-b4-pks-path-drop-the-repository-v1-13-4e77f0313206@pks.im","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-0-4e77f0313206@pks.im","subject":"[PATCH 13/16] repo-settings: introduce function to clear struct","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-06T07:58:09Z","receivedAt":"2025-02-06T07:58:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We don't provide a way to clear a `struct repo_settings`, and instead\nopen-code this in `repo_clear()`. This is mixing up concerns and means\nthat developers have to touch multiple files whenever they add a new\nfield to the structure in case the associated resources need to be\nreleased.\n\nProvide a new `repo_settings_clear()` function to improve this.\n\nSigned-off-by: Patrick Steinhardt \n---\n repo-settings.c | 10 ++++++++--\n repo-settings.h | 1 +\n repository.c | 2 +-\n 3 files changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 9d16d5399e..719cd7c85c 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -21,7 +21,6 @@ static void repo_cfg_int(struct repository *r, const char *key, int *dest,\n \n void prepare_repo_settings(struct repository *r)\n {\n-\tconst struct repo_settings defaults = REPO_SETTINGS_INIT;\n \tint experimental;\n \tint value;\n \tconst char *strval;\n@@ -35,7 +34,7 @@ void prepare_repo_settings(struct repository *r)\n \tif (r->settings.initialized)\n \t\treturn;\n \n-\tmemcpy(&r->settings, &defaults, sizeof(defaults));\n+\trepo_settings_clear(r);\n \tr->settings.initialized++;\n \n \t/* Booleans config or default, cascades to other settings */\n@@ -143,6 +142,13 @@ void prepare_repo_settings(struct repository *r)\n \t\tr->settings.packed_git_limit = ulongval;\n }\n \n+void repo_settings_clear(struct repository *r)\n+{\n+\tstruct repo_settings empty = REPO_SETTINGS_INIT;\n+\tFREE_AND_NULL(r->settings.fsmonitor);\n+\tr->settings = empty;\n+}\n+\n enum log_refs_config repo_settings_get_log_all_ref_updates(struct repository *repo)\n {\n \tconst char *value;\ndiff --git a/repo-settings.h b/repo-settings.h\nindex 93ea0c3274..c4f7e3bd8a 100644\n--- a/repo-settings.h\n+++ b/repo-settings.h\n@@ -73,6 +73,7 @@ struct repo_settings {\n }\n \n void prepare_repo_settings(struct repository *r);\n+void repo_settings_clear(struct repository *r);\n \n /* Read the value for \"core.logAllRefUpdates\". */\n enum log_refs_config repo_settings_get_log_all_ref_updates(struct repository *repo);\ndiff --git a/repository.c b/repository.c\nindex 648cd88474..6cbaf2e3da 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -380,7 +380,7 @@ void repo_clear(struct repository *repo)\n \tparsed_object_pool_clear(repo->parsed_objects);\n \tFREE_AND_NULL(repo->parsed_objects);\n \n-\tFREE_AND_NULL(repo->settings.fsmonitor);\n+\trepo_settings_clear(repo);\n \n \tif (repo->config) {\n \t\tgit_configset_clear(repo->config);\n\n-- \n2.48.1.538.gc4cfc42d60.dirty\n\n"},{"id":"511981","messageId":"20250206-b4-pks-path-drop-the-repository-v1-14-4e77f0313206@pks.im","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-0-4e77f0313206@pks.im","subject":"[PATCH 14/16] environment: move access to \"core.hooksPath\" into repo settings","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-06T07:58:10Z","receivedAt":"2025-02-06T07:58:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The \"core.hooksPath\" setting is stored in a global variable and\npopulated via the `git_default_core_config`. This may cause issues in\nthe case where one is handling multiple different repositories in a\nsingle process with different values for that config key, as we may or\nmay not see the correct value in that case. Furthermore, global state\nblocks our path towards libification.\n\nRefactor the code so that we instead store the value in `struct\nrepo_settings`. The value is computed as-needed and cached. The result\nshould be functionally the same as there aren't ever any code paths\nwhere we'd execute hooks outside the context of a repository.\n\nSigned-off-by: Patrick Steinhardt \n---\n config.c | 5 -----\n environment.c | 1 -\n environment.h | 1 -\n path.c | 7 ++++---\n repo-settings.c | 8 ++++++++\n repo-settings.h | 4 ++++\n 6 files changed, 16 insertions(+), 10 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 50f2d17b39..d932d4b134 100644\n--- a/config.c\n+++ b/config.c\n@@ -1436,11 +1436,6 @@ static int git_default_core_config(const char *var, const char *value,\n \t\treturn git_config_pathname(&git_attributes_file, var, value);\n \t}\n \n-\tif (!strcmp(var, \"core.hookspath\")) {\n-\t\tFREE_AND_NULL(git_hooks_path);\n-\t\treturn git_config_pathname(&git_hooks_path, var, value);\n-\t}\n-\n \tif (!strcmp(var, \"core.bare\")) {\n \t\tis_bare_repository_cfg = git_config_bool(var, value);\n \t\treturn 0;\ndiff --git a/environment.c b/environment.c\nindex 8389a27270..39755873ee 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -42,7 +42,6 @@ char *git_log_output_encoding;\n char *apply_default_whitespace;\n char *apply_default_ignorewhitespace;\n char *git_attributes_file;\n-char *git_hooks_path;\n int zlib_compression_level = Z_BEST_SPEED;\n int pack_compression_level = Z_DEFAULT_COMPRESSION;\n int fsync_object_files = -1;\ndiff --git a/environment.h b/environment.h\nindex 2f43340f0b..66989afbac 100644\n--- a/environment.h\n+++ b/environment.h\n@@ -160,7 +160,6 @@ extern int warn_on_object_refname_ambiguity;\n extern char *apply_default_whitespace;\n extern char *apply_default_ignorewhitespace;\n extern char *git_attributes_file;\n-extern char *git_hooks_path;\n extern int zlib_compression_level;\n extern int pack_compression_level;\n extern size_t packed_git_window_size;\ndiff --git a/path.c b/path.c\nindex ddfc901661..e81ebd3b5c 100644\n--- a/path.c\n+++ b/path.c\n@@ -387,10 +387,11 @@ void report_linked_checkout_garbage(struct repository *r)\n \tstrbuf_release(&sb);\n }\n \n-static void adjust_git_path(const struct repository *repo,\n+static void adjust_git_path(struct repository *repo,\n \t\t\t struct strbuf *buf, int git_dir_len)\n {\n \tconst char *base = buf->buf + git_dir_len;\n+\n \tif (is_dir_file(base, \"info\", \"grafts\"))\n \t\tstrbuf_splice(buf, 0, buf->len,\n \t\t\t repo->graft_file, strlen(repo->graft_file));\n@@ -399,8 +400,8 @@ static void adjust_git_path(const struct repository *repo,\n \t\t\t repo->index_file, strlen(repo->index_file));\n \telse if (dir_prefix(base, \"objects\"))\n \t\treplace_dir(buf, git_dir_len + 7, repo->objects->odb->path);\n-\telse if (git_hooks_path && dir_prefix(base, \"hooks\"))\n-\t\treplace_dir(buf, git_dir_len + 5, git_hooks_path);\n+\telse if (repo_settings_get_hooks_path(repo) && dir_prefix(base, \"hooks\"))\n+\t\treplace_dir(buf, git_dir_len + 5, repo_settings_get_hooks_path(repo));\n \telse if (repo->different_commondir)\n \t\tupdate_common_dir(buf, git_dir_len, repo->commondir);\n }\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 719cd7c85c..876d527581 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -146,6 +146,7 @@ void repo_settings_clear(struct repository *r)\n {\n \tstruct repo_settings empty = REPO_SETTINGS_INIT;\n \tFREE_AND_NULL(r->settings.fsmonitor);\n+\tFREE_AND_NULL(r->settings.hooks_path);\n \tr->settings = empty;\n }\n \n@@ -173,3 +174,10 @@ int repo_settings_get_warn_ambiguous_refs(struct repository *repo)\n \t\t\t &repo->settings.warn_ambiguous_refs, 1);\n \treturn repo->settings.warn_ambiguous_refs;\n }\n+\n+const char *repo_settings_get_hooks_path(struct repository *repo)\n+{\n+\tif (!repo->settings.hooks_path)\n+\t\trepo_config_get_pathname(repo, \"core.hookspath\", &repo->settings.hooks_path);\n+\treturn repo->settings.hooks_path;\n+}\ndiff --git a/repo-settings.h b/repo-settings.h\nindex c4f7e3bd8a..0cef970443 100644\n--- a/repo-settings.h\n+++ b/repo-settings.h\n@@ -61,6 +61,8 @@ struct repo_settings {\n \tsize_t delta_base_cache_limit;\n \tsize_t packed_git_window_size;\n \tsize_t packed_git_limit;\n+\n+\tchar *hooks_path;\n };\n #define REPO_SETTINGS_INIT { \\\n \t.index_version = -1, \\\n@@ -79,5 +81,7 @@ void repo_settings_clear(struct repository *r);\n enum log_refs_config repo_settings_get_log_all_ref_updates(struct repository *repo);\n /* Read the value for \"core.warnAmbiguousRefs\". */\n int repo_settings_get_warn_ambiguous_refs(struct repository *repo);\n+/* Read the value for \"core.hooksPath\". */\n+const char *repo_settings_get_hooks_path(struct repository *repo);\n \n #endif /* REPO_SETTINGS_H */\n\n-- \n2.48.1.538.gc4cfc42d60.dirty\n\n"},{"id":"511982","messageId":"20250206-b4-pks-path-drop-the-repository-v1-15-4e77f0313206@pks.im","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-0-4e77f0313206@pks.im","subject":"[PATCH 15/16] environment: move access to \"core.sharedRepository\" into repo settings","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-06T07:58:11Z","receivedAt":"2025-02-06T07:58:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Similar as with the preceding commit, we track \"core.sharedRepository\"\nvia a pair of global variables. Move them into `struct repo_settings` so\nthat we can instead track them per-repository.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/init-db.c | 8 ++++----\n builtin/log.c | 6 +++---\n environment.c | 26 --------------------------\n environment.h | 10 ----------\n path.c | 10 +++++-----\n repo-settings.c | 26 ++++++++++++++++++++++++++\n repo-settings.h | 9 +++++++++\n setup.c | 21 +++++++++++----------\n 8 files changed, 58 insertions(+), 58 deletions(-)\n\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 096f96b9c4..196dccdd77 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -132,8 +132,8 @@ int cmd_init_db(int argc,\n \t\t\t\t * and we know shared_repository should always be 0;\n \t\t\t\t * but just in case we play safe.\n \t\t\t\t */\n-\t\t\t\tsaved = get_shared_repository();\n-\t\t\t\tset_shared_repository(0);\n+\t\t\t\tsaved = repo_settings_get_shared_repository(the_repository);\n+\t\t\t\trepo_settings_set_shared_repository(the_repository, 0);\n \t\t\t\tswitch (safe_create_leading_directories_const(argv[0])) {\n \t\t\t\tcase SCLD_OK:\n \t\t\t\tcase SCLD_PERMS:\n@@ -145,7 +145,7 @@ int cmd_init_db(int argc,\n \t\t\t\t\tdie_errno(_(\"cannot mkdir %s\"), argv[0]);\n \t\t\t\t\tbreak;\n \t\t\t\t}\n-\t\t\t\tset_shared_repository(saved);\n+\t\t\t\trepo_settings_set_shared_repository(the_repository, saved);\n \t\t\t\tif (mkdir(argv[0], 0777) < 0)\n \t\t\t\t\tdie_errno(_(\"cannot mkdir %s\"), argv[0]);\n \t\t\t\tmkdir_tried = 1;\n@@ -175,7 +175,7 @@ int cmd_init_db(int argc,\n \t}\n \n \tif (init_shared_repository != -1)\n-\t\tset_shared_repository(init_shared_repository);\n+\t\trepo_settings_set_shared_repository(the_repository, init_shared_repository);\n \n \t/*\n \t * GIT_WORK_TREE makes sense only in conjunction with GIT_DIR\ndiff --git a/builtin/log.c b/builtin/log.c\nindex e41f88945e..04a6ef97bc 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -2309,8 +2309,8 @@ int cmd_format_patch(int argc,\n \t\t * We consider as 'outside of gitdir', therefore avoid\n \t\t * applying adjust_shared_perm in s-c-l-d.\n \t\t */\n-\t\tsaved = get_shared_repository();\n-\t\tset_shared_repository(0);\n+\t\tsaved = repo_settings_get_shared_repository(the_repository);\n+\t\trepo_settings_set_shared_repository(the_repository, 0);\n \t\tswitch (safe_create_leading_directories_const(output_directory)) {\n \t\tcase SCLD_OK:\n \t\tcase SCLD_EXISTS:\n@@ -2319,7 +2319,7 @@ int cmd_format_patch(int argc,\n \t\t\tdie(_(\"could not create leading directories \"\n \t\t\t \"of '%s'\"), output_directory);\n \t\t}\n-\t\tset_shared_repository(saved);\n+\t\trepo_settings_set_shared_repository(the_repository, saved);\n \t\tif (mkdir(output_directory, 0777) < 0 && errno != EEXIST)\n \t\t\tdie_errno(_(\"could not create directory '%s'\"),\n \t\t\t\t output_directory);\ndiff --git a/environment.c b/environment.c\nindex 39755873ee..c79acc69e7 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -206,32 +206,6 @@ const char *get_commit_output_encoding(void)\n \treturn git_commit_encoding ? git_commit_encoding : \"UTF-8\";\n }\n \n-static int the_shared_repository = PERM_UMASK;\n-static int need_shared_repository_from_config = 1;\n-\n-void set_shared_repository(int value)\n-{\n-\tthe_shared_repository = value;\n-\tneed_shared_repository_from_config = 0;\n-}\n-\n-int get_shared_repository(void)\n-{\n-\tif (need_shared_repository_from_config) {\n-\t\tconst char *var = \"core.sharedrepository\";\n-\t\tconst char *value;\n-\t\tif (!git_config_get_value(var, &value))\n-\t\t\tthe_shared_repository = git_config_perm(var, value);\n-\t\tneed_shared_repository_from_config = 0;\n-\t}\n-\treturn the_shared_repository;\n-}\n-\n-void reset_shared_repository(void)\n-{\n-\tneed_shared_repository_from_config = 1;\n-}\n-\n int use_optional_locks(void)\n {\n \treturn git_env_bool(GIT_OPTIONAL_LOCKS_ENVIRONMENT, 1);\ndiff --git a/environment.h b/environment.h\nindex 66989afbac..45e690f203 100644\n--- a/environment.h\n+++ b/environment.h\n@@ -134,16 +134,6 @@ void setup_git_env(const char *git_dir);\n */\n int have_git_dir(void);\n \n-/*\n- * Accessors for the core.sharedrepository config which lazy-load the value\n- * from the config (if not already set). The \"reset\" function can be\n- * used to unset \"set\" or cached value, meaning that the value will be loaded\n- * fresh from the config file on the next call to get_shared_repository().\n- */\n-void set_shared_repository(int value);\n-int get_shared_repository(void);\n-void reset_shared_repository(void);\n-\n extern int is_bare_repository_cfg;\n int is_bare_repository(void);\n extern char *git_work_tree_cfg;\ndiff --git a/path.c b/path.c\nindex e81ebd3b5c..a2f402baec 100644\n--- a/path.c\n+++ b/path.c\n@@ -844,17 +844,17 @@ int calc_shared_perm(int mode)\n {\n \tint tweak;\n \n-\tif (get_shared_repository() < 0)\n-\t\ttweak = -get_shared_repository();\n+\tif (repo_settings_get_shared_repository(the_repository) < 0)\n+\t\ttweak = -repo_settings_get_shared_repository(the_repository);\n \telse\n-\t\ttweak = get_shared_repository();\n+\t\ttweak = repo_settings_get_shared_repository(the_repository);\n \n \tif (!(mode & S_IWUSR))\n \t\ttweak &= ~0222;\n \tif (mode & S_IXUSR)\n \t\t/* Copy read bits to execute bits */\n \t\ttweak |= (tweak & 0444) >> 2;\n-\tif (get_shared_repository() < 0)\n+\tif (repo_settings_get_shared_repository(the_repository) < 0)\n \t\tmode = (mode & ~0777) | tweak;\n \telse\n \t\tmode |= tweak;\n@@ -867,7 +867,7 @@ int adjust_shared_perm(const char *path)\n {\n \tint old_mode, new_mode;\n \n-\tif (!get_shared_repository())\n+\tif (!repo_settings_get_shared_repository(the_repository))\n \t\treturn 0;\n \tif (get_st_mode_bits(path, &old_mode) < 0)\n \t\treturn -1;\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 876d527581..67e9cfd2e6 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -4,6 +4,7 @@\n #include \"repository.h\"\n #include \"midx.h\"\n #include \"pack-objects.h\"\n+#include \"setup.h\"\n \n static void repo_cfg_bool(struct repository *r, const char *key, int *dest,\n \t\t\t int def)\n@@ -181,3 +182,28 @@ const char *repo_settings_get_hooks_path(struct repository *repo)\n \t\trepo_config_get_pathname(repo, \"core.hookspath\", &repo->settings.hooks_path);\n \treturn repo->settings.hooks_path;\n }\n+\n+int repo_settings_get_shared_repository(struct repository *repo)\n+{\n+\tif (!repo->settings.shared_repository_initialized) {\n+\t\tconst char *var = \"core.sharedrepository\";\n+\t\tconst char *value;\n+\t\tif (!repo_config_get_value(repo, var, &value))\n+\t\t\trepo->settings.shared_repository = git_config_perm(var, value);\n+\t\telse\n+\t\t\trepo->settings.shared_repository = PERM_UMASK;\n+\t\trepo->settings.shared_repository_initialized = 1;\n+\t}\n+\treturn repo->settings.shared_repository;\n+}\n+\n+void repo_settings_set_shared_repository(struct repository *repo, int value)\n+{\n+\trepo->settings.shared_repository = value;\n+\trepo->settings.shared_repository_initialized = 1;\n+}\n+\n+void repo_settings_reset_shared_repository(struct repository *repo)\n+{\n+\trepo->settings.shared_repository_initialized = 0;\n+}\ndiff --git a/repo-settings.h b/repo-settings.h\nindex 0cef970443..ddc11967e0 100644\n--- a/repo-settings.h\n+++ b/repo-settings.h\n@@ -37,6 +37,9 @@ struct repo_settings {\n \tint pack_use_bitmap_boundary_traversal;\n \tint pack_use_multi_pack_reuse;\n \n+\tint shared_repository;\n+\tint shared_repository_initialized;\n+\n \t/*\n \t * Does this repository have core.useReplaceRefs=true (on by\n \t * default)? This provides a repository-scoped version of this\n@@ -65,6 +68,7 @@ struct repo_settings {\n \tchar *hooks_path;\n };\n #define REPO_SETTINGS_INIT { \\\n+\t.shared_repository = -1, \\\n \t.index_version = -1, \\\n \t.core_untracked_cache = UNTRACKED_CACHE_KEEP, \\\n \t.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE, \\\n@@ -84,4 +88,9 @@ int repo_settings_get_warn_ambiguous_refs(struct repository *repo);\n /* Read the value for \"core.hooksPath\". */\n const char *repo_settings_get_hooks_path(struct repository *repo);\n \n+/* Read, set or reset the value for \"core.sharedRepository\". */\n+int repo_settings_get_shared_repository(struct repository *repo);\n+void repo_settings_set_shared_repository(struct repository *repo, int value);\n+void repo_settings_reset_shared_repository(struct repository *repo);\n+\n #endif /* REPO_SETTINGS_H */\ndiff --git a/setup.c b/setup.c\nindex 30889386f7..aa65b93f53 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -2332,7 +2332,7 @@ static int create_default_files(const char *template_path,\n \t */\n \tcopy_templates(template_path);\n \tgit_config_clear();\n-\treset_shared_repository();\n+\trepo_settings_reset_shared_repository(the_repository);\n \tgit_config(git_default_config, NULL);\n \n \treinit = is_reinit();\n@@ -2342,7 +2342,8 @@ static int create_default_files(const char *template_path,\n \t * values we might have just re-read from the config.\n \t */\n \tif (init_shared_repository != -1)\n-\t\tset_shared_repository(init_shared_repository);\n+\t\trepo_settings_set_shared_repository(the_repository,\n+\t\t\t\t\t\t init_shared_repository);\n \n \tis_bare_repository_cfg = !work_tree;\n \n@@ -2350,7 +2351,7 @@ static int create_default_files(const char *template_path,\n \t * We would have created the above under user's umask -- under\n \t * shared-repository settings, we would need to fix them up.\n \t */\n-\tif (get_shared_repository()) {\n+\tif (repo_settings_get_shared_repository(the_repository)) {\n \t\tadjust_shared_perm(repo_get_git_dir(the_repository));\n \t}\n \n@@ -2597,7 +2598,7 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \t\t\t\t\t initial_branch, flags & INIT_DB_QUIET);\n \tcreate_object_directory();\n \n-\tif (get_shared_repository()) {\n+\tif (repo_settings_get_shared_repository(the_repository)) {\n \t\tchar buf[10];\n \t\t/* We do not spell \"group\" and such, so that\n \t\t * the configuration can be read by older version\n@@ -2605,12 +2606,12 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \t\t * and compatibility values for PERM_GROUP and\n \t\t * PERM_EVERYBODY.\n \t\t */\n-\t\tif (get_shared_repository() < 0)\n+\t\tif (repo_settings_get_shared_repository(the_repository) < 0)\n \t\t\t/* force to the mode value */\n-\t\t\txsnprintf(buf, sizeof(buf), \"0%o\", -get_shared_repository());\n-\t\telse if (get_shared_repository() == PERM_GROUP)\n+\t\t\txsnprintf(buf, sizeof(buf), \"0%o\", -repo_settings_get_shared_repository(the_repository));\n+\t\telse if (repo_settings_get_shared_repository(the_repository) == PERM_GROUP)\n \t\t\txsnprintf(buf, sizeof(buf), \"%d\", OLD_PERM_GROUP);\n-\t\telse if (get_shared_repository() == PERM_EVERYBODY)\n+\t\telse if (repo_settings_get_shared_repository(the_repository) == PERM_EVERYBODY)\n \t\t\txsnprintf(buf, sizeof(buf), \"%d\", OLD_PERM_EVERYBODY);\n \t\telse\n \t\t\tBUG(\"invalid value for shared_repository\");\n@@ -2622,12 +2623,12 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \t\tint len = strlen(git_dir);\n \n \t\tif (reinit)\n-\t\t\tprintf(get_shared_repository()\n+\t\t\tprintf(repo_settings_get_shared_repository(the_repository)\n \t\t\t ? _(\"Reinitialized existing shared Git repository in %s%s\\n\")\n \t\t\t : _(\"Reinitialized existing Git repository in %s%s\\n\"),\n \t\t\t git_dir, len && git_dir[len-1] != '/' ? \"/\" : \"\");\n \t\telse\n-\t\t\tprintf(get_shared_repository()\n+\t\t\tprintf(repo_settings_get_shared_repository(the_repository)\n \t\t\t ? _(\"Initialized empty shared Git repository in %s%s\\n\")\n \t\t\t : _(\"Initialized empty Git repository in %s%s\\n\"),\n \t\t\t git_dir, len && git_dir[len-1] != '/' ? \"/\" : \"\");\n\n-- \n2.48.1.538.gc4cfc42d60.dirty\n\n"},{"id":"511983","messageId":"20250206-b4-pks-path-drop-the-repository-v1-16-4e77f0313206@pks.im","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-0-4e77f0313206@pks.im","subject":"[PATCH 16/16] path: adjust last remaining users of `the_repository`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-06T07:58:12Z","receivedAt":"2025-02-06T07:58:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"With the preceding refactorings we now only have a couple of implicit\nusers of `the_repository` left in the \"path\" subsystem, all of which\ndepend on global state via `calc_shared_perm()`. Make the dependency on\n`the_repository` explicit by passing the repo as a parameter instead and\nadjust callers accordingly.\n\nNote that this change bubbles up into a couple of subsystems that were\npreviously declared as free from `the_repository`. Instead of marking\nall of them as `the_repository`-dependent again, we instead use the\nrepository that is available in the calling context. There are three\nexceptions though with \"copy.c\", \"pack-write.c\" and \"tempfile.c\".\nAdjusting these would require us to adapt callsites all over the place,\nso this is left for a future iteration.\n\nMark \"path.c\" as free from `the_repository`.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/clone.c | 2 +-\n commit-graph.c | 2 +-\n copy.c | 4 +++-\n loose.c | 2 +-\n midx-write.c | 2 +-\n object-file.c | 8 ++++----\n pack-bitmap-write.c | 2 +-\n pack-write.c | 10 ++++++----\n path.c | 25 ++++++++++++-------------\n path.h | 6 +++---\n read-cache.c | 2 +-\n refs/files-backend.c | 10 +++++-----\n refs/reftable-backend.c | 10 +++++-----\n server-info.c | 2 +-\n setup.c | 12 ++++++------\n tempfile.c | 4 +++-\n tmp-objdir.c | 17 ++++++++++-------\n 17 files changed, 64 insertions(+), 56 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 5ae6ee9db9..23eeb782aa 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1220,7 +1220,7 @@ int cmd_clone(int argc,\n \n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"%s/refs\", git_dir);\n-\tsafe_create_dir(buf.buf, 1);\n+\tsafe_create_dir(the_repository, buf.buf, 1);\n \n \t/*\n \t * additional config can be injected with -c, make sure it's included\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 2a2999a6b8..1021ccb983 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -2084,7 +2084,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t\t\treturn -1;\n \t\t}\n \n-\t\tif (adjust_shared_perm(get_tempfile_path(graph_layer))) {\n+\t\tif (adjust_shared_perm(the_repository, get_tempfile_path(graph_layer))) {\n \t\t\terror(_(\"unable to adjust shared permissions for '%s'\"),\n \t\t\t get_tempfile_path(graph_layer));\n \t\t\treturn -1;\ndiff --git a/copy.c b/copy.c\nindex d9d2092012..b668209b6c 100644\n--- a/copy.c\n+++ b/copy.c\n@@ -1,3 +1,5 @@\n+#define USE_THE_REPOSITORY_VARIABLE\n+\n #include \"git-compat-util.h\"\n #include \"copy.h\"\n #include \"path.h\"\n@@ -57,7 +59,7 @@ int copy_file(const char *dst, const char *src, int mode)\n \tif (close(fdo) != 0)\n \t\treturn error_errno(\"%s: close error\", dst);\n \n-\tif (!status && adjust_shared_perm(dst))\n+\tif (!status && adjust_shared_perm(the_repository, dst))\n \t\treturn -1;\n \n \treturn status;\ndiff --git a/loose.c b/loose.c\nindex 51ef490f93..bb602aaa36 100644\n--- a/loose.c\n+++ b/loose.c\n@@ -190,7 +190,7 @@ static int write_one_object(struct repository *repo, const struct object_id *oid\n \t\tgoto errout;\n \tif (close(fd))\n \t\tgoto errout;\n-\tadjust_shared_perm(path.buf);\n+\tadjust_shared_perm(repo, path.buf);\n \trollback_lock_file(&lock);\n \tstrbuf_release(&buf);\n \tstrbuf_release(&path);\ndiff --git a/midx-write.c b/midx-write.c\nindex 61b59d557d..48d6558253 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1336,7 +1336,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,\n \t\t\treturn -1;\n \t\t}\n \n-\t\tif (adjust_shared_perm(get_tempfile_path(incr))) {\n+\t\tif (adjust_shared_perm(r, get_tempfile_path(incr))) {\n \t\t\terror(_(\"unable to adjust shared permissions for '%s'\"),\n \t\t\t get_tempfile_path(incr));\n \t\t\treturn -1;\ndiff --git a/object-file.c b/object-file.c\nindex dc9fcaf3e9..5d782417e3 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -388,7 +388,7 @@ int mkdir_in_gitdir(const char *path)\n \t\t}\n \t\tstrbuf_release(&sb);\n \t}\n-\treturn adjust_shared_perm(path);\n+\treturn adjust_shared_perm(the_repository, path);\n }\n \n static enum scld_error safe_create_leading_directories_1(char *path, int share)\n@@ -437,7 +437,7 @@ static enum scld_error safe_create_leading_directories_1(char *path, int share)\n \t\t\t\tret = SCLD_VANISHED;\n \t\t\telse\n \t\t\t\tret = SCLD_FAILED;\n-\t\t} else if (share && adjust_shared_perm(path)) {\n+\t\t} else if (share && adjust_shared_perm(the_repository, path)) {\n \t\t\tret = SCLD_PERMS;\n \t\t}\n \t\t*slash = slash_character;\n@@ -2105,7 +2105,7 @@ int finalize_object_file_flags(const char *tmpfile, const char *filename,\n \t}\n \n out:\n-\tif (adjust_shared_perm(filename))\n+\tif (adjust_shared_perm(the_repository, filename))\n \t\treturn error(_(\"unable to set permission to '%s'\"), filename);\n \treturn 0;\n }\n@@ -2181,7 +2181,7 @@ static int create_tmpfile(struct strbuf *tmp, const char *filename)\n \t\tstrbuf_add(tmp, filename, dirlen - 1);\n \t\tif (mkdir(tmp->buf, 0777) && errno != EEXIST)\n \t\t\treturn -1;\n-\t\tif (adjust_shared_perm(tmp->buf))\n+\t\tif (adjust_shared_perm(the_repository, tmp->buf))\n \t\t\treturn -1;\n \n \t\t/* Try again */\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex a06a1f35c6..34e86d4994 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -1072,7 +1072,7 @@ void bitmap_writer_finish(struct bitmap_writer *writer,\n \tfinalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA,\n \t\t\t CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);\n \n-\tif (adjust_shared_perm(tmp_file.buf))\n+\tif (adjust_shared_perm(the_repository, tmp_file.buf))\n \t\tdie_errno(\"unable to make temporary bitmap file readable\");\n \n \tif (rename(tmp_file.buf, filename))\ndiff --git a/pack-write.c b/pack-write.c\nindex a2faeb1895..c3f4e66f02 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -1,3 +1,5 @@\n+#define USE_THE_REPOSITORY_VARIABLE\n+\n #include \"git-compat-util.h\"\n #include \"environment.h\"\n #include \"gettext.h\"\n@@ -287,7 +289,7 @@ char *write_rev_file_order(const struct git_hash_algo *hash_algo,\n \twrite_rev_index_positions(f, pack_order, nr_objects);\n \twrite_rev_trailer(hash_algo, f, hash);\n \n-\tif (adjust_shared_perm(path) < 0)\n+\tif (adjust_shared_perm(the_repository, path) < 0)\n \t\tdie(_(\"failed to make %s readable\"), path);\n \n \tfinalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA,\n@@ -350,7 +352,7 @@ static char *write_mtimes_file(const struct git_hash_algo *hash_algo,\n \twrite_mtimes_objects(f, to_pack, objects, nr_objects);\n \twrite_mtimes_trailer(hash_algo, f, hash);\n \n-\tif (adjust_shared_perm(mtimes_name) < 0)\n+\tif (adjust_shared_perm(the_repository, mtimes_name) < 0)\n \t\tdie(_(\"failed to make %s readable\"), mtimes_name);\n \n \tfinalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA,\n@@ -565,12 +567,12 @@ void stage_tmp_packfiles(const struct git_hash_algo *hash_algo,\n \tchar *rev_tmp_name = NULL;\n \tchar *mtimes_tmp_name = NULL;\n \n-\tif (adjust_shared_perm(pack_tmp_name))\n+\tif (adjust_shared_perm(the_repository, pack_tmp_name))\n \t\tdie_errno(\"unable to make temporary pack file readable\");\n \n \t*idx_tmp_name = (char *)write_idx_file(hash_algo, NULL, written_list,\n \t\t\t\t\t nr_written, pack_idx_opts, hash);\n-\tif (adjust_shared_perm(*idx_tmp_name))\n+\tif (adjust_shared_perm(the_repository, *idx_tmp_name))\n \t\tdie_errno(\"unable to make temporary index file readable\");\n \n \trev_tmp_name = write_rev_file(hash_algo, NULL, written_list, nr_written,\ndiff --git a/path.c b/path.c\nindex a2f402baec..910756c8b3 100644\n--- a/path.c\n+++ b/path.c\n@@ -2,8 +2,6 @@\n * Utilities for paths and pathnames\n */\n \n-#define USE_THE_REPOSITORY_VARIABLE\n-\n #include \"git-compat-util.h\"\n #include \"abspath.h\"\n #include \"environment.h\"\n@@ -840,21 +838,22 @@ const char *enter_repo(const char *path, unsigned flags)\n \treturn NULL;\n }\n \n-int calc_shared_perm(int mode)\n+int calc_shared_perm(struct repository *repo,\n+\t\t int mode)\n {\n \tint tweak;\n \n-\tif (repo_settings_get_shared_repository(the_repository) < 0)\n-\t\ttweak = -repo_settings_get_shared_repository(the_repository);\n+\tif (repo_settings_get_shared_repository(repo) < 0)\n+\t\ttweak = -repo_settings_get_shared_repository(repo);\n \telse\n-\t\ttweak = repo_settings_get_shared_repository(the_repository);\n+\t\ttweak = repo_settings_get_shared_repository(repo);\n \n \tif (!(mode & S_IWUSR))\n \t\ttweak &= ~0222;\n \tif (mode & S_IXUSR)\n \t\t/* Copy read bits to execute bits */\n \t\ttweak |= (tweak & 0444) >> 2;\n-\tif (repo_settings_get_shared_repository(the_repository) < 0)\n+\tif (repo_settings_get_shared_repository(repo) < 0)\n \t\tmode = (mode & ~0777) | tweak;\n \telse\n \t\tmode |= tweak;\n@@ -862,17 +861,17 @@ int calc_shared_perm(int mode)\n \treturn mode;\n }\n \n-\n-int adjust_shared_perm(const char *path)\n+int adjust_shared_perm(struct repository *repo,\n+\t\t const char *path)\n {\n \tint old_mode, new_mode;\n \n-\tif (!repo_settings_get_shared_repository(the_repository))\n+\tif (!repo_settings_get_shared_repository(repo))\n \t\treturn 0;\n \tif (get_st_mode_bits(path, &old_mode) < 0)\n \t\treturn -1;\n \n-\tnew_mode = calc_shared_perm(old_mode);\n+\tnew_mode = calc_shared_perm(repo, old_mode);\n \tif (S_ISDIR(old_mode)) {\n \t\t/* Copy read bits to execute bits */\n \t\tnew_mode |= (new_mode & 0444) >> 2;\n@@ -891,7 +890,7 @@ int adjust_shared_perm(const char *path)\n \treturn 0;\n }\n \n-void safe_create_dir(const char *dir, int share)\n+void safe_create_dir(struct repository *repo, const char *dir, int share)\n {\n \tif (mkdir(dir, 0777) < 0) {\n \t\tif (errno != EEXIST) {\n@@ -899,7 +898,7 @@ void safe_create_dir(const char *dir, int share)\n \t\t\texit(1);\n \t\t}\n \t}\n-\telse if (share && adjust_shared_perm(dir))\n+\telse if (share && adjust_shared_perm(repo, dir))\n \t\tdie(_(\"Could not make %s writable by group\"), dir);\n }\n \ndiff --git a/path.h b/path.h\nindex 373404dd9d..65fe968a13 100644\n--- a/path.h\n+++ b/path.h\n@@ -141,8 +141,8 @@ const char *git_path_shallow(struct repository *r);\n \n int ends_with_path_components(const char *path, const char *components);\n \n-int calc_shared_perm(int mode);\n-int adjust_shared_perm(const char *path);\n+int calc_shared_perm(struct repository *repo, int mode);\n+int adjust_shared_perm(struct repository *repo, const char *path);\n \n char *interpolate_path(const char *path, int real_home);\n \n@@ -219,7 +219,7 @@ char *xdg_cache_home(const char *filename);\n * directories under $GIT_DIR. Don't use it for working tree\n * directories.\n */\n-void safe_create_dir(const char *dir, int share);\n+void safe_create_dir(struct repository *repo, const char *dir, int share);\n \n # ifdef USE_THE_REPOSITORY_VARIABLE\n # include \"strbuf.h\"\ndiff --git a/read-cache.c b/read-cache.c\nindex 66ad0015a7..900738e7a8 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -3290,7 +3290,7 @@ static int write_shared_index(struct index_state *istate,\n \n \tif (ret)\n \t\treturn ret;\n-\tret = adjust_shared_perm(get_tempfile_path(*temp));\n+\tret = adjust_shared_perm(the_repository, get_tempfile_path(*temp));\n \tif (ret) {\n \t\terror(_(\"cannot fix permission bits on '%s'\"), get_tempfile_path(*temp));\n \t\treturn ret;\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 29f08dced4..6c6e67dc1c 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1831,7 +1831,7 @@ static int log_ref_setup(struct files_ref_store *refs,\n \t}\n \n \tif (*logfd >= 0)\n-\t\tadjust_shared_perm(logfile);\n+\t\tadjust_shared_perm(the_repository, logfile);\n \n \tfree(logfile);\n \treturn 0;\n@@ -3488,8 +3488,8 @@ static int files_ref_store_create_on_disk(struct ref_store *ref_store,\n \t * they do not understand the reference format extension.\n \t */\n \tstrbuf_addf(&sb, \"%s/refs\", ref_store->gitdir);\n-\tsafe_create_dir(sb.buf, 1);\n-\tadjust_shared_perm(sb.buf);\n+\tsafe_create_dir(the_repository, sb.buf, 1);\n+\tadjust_shared_perm(the_repository, sb.buf);\n \n \t/*\n \t * There is no need to create directories for common refs when creating\n@@ -3501,11 +3501,11 @@ static int files_ref_store_create_on_disk(struct ref_store *ref_store,\n \t\t */\n \t\tstrbuf_reset(&sb);\n \t\tfiles_ref_path(refs, &sb, \"refs/heads\");\n-\t\tsafe_create_dir(sb.buf, 1);\n+\t\tsafe_create_dir(the_repository, sb.buf, 1);\n \n \t\tstrbuf_reset(&sb);\n \t\tfiles_ref_path(refs, &sb, \"refs/tags\");\n-\t\tsafe_create_dir(sb.buf, 1);\n+\t\tsafe_create_dir(the_repository, sb.buf, 1);\n \t}\n \n \tstrbuf_release(&sb);\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex d39a14c5a4..9c54e2c173 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -380,7 +380,7 @@ static struct ref_store *reftable_be_init(struct repository *repo,\n \tdefault:\n \t\tBUG(\"unknown hash algorithm %d\", repo->hash_algo->format_id);\n \t}\n-\trefs->write_options.default_permissions = calc_shared_perm(0666 & ~mask);\n+\trefs->write_options.default_permissions = calc_shared_perm(the_repository, 0666 & ~mask);\n \trefs->write_options.disable_auto_compact =\n \t\t!git_env_bool(\"GIT_TEST_REFTABLE_AUTOCOMPACTION\", 1);\n \trefs->write_options.lock_timeout_ms = 100;\n@@ -470,21 +470,21 @@ static int reftable_be_create_on_disk(struct ref_store *ref_store,\n \tstruct strbuf sb = STRBUF_INIT;\n \n \tstrbuf_addf(&sb, \"%s/reftable\", refs->base.gitdir);\n-\tsafe_create_dir(sb.buf, 1);\n+\tsafe_create_dir(the_repository, sb.buf, 1);\n \tstrbuf_reset(&sb);\n \n \tstrbuf_addf(&sb, \"%s/HEAD\", refs->base.gitdir);\n \twrite_file(sb.buf, \"ref: refs/heads/.invalid\");\n-\tadjust_shared_perm(sb.buf);\n+\tadjust_shared_perm(the_repository, sb.buf);\n \tstrbuf_reset(&sb);\n \n \tstrbuf_addf(&sb, \"%s/refs\", refs->base.gitdir);\n-\tsafe_create_dir(sb.buf, 1);\n+\tsafe_create_dir(the_repository, sb.buf, 1);\n \tstrbuf_reset(&sb);\n \n \tstrbuf_addf(&sb, \"%s/refs/heads\", refs->base.gitdir);\n \twrite_file(sb.buf, \"this repository uses the reftable format\");\n-\tadjust_shared_perm(sb.buf);\n+\tadjust_shared_perm(the_repository, sb.buf);\n \n \tstrbuf_release(&sb);\n \treturn 0;\ndiff --git a/server-info.c b/server-info.c\nindex 31c3fdc118..1ca0e00d51 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -125,7 +125,7 @@ static int update_info_file(struct repository *r, char *path,\n \tuic.cur_fp = NULL;\n \n \tif (uic_is_stale(&uic)) {\n-\t\tif (adjust_shared_perm(get_tempfile_path(f)) < 0)\n+\t\tif (adjust_shared_perm(r, get_tempfile_path(f)) < 0)\n \t\t\tgoto out;\n \t\tif (rename_tempfile(&f, path) < 0)\n \t\t\tgoto out;\ndiff --git a/setup.c b/setup.c\nindex aa65b93f53..71a3d66f05 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -2088,7 +2088,7 @@ static void copy_templates_1(struct strbuf *path, struct strbuf *template_path,\n \t * with the way the namespace under .git/ is organized, should\n \t * be really carefully chosen.\n \t */\n-\tsafe_create_dir(path->buf, 1);\n+\tsafe_create_dir(the_repository, path->buf, 1);\n \twhile ((de = readdir(dir)) != NULL) {\n \t\tstruct stat st_git, st_template;\n \t\tint exists = 0;\n@@ -2352,7 +2352,7 @@ static int create_default_files(const char *template_path,\n \t * shared-repository settings, we would need to fix them up.\n \t */\n \tif (repo_settings_get_shared_repository(the_repository)) {\n-\t\tadjust_shared_perm(repo_get_git_dir(the_repository));\n+\t\tadjust_shared_perm(the_repository, repo_get_git_dir(the_repository));\n \t}\n \n \tinitialize_repository_version(fmt->hash_algo, fmt->ref_storage_format, reinit);\n@@ -2413,15 +2413,15 @@ static void create_object_directory(void)\n \tstrbuf_addstr(&path, repo_get_object_directory(the_repository));\n \tbaselen = path.len;\n \n-\tsafe_create_dir(path.buf, 1);\n+\tsafe_create_dir(the_repository, path.buf, 1);\n \n \tstrbuf_setlen(&path, baselen);\n \tstrbuf_addstr(&path, \"/pack\");\n-\tsafe_create_dir(path.buf, 1);\n+\tsafe_create_dir(the_repository, path.buf, 1);\n \n \tstrbuf_setlen(&path, baselen);\n \tstrbuf_addstr(&path, \"/info\");\n-\tsafe_create_dir(path.buf, 1);\n+\tsafe_create_dir(the_repository, path.buf, 1);\n \n \tstrbuf_release(&path);\n }\n@@ -2588,7 +2588,7 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \t */\n \tgit_config(platform_core_config, NULL);\n \n-\tsafe_create_dir(git_dir, 0);\n+\tsafe_create_dir(the_repository, git_dir, 0);\n \n \treinit = create_default_files(template_dir, original_git_dir,\n \t\t\t\t &repo_fmt, init_shared_repository);\ndiff --git a/tempfile.c b/tempfile.c\nindex ed88cf8431..82dfa3d82f 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -42,6 +42,8 @@\n * file created by its parent.\n */\n \n+#define USE_THE_REPOSITORY_VARIABLE\n+\n #include \"git-compat-util.h\"\n #include \"abspath.h\"\n #include \"path.h\"\n@@ -148,7 +150,7 @@ struct tempfile *create_tempfile_mode(const char *path, int mode)\n \t\treturn NULL;\n \t}\n \tactivate_tempfile(tempfile);\n-\tif (adjust_shared_perm(tempfile->filename.buf)) {\n+\tif (adjust_shared_perm(the_repository, tempfile->filename.buf)) {\n \t\tint save_errno = errno;\n \t\terror(\"cannot fix permission bits on %s\", tempfile->filename.buf);\n \t\tdelete_tempfile(&tempfile);\ndiff --git a/tmp-objdir.c b/tmp-objdir.c\nindex 0ea078a5c5..31d16a4c2c 100644\n--- a/tmp-objdir.c\n+++ b/tmp-objdir.c\n@@ -207,10 +207,12 @@ static int read_dir_paths(struct string_list *out, const char *path)\n \treturn 0;\n }\n \n-static int migrate_paths(struct strbuf *src, struct strbuf *dst,\n+static int migrate_paths(struct tmp_objdir *t,\n+\t\t\t struct strbuf *src, struct strbuf *dst,\n \t\t\t enum finalize_object_file_flags flags);\n \n-static int migrate_one(struct strbuf *src, struct strbuf *dst,\n+static int migrate_one(struct tmp_objdir *t,\n+\t\t struct strbuf *src, struct strbuf *dst,\n \t\t enum finalize_object_file_flags flags)\n {\n \tstruct stat st;\n@@ -219,11 +221,11 @@ static int migrate_one(struct strbuf *src, struct strbuf *dst,\n \t\treturn -1;\n \tif (S_ISDIR(st.st_mode)) {\n \t\tif (!mkdir(dst->buf, 0777)) {\n-\t\t\tif (adjust_shared_perm(dst->buf))\n+\t\t\tif (adjust_shared_perm(t->repo, dst->buf))\n \t\t\t\treturn -1;\n \t\t} else if (errno != EEXIST)\n \t\t\treturn -1;\n-\t\treturn migrate_paths(src, dst, flags);\n+\t\treturn migrate_paths(t, src, dst, flags);\n \t}\n \treturn finalize_object_file_flags(src->buf, dst->buf, flags);\n }\n@@ -233,7 +235,8 @@ static int is_loose_object_shard(const char *name)\n \treturn strlen(name) == 2 && isxdigit(name[0]) && isxdigit(name[1]);\n }\n \n-static int migrate_paths(struct strbuf *src, struct strbuf *dst,\n+static int migrate_paths(struct tmp_objdir *t,\n+\t\t\t struct strbuf *src, struct strbuf *dst,\n \t\t\t enum finalize_object_file_flags flags)\n {\n \tsize_t src_len = src->len, dst_len = dst->len;\n@@ -255,7 +258,7 @@ static int migrate_paths(struct strbuf *src, struct strbuf *dst,\n \t\tif (is_loose_object_shard(name))\n \t\t\tflags_copy |= FOF_SKIP_COLLISION_CHECK;\n \n-\t\tret |= migrate_one(src, dst, flags_copy);\n+\t\tret |= migrate_one(t, src, dst, flags_copy);\n \n \t\tstrbuf_setlen(src, src_len);\n \t\tstrbuf_setlen(dst, dst_len);\n@@ -283,7 +286,7 @@ int tmp_objdir_migrate(struct tmp_objdir *t)\n \tstrbuf_addbuf(&src, &t->path);\n \tstrbuf_addstr(&dst, repo_get_object_directory(t->repo));\n \n-\tret = migrate_paths(&src, &dst, 0);\n+\tret = migrate_paths(t, &src, &dst, 0);\n \n \tstrbuf_release(&src);\n \tstrbuf_release(&dst);\n\n-- \n2.48.1.538.gc4cfc42d60.dirty\n\n"},{"id":"511993","messageId":"CAOLa=ZRSSnqRO_=N-sVrYSRpCZkyW0KFh5=rwX0NY_VqyzKz3A@mail.gmail.com","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-1-4e77f0313206@pks.im","subject":"Re: [PATCH 01/16] path: refactor `repo_common_path()` family of functions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-02-06T11:17:12Z","receivedAt":"2025-02-06T11:17:14Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n[snip]\n\n> diff --git a/path.c b/path.c\n> index 07964f5d32..273b649e00 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -414,7 +414,7 @@ static void strbuf_worktree_gitdir(struct strbuf *buf,\n> \telse if (!wt->id)\n> \t\tstrbuf_addstr(buf, repo->commondir);\n> \telse\n> -\t\tstrbuf_git_common_path(buf, repo, \"worktrees/%s\", wt->id);\n> +\t\trepo_common_path_append(repo, buf, \"worktrees/%s\", wt->id);\n\nThis is the only place we replace `strbuf_git_common_path` with\n`repo_common_path_append`, whereas other places it is\n`repo_common_path_replace`. I assume that is because in those places the\nbuf is newly initialized.\n\n[snip]\n\nRest of the patch looks good!\n"},{"id":"511994","messageId":"CAOLa=ZT8CrZA7Se98XBwS1CZjgvFFtKj5SBQfJNkG48QCvB=JQ@mail.gmail.com","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-2-4e77f0313206@pks.im","subject":"Re: [PATCH 02/16] path: refactor `repo_git_path()` family of functions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-02-06T11:53:59Z","receivedAt":"2025-02-06T11:54:01Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n[snip]\n\n> @@ -241,11 +228,14 @@ struct strbuf *get_pathname(void);\n> # include \"strbuf.h\"\n> # include \"repository.h\"\n>\n> -/* Internal implementation detail that should not be used. */\n> +/* Internal implementation details that should not be used. */\n> void repo_common_pathv(const struct repository *repo,\n> \t\t struct strbuf *buf,\n> \t\t const char *fmt,\n> \t\t va_list args);\n> +void repo_git_pathv(struct repository *repo,\n> +\t\t const struct worktree *wt, struct strbuf *buf,\n> +\t\t const char *fmt, va_list args);\n>\n\nThe only thing that stood out to me was that we loose the `const` here\nfor `repository`, I couldn't really find a reason why we do that,\nespecially since `repo_common_pathv()` right above still has it.\nEverything else looks good in this patch.\n\n[snip]\n"},{"id":"511995","messageId":"CAOLa=ZTJ7ef9rP3fQfNwSCD54zeVPL1Rd_hQfzDpEpFiStx_PQ@mail.gmail.com","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-5-4e77f0313206@pks.im","subject":"Re: [PATCH 05/16] path: refactor `repo_submodule_path()` family of functions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-02-06T12:05:13Z","receivedAt":"2025-02-06T12:05:15Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n[snip]\n\n> diff --git a/path.c b/path.c\n> index d918d0409e..d721507be8 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -560,14 +560,15 @@ const char *repo_worktree_path_replace(const struct repository *repo,\n> }\n>\n> /* Returns 0 on success, negative on failure. */\n> -static int do_submodule_path(struct strbuf *buf, const char *path,\n> +static int do_submodule_path(struct repository *repo,\n> +\t\t\t struct strbuf *buf, const char *path,\n> \t\t\t const char *fmt, va_list args)\n> {\n> \tstruct strbuf git_submodule_common_dir = STRBUF_INIT;\n> \tstruct strbuf git_submodule_dir = STRBUF_INIT;\n> \tint ret;\n>\n> -\tret = submodule_to_gitdir(the_repository, &git_submodule_dir, path);\n> +\tret = submodule_to_gitdir(repo, &git_submodule_dir, path);\n> \tif (ret)\n> \t\tgoto cleanup;\n>\n> @@ -586,13 +587,14 @@ static int do_submodule_path(struct strbuf *buf, const char *path,\n> \treturn ret;\n> }\n>\n> -char *git_pathdup_submodule(const char *path, const char *fmt, ...)\n> +char *repo_submodule_path(struct repository *repo,\n\nTo stay consistent with the other repo_* functions, should we change\n`struct repository *repo` to `const struct repository *repo`?\n\n> +\t\t\t const char *path, const char *fmt, ...)\n> {\n> \tint err;\n> \tva_list args;\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tva_start(args, fmt);\n> -\terr = do_submodule_path(&buf, path, fmt, args);\n> +\terr = do_submodule_path(repo, &buf, path, fmt, args);\n> \tva_end(args);\n> \tif (err) {\n> \t\tstrbuf_release(&buf);\n> @@ -601,16 +603,35 @@ char *git_pathdup_submodule(const char *path, const char *fmt, ...)\n> \treturn strbuf_detach(&buf, NULL);\n> }\n>\n> -int strbuf_git_path_submodule(struct strbuf *buf, const char *path,\n> -\t\t\t const char *fmt, ...)\n> +const char *repo_submodule_path_append(struct repository *repo,\n> +\t\t\t\t struct strbuf *buf,\n> +\t\t\t\t const char *path,\n> +\t\t\t\t const char *fmt, ...)\n> {\n> \tint err;\n> \tva_list args;\n> \tva_start(args, fmt);\n> -\terr = do_submodule_path(buf, path, fmt, args);\n> +\terr = do_submodule_path(repo, buf, path, fmt, args);\n> \tva_end(args);\n> +\tif (err)\n> +\t\treturn NULL;\n> +\treturn buf->buf;\n> +}\n>\n> -\treturn err;\n> +const char *repo_submodule_path_replace(struct repository *repo,\n> +\t\t\t\t\tstruct strbuf *buf,\n> +\t\t\t\t\tconst char *path,\n> +\t\t\t\t\tconst char *fmt, ...)\n> +{\n> +\tint err;\n> +\tva_list args;\n> +\tstrbuf_reset(buf);\n> +\tva_start(args, fmt);\n> +\terr = do_submodule_path(repo, buf, path, fmt, args);\n> +\tva_end(args);\n> +\tif (err)\n> +\t\treturn NULL;\n> +\treturn buf->buf;\n> }\n>\n> void repo_common_pathv(const struct repository *repo,\n\n[snip]\n"},{"id":"512001","messageId":"Z6TFZGNjs3Xfkh9x@ArchLinux","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-1-4e77f0313206@pks.im","subject":"Re: [PATCH 01/16] path: refactor `repo_common_path()` family of functions","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-02-06T14:21:24Z","receivedAt":"2025-02-06T14:19:42Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Thu, Feb 06, 2025 at 08:57:57AM +0100, Patrick Steinhardt wrote:\n> The functions provided by the \"path\" subsystem to derive repository\n> paths for the commondir, gitdir, worktrees and submodules are quite\n> inconsistent. Some functions have a `strbuf_` prefix, others have\n> different return values, some don't provide a variant working on top of\n> `strbuf`s.\n> \n> We're thus about to refactor all of these family of functions so that\n> they follow a common pattern:\n> \n> - `repo_*_path()` returns an allocated string.\n> \n> - `repo_*_path_append()` appends the path to the caller-provided\n> buffer while returning a constant pointer to the buffer. This\n> clarifies whether the buffer is being appended to or rewritten,\n> which otherwise wasn't immediately obvious.\n> \n> - `repo_*_path_replace()` replaces contents of the buffer with the\n> computed path, again returning a pointer to the buffer contents.\n> \n\nI want to ask a design question about this. Why do we need to return the\nraw pointer to the `struct strbuf` for the last two cases? I somehow\nunderstand why you want to do this. You want to follow a common pattern\nfor those three functions. But I wonder should we let the caller to\ndecide whether they want to use the raw pointer?\n\nAnd in this patch, the return value of the last two cases has never been\nused. Until I read the next patch, I have seen the usage of the return\nvalue thus I could understand your motivation.\n\n> diff --git a/path.h b/path.h\n> index 5f6c85e5f8..3c75495e1a 100644\n> --- a/path.h\n> +++ b/path.h\n> @@ -25,22 +25,20 @@ char *mkpathdup(const char *fmt, ...)\n> \t__attribute__((format (printf, 1, 2)));\n> \n> /*\n> - * The `strbuf_git_common_path` family of functions will construct a path into a\n> + * The `repo_common_path` family of functions will construct a path into a\n> * repository's common git directory, which is shared by all worktrees.\n> */\n> -\n> -/*\n> - * Constructs a path into the common git directory of repository `repo` and\n> - * append it in the provided buffer `sb`.\n> - */\n> -void strbuf_git_common_path(struct strbuf *sb,\n> -\t\t\t const struct repository *repo,\n> -\t\t\t const char *fmt, ...)\n> +char *repo_common_path(const struct repository *repo,\n> +\t\t const char *fmt, ...)\n> +\t__attribute__((format (printf, 2, 3)));\n> +const char *repo_common_path_append(const struct repository *repo,\n> +\t\t\t\t struct strbuf *sb,\n> +\t\t\t\t const char *fmt, ...)\n> +\t__attribute__((format (printf, 3, 4)));\n> +const char *repo_common_path_replace(const struct repository *repo,\n> +\t\t\t\t struct strbuf *sb,\n> +\t\t\t\t const char *fmt, ...)\n> \t__attribute__((format (printf, 3, 4)));\n> -void repo_common_pathv(const struct repository *repo,\n> -\t\t struct strbuf *buf,\n> -\t\t const char *fmt,\n> -\t\t va_list args);\n> \n> /*\n> * The `repo_git_path` family of functions will construct a path into a repository's\n> @@ -243,6 +241,12 @@ struct strbuf *get_pathname(void);\n> # include \"strbuf.h\"\n> # include \"repository.h\"\n> \n> +/* Internal implementation detail that should not be used. */\n> +void repo_common_pathv(const struct repository *repo,\n> +\t\t struct strbuf *buf,\n> +\t\t const char *fmt,\n> +\t\t va_list args);\n> +\n\nIf we decide to make this as internal implementation, why we don't just\ndelete this declaration in the header file? Do I miss out something\nhere?\n\nThanks,\nJialuo\n"},{"id":"512002","messageId":"Z6TPJIy1yRnNI2PP@ArchLinux","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-5-4e77f0313206@pks.im","subject":"Re: [PATCH 05/16] path: refactor `repo_submodule_path()` family of functions","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-02-06T15:03:00Z","receivedAt":"2025-02-06T15:01:19Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Thu, Feb 06, 2025 at 08:58:01AM +0100, Patrick Steinhardt wrote:\n> As explained in an earlier commit, we're refactoring path-related\n> functions to provide a consistent interface for computing paths into the\n> commondir, gitdir and worktree. Refactor the \"submodule\" family of\n> functions accordingly.\n> \n> Signed-off-by: Patrick Steinhardt \n> ---\n> builtin/submodule--helper.c | 2 +-\n> path.c | 37 +++++++++++++++++++++++++++++--------\n> path.h | 30 ++++++++++++++++++------------\n> t/helper/test-ref-store.c | 7 +++----\n> worktree.c | 3 ++-\n> 5 files changed, 53 insertions(+), 26 deletions(-)\n> \n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 3a64f7e605..c1a8029714 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -1826,7 +1826,7 @@ static int clone_submodule(const struct module_clone_data *clone_data,\n> \n> \tconnect_work_tree_and_git_dir(clone_data_path, sm_gitdir, 0);\n> \n> -\tp = git_pathdup_submodule(clone_data_path, \"config\");\n> +\tp = repo_submodule_path(the_repository, clone_data_path, \"config\");\n> \tif (!p)\n> \t\tdie(_(\"could not get submodule directory for '%s'\"), clone_data_path);\n> \n> diff --git a/path.c b/path.c\n> index d918d0409e..d721507be8 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -560,14 +560,15 @@ const char *repo_worktree_path_replace(const struct repository *repo,\n> }\n> \n> /* Returns 0 on success, negative on failure. */\n> -static int do_submodule_path(struct strbuf *buf, const char *path,\n> +static int do_submodule_path(struct repository *repo,\n> +\t\t\t struct strbuf *buf, const char *path,\n> \t\t\t const char *fmt, va_list args)\n> {\n> \tstruct strbuf git_submodule_common_dir = STRBUF_INIT;\n> \tstruct strbuf git_submodule_dir = STRBUF_INIT;\n> \tint ret;\n> \n> -\tret = submodule_to_gitdir(the_repository, &git_submodule_dir, path);\n> +\tret = submodule_to_gitdir(repo, &git_submodule_dir, path);\n> \tif (ret)\n> \t\tgoto cleanup;\n> \n> @@ -586,13 +587,14 @@ static int do_submodule_path(struct strbuf *buf, const char *path,\n> \treturn ret;\n> }\n> \n> -char *git_pathdup_submodule(const char *path, const char *fmt, ...)\n> +char *repo_submodule_path(struct repository *repo,\n> +\t\t\t const char *path, const char *fmt, ...)\n> {\n> \tint err;\n> \tva_list args;\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tva_start(args, fmt);\n> -\terr = do_submodule_path(&buf, path, fmt, args);\n> +\terr = do_submodule_path(repo, &buf, path, fmt, args);\n> \tva_end(args);\n> \tif (err) {\n> \t\tstrbuf_release(&buf);\n> @@ -601,16 +603,35 @@ char *git_pathdup_submodule(const char *path, const char *fmt, ...)\n> \treturn strbuf_detach(&buf, NULL);\n> }\n> \n> -int strbuf_git_path_submodule(struct strbuf *buf, const char *path,\n> -\t\t\t const char *fmt, ...)\n> +const char *repo_submodule_path_append(struct repository *repo,\n> +\t\t\t\t struct strbuf *buf,\n> +\t\t\t\t const char *path,\n> +\t\t\t\t const char *fmt, ...)\n> {\n> \tint err;\n> \tva_list args;\n> \tva_start(args, fmt);\n> -\terr = do_submodule_path(buf, path, fmt, args);\n> +\terr = do_submodule_path(repo, buf, path, fmt, args);\n> \tva_end(args);\n> +\tif (err)\n> +\t\treturn NULL;\n> +\treturn buf->buf;\n> +}\n> \n> -\treturn err;\n> +const char *repo_submodule_path_replace(struct repository *repo,\n> +\t\t\t\t\tstruct strbuf *buf,\n> +\t\t\t\t\tconst char *path,\n> +\t\t\t\t\tconst char *fmt, ...)\n> +{\n> +\tint err;\n> +\tva_list args;\n> +\tstrbuf_reset(buf);\n> +\tva_start(args, fmt);\n> +\terr = do_submodule_path(repo, buf, path, fmt, args);\n> +\tva_end(args);\n> +\tif (err)\n> +\t\treturn NULL;\n> +\treturn buf->buf;\n> }\n\nBy reading through the patches from 1 to this. I gradually understand\nyour design now. For every refactor, we will provide three kinds of\nfunctions. All of these functions will return `const char *` and we\ncould elegantly use `NULL` to indicate the error.\n\nThanks,\nJialuo\n"},{"id":"512004","messageId":"Z6TbMGcPftyhUyC3@ArchLinux","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-10-4e77f0313206@pks.im","subject":"Re: [PATCH 10/16] path: drop `git_common_path()` in favor of `repo_common_path()`","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-02-06T15:54:24Z","receivedAt":"2025-02-06T15:52:43Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Thu, Feb 06, 2025 at 08:58:06AM +0100, Patrick Steinhardt wrote:\n\n\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index 2cea9441a6..761e302a36 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -151,7 +151,7 @@ static int delete_git_dir(const char *id)\n> \tstruct strbuf sb = STRBUF_INIT;\n> \tint ret;\n> \n> -\tstrbuf_addstr(&sb, git_common_path(\"worktrees/%s\", id));\n> +\trepo_common_path_append(the_repository, &sb, \"worktrees/%s\", id);\n> \tret = remove_dir_recursively(&sb, 0);\n> \tif (ret < 0 && errno == ENOTDIR)\n> \t\tret = unlink(sb.buf);\n> @@ -1102,6 +1102,7 @@ static int lock_worktree(int ac, const char **av, const char *prefix,\n> \t\tOPT_END()\n> \t};\n> \tstruct worktree **worktrees, *wt;\n> +\tchar *path;\n> \n> \tac = parse_options(ac, av, prefix, options, git_worktree_lock_usage, 0);\n> \tif (ac != 1)\n> @@ -1122,9 +1123,11 @@ static int lock_worktree(int ac, const char **av, const char *prefix,\n> \t\tdie(_(\"'%s' is already locked\"), av[0]);\n> \t}\n> \n> -\twrite_file(git_common_path(\"worktrees/%s/locked\", wt->id),\n> -\t\t \"%s\", reason);\n> +\tpath = repo_common_path(the_repository, \"worktrees/%s/locked\", wt->id);\n\nFrom my perspective, we may use `repo_common_path_replace` here to avoid\nusing the raw string pointer? This is because we return a changeable\npointer \"char *\". But we pass this pointer to a \"const char *\". This is\nnot critical, but we may make the semantics clearer.\n\n> +\twrite_file(path, \"%s\", reason);\n> +\n> \tfree_worktrees(worktrees);\n> +\tfree(path);\n> \treturn 0;\n> }\n> \n> @@ -1135,6 +1138,7 @@ static int unlock_worktree(int ac, const char **av, const char *prefix,\n> \t\tOPT_END()\n> \t};\n> \tstruct worktree **worktrees, *wt;\n> +\tchar *path;\n> \tint ret;\n> \n> \tac = parse_options(ac, av, prefix, options, git_worktree_unlock_usage, 0);\n> @@ -1149,8 +1153,12 @@ static int unlock_worktree(int ac, const char **av, const char *prefix,\n> \t\tdie(_(\"The main working tree cannot be locked or unlocked\"));\n> \tif (!worktree_lock_reason(wt))\n> \t\tdie(_(\"'%s' is not locked\"), av[0]);\n> -\tret = unlink_or_warn(git_common_path(\"worktrees/%s/locked\", wt->id));\n> +\n> +\tpath = repo_common_path(the_repository, \"worktrees/%s/locked\", wt->id);\n\nThis one as above.\n\n> +\tret = unlink_or_warn(path);\n> +\n> \tfree_worktrees(worktrees);\n> +\tfree(path);\n> \treturn ret;\n> }\n> \n> diff --git a/path.c b/path.c\n> index d721507be8..f6b795d75f 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -634,10 +634,10 @@ const char *repo_submodule_path_replace(struct repository *repo,\n> \treturn buf->buf;\n> }\n> \n> -void repo_common_pathv(const struct repository *repo,\n> -\t\t struct strbuf *sb,\n> -\t\t const char *fmt,\n> -\t\t va_list args)\n> +static void repo_common_pathv(const struct repository *repo,\n> +\t\t\t struct strbuf *sb,\n> +\t\t\t const char *fmt,\n> +\t\t\t va_list args)\n> {\n> \tstrbuf_addstr(sb, repo->commondir);\n> \tif (sb->len && !is_dir_sep(sb->buf[sb->len - 1]))\n> diff --git a/path.h b/path.h\n> index 904eeac068..496f27fdfd 100644\n> --- a/path.h\n> +++ b/path.h\n> @@ -233,29 +233,10 @@ struct strbuf *get_pathname(void);\n> # include \"repository.h\"\n> \n> /* Internal implementation details that should not be used. */\n> -void repo_common_pathv(const struct repository *repo,\n> -\t\t struct strbuf *buf,\n> -\t\t const char *fmt,\n> -\t\t va_list args);\n\nSo, we finally mark this function \"static\" and delete the declaration in\nthis patch. We cannot do this in the earlier patch because\n\"git_common_path\" is defined in the header file and it needs to use this\nfunction. Make sense.\n\nHowever, I somehow feel a little strange especially in [PATCH 01/16]\nthat you have added a comment:\n\n /* Internal implementation detail that should not be used. *\n\nWhen I see this comment, my first intuitive thinking is that if we\nshould not use this function, why do we need to expose this in the first\nplace?\n\nThis really introduces confusion.\n\n> @@ -343,7 +344,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,\n> \tif (!is_absolute_path(wt->path)) {\n> \t\tstrbuf_addf_gently(errmsg,\n> \t\t\t\t _(\"'%s' file does not contain absolute path to the working tree location\"),\n> -\t\t\t\t git_common_path(\"worktrees/%s/gitdir\", wt->id));\n> +\t\t\t\t repo_common_path_replace(the_repository, &buf, \"worktrees/%s/gitdir\", wt->id));\n> \t\tgoto done;\n> \t}\n> \n> @@ -365,14 +366,16 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,\n> \t\tgoto done;\n> \t}\n> \n> -\tstrbuf_realpath(&realpath, git_common_path(\"worktrees/%s\", wt->id), 1);\n> +\tstrbuf_realpath(&realpath, repo_common_path_replace(the_repository, &buf, \"worktrees/%s\", wt->id), 1);\n\nWe rely on the return value of `repo_common_path_replace` to elegantly\ndo this. Make sense.\n\n> \tret = fspathcmp(path, realpath.buf);\n> \n> \tif (ret)\n> \t\tstrbuf_addf_gently(errmsg, _(\"'%s' does not point back to '%s'\"),\n> -\t\t\t\t wt->path, git_common_path(\"worktrees/%s\", wt->id));\n> +\t\t\t\t wt->path, repo_common_path_replace(the_repository, &buf,\n> +\t\t\t\t\t\t\t\t \"worktrees/%s\", wt->id));\n> done:\n> \tfree(path);\n> +\tstrbuf_release(&buf);\n> \tstrbuf_release(&wt_path);\n> \tstrbuf_release(&realpath);\n> \treturn ret;\n> @@ -384,11 +387,13 @@ void update_worktree_location(struct worktree *wt, const char *path_,\n> \tstruct strbuf path = STRBUF_INIT;\n> \tstruct strbuf dotgit = STRBUF_INIT;\n> \tstruct strbuf gitdir = STRBUF_INIT;\n> +\tchar *wt_gitdir;\n> \n> \tif (is_main_worktree(wt))\n> \t\tBUG(\"can't relocate main worktree\");\n> \n> -\tstrbuf_realpath(&gitdir, git_common_path(\"worktrees/%s/gitdir\", wt->id), 1);\n> +\twt_gitdir = repo_common_path(the_repository, \"worktrees/%s/gitdir\", wt->id);\n> +\tstrbuf_realpath(&gitdir, wt_gitdir, 1);\n\nWhy we don't use above pattern which means the following:\n\n strbuf_realpath(&gitdir, git_common_path_replace(...), ...);\n\nI think we should be consistent. And we should not use \"char *\" type to\npass to a \"const char *\" type here although this won't be harmful to the\nprogram. However, git_common_path_replace will return a \"const char *\"\nto make sure the caller cannot change this pointer.\n\n> \tstrbuf_realpath(&path, path_, 1);\n> \tstrbuf_addf(&dotgit, \"%s/.git\", path.buf);\n> \tif (fspathcmp(wt->path, path.buf)) {\n> @@ -400,6 +405,7 @@ void update_worktree_location(struct worktree *wt, const char *path_,\n> \tstrbuf_release(&path);\n> \tstrbuf_release(&dotgit);\n> \tstrbuf_release(&gitdir);\n> +\tfree(wt_gitdir);\n> }\n> \n> int is_worktree_being_rebased(const struct worktree *wt,\n> @@ -585,6 +591,7 @@ static void repair_gitfile(struct worktree *wt,\n> \tstruct strbuf backlink = STRBUF_INIT;\n> \tchar *dotgit_contents = NULL;\n> \tconst char *repair = NULL;\n> +\tchar *path = NULL;\n> \tint err;\n> \n> \t/* missing worktree can't be repaired */\n> @@ -596,7 +603,8 @@ static void repair_gitfile(struct worktree *wt,\n> \t\tgoto done;\n> \t}\n> \n> -\tstrbuf_realpath(&repo, git_common_path(\"worktrees/%s\", wt->id), 1);\n> +\tpath = repo_common_path(the_repository, \"worktrees/%s\", wt->id);\n> +\tstrbuf_realpath(&repo, path, 1);\n\nThis one as above.\n\n> \tstrbuf_addf(&dotgit, \"%s/.git\", wt->path);\n> \tstrbuf_addf(&gitdir, \"%s/gitdir\", repo.buf);\n> \tdotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));\n> @@ -626,6 +634,7 @@ static void repair_gitfile(struct worktree *wt,\n> \n> done:\n> \tfree(dotgit_contents);\n> +\tfree(path);\n> \tstrbuf_release(&repo);\n> \tstrbuf_release(&dotgit);\n> \tstrbuf_release(&gitdir);\n> @@ -657,11 +666,13 @@ void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path\n> \tstruct strbuf gitdir = STRBUF_INIT;\n> \tstruct strbuf dotgit = STRBUF_INIT;\n> \tint is_relative_path;\n> +\tchar *path = NULL;\n> \n> \tif (is_main_worktree(wt))\n> \t\tgoto done;\n> \n> -\tstrbuf_realpath(&gitdir, git_common_path(\"worktrees/%s/gitdir\", wt->id), 1);\n> +\tpath = repo_common_path(the_repository, \"worktrees/%s/gitdir\", wt->id);\n> +\tstrbuf_realpath(&gitdir, path, 1);\n> \n\nAlso this one.\n\n> \tif (strbuf_read_file(&dotgit, gitdir.buf, 0) < 0)\n> \t\tgoto done;\n> @@ -680,6 +691,7 @@ void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path\n> done:\n> \tstrbuf_release(&gitdir);\n> \tstrbuf_release(&dotgit);\n> +\tfree(path);\n> }\n> \n> void repair_worktrees_after_gitdir_move(const char *old_path)\n> @@ -871,7 +883,11 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,\n> \tssize_t read_result;\n> \n> \t*wtpath = NULL;\n> -\tstrbuf_realpath(&repo, git_common_path(\"worktrees/%s\", id), 1);\n> +\n> +\tpath = repo_common_path(the_repository, \"worktrees/%s\", id);\n> +\tstrbuf_realpath(&repo, path, 1);\n> +\tFREE_AND_NULL(path);\n> +\n\nI somehow agree that we could use `repo_common_path` in this way where\nwe want to reuse \"path\" variable.\n\nThanks,\nJialuo\n"},{"id":"512005","messageId":"Z6Tcz79fVFGEe5Ao@ArchLinux","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-12-4e77f0313206@pks.im","subject":"Re: [PATCH 12/16] path: drop `git_path()` in favor of `repo_git_path()`","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-02-06T16:01:19Z","receivedAt":"2025-02-06T15:59:38Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Thu, Feb 06, 2025 at 08:58:08AM +0100, Patrick Steinhardt wrote:\n> Remove `git_path()` in favor of the `repo_git_path()` family of\n> functions, which makes the implicit dependency on `the_repository` go\n> away.\n> \n\nIn this commit, I have concerns that I have already commented at\n[PATCH 10/16]. I don't think we should use \"repo_git_path\" in most of\ntime, we may use \"repo_git_path_replace\" for most situation.\n\nThanks,\nJialuo\n"},{"id":"512006","messageId":"Z6Tf1uSjKpXsg9aO@ArchLinux","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-0-4e77f0313206@pks.im","subject":"Re: [PATCH 00/16] path: remove dependency on `the_repository`","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-02-06T16:14:14Z","receivedAt":"2025-02-06T16:12:32Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Thu, Feb 06, 2025 at 08:57:56AM +0100, Patrick Steinhardt wrote:\n> Hi,\n> \n> this patch series removes the dependency on `the_repository` from the\n> \"path\" subsystem. The series is structured as follows:\n> \n> - Patches 1 to 5 unifyf the interfaces that can be used to retrieve\n> repository paths (gitdir, commondir, workdir and submodule paths)\n> and adapts callers accodringly.\n> \n> - Patches 6 to 12 drop repository path functions that had an implicit\n> dependency on `the_repository`.\n> \n> - Patches 13 to 16 refactor \"path.c\"-internal code to stop depending\n> on `the_repository`.\n> \n\nThanks for working on this. However, I feel a little hard when reviewing\nthe code for patch 13 to patch 16. They are not so relevant to the\nprevious twelfth patches.\n\nOne thing I want to recommend is that we may combine the first and the\nsecond step. It is a little wired that we first refactor the code then\nwe use the new one. When reading the code, I have to step back to the\nprevious patch to understand something.\n\nThanks,\nJialuo\n"},{"id":"512026","messageId":"Z6WlCg8rP_JCKulR@pks.im","threadId":"62911","inReplyTo":"CAOLa=ZT8CrZA7Se98XBwS1CZjgvFFtKj5SBQfJNkG48QCvB=JQ@mail.gmail.com","subject":"Re: [PATCH 02/16] path: refactor `repo_git_path()` family of functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-07T06:15:38Z","receivedAt":"2025-02-07T06:16:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Feb 06, 2025 at 03:53:59AM -0800, Karthik Nayak wrote:\n> Patrick Steinhardt writes:\n> \n> [snip]\n> \n> > @@ -241,11 +228,14 @@ struct strbuf *get_pathname(void);\n> > # include \"strbuf.h\"\n> > # include \"repository.h\"\n> >\n> > -/* Internal implementation detail that should not be used. */\n> > +/* Internal implementation details that should not be used. */\n> > void repo_common_pathv(const struct repository *repo,\n> > \t\t struct strbuf *buf,\n> > \t\t const char *fmt,\n> > \t\t va_list args);\n> > +void repo_git_pathv(struct repository *repo,\n> > +\t\t const struct worktree *wt, struct strbuf *buf,\n> > +\t\t const char *fmt, va_list args);\n> >\n> \n> The only thing that stood out to me was that we loose the `const` here\n> for `repository`, I couldn't really find a reason why we do that,\n> especially since `repo_common_pathv()` right above still has it.\n> Everything else looks good in this patch.\n\nYou're right, there is none yet over here, so it's pointless to adapt\n`repo_git_pathv()`. Later on this'll change because we're converting the\n\"core.hooksPath\" variable to be stored in `struct repo_settings`, and\nthat'll require us to pass in a non-const pointer.\n\nI'll shuffle this around a bit.\n\nPatrick\n"},{"id":"512027","messageId":"Z6WlJvnHHCEW6p5P@pks.im","threadId":"62911","inReplyTo":"Z6TbMGcPftyhUyC3@ArchLinux","subject":"Re: [PATCH 10/16] path: drop `git_common_path()` in favor of `repo_common_path()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-07T06:16:06Z","receivedAt":"2025-02-07T06:16:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Feb 06, 2025 at 11:54:24PM +0800, shejialuo wrote:\n> On Thu, Feb 06, 2025 at 08:58:06AM +0100, Patrick Steinhardt wrote:\n> \n> \n> > diff --git a/builtin/worktree.c b/builtin/worktree.c\n> > index 2cea9441a6..761e302a36 100644\n> > --- a/builtin/worktree.c\n> > +++ b/builtin/worktree.c\n> > @@ -151,7 +151,7 @@ static int delete_git_dir(const char *id)\n> > \tstruct strbuf sb = STRBUF_INIT;\n> > \tint ret;\n> > \n> > -\tstrbuf_addstr(&sb, git_common_path(\"worktrees/%s\", id));\n> > +\trepo_common_path_append(the_repository, &sb, \"worktrees/%s\", id);\n> > \tret = remove_dir_recursively(&sb, 0);\n> > \tif (ret < 0 && errno == ENOTDIR)\n> > \t\tret = unlink(sb.buf);\n> > @@ -1102,6 +1102,7 @@ static int lock_worktree(int ac, const char **av, const char *prefix,\n> > \t\tOPT_END()\n> > \t};\n> > \tstruct worktree **worktrees, *wt;\n> > +\tchar *path;\n> > \n> > \tac = parse_options(ac, av, prefix, options, git_worktree_lock_usage, 0);\n> > \tif (ac != 1)\n> > @@ -1122,9 +1123,11 @@ static int lock_worktree(int ac, const char **av, const char *prefix,\n> > \t\tdie(_(\"'%s' is already locked\"), av[0]);\n> > \t}\n> > \n> > -\twrite_file(git_common_path(\"worktrees/%s/locked\", wt->id),\n> > -\t\t \"%s\", reason);\n> > +\tpath = repo_common_path(the_repository, \"worktrees/%s/locked\", wt->id);\n> \n> From my perspective, we may use `repo_common_path_replace` here to avoid\n> using the raw string pointer? This is because we return a changeable\n> pointer \"char *\". But we pass this pointer to a \"const char *\". This is\n> not critical, but we may make the semantics clearer.\n\nI don't think there's much of a point doing so though. We only use the\nstring a single time, so using a `strbuf` doesn't buy us anything. This\nwas different if we already had a buffer available that we could reuse,\nbut we don't.\n\n> > diff --git a/path.c b/path.c\n> > index d721507be8..f6b795d75f 100644\n> > --- a/path.c\n> > +++ b/path.c\n> > @@ -634,10 +634,10 @@ const char *repo_submodule_path_replace(struct repository *repo,\n> > \treturn buf->buf;\n> > }\n> > \n> > -void repo_common_pathv(const struct repository *repo,\n> > -\t\t struct strbuf *sb,\n> > -\t\t const char *fmt,\n> > -\t\t va_list args)\n> > +static void repo_common_pathv(const struct repository *repo,\n> > +\t\t\t struct strbuf *sb,\n> > +\t\t\t const char *fmt,\n> > +\t\t\t va_list args)\n> > {\n> > \tstrbuf_addstr(sb, repo->commondir);\n> > \tif (sb->len && !is_dir_sep(sb->buf[sb->len - 1]))\n> > diff --git a/path.h b/path.h\n> > index 904eeac068..496f27fdfd 100644\n> > --- a/path.h\n> > +++ b/path.h\n> > @@ -233,29 +233,10 @@ struct strbuf *get_pathname(void);\n> > # include \"repository.h\"\n> > \n> > /* Internal implementation details that should not be used. */\n> > -void repo_common_pathv(const struct repository *repo,\n> > -\t\t struct strbuf *buf,\n> > -\t\t const char *fmt,\n> > -\t\t va_list args);\n> \n> So, we finally mark this function \"static\" and delete the declaration in\n> this patch. We cannot do this in the earlier patch because\n> \"git_common_path\" is defined in the header file and it needs to use this\n> function. Make sense.\n> \n> However, I somehow feel a little strange especially in [PATCH 01/16]\n> that you have added a comment:\n> \n> /* Internal implementation detail that should not be used. *\n> \n> When I see this comment, my first intuitive thinking is that if we\n> should not use this function, why do we need to expose this in the first\n> place?\n> \n> This really introduces confusion.\n\nI've touched up the first commit message to explain this a bit better.\n\n> > @@ -384,11 +387,13 @@ void update_worktree_location(struct worktree *wt, const char *path_,\n> > \tstruct strbuf path = STRBUF_INIT;\n> > \tstruct strbuf dotgit = STRBUF_INIT;\n> > \tstruct strbuf gitdir = STRBUF_INIT;\n> > +\tchar *wt_gitdir;\n> > \n> > \tif (is_main_worktree(wt))\n> > \t\tBUG(\"can't relocate main worktree\");\n> > \n> > -\tstrbuf_realpath(&gitdir, git_common_path(\"worktrees/%s/gitdir\", wt->id), 1);\n> > +\twt_gitdir = repo_common_path(the_repository, \"worktrees/%s/gitdir\", wt->id);\n> > +\tstrbuf_realpath(&gitdir, wt_gitdir, 1);\n> \n> Why we don't use above pattern which means the following:\n> \n> strbuf_realpath(&gitdir, git_common_path_replace(...), ...);\n> \n> I think we should be consistent.\n\nWe can't because `strbuf_realpath()` is not prepared to use the buffer\nas in-out parameter.\n\n> And we should not use \"char *\" type to pass to a \"const char *\" type\n> here although this won't be harmful to the program. However,\n> git_common_path_replace will return a \"const char *\" to make sure the\n> caller cannot change this pointer.\n\nPassing a `char *` to a `const char *` parameter is fine in general and\nexpected in places where the string is allocated. Using a `strbuf`\neverywhere wouldn't buy us much in cases where we cannot reuse it.\n\nPatrick\n"},{"id":"512028","messageId":"Z6WlKtA-zvi5HJOI@pks.im","threadId":"62911","inReplyTo":"Z6Tcz79fVFGEe5Ao@ArchLinux","subject":"Re: [PATCH 12/16] path: drop `git_path()` in favor of `repo_git_path()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-07T06:16:10Z","receivedAt":"2025-02-07T06:16:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Feb 07, 2025 at 12:01:19AM +0800, shejialuo wrote:\n> On Thu, Feb 06, 2025 at 08:58:08AM +0100, Patrick Steinhardt wrote:\n> > Remove `git_path()` in favor of the `repo_git_path()` family of\n> > functions, which makes the implicit dependency on `the_repository` go\n> > away.\n> > \n> \n> In this commit, I have concerns that I have already commented at\n> [PATCH 10/16]. I don't think we should use \"repo_git_path\" in most of\n> time, we may use \"repo_git_path_replace\" for most situation.\n\nAs mentioned in that commit, I don't quite agree with you. We should do\nso if it allows us to reuse buffers, but if it doesn't it is totally\nfine to allocate and free C strings.\n\nPatrick\n"},{"id":"512029","messageId":"Z6WlLdwvHuBZg_Is@pks.im","threadId":"62911","inReplyTo":"Z6Tf1uSjKpXsg9aO@ArchLinux","subject":"Re: [PATCH 00/16] path: remove dependency on `the_repository`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-07T06:16:13Z","receivedAt":"2025-02-07T06:16:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Feb 07, 2025 at 12:14:14AM +0800, shejialuo wrote:\n> On Thu, Feb 06, 2025 at 08:57:56AM +0100, Patrick Steinhardt wrote:\n> > Hi,\n> > \n> > this patch series removes the dependency on `the_repository` from the\n> > \"path\" subsystem. The series is structured as follows:\n> > \n> > - Patches 1 to 5 unifyf the interfaces that can be used to retrieve\n> > repository paths (gitdir, commondir, workdir and submodule paths)\n> > and adapts callers accodringly.\n> > \n> > - Patches 6 to 12 drop repository path functions that had an implicit\n> > dependency on `the_repository`.\n> > \n> > - Patches 13 to 16 refactor \"path.c\"-internal code to stop depending\n> > on `the_repository`.\n> > \n> \n> Thanks for working on this. However, I feel a little hard when reviewing\n> the code for patch 13 to patch 16. They are not so relevant to the\n> previous twelfth patches.\n\nThanks for your review!\n\n> One thing I want to recommend is that we may combine the first and the\n> second step. It is a little wired that we first refactor the code then\n> we use the new one. When reading the code, I have to step back to the\n> previous patch to understand something.\n\nBut those are about very different things. The first step fills in\nmissing interfaces and unifies the calling conventions, whereas the\nsecond step removes users of functions that don't have a dependency on\n`the_repository`. From my perspective, mixing these two steps up with\none another would only lead to confusion as we're doing too many things\nat once.\n\nPatrick\n"},{"id":"512030","messageId":"Z6WlMvasi64N3dQQ@pks.im","threadId":"62911","inReplyTo":"Z6TFZGNjs3Xfkh9x@ArchLinux","subject":"Re: [PATCH 01/16] path: refactor `repo_common_path()` family of functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-07T06:16:18Z","receivedAt":"2025-02-07T06:16:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Feb 06, 2025 at 10:21:24PM +0800, shejialuo wrote:\n> On Thu, Feb 06, 2025 at 08:57:57AM +0100, Patrick Steinhardt wrote:\n> > The functions provided by the \"path\" subsystem to derive repository\n> > paths for the commondir, gitdir, worktrees and submodules are quite\n> > inconsistent. Some functions have a `strbuf_` prefix, others have\n> > different return values, some don't provide a variant working on top of\n> > `strbuf`s.\n> > \n> > We're thus about to refactor all of these family of functions so that\n> > they follow a common pattern:\n> > \n> > - `repo_*_path()` returns an allocated string.\n> > \n> > - `repo_*_path_append()` appends the path to the caller-provided\n> > buffer while returning a constant pointer to the buffer. This\n> > clarifies whether the buffer is being appended to or rewritten,\n> > which otherwise wasn't immediately obvious.\n> > \n> > - `repo_*_path_replace()` replaces contents of the buffer with the\n> > computed path, again returning a pointer to the buffer contents.\n> > \n> \n> I want to ask a design question about this. Why do we need to return the\n> raw pointer to the `struct strbuf` for the last two cases? I somehow\n> understand why you want to do this. You want to follow a common pattern\n> for those three functions. But I wonder should we let the caller to\n> decide whether they want to use the raw pointer?\n\nIt allows patterns like this:\n\n if (stat(&st, repo_common_path_replace(...)) ||\n unlink(&st, repo_common_path_replace(...)))\n ...\n\nSo the reason is not that I want to follow a common pattern, the reason\nis that it's useful to some callers.\n\n> And in this patch, the return value of the last two cases has never been\n> used. Until I read the next patch, I have seen the usage of the return\n> value thus I could understand your motivation.\n\nYeah, that's fair. I'll adapt the commit message to explain this better.\n\n> > diff --git a/path.h b/path.h\n> > index 5f6c85e5f8..3c75495e1a 100644\n> > --- a/path.h\n> > +++ b/path.h\n> > @@ -243,6 +241,12 @@ struct strbuf *get_pathname(void);\n> > # include \"strbuf.h\"\n> > # include \"repository.h\"\n> > \n> > +/* Internal implementation detail that should not be used. */\n> > +void repo_common_pathv(const struct repository *repo,\n> > +\t\t struct strbuf *buf,\n> > +\t\t const char *fmt,\n> > +\t\t va_list args);\n> > +\n> \n> If we decide to make this as internal implementation, why we don't just\n> delete this declaration in the header file? Do I miss out something\n> here?\n\nWe can't, it's still used to implement `git_common_path()` in the\nheader. We'll remove it in a subsequent commit.\n\nPatrick\n"},{"id":"512031","messageId":"Z6WlNXbw6s4ojsiP@pks.im","threadId":"62911","inReplyTo":"CAOLa=ZRSSnqRO_=N-sVrYSRpCZkyW0KFh5=rwX0NY_VqyzKz3A@mail.gmail.com","subject":"Re: [PATCH 01/16] path: refactor `repo_common_path()` family of functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-07T06:16:21Z","receivedAt":"2025-02-07T06:16:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Feb 06, 2025 at 03:17:12AM -0800, Karthik Nayak wrote:\n> Patrick Steinhardt writes:\n> \n> [snip]\n> \n> > diff --git a/path.c b/path.c\n> > index 07964f5d32..273b649e00 100644\n> > --- a/path.c\n> > +++ b/path.c\n> > @@ -414,7 +414,7 @@ static void strbuf_worktree_gitdir(struct strbuf *buf,\n> > \telse if (!wt->id)\n> > \t\tstrbuf_addstr(buf, repo->commondir);\n> > \telse\n> > -\t\tstrbuf_git_common_path(buf, repo, \"worktrees/%s\", wt->id);\n> > +\t\trepo_common_path_append(repo, buf, \"worktrees/%s\", wt->id);\n> \n> This is the only place we replace `strbuf_git_common_path` with\n> `repo_common_path_append`, whereas other places it is\n> `repo_common_path_replace`. I assume that is because in those places the\n> buf is newly initialized.\n\nYes, exactly. I think that it's easier to reason about to replace\ncallsites with `_replace()` where we can. But I'm happy to adjust the\napproach when it's putting too much of a burden on the reviewer.\n\nPatrick\n"},{"id":"512032","messageId":"Z6WlONTM0lNaXTH5@pks.im","threadId":"62911","inReplyTo":"CAOLa=ZTJ7ef9rP3fQfNwSCD54zeVPL1Rd_hQfzDpEpFiStx_PQ@mail.gmail.com","subject":"Re: [PATCH 05/16] path: refactor `repo_submodule_path()` family of functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-07T06:16:24Z","receivedAt":"2025-02-07T06:16:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Feb 06, 2025 at 04:05:13AM -0800, Karthik Nayak wrote:\n> Patrick Steinhardt writes:\n> \n> [snip]\n> \n> > diff --git a/path.c b/path.c\n> > index d918d0409e..d721507be8 100644\n> > --- a/path.c\n> > +++ b/path.c\n> > @@ -560,14 +560,15 @@ const char *repo_worktree_path_replace(const struct repository *repo,\n> > }\n> >\n> > /* Returns 0 on success, negative on failure. */\n> > -static int do_submodule_path(struct strbuf *buf, const char *path,\n> > +static int do_submodule_path(struct repository *repo,\n> > +\t\t\t struct strbuf *buf, const char *path,\n> > \t\t\t const char *fmt, va_list args)\n> > {\n> > \tstruct strbuf git_submodule_common_dir = STRBUF_INIT;\n> > \tstruct strbuf git_submodule_dir = STRBUF_INIT;\n> > \tint ret;\n> >\n> > -\tret = submodule_to_gitdir(the_repository, &git_submodule_dir, path);\n> > +\tret = submodule_to_gitdir(repo, &git_submodule_dir, path);\n> > \tif (ret)\n> > \t\tgoto cleanup;\n> >\n> > @@ -586,13 +587,14 @@ static int do_submodule_path(struct strbuf *buf, const char *path,\n> > \treturn ret;\n> > }\n> >\n> > -char *git_pathdup_submodule(const char *path, const char *fmt, ...)\n> > +char *repo_submodule_path(struct repository *repo,\n> \n> To stay consistent with the other repo_* functions, should we change\n> `struct repository *repo` to `const struct repository *repo`?\n\nSomebody noticed :) But no, we cannot, we need to internally pass the\nrepo to functions that expect a non-const pointer. This is because deep\ndown in the callstack we end up calling `repo_read_gitmodules()`, which\nmodifies the repository.\n\nI'll add a comment to the commit message.\n\nPatrick\n"},{"id":"512035","messageId":"CAOLa=ZTJDG+YgrP42b5x0NXxQn4kS2adKu2KncUd2Q2NTMGsaw@mail.gmail.com","threadId":"62911","inReplyTo":"Z6WlONTM0lNaXTH5@pks.im","subject":"Re: [PATCH 05/16] path: refactor `repo_submodule_path()` family of functions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-02-07T07:04:47Z","receivedAt":"2025-02-07T07:04:49Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Thu, Feb 06, 2025 at 04:05:13AM -0800, Karthik Nayak wrote:\n>> Patrick Steinhardt writes:\n>>\n>> [snip]\n>>\n>> > diff --git a/path.c b/path.c\n>> > index d918d0409e..d721507be8 100644\n>> > --- a/path.c\n>> > +++ b/path.c\n>> > @@ -560,14 +560,15 @@ const char *repo_worktree_path_replace(const struct repository *repo,\n>> > }\n>> >\n>> > /* Returns 0 on success, negative on failure. */\n>> > -static int do_submodule_path(struct strbuf *buf, const char *path,\n>> > +static int do_submodule_path(struct repository *repo,\n>> > +\t\t\t struct strbuf *buf, const char *path,\n>> > \t\t\t const char *fmt, va_list args)\n>> > {\n>> > \tstruct strbuf git_submodule_common_dir = STRBUF_INIT;\n>> > \tstruct strbuf git_submodule_dir = STRBUF_INIT;\n>> > \tint ret;\n>> >\n>> > -\tret = submodule_to_gitdir(the_repository, &git_submodule_dir, path);\n>> > +\tret = submodule_to_gitdir(repo, &git_submodule_dir, path);\n>> > \tif (ret)\n>> > \t\tgoto cleanup;\n>> >\n>> > @@ -586,13 +587,14 @@ static int do_submodule_path(struct strbuf *buf, const char *path,\n>> > \treturn ret;\n>> > }\n>> >\n>> > -char *git_pathdup_submodule(const char *path, const char *fmt, ...)\n>> > +char *repo_submodule_path(struct repository *repo,\n>>\n>> To stay consistent with the other repo_* functions, should we change\n>> `struct repository *repo` to `const struct repository *repo`?\n>\n> Somebody noticed :) But no, we cannot, we need to internally pass the\n> repo to functions that expect a non-const pointer. This is because deep\n> down in the callstack we end up calling `repo_read_gitmodules()`, which\n> modifies the repository.\n>\n\nOkay that makes sense.\n\n> I'll add a comment to the commit message.\n>\n> Patrick\n>\n\nOkay perfect, thanks!\n"},{"id":"512036","messageId":"CAOLa=ZSXFqvwFEfJ+cktqzSUf0XMoFC8Z0S0DJHcg4jYN4+DcA@mail.gmail.com","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-9-4e77f0313206@pks.im","subject":"Re: [PATCH 09/16] worktree: return allocated string from `get_worktree_git_dir()`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-02-07T07:15:58Z","receivedAt":"2025-02-07T07:16:00Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> The `get_worktree_git_dir()` function returns a string constant that\n> does not need to be free'd by the caller. For `repo_get_git_dir()` and\n> `repo_get_common_dir()` this is because we return strings owned by\n> `the_repository`.\n>\n\nNot sure what the second sentence is signifying here. What relation does\n`get_worktree_git_dir()` have with `repo_get_git_dir()` and\n`repo_get_common_dir()`?\n\n> But for `git_common_path()` it's a bit less obvious though, because that\n> function does end up allocating memory. This doesn't result in a memory\n> leak either because we write into a buffer returned by `get_pathname()`,\n> which returns one out of four static buffers.\n>\n\nNow `git_common_path()`, what is binding all these functions together?\nSneaking down to the code, it looks like `get_worktree_git_dir()` calls\none of the other three functions. Maybe we should start with that?\n\n>\n> We're about to drop `git_common_path()` in favor of `repo_common_path()`,\n> which doesn't use the same mechanism but instead returns an allocated\n> string owned by the caller. While we could adapt `get_worktree_git_dir()`\n> to also use `get_pathname()` and print the derived common path into that\n> buffer, the whole schema feels a lot like premature optimization in this\n> context. There are some callsites where we call `get_worktree_git_dir()`\n> in a loop that iterates through all worktrees. But none of these loops\n> seem to be even remotely in the hot path, so saving a single allocation\n> there does not feel worth it.\n>\n> Refactor the function to instead consistently return an allocated path\n> so that we can start using `repo_common_path()` in a subsequent commit.\n>\n\nThis makes sense.\n\n[snip]\n\n\n> diff --git a/revision.c b/revision.c\n> index 474fa1e767..be72f226f3 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1874,15 +1874,20 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)\n> \tfor (p = worktrees; *p; p++) {\n> \t\tstruct worktree *wt = *p;\n> \t\tstruct index_state istate = INDEX_STATE_INIT(revs->repo);\n> +\t\tchar *gitdir;\n>\n\nNit: should this be named `wt_gitdir` to stay consistent?\n\n> \t\tif (wt->is_current)\n> \t\t\tcontinue; /* current index already taken care of */\n>\n> +\t\tgitdir = get_worktree_git_dir(wt);\n> +\n> \t\tif (read_index_from(&istate,\n> \t\t\t\t worktree_git_path(the_repository, wt, \"index\"),\n> -\t\t\t\t get_worktree_git_dir(wt)) > 0)\n> +\t\t\t\t gitdir) > 0)\n> \t\t\tdo_add_index_objects_to_pending(revs, &istate, flags);\n> +\n> \t\tdiscard_index(&istate);\n> +\t\tfree(gitdir);\n> \t}\n> \tfree_worktrees(worktrees);\n> }\n\n[snip]\n\n> diff --git a/worktree.c b/worktree.c\n> index 8f4fc10c44..3b94535963 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -59,8 +59,9 @@ static void add_head_info(struct worktree *wt)\n> static int is_current_worktree(struct worktree *wt)\n> {\n> \tchar *git_dir = absolute_pathdup(repo_get_git_dir(the_repository));\n> -\tconst char *wt_git_dir = get_worktree_git_dir(wt);\n> +\tchar *wt_git_dir = get_worktree_git_dir(wt);\n\nNit: here too, even though it is a pre-existing name. Perhaps it is just\nme. So feel free to ignore :)\n\n> \tint is_current = !fspathcmp(git_dir, absolute_path(wt_git_dir));\n> +\tfree(wt_git_dir);\n> \tfree(git_dir);\n> \treturn is_current;\n> }\n> @@ -175,14 +176,14 @@ struct worktree **get_worktrees(void)\n> \treturn get_worktrees_internal(0);\n> }\n>\n> -const char *get_worktree_git_dir(const struct worktree *wt)\n> +char *get_worktree_git_dir(const struct worktree *wt)\n> {\n> \tif (!wt)\n> -\t\treturn repo_get_git_dir(the_repository);\n> +\t\treturn xstrdup(repo_get_git_dir(the_repository));\n> \telse if (!wt->id)\n> -\t\treturn repo_get_common_dir(the_repository);\n> +\t\treturn xstrdup(repo_get_common_dir(the_repository));\n> \telse\n> -\t\treturn git_common_path(\"worktrees/%s\", wt->id);\n> +\t\treturn xstrdup(git_common_path(\"worktrees/%s\", wt->id));\n> }\n>\n\nSo this is the crux of the patch, we allocate the string for the first\ntwo function's return value to ensure that `get_worktree_git_dir()`\nreturns a string which needs to be free'd. Alright!\n"},{"id":"512046","messageId":"CAOLa=ZSScTrK1RH6+VgHv+P-eLsQ2uXwP_ZKN-rwObca1aTa=A@mail.gmail.com","threadId":"62911","inReplyTo":"20250206-b4-pks-path-drop-the-repository-v1-0-4e77f0313206@pks.im","subject":"Re: [PATCH 00/16] path: remove dependency on `the_repository`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-02-07T08:17:13Z","receivedAt":"2025-02-07T08:17:15Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> Hi,\n>\n> this patch series removes the dependency on `the_repository` from the\n> \"path\" subsystem. The series is structured as follows:\n>\n> - Patches 1 to 5 unifyf the interfaces that can be used to retrieve\n> repository paths (gitdir, commondir, workdir and submodule paths)\n> and adapts callers accodringly.\n>\n> - Patches 6 to 12 drop repository path functions that had an implicit\n> dependency on `the_repository`.\n>\n> - Patches 13 to 16 refactor \"path.c\"-internal code to stop depending\n> on `the_repository`.\n>\n> Thanks!\n>\n> Patrick\n>\n\nI've reviewed the series, apart from some small nits, the series looks\ngreat. Thanks for working on this.\n\nKarthik\n\n[snip]\n"},{"id":"512051","messageId":"Z6XlKVwsYNskS_nR@pks.im","threadId":"62911","inReplyTo":"CAOLa=ZSXFqvwFEfJ+cktqzSUf0XMoFC8Z0S0DJHcg4jYN4+DcA@mail.gmail.com","subject":"Re: [PATCH 09/16] worktree: return allocated string from `get_worktree_git_dir()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-07T10:49:13Z","receivedAt":"2025-02-07T10:49:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Feb 06, 2025 at 11:15:58PM -0800, Karthik Nayak wrote:\n> Patrick Steinhardt writes:\n> \n> > The `get_worktree_git_dir()` function returns a string constant that\n> > does not need to be free'd by the caller. For `repo_get_git_dir()` and\n> > `repo_get_common_dir()` this is because we return strings owned by\n> > `the_repository`.\n> >\n> \n> Not sure what the second sentence is signifying here. What relation does\n> `get_worktree_git_dir()` have with `repo_get_git_dir()` and\n> `repo_get_common_dir()`?\n\nThese are getting called in `get_worktree_git_dir()`. I'll rephrase the\nmessage to make this clearer.\n\n> > But for `git_common_path()` it's a bit less obvious though, because that\n> > function does end up allocating memory. This doesn't result in a memory\n> > leak either because we write into a buffer returned by `get_pathname()`,\n> > which returns one out of four static buffers.\n> >\n> \n> Now `git_common_path()`, what is binding all these functions together?\n> Sneaking down to the code, it looks like `get_worktree_git_dir()` calls\n> one of the other three functions. Maybe we should start with that?\n\nYeah, I've reworded the commit message accordingly.\n\n> > diff --git a/revision.c b/revision.c\n> > index 474fa1e767..be72f226f3 100644\n> > --- a/revision.c\n> > +++ b/revision.c\n> > @@ -1874,15 +1874,20 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)\n> > \tfor (p = worktrees; *p; p++) {\n> > \t\tstruct worktree *wt = *p;\n> > \t\tstruct index_state istate = INDEX_STATE_INIT(revs->repo);\n> > +\t\tchar *gitdir;\n> >\n> \n> Nit: should this be named `wt_gitdir` to stay consistent?\n\nMakes sense.\n\n> > diff --git a/worktree.c b/worktree.c\n> > index 8f4fc10c44..3b94535963 100644\n> > --- a/worktree.c\n> > +++ b/worktree.c\n> > @@ -59,8 +59,9 @@ static void add_head_info(struct worktree *wt)\n> > static int is_current_worktree(struct worktree *wt)\n> > {\n> > \tchar *git_dir = absolute_pathdup(repo_get_git_dir(the_repository));\n> > -\tconst char *wt_git_dir = get_worktree_git_dir(wt);\n> > +\tchar *wt_git_dir = get_worktree_git_dir(wt);\n> \n> Nit: here too, even though it is a pre-existing name. Perhaps it is just\n> me. So feel free to ignore :)\n\nI'll leave that one as-is, even though I share your sentiment.\n\nPatrick\n"}]}