{"thread":{"id":"61050","subject":"[RFC PATCH 0/3] Parallel submodule status","startedAt":"2024-03-05T01:22:25Z","lastAt":"2024-03-13T17:52:04Z","messageCount":17,"participants":["Atneya Nair","Junio C Hamano","Eric Sunshine","Jeff King","Jean-Noël Avila"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"489933","messageId":"20240305012112.1598053-2-atneya@google.com","threadId":"61050","inReplyTo":null,"subject":"[RFC PATCH 0/3] Parallel submodule status","fromName":"Atneya Nair","fromEmail":"atneya@google.com","sentAt":"2024-03-05T01:21:10Z","receivedAt":"2024-03-05T01:22:25Z","isPatch":true,"sender":{"key":"atneya@google.com","avatar":null},"body":"This set tries to speed up git status (and other commands which examine\nthe tree state) for repositories with many submodules.\n\nCurrently, submodules, unlike regular files, are examined serially\ndespite the expense of this operation. Fsmonitor also doesn't support\nsubmodules, so there isn't a great alternative here.\n\nAtneya Nair (3):\n Make read_gitfile and resolve_gitfile thread safe\n Make ce_compare_gitlink thread-safe\n Preload submodule state in refresh_index\n\n builtin/init-db.c | 7 ++++---\n builtin/rev-parse.c | 4 +++-\n config.c | 3 ++-\n config.h | 2 +-\n preload-index.c | 25 ++++++++++++++++++++++---\n read-cache-ll.h | 1 +\n read-cache.c | 3 +++\n refs.c | 9 +++++++++\n repository.c | 9 +++++----\n setup.c | 36 +++++++++++++++++++++++++-----------\n setup.h | 7 +++----\n submodule.c | 32 +++++++++++++++++++++++---------\n worktree.c | 27 +++++++++++++--------------\n 13 files changed, 114 insertions(+), 51 deletions(-)\n\n\nbase-commit: f41f85c9ec8d4d46de0fd5fded88db94d3ec8c11\n-- \n2.44.0.rc1.240.g4c46232300-goog\n\n"},{"id":"489934","messageId":"20240305012112.1598053-3-atneya@google.com","threadId":"61050","inReplyTo":"20240305012112.1598053-2-atneya@google.com","subject":"[RFC PATCH 1/3] Make read_gitfile and resolve_gitfile thread safe","fromName":"Atneya Nair","fromEmail":"atneya@google.com","sentAt":"2024-03-05T01:21:11Z","receivedAt":"2024-03-05T01:22:27Z","isPatch":true,"sender":{"key":"atneya@google.com","avatar":null},"body":"Continue the work in commit: 3d7747e318532a36a263c61cdf92f2decb6424ff\nto remove unsafe shared buffer usage in read_gitfile_gently.\n\nMigrate read_gitfile_gently and resolve_gitfile_gently to take strbuf\nout params to allocate their return values into, rather than returning\na view into a shared buffer.\n\nLeave the shared buffer in case a caller passes null for this param (for\ncases we haven't cleaned up yet).\n\nMigrate callers of resolve_gitfile to resolve_gitfile_gently.\n\nSigned-off-by: Atneya Nair \n---\n\nNotes:\n Open questions:\n - Is checking the return value of read_gitfile necessary if on error,\n we are supposed to die, or set the error field to a non-zero value?\n - Should we clean up the other call-sites of read_gitfile?\n\n builtin/init-db.c | 7 ++++---\n builtin/rev-parse.c | 4 +++-\n repository.c | 9 +++++----\n setup.c | 36 +++++++++++++++++++++++++-----------\n setup.h | 7 +++----\n submodule.c | 32 +++++++++++++++++++++++---------\n worktree.c | 27 +++++++++++++--------------\n 7 files changed, 76 insertions(+), 46 deletions(-)\n\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 0170469b84..9135d07a0d 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -198,11 +198,11 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \t */\n \tif (real_git_dir) {\n \t\tint err;\n-\t\tconst char *p;\n \t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tstruct strbuf gitfile = STRBUF_INIT;\n \n-\t\tp = read_gitfile_gently(git_dir, &err);\n-\t\tif (p && get_common_dir(&sb, p)) {\n+\t\tread_gitfile_gently(git_dir, &err, &gitfile);\n+\t\tif (!err && get_common_dir(&sb, gitfile.buf)) {\n \t\t\tstruct strbuf mainwt = STRBUF_INIT;\n \n \t\t\tstrbuf_addbuf(&mainwt, &sb);\n@@ -213,6 +213,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \t\t\tgit_dir = strbuf_detach(&sb, NULL);\n \t\t}\n \t\tstrbuf_release(&sb);\n+\t\tstrbuf_release(&gitfile);\n \t}\n \n \tif (is_bare_repository_cfg < 0)\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex d08987646a..e1db6b3231 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -728,12 +728,14 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--resolve-git-dir\")) {\n \t\t\t\tconst char *gitdir = argv[++i];\n+\t\t\t\tstruct strbuf resolved_gitdir = STRBUF_INIT;\n \t\t\t\tif (!gitdir)\n \t\t\t\t\tdie(_(\"--resolve-git-dir requires an argument\"));\n-\t\t\t\tgitdir = resolve_gitdir(gitdir);\n+\t\t\t\tgitdir = resolve_gitdir_gently(gitdir, NULL, &resolved_gitdir);\n \t\t\t\tif (!gitdir)\n \t\t\t\t\tdie(_(\"not a gitdir '%s'\"), argv[i]);\n \t\t\t\tputs(gitdir);\n+\t\t\t\tstrbuf_release(&resolved_gitdir);\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t}\ndiff --git a/repository.c b/repository.c\nindex 7aacb51b65..3ca6dbcf16 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -118,7 +118,7 @@ static int repo_init_gitdir(struct repository *repo, const char *gitdir)\n \tint ret = 0;\n \tint error = 0;\n \tchar *abspath = NULL;\n-\tconst char *resolved_gitdir;\n+\tstruct strbuf resolved_gitdir = STRBUF_INIT;\n \tstruct set_gitdir_args args = { NULL };\n \n \tabspath = real_pathdup(gitdir, 0);\n@@ -128,15 +128,16 @@ static int repo_init_gitdir(struct repository *repo, const char *gitdir)\n \t}\n \n \t/* 'gitdir' must reference the gitdir directly */\n-\tresolved_gitdir = resolve_gitdir_gently(abspath, &error);\n-\tif (!resolved_gitdir) {\n+\tresolve_gitdir_gently(abspath, &error, &resolved_gitdir);\n+\tif (error) {\n \t\tret = -1;\n \t\tgoto out;\n \t}\n \n-\trepo_set_gitdir(repo, resolved_gitdir, &args);\n+\trepo_set_gitdir(repo, resolved_gitdir.buf, &args);\n \n out:\n+\tstrbuf_release(&resolved_gitdir);\n \tfree(abspath);\n \treturn ret;\n }\ndiff --git a/setup.c b/setup.c\nindex b69b1cbc2a..2e118cf216 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -397,14 +397,17 @@ int is_nonbare_repository_dir(struct strbuf *path)\n \tint ret = 0;\n \tint gitfile_error;\n \tsize_t orig_path_len = path->len;\n+\tstruct strbuf gitfile = STRBUF_INIT;\n+\n \tassert(orig_path_len != 0);\n \tstrbuf_complete(path, '/');\n \tstrbuf_addstr(path, \".git\");\n-\tif (read_gitfile_gently(path->buf, &gitfile_error) || is_git_directory(path->buf))\n+\tif (read_gitfile_gently(path->buf, &gitfile_error, &gitfile) || is_git_directory(path->buf))\n \t\tret = 1;\n \tif (gitfile_error == READ_GITFILE_ERR_OPEN_FAILED ||\n \t gitfile_error == READ_GITFILE_ERR_READ_FAILED)\n \t\tret = 1;\n+\tstrbuf_release(&gitfile);\n \tstrbuf_setlen(path, orig_path_len);\n \treturn ret;\n }\n@@ -830,7 +833,8 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)\n \n /*\n * Try to read the location of the git directory from the .git file,\n- * return path to git directory if found. The return value comes from\n+ * return path to git directory if found. If passed a valid strbuf, the return\n+ * value is is a ptr to within the buffer. If strbuf is null, the return value comes from\n * a shared buffer.\n *\n * On failure, if return_error_code is not NULL, return_error_code\n@@ -838,7 +842,7 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)\n * return_error_code is NULL the function will die instead (for most\n * cases).\n */\n-const char *read_gitfile_gently(const char *path, int *return_error_code)\n+const char *read_gitfile_gently(const char *path, int *return_error_code, struct strbuf* result_buf)\n {\n \tconst int max_file_size = 1 << 20; /* 1MB */\n \tint error_code = 0;\n@@ -848,7 +852,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n \tstruct stat st;\n \tint fd;\n \tssize_t len;\n-\tstatic struct strbuf realpath = STRBUF_INIT;\n+\tstatic struct strbuf shared = STRBUF_INIT;\n+\tif (!result_buf) {\n+\t\tresult_buf = &shared;\n+\t}\n \n \tif (stat(path, &st)) {\n \t\t/* NEEDSWORK: discern between ENOENT vs other errors */\n@@ -900,8 +907,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n \t\tgoto cleanup_return;\n \t}\n \n-\tstrbuf_realpath(&realpath, dir, 1);\n-\tpath = realpath.buf;\n+\tstrbuf_realpath(result_buf, dir, 1);\n+\tpath = result_buf->buf;\n+\t// TODO is this valid?\n+\tif (!path) die(_(\"Unexpected null from realpath '%s'\"), dir);\n \n cleanup_return:\n \tif (return_error_code)\n@@ -1316,12 +1325,13 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\tint offset = dir->len, error_code = 0;\n \t\tchar *gitdir_path = NULL;\n \t\tchar *gitfile = NULL;\n+\t\tstruct strbuf gitdirenvbuf = STRBUF_INIT;\n \n \t\tif (offset > min_offset)\n \t\t\tstrbuf_addch(dir, '/');\n \t\tstrbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);\n \t\tgitdirenv = read_gitfile_gently(dir->buf, die_on_error ?\n-\t\t\t\t\t\tNULL : &error_code);\n+\t\t\t\t\t\tNULL : &error_code, &gitdirenvbuf);\n \t\tif (!gitdirenv) {\n \t\t\tif (die_on_error ||\n \t\t\t error_code == READ_GITFILE_ERR_NOT_A_FILE) {\n@@ -1330,8 +1340,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\t\t\t\tgitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;\n \t\t\t\t\tgitdir_path = xstrdup(dir->buf);\n \t\t\t\t}\n-\t\t\t} else if (error_code != READ_GITFILE_ERR_STAT_FAILED)\n+\t\t\t} else if (error_code != READ_GITFILE_ERR_STAT_FAILED) {\n+\t\t\t\tstrbuf_release(&gitdirenvbuf);\n \t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n+\t\t\t}\n \t\t} else\n \t\t\tgitfile = xstrdup(dir->buf);\n \t\t/*\n@@ -1365,9 +1377,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\t\t */\n \t\t\tfree(gitdir_path);\n \t\t\tfree(gitfile);\n-\n+\t\t\tstrbuf_release(&gitdirenvbuf);\n \t\t\treturn ret;\n \t\t}\n+\t\tstrbuf_release(&gitdirenvbuf);\n \n \t\tif (is_git_directory(dir->buf)) {\n \t\t\ttrace2_data_string(\"setup\", NULL, \"implicit-bare-repository\", dir->buf);\n@@ -1692,11 +1705,12 @@ const char *setup_git_directory(void)\n \treturn setup_git_directory_gently(NULL);\n }\n \n-const char *resolve_gitdir_gently(const char *suspect, int *return_error_code)\n+const char *resolve_gitdir_gently(const char *suspect, int *return_error_code,\n+\t\tstruct strbuf* return_buf)\n {\n \tif (is_git_directory(suspect))\n \t\treturn suspect;\n-\treturn read_gitfile_gently(suspect, return_error_code);\n+\treturn read_gitfile_gently(suspect, return_error_code, return_buf);\n }\n \n /* if any standard file descriptor is missing open it to /dev/null */\ndiff --git a/setup.h b/setup.h\nindex 3599aec93c..cf5a63762b 100644\n--- a/setup.h\n+++ b/setup.h\n@@ -36,10 +36,9 @@ int is_nonbare_repository_dir(struct strbuf *path);\n #define READ_GITFILE_ERR_NOT_A_REPO 7\n #define READ_GITFILE_ERR_TOO_LARGE 8\n void read_gitfile_error_die(int error_code, const char *path, const char *dir);\n-const char *read_gitfile_gently(const char *path, int *return_error_code);\n-#define read_gitfile(path) read_gitfile_gently((path), NULL)\n-const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);\n-#define resolve_gitdir(path) resolve_gitdir_gently((path), NULL)\n+const char *read_gitfile_gently(const char *path, int *return_error_code, struct strbuf *buf);\n+#define read_gitfile(path) read_gitfile_gently((path), NULL, NULL)\n+const char *resolve_gitdir_gently(const char *suspect, int *return_error_code, struct strbuf *buf);\n \n void setup_work_tree(void);\n \ndiff --git a/submodule.c b/submodule.c\nindex 213da79f66..455444321b 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -316,9 +316,10 @@ int is_submodule_populated_gently(const char *path, int *return_error_code)\n \tint ret = 0;\n \tchar *gitdir = xstrfmt(\"%s/.git\", path);\n \n-\tif (resolve_gitdir_gently(gitdir, return_error_code))\n+\tstruct strbuf resolved_gitdir_buf = STRBUF_INIT;\n+\tif (resolve_gitdir_gently(gitdir, return_error_code, &resolved_gitdir_buf))\n \t\tret = 1;\n-\n+\tstrbuf_release(&resolved_gitdir_buf);\n \tfree(gitdir);\n \treturn ret;\n }\n@@ -1879,22 +1880,25 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf gitdirbuf = STRBUF_INIT;\n \tFILE *fp;\n \tunsigned dirty_submodule = 0;\n \tconst char *git_dir;\n \tint ignore_cp_exit_code = 0;\n \n \tstrbuf_addf(&buf, \"%s/.git\", path);\n-\tgit_dir = read_gitfile(buf.buf);\n+\tgit_dir = read_gitfile_gently(buf.buf, NULL, &gitdirbuf);\n \tif (!git_dir)\n \t\tgit_dir = buf.buf;\n \tif (!is_git_directory(git_dir)) {\n \t\tif (is_directory(git_dir))\n \t\t\tdie(_(\"'%s' not recognized as a git repository\"), git_dir);\n \t\tstrbuf_release(&buf);\n+\t\tstrbuf_release(&gitdirbuf);\n \t\t/* The submodule is not checked out, so it is not modified */\n \t\treturn 0;\n \t}\n+\t\tstrbuf_release(&gitdirbuf);\n \tstrbuf_reset(&buf);\n \n \tstrvec_pushl(&cp.args, \"status\", \"--porcelain=2\", NULL);\n@@ -1958,15 +1962,16 @@ int submodule_uses_gitfile(const char *path)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tconst char *git_dir;\n+\tstruct strbuf gitfilebuf = STRBUF_INIT;\n \n \tstrbuf_addf(&buf, \"%s/.git\", path);\n-\tgit_dir = read_gitfile(buf.buf);\n-\tif (!git_dir) {\n+\tread_gitfile_gently(buf.buf, NULL, &gitfilebuf);\n+\tif (!gitfilebuf.buf) {\n \t\tstrbuf_release(&buf);\n \t\treturn 0;\n \t}\n \tstrbuf_release(&buf);\n+\tstrbuf_release(&gitfilebuf);\n \n \t/* Now test that all nested submodules use a gitfile too */\n \tstrvec_pushl(&cp.args,\n@@ -2276,6 +2281,7 @@ static void relocate_single_git_dir_into_superproject(const char *path,\n {\n \tchar *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;\n \tstruct strbuf new_gitdir = STRBUF_INIT;\n+\tstruct strbuf gitfilebuf = STRBUF_INIT;\n \tconst struct submodule *sub;\n \n \tif (submodule_uses_worktrees(path))\n@@ -2283,9 +2289,12 @@ static void relocate_single_git_dir_into_superproject(const char *path,\n \t\t \"more than one worktree not supported\"), path);\n \n \told_git_dir = xstrfmt(\"%s/.git\", path);\n-\tif (read_gitfile(old_git_dir))\n+\tif (read_gitfile_gently(old_git_dir, NULL, &gitfilebuf)) {\n \t\t/* If it is an actual gitfile, it doesn't need migration. */\n+\t\tstrbuf_release(&gitfilebuf);\n \t\treturn;\n+\t}\n+\tstrbuf_release(&gitfilebuf);\n \n \treal_old_git_dir = real_pathdup(old_git_dir, 1);\n \n@@ -2343,8 +2352,9 @@ void absorb_git_dir_into_superproject(const char *path,\n \tint err_code;\n \tconst char *sub_git_dir;\n \tstruct strbuf gitdir = STRBUF_INIT;\n+\tstruct strbuf resolved_gitdir_buf = STRBUF_INIT;\n \tstrbuf_addf(&gitdir, \"%s/.git\", path);\n-\tsub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code);\n+\tsub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code, &resolved_gitdir_buf);\n \n \t/* Not populated? */\n \tif (!sub_git_dir) {\n@@ -2385,6 +2395,8 @@ void absorb_git_dir_into_superproject(const char *path,\n \t\tfree(real_sub_git_dir);\n \t\tfree(real_common_git_dir);\n \t}\n+\n+\tstrbuf_release(&resolved_gitdir_buf);\n \tstrbuf_release(&gitdir);\n \n \tabsorb_git_dir_into_superproject_recurse(path, super_prefix);\n@@ -2484,17 +2496,19 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule)\n \tconst struct submodule *sub;\n \tconst char *git_dir;\n \tint ret = 0;\n+\tstruct strbuf gitfilebuf = STRBUF_INIT;\n \n \tstrbuf_reset(buf);\n \tstrbuf_addstr(buf, submodule);\n \tstrbuf_complete(buf, '/');\n \tstrbuf_addstr(buf, \".git\");\n \n-\tgit_dir = read_gitfile(buf->buf);\n+\tgit_dir = read_gitfile_gently(buf->buf, NULL, &gitfilebuf);\n \tif (git_dir) {\n \t\tstrbuf_reset(buf);\n \t\tstrbuf_addstr(buf, git_dir);\n \t}\n+\tstrbuf_release(&gitfilebuf);\n \tif (!is_git_directory(buf->buf)) {\n \t\tsub = submodule_from_path(the_repository, null_oid(),\n \t\t\t\t\t submodule);\ndiff --git a/worktree.c b/worktree.c\nindex b02a05a74a..a6f125c8da 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -309,7 +309,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-\tchar *path = NULL;\n+\tstruct strbuf gitfile = STRBUF_INIT;\n \tint err, ret = -1;\n \n \tstrbuf_addf(&wt_path, \"%s/.git\", wt->path);\n@@ -353,21 +353,20 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,\n \t\tgoto done;\n \t}\n \n-\tpath = xstrdup_or_null(read_gitfile_gently(wt_path.buf, &err));\n-\tif (!path) {\n+\tif (!read_gitfile_gently(wt_path.buf, &err, &gitfile)) {\n \t\tstrbuf_addf_gently(errmsg, _(\"'%s' is not a .git file, error code %d\"),\n \t\t\t\t wt_path.buf, err);\n \t\tgoto done;\n \t}\n \n \tstrbuf_realpath(&realpath, git_common_path(\"worktrees/%s\", wt->id), 1);\n-\tret = fspathcmp(path, realpath.buf);\n+\tret = fspathcmp(gitfile.buf, 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 done:\n-\tfree(path);\n+\tstrbuf_release(&gitfile);\n \tstrbuf_release(&wt_path);\n \tstrbuf_release(&realpath);\n \treturn ret;\n@@ -567,7 +566,7 @@ static void repair_gitfile(struct worktree *wt,\n {\n \tstruct strbuf dotgit = STRBUF_INIT;\n \tstruct strbuf repo = STRBUF_INIT;\n-\tchar *backlink;\n+\tstruct strbuf backlink = STRBUF_INIT;\n \tconst char *repair = NULL;\n \tint err;\n \n@@ -582,13 +581,13 @@ static void repair_gitfile(struct worktree *wt,\n \n \tstrbuf_realpath(&repo, git_common_path(\"worktrees/%s\", wt->id), 1);\n \tstrbuf_addf(&dotgit, \"%s/.git\", wt->path);\n-\tbacklink = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));\n+\tread_gitfile_gently(dotgit.buf, &err, &backlink);\n \n \tif (err == READ_GITFILE_ERR_NOT_A_FILE)\n \t\tfn(1, wt->path, _(\".git is not a file\"), cb_data);\n \telse if (err)\n \t\trepair = _(\".git file broken\");\n-\telse if (fspathcmp(backlink, repo.buf))\n+\telse if (fspathcmp(backlink.buf, repo.buf))\n \t\trepair = _(\".git file incorrect\");\n \n \tif (repair) {\n@@ -596,7 +595,7 @@ static void repair_gitfile(struct worktree *wt,\n \t\twrite_file(dotgit.buf, \"gitdir: %s\", repo.buf);\n \t}\n \n-\tfree(backlink);\n+\tstrbuf_release(&backlink);\n \tstrbuf_release(&repo);\n \tstrbuf_release(&dotgit);\n }\n@@ -685,7 +684,7 @@ void repair_worktree_at_path(const char *path,\n \tstruct strbuf realdotgit = STRBUF_INIT;\n \tstruct strbuf gitdir = STRBUF_INIT;\n \tstruct strbuf olddotgit = STRBUF_INIT;\n-\tchar *backlink = NULL;\n+\tstruct strbuf backlink = STRBUF_INIT;\n \tconst char *repair = NULL;\n \tint err;\n \n@@ -701,12 +700,12 @@ void repair_worktree_at_path(const char *path,\n \t\tgoto done;\n \t}\n \n-\tbacklink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));\n+\tread_gitfile_gently(realdotgit.buf, &err, &backlink);\n \tif (err == READ_GITFILE_ERR_NOT_A_FILE) {\n \t\tfn(1, realdotgit.buf, _(\"unable to locate repository; .git is not a file\"), cb_data);\n \t\tgoto done;\n \t} else if (err == READ_GITFILE_ERR_NOT_A_REPO) {\n-\t\tif (!(backlink = infer_backlink(realdotgit.buf))) {\n+\t\tif (!(backlink.buf = infer_backlink(realdotgit.buf))) {\n \t\t\tfn(1, realdotgit.buf, _(\"unable to locate repository; .git file does not reference a repository\"), cb_data);\n \t\t\tgoto done;\n \t\t}\n@@ -715,7 +714,7 @@ void repair_worktree_at_path(const char *path,\n \t\tgoto done;\n \t}\n \n-\tstrbuf_addf(&gitdir, \"%s/gitdir\", backlink);\n+\tstrbuf_addf(&gitdir, \"%s/gitdir\", backlink.buf);\n \tif (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0)\n \t\trepair = _(\"gitdir unreadable\");\n \telse {\n@@ -729,7 +728,7 @@ void repair_worktree_at_path(const char *path,\n \t\twrite_file(gitdir.buf, \"%s\", realdotgit.buf);\n \t}\n done:\n-\tfree(backlink);\n+\tstrbuf_release(&backlink);\n \tstrbuf_release(&olddotgit);\n \tstrbuf_release(&gitdir);\n \tstrbuf_release(&realdotgit);\n-- \n2.44.0.rc1.240.g4c46232300-goog\n\n"},{"id":"489935","messageId":"20240305012112.1598053-4-atneya@google.com","threadId":"61050","inReplyTo":"20240305012112.1598053-2-atneya@google.com","subject":"[RFC PATCH 2/3] Make ce_compare_gitlink thread-safe","fromName":"Atneya Nair","fromEmail":"atneya@google.com","sentAt":"2024-03-05T01:21:12Z","receivedAt":"2024-03-05T01:22:29Z","isPatch":true,"sender":{"key":"atneya@google.com","avatar":null},"body":"To enable parallel update of the read cache for submodules,\nce_compare_gitlink must be thread safe (for different objects).\n\nRemove string interning in do_config_from (called from\nrepo_submodule_init) and add locking around accessing the ref_store_map.\n\nSigned-off-by: Atneya Nair \n---\n\nNotes:\n Chasing down thread unsafe code was done using tsan.\n \n Open questions:\n - Is there any additional thread-unsafety that was missed?\n - Is it safe to strdup in do_config_from (do the filenames need static\n lifetime)? What is the performance implication (it seems small)?\n - Is the locking around ref_store_map appropriate and/or can it be made\n more granular?\n\n config.c | 3 ++-\n config.h | 2 +-\n refs.c | 9 +++++++++\n 3 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 3cfeb3d8bd..d7f73d8745 100644\n--- a/config.c\n+++ b/config.c\n@@ -1017,7 +1017,7 @@ static void kvi_from_source(struct config_source *cs,\n \t\t\t enum config_scope scope,\n \t\t\t struct key_value_info *out)\n {\n-\tout->filename = strintern(cs->name);\n+\tout->filename = strdup(cs->name);\n \tout->origin_type = cs->origin_type;\n \tout->linenr = cs->linenr;\n \tout->scope = scope;\n@@ -1857,6 +1857,7 @@ static int do_config_from(struct config_source *top, config_fn_t fn,\n \n \tstrbuf_release(&top->value);\n \tstrbuf_release(&top->var);\n+\tfree(kvi.filename);\n \n \treturn ret;\n }\ndiff --git a/config.h b/config.h\nindex 5dba984f77..b78f1b6667 100644\n--- a/config.h\n+++ b/config.h\n@@ -118,7 +118,7 @@ struct config_options {\n \n /* Config source metadata for a given config key-value pair */\n struct key_value_info {\n-\tconst char *filename;\n+\tchar *filename;\n \tint linenr;\n \tenum config_origin_type origin_type;\n \tenum config_scope scope;\ndiff --git a/refs.c b/refs.c\nindex c633abf284..cce8a31b22 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2126,6 +2126,9 @@ struct ref_store *get_submodule_ref_store(const char *submodule)\n \tsize_t len;\n \tstruct repository *subrepo;\n \n+\t// TODO is this locking tolerable, and/or can we get any finer\n+\tstatic pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;\n+\n \tif (!submodule)\n \t\treturn NULL;\n \n@@ -2139,7 +2142,9 @@ struct ref_store *get_submodule_ref_store(const char *submodule)\n \t\t/* We need to strip off one or more trailing slashes */\n \t\tsubmodule = to_free = xmemdupz(submodule, len);\n \n+\tpthread_mutex_lock(&lock);\n \trefs = lookup_ref_store_map(&submodule_ref_stores, submodule);\n+\tpthread_mutex_unlock(&lock);\n \tif (refs)\n \t\tgoto done;\n \n@@ -2162,10 +2167,14 @@ struct ref_store *get_submodule_ref_store(const char *submodule)\n \t\tfree(subrepo);\n \t\tgoto done;\n \t}\n+\n+\tpthread_mutex_lock(&lock);\n+\t// TODO maybe lock this separately\n \trefs = ref_store_init(subrepo, submodule_sb.buf,\n \t\t\t REF_STORE_READ | REF_STORE_ODB);\n \tregister_ref_store_map(&submodule_ref_stores, \"submodule\",\n \t\t\t refs, submodule);\n+\tpthread_mutex_unlock(&lock);\n \n done:\n \tstrbuf_release(&submodule_sb);\n-- \n2.44.0.rc1.240.g4c46232300-goog\n\n"},{"id":"489936","messageId":"20240305012112.1598053-5-atneya@google.com","threadId":"61050","inReplyTo":"20240305012112.1598053-2-atneya@google.com","subject":"[RFC PATCH 3/3] Preload submodule state in refresh_index","fromName":"Atneya Nair","fromEmail":"atneya@google.com","sentAt":"2024-03-05T01:21:13Z","receivedAt":"2024-03-05T01:22:31Z","isPatch":true,"sender":{"key":"atneya@google.com","avatar":null},"body":"refresh_index currently parallelizes updating cache_entries for regular\nfiles based on lstat. Expand preload to parallelize exploring the\nchecked out state of submodules, which is substantially more expensive.\n\nCache the state of the submodule in memory, to avoid unnecessary\nre-computation (similar to regular files).\n\nThis speeds up git status, and other operations which examine the read\nindex, especially in repositories with many submodules.\n\nSigned-off-by: Atneya Nair \n---\n\nNotes:\n For now, I added a new field to store the submodule state.\n \n Open questions:\n - Where can we efficiently store the submodule state? I assume we can\n re-use some of the ce_flags which aren't used for submodules?\n \n - Why can threads only go up to 64? Can we make this user configurable?\n\n preload-index.c | 25 ++++++++++++++++++++++---\n read-cache-ll.h | 1 +\n read-cache.c | 3 +++\n 3 files changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/preload-index.c b/preload-index.c\nindex 63fd35d64b..091b22fa4c 100644\n--- a/preload-index.c\n+++ b/preload-index.c\n@@ -22,7 +22,7 @@\n * to have at least 500 lstat's per thread for it to\n * be worth starting a thread.\n */\n-#define MAX_PARALLEL (20)\n+#define MAX_PARALLEL (60)\n #define THREAD_COST (500)\n \n struct progress_data {\n@@ -59,8 +59,21 @@ static void *preload_thread(void *_data)\n \n \t\tif (ce_stage(ce))\n \t\t\tcontinue;\n-\t\tif (S_ISGITLINK(ce->ce_mode))\n+\t\tif (S_ISGITLINK(ce->ce_mode)) {\n+\t\t\t// This call evaluates the submodule HEAD for GITLINK, which really does determine\n+\t\t\t// if there is a change (for index purposes). We can't use the traditional path of\n+\t\t\t// marking as VALID, because valid can't be used for submodules due to other code\n+\t\t\t// paths in which valid may skip investigation of the worktree in the submodule.\n+\t\t\t// Gitlinks also aren't statable, or fsmonitorable, so caching doesn't have the same\n+\t\t\t// semantics.\n+\t\t\t// Use a special entry to mark the ref change state and its validity. Future calls\n+\t\t\t// to ce_compare_gitlink will leverage this.\n+\t\t\tif (lstat(ce->name, &st))\n+\t\t\t\tcontinue;\n+\t\t\tce->sub_ref_state = (!!(ie_match_stat(index, ce, &st,\n+\t\t\t\t\tCE_MATCH_RACY_IS_DIRTY|CE_MATCH_IGNORE_FSMONITOR) & DATA_CHANGED) << 1) | 0x1;\n \t\t\tcontinue;\n+\t\t}\n \t\tif (ce_uptodate(ce))\n \t\t\tcontinue;\n \t\tif (ce_skip_worktree(ce))\n@@ -107,11 +120,17 @@ void preload_index(struct index_state *index,\n \tstruct thread_data data[MAX_PARALLEL];\n \tstruct progress_data pd;\n \tint t2_sum_lstat = 0;\n+\tint link_count = 0;\n \n \tif (!HAVE_THREADS || !core_preload_index)\n \t\treturn;\n \n-\tthreads = index->cache_nr / THREAD_COST;\n+\tfor (i = 0; i < index->cache_nr; i++) {\n+\t\tlink_count += (S_ISGITLINK(index->cache[i]->ce_mode));\n+\t}\n+\t// Exploring gitlinks are much more expensive than lstat, so modify the cost\n+\tthreads = (index->cache_nr / THREAD_COST) + (link_count / 25);\n+\n \tif ((index->cache_nr > 1) && (threads < 2) && git_env_bool(\"GIT_TEST_PRELOAD_INDEX\", 0))\n \t\tthreads = 2;\n \tif (threads < 2)\ndiff --git a/read-cache-ll.h b/read-cache-ll.h\nindex 2a50a784f0..5555bb0ae9 100644\n--- a/read-cache-ll.h\n+++ b/read-cache-ll.h\n@@ -27,6 +27,7 @@ struct cache_entry {\n \tunsigned int mem_pool_allocated;\n \tunsigned int ce_namelen;\n \tunsigned int index;\t/* for link extension */\n+\tunsigned int sub_ref_state; /* TODO pack somewhere. Lowest bit valid, second lowest dirty. */\n \tstruct object_id oid;\n \tchar name[FLEX_ARRAY]; /* more */\n };\ndiff --git a/read-cache.c b/read-cache.c\nindex f546cf7875..541d40ca30 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -271,6 +271,9 @@ static int ce_compare_gitlink(const struct cache_entry *ce)\n \t *\n \t * If so, we consider it always to match.\n \t */\n+\tif (ce->sub_ref_state & 0x1)\n+\t\t/* Check the cached value */\n+\t\treturn ce->sub_ref_state >> 1;\n \tif (resolve_gitlink_ref(ce->name, \"HEAD\", &oid) < 0)\n \t\treturn 0;\n \treturn !oideq(&oid, &ce->oid);\n-- \n2.44.0.rc1.240.g4c46232300-goog\n\n"},{"id":"489939","messageId":"xmqqle6xbep5.fsf@gitster.g","threadId":"61050","inReplyTo":"20240305012112.1598053-3-atneya@google.com","subject":"Re: [RFC PATCH 1/3] Make read_gitfile and resolve_gitfile thread safe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-05T02:22:14Z","receivedAt":"2024-03-05T02:22:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Atneya Nair writes:\n\n> Continue the work in commit: 3d7747e318532a36a263c61cdf92f2decb6424ff\n> to remove unsafe shared buffer usage in read_gitfile_gently.\n\nThe usual way to compose a log message of this project is to\n\n - Give an observation on how the current system work in the present\n tense (so no need to say \"Currently X is Y\", just \"X is Y\"), and\n discuss what you perceive as a problem in it.\n\n - Propose a solution (optional---often, problem description\n trivially leads to an obvious solution in reader's minds).\n\n - Give commands to the codebase to \"become like so\".\n\nin this order. The introductory part may become like so. Notice\nthe format in which we usually refer to an existing commit:\n\n 3d7747e3 (real_path: remove unsafe API, 2020-03-10) started the\n process of making more API functions thread-safe. Many callers\n of read_gitfile() still depend on the shared buffer used to hold\n the return value, and convenience of not having to allocate/free\n their own storage, but this hinders multi-threading.\n\nAnd the remainder of the proposed log message should propose a\nsolution and tell the codebase to become like so.\n\nSee Documentation/SubmittingPatches for other general guidelines.\nDocumentation/CodingGuidelines would also be helpful.\n\n> Migrate read_gitfile_gently and resolve_gitfile_gently to take strbuf\n> out params to allocate their return values into, rather than returning\n> a view into a shared buffer.\n>\n> Leave the shared buffer in case a caller passes null for this param (for\n> cases we haven't cleaned up yet).\n>\n> Migrate callers of resolve_gitfile to resolve_gitfile_gently.\n>\n> Signed-off-by: Atneya Nair \n> ---\n>\n> Notes:\n> Open questions:\n> - Is checking the return value of read_gitfile necessary if on error,\n> we are supposed to die, or set the error field to a non-zero value?\n\nThe conversion done by this step seems to be a more or less faithful\nrewrite. If the original checked for an error from the original\nread_gitfile_gently() by seeing if it returned a NULL, the updated\ncode should instead check for what is returned in &err. And that\nseems to be what you did, which is good.\n\n> - Should we clean up the other call-sites of read_gitfile?\n\nIt is OK to do the ones you care about (read: needed to be converted\nbefore you can do the multi-threading) first, and then update the\nrest.\n\nThe conversion itself in the patch looked obvious and trivially\ncorrect from a cursory read, but I have to admit that my\nconcentration level was not sufficiently high while I reviewed the\npatch, as my reading was constantly hiccupping whenever I saw a\ncoding style glitches. The authors can help by sticking to what the\nDocumentation/CodingGuidelines document says, paying special\nattention to the comment styles, decl-after-statement, rules\nregarding braces around a single-statement block.\n\nThanks.\n\n> builtin/init-db.c | 7 ++++---\n> builtin/rev-parse.c | 4 +++-\n> repository.c | 9 +++++----\n> setup.c | 36 +++++++++++++++++++++++++-----------\n> setup.h | 7 +++----\n> submodule.c | 32 +++++++++++++++++++++++---------\n> worktree.c | 27 +++++++++++++--------------\n> 7 files changed, 76 insertions(+), 46 deletions(-)\n>\n> diff --git a/builtin/init-db.c b/builtin/init-db.c\n> index 0170469b84..9135d07a0d 100644\n> --- a/builtin/init-db.c\n> +++ b/builtin/init-db.c\n> @@ -198,11 +198,11 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n> \t */\n> \tif (real_git_dir) {\n> \t\tint err;\n> -\t\tconst char *p;\n> \t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\tstruct strbuf gitfile = STRBUF_INIT;\n> \n> -\t\tp = read_gitfile_gently(git_dir, &err);\n> -\t\tif (p && get_common_dir(&sb, p)) {\n> +\t\tread_gitfile_gently(git_dir, &err, &gitfile);\n> +\t\tif (!err && get_common_dir(&sb, gitfile.buf)) {\n> \t\t\tstruct strbuf mainwt = STRBUF_INIT;\n> \n> \t\t\tstrbuf_addbuf(&mainwt, &sb);\n> @@ -213,6 +213,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n> \t\t\tgit_dir = strbuf_detach(&sb, NULL);\n> \t\t}\n> \t\tstrbuf_release(&sb);\n> +\t\tstrbuf_release(&gitfile);\n> \t}\n> \n> \tif (is_bare_repository_cfg < 0)\n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index d08987646a..e1db6b3231 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -728,12 +728,14 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n> \t\t\t}\n> \t\t\tif (!strcmp(arg, \"--resolve-git-dir\")) {\n> \t\t\t\tconst char *gitdir = argv[++i];\n> +\t\t\t\tstruct strbuf resolved_gitdir = STRBUF_INIT;\n> \t\t\t\tif (!gitdir)\n> \t\t\t\t\tdie(_(\"--resolve-git-dir requires an argument\"));\n> -\t\t\t\tgitdir = resolve_gitdir(gitdir);\n> +\t\t\t\tgitdir = resolve_gitdir_gently(gitdir, NULL, &resolved_gitdir);\n> \t\t\t\tif (!gitdir)\n> \t\t\t\t\tdie(_(\"not a gitdir '%s'\"), argv[i]);\n> \t\t\t\tputs(gitdir);\n> +\t\t\t\tstrbuf_release(&resolved_gitdir);\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \t\t}\n> diff --git a/repository.c b/repository.c\n> index 7aacb51b65..3ca6dbcf16 100644\n> --- a/repository.c\n> +++ b/repository.c\n> @@ -118,7 +118,7 @@ static int repo_init_gitdir(struct repository *repo, const char *gitdir)\n> \tint ret = 0;\n> \tint error = 0;\n> \tchar *abspath = NULL;\n> -\tconst char *resolved_gitdir;\n> +\tstruct strbuf resolved_gitdir = STRBUF_INIT;\n> \tstruct set_gitdir_args args = { NULL };\n> \n> \tabspath = real_pathdup(gitdir, 0);\n> @@ -128,15 +128,16 @@ static int repo_init_gitdir(struct repository *repo, const char *gitdir)\n> \t}\n> \n> \t/* 'gitdir' must reference the gitdir directly */\n> -\tresolved_gitdir = resolve_gitdir_gently(abspath, &error);\n> -\tif (!resolved_gitdir) {\n> +\tresolve_gitdir_gently(abspath, &error, &resolved_gitdir);\n> +\tif (error) {\n> \t\tret = -1;\n> \t\tgoto out;\n> \t}\n> \n> -\trepo_set_gitdir(repo, resolved_gitdir, &args);\n> +\trepo_set_gitdir(repo, resolved_gitdir.buf, &args);\n> \n> out:\n> +\tstrbuf_release(&resolved_gitdir);\n> \tfree(abspath);\n> \treturn ret;\n> }\n> diff --git a/setup.c b/setup.c\n> index b69b1cbc2a..2e118cf216 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -397,14 +397,17 @@ int is_nonbare_repository_dir(struct strbuf *path)\n> \tint ret = 0;\n> \tint gitfile_error;\n> \tsize_t orig_path_len = path->len;\n> +\tstruct strbuf gitfile = STRBUF_INIT;\n> +\n> \tassert(orig_path_len != 0);\n> \tstrbuf_complete(path, '/');\n> \tstrbuf_addstr(path, \".git\");\n> -\tif (read_gitfile_gently(path->buf, &gitfile_error) || is_git_directory(path->buf))\n> +\tif (read_gitfile_gently(path->buf, &gitfile_error, &gitfile) || is_git_directory(path->buf))\n> \t\tret = 1;\n> \tif (gitfile_error == READ_GITFILE_ERR_OPEN_FAILED ||\n> \t gitfile_error == READ_GITFILE_ERR_READ_FAILED)\n> \t\tret = 1;\n> +\tstrbuf_release(&gitfile);\n> \tstrbuf_setlen(path, orig_path_len);\n> \treturn ret;\n> }\n> @@ -830,7 +833,8 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)\n> \n> /*\n> * Try to read the location of the git directory from the .git file,\n> - * return path to git directory if found. The return value comes from\n> + * return path to git directory if found. If passed a valid strbuf, the return\n> + * value is is a ptr to within the buffer. If strbuf is null, the return value comes from\n> * a shared buffer.\n> *\n> * On failure, if return_error_code is not NULL, return_error_code\n> @@ -838,7 +842,7 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)\n> * return_error_code is NULL the function will die instead (for most\n> * cases).\n> */\n> -const char *read_gitfile_gently(const char *path, int *return_error_code)\n> +const char *read_gitfile_gently(const char *path, int *return_error_code, struct strbuf* result_buf)\n> {\n> \tconst int max_file_size = 1 << 20; /* 1MB */\n> \tint error_code = 0;\n> @@ -848,7 +852,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n> \tstruct stat st;\n> \tint fd;\n> \tssize_t len;\n> -\tstatic struct strbuf realpath = STRBUF_INIT;\n> +\tstatic struct strbuf shared = STRBUF_INIT;\n> +\tif (!result_buf) {\n> +\t\tresult_buf = &shared;\n> +\t}\n> \n> \tif (stat(path, &st)) {\n> \t\t/* NEEDSWORK: discern between ENOENT vs other errors */\n> @@ -900,8 +907,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n> \t\tgoto cleanup_return;\n> \t}\n> \n> -\tstrbuf_realpath(&realpath, dir, 1);\n> -\tpath = realpath.buf;\n> +\tstrbuf_realpath(result_buf, dir, 1);\n> +\tpath = result_buf->buf;\n> +\t// TODO is this valid?\n> +\tif (!path) die(_(\"Unexpected null from realpath '%s'\"), dir);\n> \n> cleanup_return:\n> \tif (return_error_code)\n> @@ -1316,12 +1325,13 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n> \t\tint offset = dir->len, error_code = 0;\n> \t\tchar *gitdir_path = NULL;\n> \t\tchar *gitfile = NULL;\n> +\t\tstruct strbuf gitdirenvbuf = STRBUF_INIT;\n> \n> \t\tif (offset > min_offset)\n> \t\t\tstrbuf_addch(dir, '/');\n> \t\tstrbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);\n> \t\tgitdirenv = read_gitfile_gently(dir->buf, die_on_error ?\n> -\t\t\t\t\t\tNULL : &error_code);\n> +\t\t\t\t\t\tNULL : &error_code, &gitdirenvbuf);\n> \t\tif (!gitdirenv) {\n> \t\t\tif (die_on_error ||\n> \t\t\t error_code == READ_GITFILE_ERR_NOT_A_FILE) {\n> @@ -1330,8 +1340,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n> \t\t\t\t\tgitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;\n> \t\t\t\t\tgitdir_path = xstrdup(dir->buf);\n> \t\t\t\t}\n> -\t\t\t} else if (error_code != READ_GITFILE_ERR_STAT_FAILED)\n> +\t\t\t} else if (error_code != READ_GITFILE_ERR_STAT_FAILED) {\n> +\t\t\t\tstrbuf_release(&gitdirenvbuf);\n> \t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n> +\t\t\t}\n> \t\t} else\n> \t\t\tgitfile = xstrdup(dir->buf);\n> \t\t/*\n> @@ -1365,9 +1377,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n> \t\t\t */\n> \t\t\tfree(gitdir_path);\n> \t\t\tfree(gitfile);\n> -\n> +\t\t\tstrbuf_release(&gitdirenvbuf);\n> \t\t\treturn ret;\n> \t\t}\n> +\t\tstrbuf_release(&gitdirenvbuf);\n> \n> \t\tif (is_git_directory(dir->buf)) {\n> \t\t\ttrace2_data_string(\"setup\", NULL, \"implicit-bare-repository\", dir->buf);\n> @@ -1692,11 +1705,12 @@ const char *setup_git_directory(void)\n> \treturn setup_git_directory_gently(NULL);\n> }\n> \n> -const char *resolve_gitdir_gently(const char *suspect, int *return_error_code)\n> +const char *resolve_gitdir_gently(const char *suspect, int *return_error_code,\n> +\t\tstruct strbuf* return_buf)\n> {\n> \tif (is_git_directory(suspect))\n> \t\treturn suspect;\n> -\treturn read_gitfile_gently(suspect, return_error_code);\n> +\treturn read_gitfile_gently(suspect, return_error_code, return_buf);\n> }\n> \n> /* if any standard file descriptor is missing open it to /dev/null */\n> diff --git a/setup.h b/setup.h\n> index 3599aec93c..cf5a63762b 100644\n> --- a/setup.h\n> +++ b/setup.h\n> @@ -36,10 +36,9 @@ int is_nonbare_repository_dir(struct strbuf *path);\n> #define READ_GITFILE_ERR_NOT_A_REPO 7\n> #define READ_GITFILE_ERR_TOO_LARGE 8\n> void read_gitfile_error_die(int error_code, const char *path, const char *dir);\n> -const char *read_gitfile_gently(const char *path, int *return_error_code);\n> -#define read_gitfile(path) read_gitfile_gently((path), NULL)\n> -const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);\n> -#define resolve_gitdir(path) resolve_gitdir_gently((path), NULL)\n> +const char *read_gitfile_gently(const char *path, int *return_error_code, struct strbuf *buf);\n> +#define read_gitfile(path) read_gitfile_gently((path), NULL, NULL)\n> +const char *resolve_gitdir_gently(const char *suspect, int *return_error_code, struct strbuf *buf);\n> \n> void setup_work_tree(void);\n> \n> diff --git a/submodule.c b/submodule.c\n> index 213da79f66..455444321b 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -316,9 +316,10 @@ int is_submodule_populated_gently(const char *path, int *return_error_code)\n> \tint ret = 0;\n> \tchar *gitdir = xstrfmt(\"%s/.git\", path);\n> \n> -\tif (resolve_gitdir_gently(gitdir, return_error_code))\n> +\tstruct strbuf resolved_gitdir_buf = STRBUF_INIT;\n> +\tif (resolve_gitdir_gently(gitdir, return_error_code, &resolved_gitdir_buf))\n> \t\tret = 1;\n> -\n> +\tstrbuf_release(&resolved_gitdir_buf);\n> \tfree(gitdir);\n> \treturn ret;\n> }\n> @@ -1879,22 +1880,25 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n> {\n> \tstruct child_process cp = CHILD_PROCESS_INIT;\n> \tstruct strbuf buf = STRBUF_INIT;\n> +\tstruct strbuf gitdirbuf = STRBUF_INIT;\n> \tFILE *fp;\n> \tunsigned dirty_submodule = 0;\n> \tconst char *git_dir;\n> \tint ignore_cp_exit_code = 0;\n> \n> \tstrbuf_addf(&buf, \"%s/.git\", path);\n> -\tgit_dir = read_gitfile(buf.buf);\n> +\tgit_dir = read_gitfile_gently(buf.buf, NULL, &gitdirbuf);\n> \tif (!git_dir)\n> \t\tgit_dir = buf.buf;\n> \tif (!is_git_directory(git_dir)) {\n> \t\tif (is_directory(git_dir))\n> \t\t\tdie(_(\"'%s' not recognized as a git repository\"), git_dir);\n> \t\tstrbuf_release(&buf);\n> +\t\tstrbuf_release(&gitdirbuf);\n> \t\t/* The submodule is not checked out, so it is not modified */\n> \t\treturn 0;\n> \t}\n> +\t\tstrbuf_release(&gitdirbuf);\n> \tstrbuf_reset(&buf);\n> \n> \tstrvec_pushl(&cp.args, \"status\", \"--porcelain=2\", NULL);\n> @@ -1958,15 +1962,16 @@ int submodule_uses_gitfile(const char *path)\n> {\n> \tstruct child_process cp = CHILD_PROCESS_INIT;\n> \tstruct strbuf buf = STRBUF_INIT;\n> -\tconst char *git_dir;\n> +\tstruct strbuf gitfilebuf = STRBUF_INIT;\n> \n> \tstrbuf_addf(&buf, \"%s/.git\", path);\n> -\tgit_dir = read_gitfile(buf.buf);\n> -\tif (!git_dir) {\n> +\tread_gitfile_gently(buf.buf, NULL, &gitfilebuf);\n> +\tif (!gitfilebuf.buf) {\n> \t\tstrbuf_release(&buf);\n> \t\treturn 0;\n> \t}\n> \tstrbuf_release(&buf);\n> +\tstrbuf_release(&gitfilebuf);\n> \n> \t/* Now test that all nested submodules use a gitfile too */\n> \tstrvec_pushl(&cp.args,\n> @@ -2276,6 +2281,7 @@ static void relocate_single_git_dir_into_superproject(const char *path,\n> {\n> \tchar *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;\n> \tstruct strbuf new_gitdir = STRBUF_INIT;\n> +\tstruct strbuf gitfilebuf = STRBUF_INIT;\n> \tconst struct submodule *sub;\n> \n> \tif (submodule_uses_worktrees(path))\n> @@ -2283,9 +2289,12 @@ static void relocate_single_git_dir_into_superproject(const char *path,\n> \t\t \"more than one worktree not supported\"), path);\n> \n> \told_git_dir = xstrfmt(\"%s/.git\", path);\n> -\tif (read_gitfile(old_git_dir))\n> +\tif (read_gitfile_gently(old_git_dir, NULL, &gitfilebuf)) {\n> \t\t/* If it is an actual gitfile, it doesn't need migration. */\n> +\t\tstrbuf_release(&gitfilebuf);\n> \t\treturn;\n> +\t}\n> +\tstrbuf_release(&gitfilebuf);\n> \n> \treal_old_git_dir = real_pathdup(old_git_dir, 1);\n> \n> @@ -2343,8 +2352,9 @@ void absorb_git_dir_into_superproject(const char *path,\n> \tint err_code;\n> \tconst char *sub_git_dir;\n> \tstruct strbuf gitdir = STRBUF_INIT;\n> +\tstruct strbuf resolved_gitdir_buf = STRBUF_INIT;\n> \tstrbuf_addf(&gitdir, \"%s/.git\", path);\n> -\tsub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code);\n> +\tsub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code, &resolved_gitdir_buf);\n> \n> \t/* Not populated? */\n> \tif (!sub_git_dir) {\n> @@ -2385,6 +2395,8 @@ void absorb_git_dir_into_superproject(const char *path,\n> \t\tfree(real_sub_git_dir);\n> \t\tfree(real_common_git_dir);\n> \t}\n> +\n> +\tstrbuf_release(&resolved_gitdir_buf);\n> \tstrbuf_release(&gitdir);\n> \n> \tabsorb_git_dir_into_superproject_recurse(path, super_prefix);\n> @@ -2484,17 +2496,19 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule)\n> \tconst struct submodule *sub;\n> \tconst char *git_dir;\n> \tint ret = 0;\n> +\tstruct strbuf gitfilebuf = STRBUF_INIT;\n> \n> \tstrbuf_reset(buf);\n> \tstrbuf_addstr(buf, submodule);\n> \tstrbuf_complete(buf, '/');\n> \tstrbuf_addstr(buf, \".git\");\n> \n> -\tgit_dir = read_gitfile(buf->buf);\n> +\tgit_dir = read_gitfile_gently(buf->buf, NULL, &gitfilebuf);\n> \tif (git_dir) {\n> \t\tstrbuf_reset(buf);\n> \t\tstrbuf_addstr(buf, git_dir);\n> \t}\n> +\tstrbuf_release(&gitfilebuf);\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> diff --git a/worktree.c b/worktree.c\n> index b02a05a74a..a6f125c8da 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -309,7 +309,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> -\tchar *path = NULL;\n> +\tstruct strbuf gitfile = STRBUF_INIT;\n> \tint err, ret = -1;\n> \n> \tstrbuf_addf(&wt_path, \"%s/.git\", wt->path);\n> @@ -353,21 +353,20 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,\n> \t\tgoto done;\n> \t}\n> \n> -\tpath = xstrdup_or_null(read_gitfile_gently(wt_path.buf, &err));\n> -\tif (!path) {\n> +\tif (!read_gitfile_gently(wt_path.buf, &err, &gitfile)) {\n> \t\tstrbuf_addf_gently(errmsg, _(\"'%s' is not a .git file, error code %d\"),\n> \t\t\t\t wt_path.buf, err);\n> \t\tgoto done;\n> \t}\n> \n> \tstrbuf_realpath(&realpath, git_common_path(\"worktrees/%s\", wt->id), 1);\n> -\tret = fspathcmp(path, realpath.buf);\n> +\tret = fspathcmp(gitfile.buf, 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> done:\n> -\tfree(path);\n> +\tstrbuf_release(&gitfile);\n> \tstrbuf_release(&wt_path);\n> \tstrbuf_release(&realpath);\n> \treturn ret;\n> @@ -567,7 +566,7 @@ static void repair_gitfile(struct worktree *wt,\n> {\n> \tstruct strbuf dotgit = STRBUF_INIT;\n> \tstruct strbuf repo = STRBUF_INIT;\n> -\tchar *backlink;\n> +\tstruct strbuf backlink = STRBUF_INIT;\n> \tconst char *repair = NULL;\n> \tint err;\n> \n> @@ -582,13 +581,13 @@ static void repair_gitfile(struct worktree *wt,\n> \n> \tstrbuf_realpath(&repo, git_common_path(\"worktrees/%s\", wt->id), 1);\n> \tstrbuf_addf(&dotgit, \"%s/.git\", wt->path);\n> -\tbacklink = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));\n> +\tread_gitfile_gently(dotgit.buf, &err, &backlink);\n> \n> \tif (err == READ_GITFILE_ERR_NOT_A_FILE)\n> \t\tfn(1, wt->path, _(\".git is not a file\"), cb_data);\n> \telse if (err)\n> \t\trepair = _(\".git file broken\");\n> -\telse if (fspathcmp(backlink, repo.buf))\n> +\telse if (fspathcmp(backlink.buf, repo.buf))\n> \t\trepair = _(\".git file incorrect\");\n> \n> \tif (repair) {\n> @@ -596,7 +595,7 @@ static void repair_gitfile(struct worktree *wt,\n> \t\twrite_file(dotgit.buf, \"gitdir: %s\", repo.buf);\n> \t}\n> \n> -\tfree(backlink);\n> +\tstrbuf_release(&backlink);\n> \tstrbuf_release(&repo);\n> \tstrbuf_release(&dotgit);\n> }\n> @@ -685,7 +684,7 @@ void repair_worktree_at_path(const char *path,\n> \tstruct strbuf realdotgit = STRBUF_INIT;\n> \tstruct strbuf gitdir = STRBUF_INIT;\n> \tstruct strbuf olddotgit = STRBUF_INIT;\n> -\tchar *backlink = NULL;\n> +\tstruct strbuf backlink = STRBUF_INIT;\n> \tconst char *repair = NULL;\n> \tint err;\n> \n> @@ -701,12 +700,12 @@ void repair_worktree_at_path(const char *path,\n> \t\tgoto done;\n> \t}\n> \n> -\tbacklink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));\n> +\tread_gitfile_gently(realdotgit.buf, &err, &backlink);\n> \tif (err == READ_GITFILE_ERR_NOT_A_FILE) {\n> \t\tfn(1, realdotgit.buf, _(\"unable to locate repository; .git is not a file\"), cb_data);\n> \t\tgoto done;\n> \t} else if (err == READ_GITFILE_ERR_NOT_A_REPO) {\n> -\t\tif (!(backlink = infer_backlink(realdotgit.buf))) {\n> +\t\tif (!(backlink.buf = infer_backlink(realdotgit.buf))) {\n> \t\t\tfn(1, realdotgit.buf, _(\"unable to locate repository; .git file does not reference a repository\"), cb_data);\n> \t\t\tgoto done;\n> \t\t}\n> @@ -715,7 +714,7 @@ void repair_worktree_at_path(const char *path,\n> \t\tgoto done;\n> \t}\n> \n> -\tstrbuf_addf(&gitdir, \"%s/gitdir\", backlink);\n> +\tstrbuf_addf(&gitdir, \"%s/gitdir\", backlink.buf);\n> \tif (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0)\n> \t\trepair = _(\"gitdir unreadable\");\n> \telse {\n> @@ -729,7 +728,7 @@ void repair_worktree_at_path(const char *path,\n> \t\twrite_file(gitdir.buf, \"%s\", realdotgit.buf);\n> \t}\n> done:\n> -\tfree(backlink);\n> +\tstrbuf_release(&backlink);\n> \tstrbuf_release(&olddotgit);\n> \tstrbuf_release(&gitdir);\n> \tstrbuf_release(&realdotgit);\n"},{"id":"489940","messageId":"CAPig+cQvyScjiWe1ghFrv9B=v2+JxkkErdCyYFSA_8dTrWu60g@mail.gmail.com","threadId":"61050","inReplyTo":"20240305012112.1598053-3-atneya@google.com","subject":"Re: [RFC PATCH 1/3] Make read_gitfile and resolve_gitfile thread safe","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-05T04:29:46Z","receivedAt":"2024-03-05T04:29:59Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 4, 2024 at 8:22 PM Atneya Nair wrote:\n> Continue the work in commit: 3d7747e318532a36a263c61cdf92f2decb6424ff\n> to remove unsafe shared buffer usage in read_gitfile_gently.\n>\n> Migrate read_gitfile_gently and resolve_gitfile_gently to take strbuf\n> out params to allocate their return values into, rather than returning\n> a view into a shared buffer.\n>\n> Leave the shared buffer in case a caller passes null for this param (for\n> cases we haven't cleaned up yet).\n>\n> Migrate callers of resolve_gitfile to resolve_gitfile_gently.\n>\n> Signed-off-by: Atneya Nair \n> ---\n> diff --git a/builtin/init-db.c b/builtin/init-db.c\n> @@ -198,11 +198,11 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n> - const char *p;\n> + struct strbuf gitfile = STRBUF_INIT;\n>\n> - p = read_gitfile_gently(git_dir, &err);\n> - if (p && get_common_dir(&sb, p)) {\n> + read_gitfile_gently(git_dir, &err, &gitfile);\n> + if (!err && get_common_dir(&sb, gitfile.buf)) {\n> struct strbuf mainwt = STRBUF_INIT;\n\nIf you're going to adopt this idiom of checking `err` rather than the\nreturn code of read_gitfile_gently(), then you should document that\n`err` will be set to zero in the success case. Presently, the\ndocumentation for read_gitfile_gently() only talks about the failure\ncase and doesn't mention that zero indicates success.\n\n> diff --git a/setup.c b/setup.c\n> @@ -830,7 +833,8 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)\n> /*\n> * Try to read the location of the git directory from the .git file,\n> - * return path to git directory if found. The return value comes from\n> + * return path to git directory if found. If passed a valid strbuf, the return\n> + * value is is a ptr to within the buffer. If strbuf is null, the return value comes from\n> * a shared buffer.\n\nWhat is \"a valid strbuf\"? Perhaps say instead \"if `result_buf` is not\nNULL, ...\". The \"is not NULL\" wording is consistent with the existing\nwording used below...\n\nAlso...\n s/is is/is/\n s/ptr/pointer/\n\n> * On failure, if return_error_code is not NULL, return_error_code\n\n... \"is not NULL\" wording is already used here.\n\n> @@ -848,7 +852,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n> - static struct strbuf realpath = STRBUF_INIT;\n> + static struct strbuf shared = STRBUF_INIT;\n> + if (!result_buf) {\n> + result_buf = &shared;\n> + }\n\nJunio mentioned style violations in his response. Omit braces around\none line `if` bodies.\n\n> @@ -900,8 +907,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n> - strbuf_realpath(&realpath, dir, 1);\n> - path = realpath.buf;\n> + strbuf_realpath(result_buf, dir, 1);\n> + path = result_buf->buf;\n\nIt's a minor thing, but if you name the function argument `realpath`,\nthen the diff becomes less noisy since changes such as these do not\nneed to be made. On the other hand, if `realpath` isn't a good output\nvariable name, then by all means choose a better name.\n\n> @@ -1316,12 +1325,13 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n> + struct strbuf gitdirenvbuf = STRBUF_INIT;\n> gitdirenv = read_gitfile_gently(dir->buf, die_on_error ?\n> - NULL : &error_code);\n> + NULL : &error_code, &gitdirenvbuf);\n> if (!gitdirenv) {\n> @@ -1330,8 +1340,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n> - } else if (error_code != READ_GITFILE_ERR_STAT_FAILED)\n> + } else if (error_code != READ_GITFILE_ERR_STAT_FAILED) {\n> + strbuf_release(&gitdirenvbuf);\n> return GIT_DIR_INVALID_GITFILE;\n> + }\n\nReleasing the strbuf before `return`. Good.\n\n> @@ -1365,9 +1377,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n> free(gitdir_path);\n> free(gitfile);\n> + strbuf_release(&gitdirenvbuf);\n> return ret;\n\nLikewise. Good.\n\n> }\n> + strbuf_release(&gitdirenvbuf);\n>\n> if (is_git_directory(dir->buf)) {\n> trace2_data_string(\"setup\", NULL, \"implicit-bare-repository\", dir->buf);\n\nThere are additional `return` statements (not shown in the context)\nfollowing this code, but you make this final strbuf_release() call\nbefore any of those other `return` statements can be taken. Good.\n\n> diff --git a/submodule.c b/submodule.c\n> @@ -316,9 +316,10 @@ int is_submodule_populated_gently(const char *path, int *return_error_code)\n> int ret = 0;\n> char *gitdir = xstrfmt(\"%s/.git\", path);\n>\n> - if (resolve_gitdir_gently(gitdir, return_error_code))\n> + struct strbuf resolved_gitdir_buf = STRBUF_INIT;\n> + if (resolve_gitdir_gently(gitdir, return_error_code, &resolved_gitdir_buf))\n> ret = 1;\n\nStyle: Declare `resolved_gitdir_buf` along with `ret` and `gitdir`,\nthen have a blank line before the actual code.\n\n> @@ -1879,22 +1880,25 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n> + struct strbuf gitdirbuf = STRBUF_INIT;\n> + strbuf_release(&gitdirbuf);\n> /* The submodule is not checked out, so it is not modified */\n> return 0;\n> }\n> + strbuf_release(&gitdirbuf);\n> strbuf_reset(&buf);\n\nStyle: Strange indentation?\n\n> @@ -1958,15 +1962,16 @@ int submodule_uses_gitfile(const char *path)\n> - const char *git_dir;\n> + struct strbuf gitfilebuf = STRBUF_INIT;\n>\n> strbuf_addf(&buf, \"%s/.git\", path);\n> - git_dir = read_gitfile(buf.buf);\n> - if (!git_dir) {\n> + read_gitfile_gently(buf.buf, NULL, &gitfilebuf);\n> + if (!gitfilebuf.buf) {\n> strbuf_release(&buf);\n> return 0;\n> }\n\nNot sure what you're trying to do here. strbuf guarantees that its\n`buf` member will never be NULL, so the new `if (!gitfilebuf.buf)`\nconditional seems to be dead code. If you really want to check whether\nan error occurred, pass non-NULL for the second argument and check the\nreturn value of read_gitfile_gently() or check the error code.\n\n> diff --git a/worktree.c b/worktree.c\n> @@ -685,7 +684,7 @@ void repair_worktree_at_path(const char *path,\n> - char *backlink = NULL;\n> + struct strbuf backlink = STRBUF_INIT;\n> @@ -701,12 +700,12 @@ void repair_worktree_at_path(const char *path,\n> - backlink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));\n> + read_gitfile_gently(realdotgit.buf, &err, &backlink);\n> if (err == READ_GITFILE_ERR_NOT_A_FILE) {\n> fn(1, realdotgit.buf, _(\"unable to locate repository; .git is not a file\"), cb_data);\n> goto done;\n> } else if (err == READ_GITFILE_ERR_NOT_A_REPO) {\n> - if (!(backlink = infer_backlink(realdotgit.buf))) {\n> + if (!(backlink.buf = infer_backlink(realdotgit.buf))) {\n\nDon't do this. Never modify the internal state of strbuf directly;\nconsider the state read-only. Modifications should only be made via\nthe API. You'll need to rewrite this code a bit to make it work\ncorrectly with the changes proposed by this patch.\n"},{"id":"489969","messageId":"xmqqwmqg38u2.fsf@gitster.g","threadId":"61050","inReplyTo":"20240305012112.1598053-4-atneya@google.com","subject":"Re: [RFC PATCH 2/3] Make ce_compare_gitlink thread-safe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-05T17:08:21Z","receivedAt":"2024-03-05T17:08:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Atneya Nair writes:\n\n> To enable parallel update of the read cache for submodules,\n> ce_compare_gitlink must be thread safe (for different objects).\n>\n> Remove string interning in do_config_from (called from\n> repo_submodule_init) and add locking around accessing the ref_store_map.\n\nThis step does two independent things, even though they may have\ndependencies, i.e., for one to be a solution for the problem it is\ntackling, the other may have to be there already. E.g., even after\ncalls to ce_compare_gitlink() get serialized via a mutex, it may for\nsome reason not work without giving each kvi.filename its own copy\n[*], and if that is the case, you may need to have the \"stop\ninterning\" step in a single patch with its own justification, and\nthen \"have mutex around ref_store calls\" patch has to come after it.\n\n Side note: I do not know if that is the case myself. I didn't\n write this commit, you did. The above is just a sample to\n illustrate the expected level of depth to explain your thinking\n in the log message.\n\nOr if these two things must happen at the same time, please explain\nin the proposed log message why they have to happen in the same\ncommit. The two paragraphs you wrote there don't explain that, so I\nam assuming that it is not the case.\n\nThe use of strintern() comes originally from 3df8fd62 (add line\nnumber and file name info to `config_set`, 2014-08-07) by Tanay\nAbhra , and survived a handful of changes\n\n 809d8680 (config: pass ctx with config files, 2023-06-28)\n a798a56c (config.c: plumb the_reader through callbacks, 2023-03-28)\n c97f3ed2 (config.c: plumb config_source through static fns, 2023-03-28)\n\nall of which were done by Glen Choo , so they may\nknow how safe the change on the config side would be (I still do\nnot understand why you'd want to do this in the first place, though,\nespecially if you are protecting the callsites with mutex).\n\nI also think Emily's (who you already have on the \"CC:\" line) group\nwants to libify the config machinery and suspect they may still be\nmaking changes to the code, so you may want to coordinate with them\nto avoid duplicated work and overlapping changes.\n\n> Signed-off-by: Atneya Nair \n> ---\n>\n> Notes:\n> Chasing down thread unsafe code was done using tsan.\n\nVery nice to know.\n\n> config.c | 3 ++-\n> config.h | 2 +-\n> refs.c | 9 +++++++++\n> 3 files changed, 12 insertions(+), 2 deletions(-)\n>\n> diff --git a/config.c b/config.c\n> index 3cfeb3d8bd..d7f73d8745 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1017,7 +1017,7 @@ static void kvi_from_source(struct config_source *cs,\n> \t\t\t enum config_scope scope,\n> \t\t\t struct key_value_info *out)\n> {\n> -\tout->filename = strintern(cs->name);\n> +\tout->filename = strdup(cs->name);\n> \tout->origin_type = cs->origin_type;\n> \tout->linenr = cs->linenr;\n> \tout->scope = scope;\n> @@ -1857,6 +1857,7 @@ static int do_config_from(struct config_source *top, config_fn_t fn,\n> \n> \tstrbuf_release(&top->value);\n> \tstrbuf_release(&top->var);\n> +\tfree(kvi.filename);\n> \n> \treturn ret;\n> }\n> diff --git a/config.h b/config.h\n> index 5dba984f77..b78f1b6667 100644\n> --- a/config.h\n> +++ b/config.h\n> @@ -118,7 +118,7 @@ struct config_options {\n> \n> /* Config source metadata for a given config key-value pair */\n> struct key_value_info {\n> -\tconst char *filename;\n> +\tchar *filename;\n> \tint linenr;\n> \tenum config_origin_type origin_type;\n> \tenum config_scope scope;\n> diff --git a/refs.c b/refs.c\n> index c633abf284..cce8a31b22 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2126,6 +2126,9 @@ struct ref_store *get_submodule_ref_store(const char *submodule)\n> \tsize_t len;\n> \tstruct repository *subrepo;\n> \n> +\t// TODO is this locking tolerable, and/or can we get any finer\n> +\tstatic pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;\n> +\n> \tif (!submodule)\n> \t\treturn NULL;\n> \n> @@ -2139,7 +2142,9 @@ struct ref_store *get_submodule_ref_store(const char *submodule)\n> \t\t/* We need to strip off one or more trailing slashes */\n> \t\tsubmodule = to_free = xmemdupz(submodule, len);\n> \n> +\tpthread_mutex_lock(&lock);\n> \trefs = lookup_ref_store_map(&submodule_ref_stores, submodule);\n> +\tpthread_mutex_unlock(&lock);\n> \tif (refs)\n> \t\tgoto done;\n> \n> @@ -2162,10 +2167,14 @@ struct ref_store *get_submodule_ref_store(const char *submodule)\n> \t\tfree(subrepo);\n> \t\tgoto done;\n> \t}\n> +\n> +\tpthread_mutex_lock(&lock);\n> +\t// TODO maybe lock this separately\n> \trefs = ref_store_init(subrepo, submodule_sb.buf,\n> \t\t\t REF_STORE_READ | REF_STORE_ODB);\n> \tregister_ref_store_map(&submodule_ref_stores, \"submodule\",\n> \t\t\t refs, submodule);\n> +\tpthread_mutex_unlock(&lock);\n> \n> done:\n> \tstrbuf_release(&submodule_sb);\n"},{"id":"489977","messageId":"xmqqsf141pf5.fsf@gitster.g","threadId":"61050","inReplyTo":"xmqqwmqg38u2.fsf@gitster.g","subject":"Re: [RFC PATCH 2/3] Make ce_compare_gitlink thread-safe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-05T18:53:02Z","receivedAt":"2024-03-05T18:53:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> The use of strintern() comes originally from 3df8fd62 ...\n> ..., so they may\n> know how safe the change on the config side would be (I still do\n> not understand why you'd want to do this in the first place, though,\n> especially if you are protecting the callsites with mutex).\n\nThe risks of turning code that uses strintern() to use strdup() are\n\n * you will leak the allocated string unless you explicitly free the\n string you now own.\n\n * you may consume too much memory if you are creating too many\n copies of the same string (e.g. if you need filename for each\n line in a file in an application, the memory consumption can\n become 1000-fold).\n\n * the code may be taking advantage of the fact that two such\n strings can be compared for (in)equality simply by comparing\n their addresses, which you would need to adjust to use !strcmp()\n and the like.\n\nI just checked to make sure that the last one is not the case for\nour codebase, and you said you didn't see the second one is the case,\nso the change may be a safe one to make.\n\nOne more thing. Do not use strdup() without checking its return\nvalue for failure. It would be an easy fix to use xstrdup() instead.\n\nThanks.\n\n>> diff --git a/config.c b/config.c\n>> index 3cfeb3d8bd..d7f73d8745 100644\n>> --- a/config.c\n>> +++ b/config.c\n>> @@ -1017,7 +1017,7 @@ static void kvi_from_source(struct config_source *cs,\n>> \t\t\t enum config_scope scope,\n>> \t\t\t struct key_value_info *out)\n>> {\n>> -\tout->filename = strintern(cs->name);\n>> +\tout->filename = strdup(cs->name);\n>> \tout->origin_type = cs->origin_type;\n>> \tout->linenr = cs->linenr;\n>> \tout->scope = scope;\n>> @@ -1857,6 +1857,7 @@ static int do_config_from(struct config_source *top, config_fn_t fn,\n>> \n>> \tstrbuf_release(&top->value);\n>> \tstrbuf_release(&top->var);\n>> +\tfree(kvi.filename);\n>> \n>> \treturn ret;\n>> }\n\n"},{"id":"490054","messageId":"20240306012323.GA3817803@coredump.intra.peff.net","threadId":"61050","inReplyTo":"xmqqsf141pf5.fsf@gitster.g","subject":"Re: [RFC PATCH 2/3] Make ce_compare_gitlink thread-safe","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-06T01:23:23Z","receivedAt":"2024-03-06T01:23:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 05, 2024 at 10:53:02AM -0800, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > The use of strintern() comes originally from 3df8fd62 ...\n> > ..., so they may\n> > know how safe the change on the config side would be (I still do\n> > not understand why you'd want to do this in the first place, though,\n> > especially if you are protecting the callsites with mutex).\n> \n> The risks of turning code that uses strintern() to use strdup() are\n> \n> * you will leak the allocated string unless you explicitly free the\n> string you now own.\n> \n> * you may consume too much memory if you are creating too many\n> copies of the same string (e.g. if you need filename for each\n> line in a file in an application, the memory consumption can\n> become 1000-fold).\n> \n> * the code may be taking advantage of the fact that two such\n> strings can be compared for (in)equality simply by comparing\n> their addresses, which you would need to adjust to use !strcmp()\n> and the like.\n\nThere is one more, I think: if you _do_ free the allocated string to\navoid the leak you mention, then some other code which was relying on\nthe lifetime of that string to be effectively infinite will now have a\nuser-after-free.\n\nAnd I think that may be the case here. The \"kvi\" struct itself is local\nto do_config_from(). But when we load the caching configset, we do so in\nconfigset_add_value() which makes a shallow copy of the kvi struct. And\nthen that shallow copy may live on and be accessed with further calls to\ngit_config().\n\nSo doing just this:\n\ndiff --git a/config.c b/config.c\nindex 3cfeb3d8bd..2f6c83ffe7 100644\n--- a/config.c\n+++ b/config.c\n@@ -1017,7 +1017,7 @@ static void kvi_from_source(struct config_source *cs,\n \t\t\t enum config_scope scope,\n \t\t\t struct key_value_info *out)\n {\n-\tout->filename = strintern(cs->name);\n+\tout->filename = xstrdup(cs->name);\n \tout->origin_type = cs->origin_type;\n \tout->linenr = cs->linenr;\n \tout->scope = scope;\n@@ -1855,6 +1855,7 @@ static int do_config_from(struct config_source *top, config_fn_t fn,\n \n \tret = git_parse_source(top, fn, &kvi, data, opts);\n \n+\tfree((char *)kvi.filename);\n \tstrbuf_release(&top->value);\n \tstrbuf_release(&top->var);\n \n\nwill cause t4013.199 (among others) to fail when built with\nSANITIZE=address, as it detects the user-after-free. I think you'd need\nthis on top:\n\ndiff --git a/config.c b/config.c\nindex 2f6c83ffe7..9854ca002d 100644\n--- a/config.c\n+++ b/config.c\n@@ -2262,6 +2262,7 @@ static int configset_add_value(const struct key_value_info *kvi_p,\n \tl_item->value_index = e->value_list.nr - 1;\n \n \t*kv_info = *kvi_p;\n+\tkv_info->filename = xstrdup_or_null(kvi_p->filename); /* deep copy! */\n \tsi->util = kv_info;\n \n \treturn 0;\n\nthough probably an actual kvi_copy() function would be less horrible.\n\nA few other things to note, looking at this code:\n\n - isn't kvi->path in the same boat? We do not duplicate it at all, so\n it seems like the shallow copy made in the configset could cause a\n user-after-free.\n\n - the \"fix\" I showed above hits your point 2: now we are making a lot\n more copies of that string. I will note that we're already making a\n lot of copies of the kvi struct in the first place, so unless you\n have really long pathnames, it probably isn't a big difference.\n\n But it possibly could make sense to have the configset own a single\n duplicate string, and then let the kvi structs it holds point to\n that string. But IMHO all of this should be details of the configset\n code, and the main config-iteration code should not have to worry\n about this at all. I.e., I think kvi_from_source() should not be\n duplicating anything in the first place.\n\n-Peff\n"},{"id":"490055","messageId":"xmqqzfvcunny.fsf@gitster.g","threadId":"61050","inReplyTo":"20240306012323.GA3817803@coredump.intra.peff.net","subject":"Re: [RFC PATCH 2/3] Make ce_compare_gitlink thread-safe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-06T01:58:09Z","receivedAt":"2024-03-06T01:58:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> There is one more, I think: if you _do_ free the allocated string to\n> avoid the leak you mention, then some other code which was relying on\n> the lifetime of that string to be effectively infinite will now have a\n> user-after-free.\n\nAh, yes, you're right. I completely forgot about that shallow copy.\n\n> A few other things to note, looking at this code:\n>\n> - isn't kvi->path in the same boat? We do not duplicate it at all, so\n> it seems like the shallow copy made in the configset could cause a\n> user-after-free.\n>\n> - the \"fix\" I showed above hits your point 2: now we are making a lot\n> more copies of that string. I will note that we're already making a\n> lot of copies of the kvi struct in the first place, so unless you\n> have really long pathnames, it probably isn't a big difference.\n>\n> But it possibly could make sense to have the configset own a single\n> duplicate string, and then let the kvi structs it holds point to\n> that string. But IMHO all of this should be details of the configset\n> code, and the main config-iteration code should not have to worry\n> about this at all. I.e., I think kvi_from_source() should not be\n> duplicating anything in the first place.\n\nThanks for a detailed write-up.\n"},{"id":"490063","messageId":"10042df8-5d06-47cd-9202-ea6965f50784@gmail.com","threadId":"61050","inReplyTo":"20240305012112.1598053-3-atneya@google.com","subject":"Re: [RFC PATCH 1/3] Make read_gitfile and resolve_gitfile thread safe","fromName":"Jean-Noël Avila","fromEmail":"avila.jn@gmail.com","sentAt":"2024-03-06T08:13:15Z","receivedAt":"2024-03-06T08:13:18Z","isPatch":true,"sender":{"key":"jn.avila@free.fr","avatar":"https://avatars.githubusercontent.com/u/156172?v=4"},"body":"Le 05/03/2024 à 02:21, Atneya Nair a écrit :\n\n\n\n> @@ -830,7 +833,8 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)\n> \n> /*\n> * Try to read the location of the git directory from the .git file,\n> - * return path to git directory if found. The return value comes from\n> + * return path to git directory if found. If passed a valid strbuf, the return\n> + * value is is a ptr to within the buffer. If strbuf is null, the return value comes from\n> * a shared buffer.\n> *\n> * On failure, if return_error_code is not NULL, return_error_code\n> @@ -838,7 +842,7 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)\n> * return_error_code is NULL the function will die instead (for most\n> * cases).\n> */\n> -const char *read_gitfile_gently(const char *path, int *return_error_code)\n> +const char *read_gitfile_gently(const char *path, int *return_error_code, struct strbuf* result_buf)\n> {\n> \tconst int max_file_size = 1 << 20; /* 1MB */\n> \tint error_code = 0;\n> @@ -848,7 +852,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n> \tstruct stat st;\n> \tint fd;\n> \tssize_t len;\n> -\tstatic struct strbuf realpath = STRBUF_INIT;\n> +\tstatic struct strbuf shared = STRBUF_INIT;\n> +\tif (!result_buf) {\n> +\t\tresult_buf = &shared;\n> +\t}\n> \n\nQuestion about general style: is it accepted practice to override a\nparameter of a function?\n\nI would have written:\n\n\n@@ -838,7 +842,7 @@ void read_gitfile_error_die(int error_code, const\nchar *path, const char *dir)\n * return_error_code is NULL the function will die instead (for most\n * cases).\n */\n-const char *read_gitfile_gently(const char *path, int *return_error_code)\n+const char *read_gitfile_gently(const char *path, int\n*return_error_code, struct strbuf* input_buf)\n {\n \tconst int max_file_size = 1 << 20; /* 1MB */\n \tint error_code = 0;\n@@ -848,7 +852,10 @@ const char *read_gitfile_gently(const char *path,\nint *return_error_code)\n \tstruct stat st;\n \tint fd;\n \tssize_t len;\n-\tstatic struct strbuf realpath = STRBUF_INIT;\n+\tstatic struct strbuf shared = STRBUF_INIT;\n+ struct strbuf* result_buf;\n+\tif (!input_buf) {\n+\t\tresult_buf = &shared;\n+\t} else {\n+\t\tresult_buf = input_buf;\n+\t}\n\n\n> \tif (stat(path, &st)) {\n> \t\t/* NEEDSWORK: discern between ENOENT vs other errors */\n> @@ -900,8 +907,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n> \t\tgoto cleanup_return;\n> \t}\n> \n> -\tstrbuf_realpath(&realpath, dir, 1);\n> -\tpath = realpath.buf;\n> +\tstrbuf_realpath(result_buf, dir, 1);\n> +\tpath = result_buf->buf;\n> +\t// TODO is this valid?\n> +\tif (!path) die(_(\"Unexpected null from realpath '%s'\"), dir);\n\nIn fact, this is not a null path, but an empty path (null is not part of\nthe string).\nBy the way, shouldn't this be an internal bug instead of a message to\nthe user?\n\n\nThanks\n\n\n"},{"id":"490099","messageId":"xmqqttljs3he.fsf@gitster.g","threadId":"61050","inReplyTo":"10042df8-5d06-47cd-9202-ea6965f50784@gmail.com","subject":"Re: [RFC PATCH 1/3] Make read_gitfile and resolve_gitfile thread safe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-06T16:57:01Z","receivedAt":"2024-03-06T16:57:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jean-Noël Avila writes:\n\n>> -const char *read_gitfile_gently(const char *path, int *return_error_code)\n>> +const char *read_gitfile_gently(const char *path, int *return_error_code, struct strbuf* result_buf)\n>> {\n>> \tconst int max_file_size = 1 << 20; /* 1MB */\n>> \tint error_code = 0;\n>> @@ -848,7 +852,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n>> \tstruct stat st;\n>> \tint fd;\n>> \tssize_t len;\n>> -\tstatic struct strbuf realpath = STRBUF_INIT;\n>> +\tstatic struct strbuf shared = STRBUF_INIT;\n>> +\tif (!result_buf) {\n>> +\t\tresult_buf = &shared;\n>> +\t}\n>> \n>\n> Question about general style: is it accepted practice to override a\n> parameter of a function?\n\nWe do not forbid it. We have a rule against enclosing a single\nstatement block inside {braces}, though ;-).\n\n> I would have written:\n\nIf it matters to know what the caller-supplied value of the\nparameter was, then we would probably write that way. If it does\nnot, then the above is perfectly fine. Even with the above, if a\nlater code really wanted to, it can compare the pointers to find out\nif the caller was uninterested in the result (i.e., passed NULL),\nbut at that point, we may be better off to (re)write it your way.\n\n>> -\tstrbuf_realpath(&realpath, dir, 1);\n>> -\tpath = realpath.buf;\n>> +\tstrbuf_realpath(result_buf, dir, 1);\n>> +\tpath = result_buf->buf;\n>> +\t// TODO is this valid?\n>> +\tif (!path) die(_(\"Unexpected null from realpath '%s'\"), dir);\n>\n> In fact, this is not a null path, but an empty path (null is not part of\n> the string).\n> By the way, shouldn't this be an internal bug instead of a message to\n> the user?\n\nUnless the strbuf instance the result_buf pointer points at is\ncorrupt, its .buf member should *NEVER* be NULL. Testing for NULL\nis meaningless, unless you are manually futzing with the members of\nstrbuf (you shouldn't).\n\nThanks for carefully reading.\n\n"},{"id":"490503","messageId":"CA+D9mTEJFh3t_OrrNFD=EZHQO_EXa0__r5=_mqOqNfyh_uoq_w@mail.gmail.com","threadId":"61050","inReplyTo":"CAPig+cQvyScjiWe1ghFrv9B=v2+JxkkErdCyYFSA_8dTrWu60g@mail.gmail.com","subject":"Re: [RFC PATCH 1/3] Make read_gitfile and resolve_gitfile thread safe","fromName":"Atneya Nair","fromEmail":"atneya@google.com","sentAt":"2024-03-12T20:38:52Z","receivedAt":"2024-03-12T20:39:31Z","isPatch":true,"sender":{"key":"atneya@google.com","avatar":null},"body":"On Mon, Mar 4, 2024 at 8:29 PM Eric Sunshine wrote:\n\n> Style: Declare `resolved_gitdir_buf` along with `ret` and `gitdir`,\n> then have a blank line before the actual code.\n\nThanks for the detailed style feedback.\n\n>\n> > } else if (err == READ_GITFILE_ERR_NOT_A_REPO) {\n> > - if (!(backlink = infer_backlink(realdotgit.buf))) {\n> > + if (!(backlink.buf = infer_backlink(realdotgit.buf))) {\n>\n> Don't do this. Never modify the internal state of strbuf directly;\n> consider the state read-only. Modifications should only be made via\n> the API. You'll need to rewrite this code a bit to make it work\n> correctly with the changes proposed by this patch.\n\nGood catch, I should've paid more attention in the refactoring.\n\nFixed all of the discussed notes in v2.\n"},{"id":"490504","messageId":"CA+D9mTFLehceHtO+_3w4pCW+XKNx1FjxJv_qR+sfwR94kPJMVw@mail.gmail.com","threadId":"61050","inReplyTo":"xmqqttljs3he.fsf@gitster.g","subject":"Re: [RFC PATCH 1/3] Make read_gitfile and resolve_gitfile thread safe","fromName":"Atneya Nair","fromEmail":"atneya@google.com","sentAt":"2024-03-12T20:44:37Z","receivedAt":"2024-03-12T20:45:16Z","isPatch":true,"sender":{"key":"atneya@google.com","avatar":null},"body":"On Wed, Mar 6, 2024 at 8:57 AM Junio C Hamano wrote:\n\n> >> + if (!path) die(_(\"Unexpected null from realpath '%s'\"), dir);\n> >\n> > In fact, this is not a null path, but an empty path (null is not part of\n> > the string).\n> > By the way, shouldn't this be an internal bug instead of a message to\n> > the user?\n>\n> Unless the strbuf instance the result_buf pointer points at is\n> corrupt, its .buf member should *NEVER* be NULL. Testing for NULL\n> is meaningless, unless you are manually futzing with the members of\n> strbuf (you shouldn't).\n>\n> Thanks for carefully reading.\n>\n\nThanks for pointing this out, I fixed this issue in v2.\n"},{"id":"490517","messageId":"CA+D9mTEyYshL5e_G9AN=6s_zhbtjQNh+_=a9+Y2J2iLQACFeHQ@mail.gmail.com","threadId":"61050","inReplyTo":"20240306012323.GA3817803@coredump.intra.peff.net","subject":"Re: [RFC PATCH 2/3] Make ce_compare_gitlink thread-safe","fromName":"Atneya Nair","fromEmail":"atneya@google.com","sentAt":"2024-03-12T22:13:16Z","receivedAt":"2024-03-12T22:13:55Z","isPatch":true,"sender":{"key":"atneya@google.com","avatar":null},"body":"On Tue, Mar 5, 2024 at 5:23 PM Jeff King wrote:\n\n> And I think that may be the case here. The \"kvi\" struct itself is local\n> to do_config_from(). But when we load the caching configset, we do so in\n> configset_add_value() which makes a shallow copy of the kvi struct. And\n> then that shallow copy may live on and be accessed with further calls to\n> git_config().\n\nI missed the shallow copy of kvi in do_config_from. Unfortunately, we also\npass this back into a callback function, and although it is often not used,\nchanging the code to be more precise about string ownership here seems\nnon-trivial.\n\n>\n> though probably an actual kvi_copy() function would be less horrible.\n>\n> A few other things to note, looking at this code:\n>\n> - isn't kvi->path in the same boat? We do not duplicate it at all, so\n> it seems like the shallow copy made in the configset could cause a\n> user-after-free.\n\nThe situation with path seems to indicate that indeed ownership is correctly\nhandled up the call-stack (i.e. the config_source owns the file/path strings),\nand these strings are valid for the duration of the shallow copies through the\ncall-graph. But, the fact that filename in particular is interned may indicate\nthat the behavior of leaking the string to static lifetime is used by\na caller at\nsome point.\n\n> But it possibly could make sense to have the configset own a single\n> duplicate string, and then let the kvi structs it holds point to\n> that string. But IMHO all of this should be details of the configset\n> code, and the main config-iteration code should not have to worry\n> about this at all. I.e., I think kvi_from_source() should not be\n> duplicating anything in the first place.\n>\n> -Peff\n\nBased on this discussion, I think the cleanest solution is a lock on\nthe strintern hashmap. I originally wanted to avoid adding locking, since most\ncall-paths are single threaded, and I thought we were only using the\nstring locally.\nHowever, copying the string is more expensive and potentially intrusive to many\ncall-sites, and it is much more difficult to reason about the correctness of the\nchange, so I think adding a lock is preferable. I'll switch to that in\nv2 along with the\nmore descriptive commit message.\n"},{"id":"490518","messageId":"CA+D9mTHD6iwXuSdgAg1JZHYu1s0G6y8Kgv07V_JUkKU2Kcm_vA@mail.gmail.com","threadId":"61050","inReplyTo":"xmqqwmqg38u2.fsf@gitster.g","subject":"Re: [RFC PATCH 2/3] Make ce_compare_gitlink thread-safe","fromName":"Atneya Nair","fromEmail":"atneya@google.com","sentAt":"2024-03-12T22:15:29Z","receivedAt":"2024-03-12T22:16:07Z","isPatch":true,"sender":{"key":"atneya@google.com","avatar":null},"body":"On Tue, Mar 5, 2024 at 9:08 AM Junio C Hamano wrote:\n>\n> Atneya Nair writes:\n>\n> > To enable parallel update of the read cache for submodules,\n> > ce_compare_gitlink must be thread safe (for different objects).\n> >\n> > Remove string interning in do_config_from (called from\n> > repo_submodule_init) and add locking around accessing the ref_store_map.\n>\n> This step does two independent things, even though they may have\n> dependencies, i.e., for one to be a solution for the problem it is\n> tackling, the other may have to be there already. E.g., even after\n> calls to ce_compare_gitlink() get serialized via a mutex, it may for\n> some reason not work without giving each kvi.filename its own copy\n> [*], and if that is the case, you may need to have the \"stop\n> interning\" step in a single patch with its own justification, and\n> then \"have mutex around ref_store calls\" patch has to come after it.\n>\n\nWhat is the appropriate way to split an intermediate patch into two? Simply\nadding the additional commit as 3 of 4 would cause a mismatch between patch-set\nversions? Would that cause any issues?\n"},{"id":"490563","messageId":"xmqqedcec94w.fsf@gitster.g","threadId":"61050","inReplyTo":"CA+D9mTHD6iwXuSdgAg1JZHYu1s0G6y8Kgv07V_JUkKU2Kcm_vA@mail.gmail.com","subject":"Re: [RFC PATCH 2/3] Make ce_compare_gitlink thread-safe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-13T17:51:59Z","receivedAt":"2024-03-13T17:52:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Atneya Nair writes:\n\n> What is the appropriate way to split an intermediate patch into two? Simply\n> adding the additional commit as 3 of 4 would cause a mismatch between patch-set\n> versions? Would that cause any issues?\n\nIf you have three patch series originally and the second one is too\nbig, then you'd end up with four patch series whose second and third\ncorresponds to the second patch of the original series, and the last\npatch of the updated series that is numbered 4 will correspond to\nthe third one from the original. That is perfectly normal and you\ncan see the correspondence with help from tools like range-diff.\n"}]}