{"thread":{"id":"62260","subject":"[PATCH v2 0/4] Link worktrees with relative paths","startedAt":"2024-10-06T06:00:39Z","lastAt":"2024-10-07T16:59:32Z","messageCount":38,"participants":["Caleb White","Eric Sunshine","shejialuo","Kristoffer Haugsbakk"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"504156","messageId":"20241006060017.171788-1-cdwhite3@pm.me","threadId":"62260","inReplyTo":null,"subject":"[PATCH v2 0/4] Link worktrees with relative paths","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-06T06:00:30Z","receivedAt":"2024-10-06T06:00:39Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"This is a resubmission attempt to try to fix the damaged\npatch noted in previous discussions.\n\nThanks!\n\nCaleb White (4):\n  worktree: refactor infer_backlink() to use *strbuf\n  worktree: link worktrees with relative paths\n  worktree: sync worktree paths after gitdir move\n  worktree: prevent null pointer dereference\n\n builtin/worktree.c           |  17 +--\n setup.c                      |   2 +-\n t/t2408-worktree-relative.sh |  39 ++++++\n worktree.c                   | 235 +++++++++++++++++++++++++++--------\n worktree.h                   |  10 ++\n 5 files changed, 240 insertions(+), 63 deletions(-)\n create mode 100755 t/t2408-worktree-relative.sh\n\n-- \n2.46.2\n\n"},{"id":"504157","messageId":"20241006060017.171788-2-cdwhite3@pm.me","threadId":"62260","inReplyTo":"20241006060017.171788-1-cdwhite3@pm.me","subject":"[PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-06T06:00:57Z","receivedAt":"2024-10-06T06:01:03Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"This refactors the `infer_backlink` function to return an integer\nresult and use a pre-allocated `strbuf` for the inferred backlink\npath, replacing the previous `char*` return type.\n\nThis lays the groundwork for the next patch, which needs the\nresultant backlink as a `strbuf`. There was no need to go from\n`strbuf -> char* -> strbuf` again. This change also aligns the\nfunction's signature with other `strbuf`-based functions.\n\nSigned-off-by: Caleb White <cdwhite3@pm.me>\n---\n worktree.c | 26 ++++++++++++++------------\n 1 file changed, 14 insertions(+), 12 deletions(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex 0f032cc..c6d2ede 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -642,10 +642,9 @@ static int is_main_worktree_path(const char *path)\n  * be able to infer the gitdir by manually reading /path/to/worktree/.git,\n  * extracting the <id>, and checking if <repo>/worktrees/<id> exists.\n  */\n-static char *infer_backlink(const char *gitfile)\n+static int infer_backlink(st\nruct strbuf *inferred, const char *gitfile)\n {\n \tstruct strbuf actual = STRBUF_INIT;\n-\tstruct strbuf inferred = STRBUF_INIT;\n \tconst char *id;\n \n \tif (strbuf_read_file(&actual, gitfile, 0) < 0)\n@@ -658,17 +657,16 @@ static char *infer_backlink(const char *gitfile)\n \tid++; /* advance past '/' to point at <id> */\n \tif (!*id)\n \t\tgoto error;\n-\tstrbuf_git_common_path(&inferred, the_repository, \"worktrees/%s\", id);\n-\tif (!is_directory(inferred.buf))\n+\tstrbuf_git_common_path(inferred, the_repository, \"worktrees/%s\", id);\n+\tif (!is_directory(inferred->buf))\n \t\tgoto error;\n \n \tstrbuf_release(&actual);\n-\treturn strbuf_detach(&inferred, NULL);\n+\treturn 0;\n \n error:\n \tstrbuf_release(&actual);\n-\tstrbuf_release(&inferred);\n-\treturn NULL;\n+\treturn 1;\n }\n \n /*\n@@ -680,9 +678,10 @@ void repair_worktree_at_path(const char *path,\n {\n \tstruct strbuf dotgit = STRBUF_INIT;\n \tstruct strbuf realdotgit = STRBUF_INIT;\n+\tstruct strbuf backlink = STRBUF_INIT;\n \tstruct strbuf gitd\nir = STRBUF_INIT;\n \tstruct strbuf olddotgit = STRBUF_INIT;\n-\tchar *backlink = NULL;\n+\tchar *git_contents = NULL;\n \tconst char *repair = NULL;\n \tint err;\n \n@@ -698,21 +697,23 @@ 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+\tgit_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));\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 (infer_backlink(&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 \t} else if (err) {\n \t\tfn(1, realdotgit.buf, _(\"unable to locate repository; .git file broken\"), cb_data);\n \t\tgoto done;\n+\t} else if (git_conte\nnts) {\n+\t\tstrbuf_addstr(&backlink, git_contents);\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@@ -726,8 +727,9 @@ 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+\tfree(git_contents);\n \tstrbuf_release(&olddotgit);\n+\tstrbuf_release(&backlink);\n \tstrbuf_release(&gitdir);\n \tstrbuf_release(&realdotgit);\n \tstrbuf_release(&dotgit);\n-- \n2.46.2\n\n"},{"id":"504158","messageId":"20241006060017.171788-3-cdwhite3@pm.me","threadId":"62260","inReplyTo":"20241006060017.171788-1-cdwhite3@pm.me","subject":"[PATCH v2 2/4] worktree: link worktrees with relative paths","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-06T06:01:17Z","receivedAt":"2024-10-06T06:01:23Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"This modifies Git’s handling of worktree linking to use relative\npaths instead of absolute paths. Previously, when creating a worktree,\nGit would store the absolute paths to both the main repository and the\nlinked worktrees. These hardcoded absolute paths cause breakages such\nas when the repository is moved to a different directory or during\ncontainerized development where the absolute differs between systems.\n\nBy switching to relative paths, we help ensure that worktree links are\nmore resilient when the repository is moved. While links external to the\nrepository may still break, Git still automatically handles many common\nscenarios, reducing the need for manual repair. This is particularly\nuseful in containerized or portable development environments, where the\nabsolute path to the repository can differ between systems. Developers\nno longer need to reinitialize or repair worktrees after relocating the\nrepository, improving workflow efficiency and reducing breakages.\n\nFor self-contained repositories (such as using a bare repository with\nworktrees), where both the repository and its worktrees are located\nwithin the same directory structure, using relative paths guarantees all\nlinks remain functional regardless of where the directory is located.\n\nSigned-off-by: Caleb White <cdwhite3@pm.me>\n---\n builtin/worktree.c           |  17 ++--\n t/t2408-worktree-relative.sh |  39 +++++++++\n worktree.c                   | 152 +++++++++++++++++++++++++----------\n 3 files changed, 159 insertions(+), 49 deletions(-)\n create mode 100755 t/t2408-worktree-relative.sh\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex fc31d07..99cee56 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -414,7 +414,8 @@ static int add_worktree(const char *path, const char *refname,\n \t\t\tconst struct add_opts *opts)\n {\n \tstruct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT;\n-\tstruct strbuf sb = STRBUF_INIT, realpath = STRBUF_INIT;\n+\tstruct strbuf sb = STRBUF_INIT, sb_tmp = STRBUF_INIT;\n+\tstruct strbuf sb_path_realpath = STRBUF_INIT, sb_repo_realpath = STRBUF_INIT;\n \tconst char *name;\n \tstruct strvec child_env = STRVEC_INIT;\n \tunsigned int counter = 0;\n@@ -490,11 +491,11 @@ static int add_worktree(const char *path, const char *refname,\n \n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/gitdir\", sb_repo.buf);\n-\tstrbuf_realpath(&realpath, sb_git.buf, 1);\n-\twrite_file(sb.buf, \"%s\", realpath.buf);\n-\tstrbuf_realpath(&realpath, repo_get_common_dir(the_repository), 1);\n-\twrite_file(sb_git.buf, \"gitdir: %s/worktrees/%s\",\n-\t\t   realpath.buf, name);\n+\tstrbuf_realpath(&sb_path_realpath, path, 1);\n+\tstrbuf_realpath(&sb_repo_realpath, sb_repo.buf, 1);\n+\twrite_file(sb.buf, \"%s/.git\", relative_path(sb_path_realpath.buf, sb_repo_realpath.buf, &sb_tmp));\n+\tstrbuf_reset(&sb_tmp);\n+\twrite_file(sb_git.buf, \"gitdir: %s\", relative_path(sb_repo_realpath.buf, sb_path_realpath.buf, &sb_tmp));\n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/commondir\", sb_repo.buf);\n \twrite_file(sb.buf, \"../..\");\n@@ -578,11 +579,13 @@ static int add_worktree(const char *path, const char *refname,\n \n \tstrvec_clear(&child_env);\n \tstrbuf_release(&sb);\n+\tstrbuf_release(&sb_tmp);\n \tstrbuf_release(&symref);\n \tstrbuf_release(&sb_repo);\n+\tstrbuf_release(&sb_repo_realpath);\n \tstrbuf_release(&sb_git);\n+\tstrbuf_release(&sb_path_realpath);\n \tstrbuf_release(&sb_name);\n-\tstrbuf_release(&realpath);\n \tfree_worktree(wt);\n \treturn ret;\n }\ndiff --git a/t/t2408-worktree-relative.sh b/t/t2408-worktree-relative.sh\nnew file mode 100755\nindex 0000000..a3136db\n--- /dev/null\n+++ b/t/t2408-worktree-relative.sh\n@@ -0,0 +1,39 @@\n+#!/bin/sh\n+\n+test_description='test worktrees linked with relative paths'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+test_expect_success 'links worktrees with relative paths' '\n+\ttest_when_finished rm -rf repo &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit initial &&\n+\t\tgit worktree add wt1 &&\n+\t\techo \"../../../wt1/.git\" >expected_gitdir &&\n+\t\tcat .git/worktrees/wt1/gitdir >actual_gitdir &&\n+\t\techo \"gitdir: ../.git/worktrees/wt1\" >expected_git &&\n+\t\tcat wt1/.git >actual_git &&\n+\t\ttest_cmp expected_gitdir actual_gitdir &&\n+\t\ttest_cmp expected_git actual_git\n+\t)\n+'\n+\n+test_expect_success 'move repo without breaking relative internal links' '\n+\ttest_when_finished rm -rf repo moved &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit initial &&\n+\t\tgit worktree add wt1 &&\n+\t\tcd .. &&\n+\t\tmv repo moved &&\n+\t\tcd moved/wt1 &&\n+\t\tgit status >out 2>err &&\n+\t\ttest_must_be_empty err\n+\t)\n+'\n+\n+test_done\ndiff --git a/worktree.c b/worktree.c\nindex c6d2ede..fc14e9a 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -110,6 +110,12 @@ struct worktree *get_linked_worktree(const char *id,\n \tstrbuf_rtrim(&worktree_path);\n \tstrbuf_strip_suffix(&worktree_path, \"/.git\");\n \n+\tif (!is_absolute_path(worktree_path.buf)) {\n+\t    strbuf_strip_suffix(&path, \"gitdir\");\n+\t    strbuf_addbuf(&path, &worktree_path);\n+\t    strbuf_realpath_forgiving(&worktree_path, path.buf, 0);\n+\t}\n+\n \tCALLOC_ARRAY(worktree, 1);\n \tworktree->repo = the_repository;\n \tworktree->path = strbuf_detach(&worktree_path, NULL);\n@@ -373,18 +379,30 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,\n void update_worktree_location(struct worktree *wt, const char *path_)\n {\n \tstruct strbuf path = STRBUF_INIT;\n+\tstruct strbuf repo = STRBUF_INIT;\n+\tstruct strbuf tmp = STRBUF_INIT;\n+\tchar *file = NULL;\n \n \tif (is_main_worktree(wt))\n \t\tBUG(\"can't relocate main worktree\");\n \n+\tstrbuf_realpath(&repo, git_common_path(\"worktrees/%s\", wt->id), 1);\n \tstrbuf_realpath(&path, path_, 1);\n \tif (fspathcmp(wt->path, path.buf)) {\n-\t\twrite_file(git_common_path(\"worktrees/%s/gitdir\", wt->id),\n-\t\t\t   \"%s/.git\", path.buf);\n+\t\tfile = xstrfmt(\"%s/gitdir\", repo.buf);\n+\t\twrite_file(file, \"%s/.git\", relative_path(path.buf, repo.buf, &tmp));\n+\t\tfree(file);\n+\t\tstrbuf_reset(&tmp);\n+\t\tfile = xstrfmt(\"%s/.git\", path.buf);\n+\t\twrite_file(file, \"gitdir: %s\", relative_path(repo.buf, path.buf, &tmp));\n+\n \t\tfree(wt->path);\n \t\twt->path = strbuf_detach(&path, NULL);\n \t}\n+\tfree(file);\n \tstrbuf_release(&path);\n+\tstrbuf_release(&repo);\n+\tstrbuf_release(&tmp);\n }\n \n int is_worktree_being_rebased(const struct worktree *wt,\n@@ -564,38 +582,52 @@ 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+\tstruct strbuf tmp = STRBUF_INIT;\n+\tchar *git_contents = NULL;\n \tconst char *repair = NULL;\n \tint err;\n \n \t/* missing worktree can't be repaired */\n \tif (!file_exists(wt->path))\n-\t\treturn;\n+\t\tgoto done;\n \n \tif (!is_directory(wt->path)) {\n \t\tfn(1, wt->path, _(\"not a directory\"), cb_data);\n-\t\treturn;\n+\t\tgoto done;\n \t}\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+\tgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));\n+\n+\tif (git_contents) {\n+\t\tif (is_absolute_path(git_contents)) {\n+\t\t\tstrbuf_addstr(&backlink, git_contents);\n+\t\t} else {\n+\t\t\tstrbuf_addf(&backlink, \"%s/%s\", wt->path, git_contents);\n+\t\t\tstrbuf_realpath_forgiving(&backlink, backlink.buf, 0);\n+\t\t}\n+\t}\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 \t\tfn(0, wt->path, repair, cb_data);\n-\t\twrite_file(dotgit.buf, \"gitdir: %s\", repo.buf);\n+\t\twrite_file(dotgit.buf, \"gitdir: %s\", relative_path(repo.buf, wt->path, &tmp));\n \t}\n \n-\tfree(backlink);\n+done:\n+\tfree(git_contents);\n \tstrbuf_release(&repo);\n \tstrbuf_release(&dotgit);\n+\tstrbuf_release(&backlink);\n+\tstrbuf_release(&tmp);\n }\n \n static void repair_noop(int iserr UNUSED,\n@@ -681,6 +713,8 @@ void repair_worktree_at_path(const char *path,\n \tstruct strbuf backlink = STRBUF_INIT;\n \tstruct strbuf gitdir = STRBUF_INIT;\n \tstruct strbuf olddotgit = STRBUF_INIT;\n+\tstruct strbuf realolddotgit = STRBUF_INIT;\n+\tstruct strbuf tmp = STRBUF_INIT;\n \tchar *git_contents = NULL;\n \tconst char *repair = NULL;\n \tint err;\n@@ -710,97 +744,131 @@ void repair_worktree_at_path(const char *path,\n \t\tfn(1, realdotgit.buf, _(\"unable to locate repository; .git file broken\"), cb_data);\n \t\tgoto done;\n \t} else if (git_contents) {\n-\t\tstrbuf_addstr(&backlink, git_contents);\n+\t\tif (is_absolute_path(git_contents)) {\n+\t\t\tstrbuf_addstr(&backlink, git_contents);\n+\t\t} else {\n+\t\t\tstrbuf_addbuf(&backlink, &realdotgit);\n+\t\t\tstrbuf_strip_suffix(&backlink, \".git\");\n+\t\t\tstrbuf_addstr(&backlink, git_contents);\n+\t\t}\n \t}\n \n+\tstrbuf_realpath_forgiving(&backlink, backlink.buf, 0);\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 \t\tstrbuf_rtrim(&olddotgit);\n-\t\tif (fspathcmp(olddotgit.buf, realdotgit.buf))\n+\t\tif (is_absolute_path(olddotgit.buf)) {\n+\t\t\tstrbuf_addbuf(&realolddotgit, &olddotgit);\n+\t\t} else {\n+\t\t\tstrbuf_addf(&realolddotgit, \"%s/%s\", backlink.buf, olddotgit.buf);\n+\t\t\tstrbuf_realpath_forgiving(&realolddotgit, realolddotgit.buf, 0);\n+\t\t}\n+\t\tif (fspathcmp(realolddotgit.buf, realdotgit.buf))\n \t\t\trepair = _(\"gitdir incorrect\");\n \t}\n \n \tif (repair) {\n \t\tfn(0, gitdir.buf, repair, cb_data);\n-\t\twrite_file(gitdir.buf, \"%s\", realdotgit.buf);\n+\t\twrite_file(gitdir.buf, \"%s\", relative_path(realdotgit.buf, backlink.buf, &tmp));\n \t}\n done:\n \tfree(git_contents);\n \tstrbuf_release(&olddotgit);\n+\tstrbuf_release(&realolddotgit);\n \tstrbuf_release(&backlink);\n \tstrbuf_release(&gitdir);\n \tstrbuf_release(&realdotgit);\n \tstrbuf_release(&dotgit);\n+\tstrbuf_release(&tmp);\n }\n \n int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, timestamp_t expire)\n {\n \tstruct stat st;\n-\tchar *path;\n+\tstruct strbuf dotgit = STRBUF_INIT;\n+\tstruct strbuf gitdir = STRBUF_INIT;\n+\tstruct strbuf repo = STRBUF_INIT;\n+\tchar *path = NULL;\n+\tchar *file = NULL;\n+\tint rc = 0;\n \tint fd;\n \tsize_t len;\n \tssize_t read_result;\n \n \t*wtpath = NULL;\n-\tif (!is_directory(git_path(\"worktrees/%s\", id))) {\n+\tstrbuf_realpath(&repo, git_common_path(\"worktrees/%s\", id), 1);\n+\tstrbuf_addf(&gitdir, \"%s/gitdir\", repo.buf);\n+\tif (!is_directory(repo.buf)) {\n \t\tstrbuf_addstr(reason, _(\"not a valid directory\"));\n-\t\treturn 1;\n+\t\trc = 1;\n+\t\tgoto done;\n \t}\n-\tif (file_exists(git_path(\"worktrees/%s/locked\", id)))\n-\t\treturn 0;\n-\tif (stat(git_path(\"worktrees/%s/gitdir\", id), &st)) {\n+\tfile = xstrfmt(\"%s/locked\", repo.buf);\n+\tif (file_exists(file)) {\n+\t\tgoto done;\n+\t}\n+\tif (stat(gitdir.buf, &st)) {\n \t\tstrbuf_addstr(reason, _(\"gitdir file does not exist\"));\n-\t\treturn 1;\n+\t\trc = 1;\n+\t\tgoto done;\n \t}\n-\tfd = open(git_path(\"worktrees/%s/gitdir\", id), O_RDONLY);\n+\tfd = open(gitdir.buf, O_RDONLY);\n \tif (fd < 0) {\n \t\tstrbuf_addf(reason, _(\"unable to read gitdir file (%s)\"),\n \t\t\t    strerror(errno));\n-\t\treturn 1;\n+\t\trc = 1;\n+\t\tgoto done;\n \t}\n \tlen = xsize_t(st.st_size);\n \tpath = xmallocz(len);\n \n \tread_result = read_in_full(fd, path, len);\n+\tclose(fd);\n \tif (read_result < 0) {\n \t\tstrbuf_addf(reason, _(\"unable to read gitdir file (%s)\"),\n \t\t\t    strerror(errno));\n-\t\tclose(fd);\n-\t\tfree(path);\n-\t\treturn 1;\n-\t}\n-\tclose(fd);\n-\n-\tif (read_result != len) {\n+\t\trc = 1;\n+\t\tgoto done;\n+\t} else if (read_result != len) {\n \t\tstrbuf_addf(reason,\n \t\t\t    _(\"short read (expected %\"PRIuMAX\" bytes, read %\"PRIuMAX\")\"),\n \t\t\t    (uintmax_t)len, (uintmax_t)read_result);\n-\t\tfree(path);\n-\t\treturn 1;\n+\t\trc = 1;\n+\t\tgoto done;\n \t}\n \twhile (len && (path[len - 1] == '\\n' || path[len - 1] == '\\r'))\n \t\tlen--;\n \tif (!len) {\n \t\tstrbuf_addstr(reason, _(\"invalid gitdir file\"));\n-\t\tfree(path);\n-\t\treturn 1;\n+\t\trc = 1;\n+\t\tgoto done;\n \t}\n \tpath[len] = '\\0';\n-\tif (!file_exists(path)) {\n-\t\tif (stat(git_path(\"worktrees/%s/index\", id), &st) ||\n-\t\t    st.st_mtime <= expire) {\n+\tif (is_absolute_path(path)) {\n+\t\tstrbuf_addstr(&dotgit, path);\n+\t} else {\n+\t\tstrbuf_addf(&dotgit, \"%s/%s\", repo.buf, path);\n+\t\tstrbuf_realpath_forgiving(&dotgit, dotgit.buf, 0);\n+\t}\n+\tif (!file_exists(dotgit.buf)) {\n+\t\tfree(file);\n+\t\tfile = xstrfmt(\"%s/index\", repo.buf);\n+\t\tif (stat(file, &st) || st.st_mtime <= expire) {\n \t\t\tstrbuf_addstr(reason, _(\"gitdir file points to non-existent location\"));\n-\t\t\tfree(path);\n-\t\t\treturn 1;\n-\t\t} else {\n-\t\t\t*wtpath = path;\n-\t\t\treturn 0;\n+\t\t\trc = 1;\n+\t\t\tgoto done;\n \t\t}\n \t}\n-\t*wtpath = path;\n-\treturn 0;\n+\t*wtpath = strbuf_detach(&dotgit, NULL);\n+done:\n+\tfree(path);\n+\tfree(file);\n+\tstrbuf_release(&dotgit);\n+\tstrbuf_release(&gitdir);\n+\tstrbuf_release(&repo);\n+\treturn rc;\n }\n \n static int move_config_setting(const char *key, const char *value,\n-- \n2.46.2\n\n"},{"id":"504159","messageId":"20241006060017.171788-4-cdwhite3@pm.me","threadId":"62260","inReplyTo":"20241006060017.171788-1-cdwhite3@pm.me","subject":"[PATCH v2 3/4] worktree: sync worktree paths after gitdir move","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-06T06:01:27Z","receivedAt":"2024-10-06T06:01:32Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"When re-initializing a repository with a separate gitdir (the original\ngitdir is moved to a new location), any linked worktrees become broken\nand must be repaired to reflect the new gitdir location. For absolute\npaths, this breakage is one-way, but is both ways for relative paths\n(the `<worktree>/.git` and the `<repo>/worktrees/<id>/gitdir`).\n\nPreviously, `repair_worktrees` was being called which loops through all\nthe worktrees in the repository and updates the `<worktree>/.git` files\nto point to the new gitdir. However, when both sides of the worktrees\nare broken, the previous gitdir location is required to reestablish the\nlink.\n\nTo fix this, the function `repair_worktrees_after_gitdir_move` is\nintroduced. It takes the old gitdir path as an argument and repairs both\nsides of the worktree.\n\nThis change fixes the following test cases in t0001-init.sh:\n- re-init to move gitdir with linked worktrees\n- re-init to move gitdir within linked worktree\n\nSigned-off-by: Caleb \nWhite <cdwhite3@pm.me>\n---\n setup.c    |  2 +-\n worktree.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++\n worktree.h | 10 ++++++++++\n 3 files changed, 65 insertions(+), 1 deletion(-)\n\ndiff --git a/setup.c b/setup.c\nindex 94e79b2..7b648de 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -2420,7 +2420,7 @@ static void separate_git_dir(const char *git_dir, const char *git_link)\n \n \t\tif (rename(src, git_dir))\n \t\t\tdie_errno(_(\"unable to move %s to %s\"), src, git_dir);\n-\t\trepair_worktrees(NULL, NULL);\n+\t\trepair_worktrees_after_gitdir_move(src);\n \t}\n \n \twrite_file(git_link, \"gitdir: %s\", git_dir);\ndiff --git a/worktree.c b/worktree.c\nindex fc14e9a..b08ecce 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -650,6 +650,60 @@ void repair_worktrees(worktree_repair_fn fn, void *cb_data)\n \tfree_worktrees(worktrees);\n }\n \n+void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path)\n+{\n+\tstruct strbuf path = STRBUF_INIT;\n+\tstruct strbuf repo = STRBUF_\nINIT;\n+\tstruct strbuf gitdir = STRBUF_INIT;\n+\tstruct strbuf dotgit = STRBUF_INIT;\n+\tstruct strbuf olddotgit = STRBUF_INIT;\n+\tstruct strbuf tmp = STRBUF_INIT;\n+\n+\tif (is_main_worktree(wt))\n+\t\tgoto done;\n+\n+\tstrbuf_realpath(&repo, git_common_path(\"worktrees/%s\", wt->id), 1);\n+\tstrbuf_addf(&gitdir, \"%s/gitdir\", repo.buf);\n+\n+\tif (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0)\n+\t\tgoto done;\n+\n+\tstrbuf_rtrim(&olddotgit);\n+\tif (is_absolute_path(olddotgit.buf)) {\n+\t\tstrbuf_addbuf(&dotgit, &olddotgit);\n+\t} else {\n+\t\tstrbuf_addf(&dotgit, \"%s/worktrees/%s/%s\", old_path, wt->id, olddotgit.buf);\n+\t\tstrbuf_realpath_forgiving(&dotgit, dotgit.buf, 0);\n+\t}\n+\n+\tif (!file_exists(dotgit.buf))\n+\t\tgoto done;\n+\n+\tstrbuf_addbuf(&path, &dotgit);\n+\tstrbuf_strip_suffix(&path, \"/.git\");\n+\n+\twrite_file(dotgit.buf, \"gitdir: %s\", relative_path(repo.buf, path.buf, &tmp));\n+\tstrbuf_reset(&tmp);\n+\twrite_file(gitdir.buf, \"%s\", relative_path(dotgit.buf, repo.buf, &tmp));\n+done:\n+\tstrbu\nf_release(&path);\n+\tstrbuf_release(&repo);\n+\tstrbuf_release(&gitdir);\n+\tstrbuf_release(&dotgit);\n+\tstrbuf_release(&olddotgit);\n+\tstrbuf_release(&tmp);\n+}\n+\n+void repair_worktrees_after_gitdir_move(const char *old_path)\n+{\n+\tstruct worktree **worktrees = get_worktrees_internal(1);\n+\tstruct worktree **wt = worktrees + 1; /* +1 skips main worktree */\n+\n+\tfor (; *wt; wt++)\n+\t\trepair_worktree_after_gitdir_move(*wt, old_path);\n+\tfree_worktrees(worktrees);\n+}\n+\n static int is_main_worktree_path(const char *path)\n {\n \tstruct strbuf target = STRBUF_INIT;\ndiff --git a/worktree.h b/worktree.h\nindex 11279d0..e961186 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -131,6 +131,16 @@ typedef void (* worktree_repair_fn)(int iserr, const char *path,\n  */\n void repair_worktrees(worktree_repair_fn, void *cb_data);\n \n+/*\n+ * Repair the linked worktrees after the gitdir has been moved.\n+ */\n+void repair_worktrees_after_gitdir_move(const char *old_path);\n+\n+/*\n+ * Repair the l\ninked worktree after the gitdir has been moved.\n+ */\n+void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path);\n+\n /*\n  * Repair administrative files corresponding to the worktree at the given path.\n  * The worktree's .git file pointing at the repository must be intact for the\n-- \n2.46.2\n\n"},{"id":"504160","messageId":"20241006060017.171788-5-cdwhite3@pm.me","threadId":"62260","inReplyTo":"20241006060017.171788-1-cdwhite3@pm.me","subject":"[PATCH v2 4/4] worktree: prevent null pointer dereference","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-06T06:01:35Z","receivedAt":"2024-10-06T06:01:40Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"If worktrees is NULL, free_worktrees() should return immediately to\nprevent a null pointer dereference.\n\nSigned-off-by: Caleb White <cdwhite3@pm.me>\n---\n worktree.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex b08ecce..1cf15b0 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -28,8 +28,9 @@ void free_worktree(struct worktree *worktree)\n \n void free_worktrees(struct worktree **worktrees)\n {\n-\tint i = 0;\n-\tfor (i = 0; worktrees[i]; i++)\n+\tif  (!worktrees)\n+\t\treturn;\n+\tfor (int i = 0; worktrees[i]; i++)\n \t\tfree_worktree(worktrees[i]);\n \tfree (worktrees);\n }\n-- \n2.46.2\n\n"},{"id":"504163","messageId":"CAPig+cTuYaeGs_nsDZcXVQCu2qzt8OzWjv4spMw_8At78av5ig@mail.gmail.com","threadId":"62260","inReplyTo":"20241006060017.171788-1-cdwhite3@pm.me","subject":"Re: [PATCH v2 0/4] Link worktrees with relative paths","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-06T06:18:06Z","receivedAt":"2024-10-06T06:18:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Oct 6, 2024 at 2:00 AM Caleb White <cdwhite3@pm.me> wrote:\n> This is a resubmission attempt to try to fix the damaged\n> patch noted in previous discussions.\n\nFor the record, for those finding this message in the mailing list\narchive, v1 is at:\nhttps://lore.kernel.org/git/20241006045847.159937-1-cdwhite3@pm.me/\n"},{"id":"504178","messageId":"CAPig+cT5BHuWMrjefP9BMocE7EFXgzRyHMFbUmEZv73F4g+XEQ@mail.gmail.com","threadId":"62260","inReplyTo":"20241006060017.171788-3-cdwhite3@pm.me","subject":"Re: [PATCH v2 2/4] worktree: link worktrees with relative paths","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-06T11:05:06Z","receivedAt":"2024-10-06T11:05:20Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Oct 6, 2024 at 2:01 AM Caleb White <cdwhite3@pm.me> wrote:\n> This modifies Git’s handling of worktree linking to use relative\n> paths instead of absolute paths. Previously, when creating a worktree,\n> Git would store the absolute paths to both the main repository and the\n> linked worktrees. These hardcoded absolute paths cause breakages such\n> as when the repository is moved to a different directory or during\n> containerized development where the absolute differs between systems.\n>\n> By switching to relative paths, we help ensure that worktree links are\n> more resilient when the repository is moved. While links external to the\n> repository may still break, Git still automatically handles many common\n> scenarios, reducing the need for manual repair. This is particularly\n> useful in containerized or portable development environments, where the\n> absolute path to the repository can differ between systems. Developers\n> no longer need to reinitialize or repair worktrees after relocating the\n> repository, improving workflow efficiency and reducing breakages.\n>\n> For self-contained repositories (such as using a bare repository with\n> worktrees), where both the repository and its worktrees are located\n> within the same directory structure, using relative paths guarantees all\n> links remain functional regardless of where the directory is located.\n>\n> Signed-off-by: Caleb White <cdwhite3@pm.me>\n\nWhen you reroll, please extend the commit message to give a more\ndetailed overview of how this patch actually changes the behavior both\nat a high level and at a low level. This is especially important since\nthis patch is sufficiently long and involved that it's not easy to\nglean these details at-a-glance from the code changes themselves.\n\nFor instance, the cover letter talked about retaining correct behavior\nfor both absolute and relative paths, but this commit message mentions\nonly relative paths, so it's not obvious just from reading the commit\nmessage whether absolute paths are still supported or, if they are,\nwhat exactly that support looks like or how the user controls the\nchoice between the two path types. This sort of information should be\npresent in the commit message.\n\nSimilarly, if there is a chance that this change may break existing\ntooling and workflows, then the commit message should talk about those\nrisks, as well, and what if any remediations are available. Also, if\nthere are such risks, then Documentation/git-worktree.txt may need to\nbe updated to warn users.\n\nRegarding what you wrote above, there seems to be a good deal of\nredundancy between the first two paragraphs; combining the paragraphs\nand folding out the duplication might make the message more\nstreamlined. I do like the discussion about containerized environments\nbeing used as (at least one) justification for employing relative\npaths, and think that may be a good lead-in for the commit message.\nPlease see [1] for some helpful hints for composing a good commit\nmessage.\n\nI'm not going to review the code itself right now since I haven't been\nable to apply the patches or play around with the functionality, but I\nwanted to get the above written up since I think this series is going\nto need to be rerolled anyhow.\n\n[1]: https://lore.kernel.org/git/xmqqmsm6sc0q.fsf@gitster.g/\n"},{"id":"504179","messageId":"CAPig+cQApvzpuCBfViPD_hJjqe_poFO8uB1GapeVpCd2EWvEug@mail.gmail.com","threadId":"62260","inReplyTo":"20241006060017.171788-4-cdwhite3@pm.me","subject":"Re: [PATCH v2 3/4] worktree: sync worktree paths after gitdir move","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-06T11:12:04Z","receivedAt":"2024-10-06T11:12:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Oct 6, 2024 at 2:01 AM Caleb White <cdwhite3@pm.me> wrote:\n> When re-initializing a repository with a separate gitdir (the original\n> gitdir is moved to a new location), any linked worktrees become broken\n> and must be repaired to reflect the new gitdir location. For absolute\n> paths, this breakage is one-way, but is both ways for relative paths\n> (the `<worktree>/.git` and the `<repo>/worktrees/<id>/gitdir`).\n>\n> Previously, `repair_worktrees` was being called which loops through all\n> the worktrees in the repository and updates the `<worktree>/.git` files\n> to point to the new gitdir. However, when both sides of the worktrees\n> are broken, the previous gitdir location is required to reestablish the\n> link.\n>\n> To fix this, the function `repair_worktrees_after_gitdir_move` is\n> introduced. It takes the old gitdir path as an argument and repairs both\n> sides of the worktree.\n>\n> This change fixes the following test cases in t0001-init.sh:\n> - re-init to move gitdir with linked worktrees\n> - re-init to move gitdir within linked worktree\n\nIf I understand correctly, this patch is fixing breakage resulting\nfrom the preceding patch. Unfortunately, this approach is problematic\nsince it breaks bisectability of the project. In particular, it's not\nsufficient for the full test suite to pass only at the end of the\npatch series; rather, the entire test suite should continue to pass\nafter application of *each* patch in a series; we don't want one patch\nto break the test suite and a subsequent patch to fix it again.\n\nSo, if my understanding is correct, please put some thought into how\nto reorganize this patch series to ensure that the full test suite\npasses for each patch.\n"},{"id":"504181","messageId":"CAPig+cSdVVy4huueVQpiO_Gvn4SAXAiQj-uVnuScgfOOFJ6h0g@mail.gmail.com","threadId":"62260","inReplyTo":"20241006060017.171788-5-cdwhite3@pm.me","subject":"Re: [PATCH v2 4/4] worktree: prevent null pointer dereference","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-06T11:24:43Z","receivedAt":"2024-10-06T11:24:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Oct 6, 2024 at 2:01 AM Caleb White <cdwhite3@pm.me> wrote:\n> If worktrees is NULL, free_worktrees() should return immediately to\n> prevent a null pointer dereference.\n>\n> Signed-off-by: Caleb White <cdwhite3@pm.me>\n\nCritical questions: It is not clear why this patch is needed,\nespecially coming at the end of the series. Is there some code in a\npatch earlier in the series which might call free_worktrees() with\nNULL? If so, then this patch should come before that patch. If not,\nthen why do we need this patch at all?\n\nDevil's advocate question: Why is it the responsibility of\nfree_worktrees() to check this condition as opposed to being the\ncaller's responsibility?\n\nThe commit message should explain the need for this patch and answer\nthese questions, not just say what change is being made.\n\n> diff --git a/worktree.c b/worktree.c\n> @@ -28,8 +28,9 @@ void free_worktree(struct worktree *worktree)\n>  void free_worktrees(struct worktree **worktrees)\n>  {\n> -       int i = 0;\n> -       for (i = 0; worktrees[i]; i++)\n> +       if  (!worktrees)\n> +               return;\n> +       for (int i = 0; worktrees[i]; i++)\n>                 free_worktree(worktrees[i]);\n\nAlthough it's true that this project has recently started allowing\ndeclaration of the variable in the `for` statement, that change is\nunrelated to the stated purpose in the commit message. True, it's a\nminor thing in this case, but it causes a hiccup for reviewers when\nunrelated changes are piggybacked like this with the \"real\" change\nsince it adds noise which obscures what the reviewer should really be\nfocusing on.\n"},{"id":"504191","messageId":"ZwKoNg-HYGaohvCd@ArchLinux","threadId":"62260","inReplyTo":"20241006060017.171788-2-cdwhite3@pm.me","subject":"Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-10-06T15:09:42Z","receivedAt":"2024-10-06T15:09:37Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Sun, Oct 06, 2024 at 06:00:57AM +0000, Caleb White wrote:\n> This refactors the `infer_backlink` function to return an integer\n> result and use a pre-allocated `strbuf` for the inferred backlink\n> path, replacing the previous `char*` return type.\n> \n> This lays the groundwork for the next patch, which needs the\n> resultant backlink as a `strbuf`. There was no need to go from\n> `strbuf -> char* -> strbuf` again. This change also aligns the\n> function's signature with other `strbuf`-based functions.\n> \n\nI think we should first say why we need to add the change in the commit\nmessage which means we should express our motivation in the first. It's\nwired to say \"I have done something\" and then talk about the motivation\nwhy we do this.\n\n> Signed-off-by: Caleb White <cdwhite3@pm.me>\n> ---\n>  worktree.c | 26 ++++++++++++++------------\n>  1 file changed, 14 insertions(+), 12 deletions(-)\n> \n> diff --git a/worktree.c b/worktree.c\n> index 0f032cc..c6d2ede 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -642,10 +642,9 @@ static int is_main_worktree_path(const char *path)\n>   * be able to infer the gitdir by manually reading /path/to/worktree/.git,\n>   * extracting the <id>, and checking if <repo>/worktrees/<id> exists.\n>   */\n> -static char *infer_backlink(const char *gitfile)\n> +static int infer_backlink(st\n> ruct strbuf *inferred, const char *gitfile)\n\nThis line is so strange. Why it generates a newline here?\n\n>  {\n>  \tstruct strbuf actual = STRBUF_INIT;\n> -\tstruct strbuf inferred = STRBUF_INIT;\n>  \tconst char *id;\n>  \n>  \tif (strbuf_read_file(&actual, gitfile, 0) < 0)\n> @@ -658,17 +657,16 @@ static char *infer_backlink(const char *gitfile)\n>  \tid++; /* advance past '/' to point at <id> */\n>  \tif (!*id)\n>  \t\tgoto error;\n> -\tstrbuf_git_common_path(&inferred, the_repository, \"worktrees/%s\", id);\n> -\tif (!is_directory(inferred.buf))\n> +\tstrbuf_git_common_path(inferred, the_repository, \"worktrees/%s\", id);\n> +\tif (!is_directory(inferred->buf))\n>  \t\tgoto error;\n>  \n>  \tstrbuf_release(&actual);\n> -\treturn strbuf_detach(&inferred, NULL);\n> +\treturn 0;\n>  \n>  error:\n>  \tstrbuf_release(&actual);\n> -\tstrbuf_release(&inferred);\n\nBecause we leave the life cycle of the \"inferred\" to be outside of this\nfunction, we should not use \"strbuf_release\" to release the memory here.\nMake sense.\n\n> -\treturn NULL;\n> +\treturn 1;\n>  }\n>  \n>  /*\n> @@ -680,9 +678,10 @@ void repair_worktree_at_path(const char *path,\n>  {\n>  \tstruct strbuf dotgit = STRBUF_INIT;\n>  \tstruct strbuf realdotgit = STRBUF_INIT;\n> +\tstruct strbuf backlink = STRBUF_INIT;\n\nHere, we replace \"char *backlink\" to \"struct strbuf backlink\", we need\nto align the changed \"infer_backlink\" function. That's OK.\n\n>  \tstruct strbuf gitd\n> ir = STRBUF_INIT;\n\nAnother strange newline here.\n\n>  \tstruct strbuf olddotgit = STRBUF_INIT;\n> -\tchar *backlink = NULL;\n> +\tchar *git_contents = NULL;\n>  \tconst char *repair = NULL;\n>  \tint err;\n>  \n> @@ -698,21 +697,23 @@ 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> +\tgit_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));\n\nSo, we create a new variable \"git_contents\" here. I suspect this is a\npoor design. In the original logic, we will do the following things for\n\"backlink\".\n\n  1. Call the \"read_gitfile_gently\" function. If it encounters error, it\n     will return NULL and the \"err\" variable will be set to NON-zero.\n  2. If the value of \"err\" is 0, we would simply execute the\n     \"strbuf_addstr(&gitdir, \"%s/gitdir\", backlink)\".\n  3. If not, and the \"err\" is \"READ_GITFILE_ERR_NOT_A_REPO\". We will\n     call \"infer_backlink\" to set the \"backlink\".\n\nBecause now \"backlink\" is \"struct strbuf\", we cannot just assign it by\nusing \"xstrdup_or_null(read_gitfile_gently(...))\". So, we create a new\nvariable \"git_contents\" here.\n\nAnd we will check whether \"git_contents\" is NULL to set the value for\nthe \"backlink\".\n\nWhy not simply do the following things here (I don't think\n\"git_contents\" is a good name, however I am not familiar with the\nworktree, I cannot give some advice here).\n\n    const char *git_contents;\n    git_contents = read_gitfile_gently(...);\n    if (git_contents)\n        strbuf_addstr(&backlink, git_contents);\n\nEven more, we could enhance the logic here. If \"git_contents\" is not\nNULL, there is no need for us to check the \"err\" variable.\n\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 (infer_backlink(&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>  \t} else if (err) {\n>  \t\tfn(1, realdotgit.buf, _(\"unable to locate repository; .git file broken\"), cb_data);\n>  \t\tgoto done;\n> +\t} else if (git_conte\n> nts) {\n\nAnother strange newline here. As I have talked about above, we should\nnot check \"git_contents\" here.\n\n> +\t\tstrbuf_addstr(&backlink, git_contents);\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> @@ -726,8 +727,9 @@ 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> +\tfree(git_contents);\n>  \tstrbuf_release(&olddotgit);\n> +\tstrbuf_release(&backlink);\n>  \tstrbuf_release(&gitdir);\n>  \tstrbuf_release(&realdotgit);\n>  \tstrbuf_release(&dotgit);\n> -- \n> 2.46.2\n> \n\nThanks,\nJialuo\n"},{"id":"504192","messageId":"b5bf4d46-1357-4584-b5b0-012db4626cc3@app.fastmail.com","threadId":"62260","inReplyTo":"ZwKoNg-HYGaohvCd@ArchLinux","subject":"Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-10-06T15:13:25Z","receivedAt":"2024-10-06T15:15:48Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Sun, Oct 6, 2024, at 17:09, shejialuo wrote:\n> On Sun, Oct 06, 2024 at 06:00:57AM +0000, Caleb White wrote:\n>> This refactors the `infer_backlink` function to return an integer\n>> result and use a pre-allocated `strbuf` for the inferred backlink\n>> path, replacing the previous `char*` return type.\n>>\n>> This lays the groundwork for the next patch, which needs the\n>> resultant backlink as a `strbuf`. There was no need to go from\n>> `strbuf -> char* -> strbuf` again. This change also aligns the\n>> function's signature with other `strbuf`-based functions.\n>>\n>\n> I think we should first say why we need to add the change in the commit\n> message which means we should express our motivation in the first. It's\n> wired to say \"I have done something\" and then talk about the motivation\n> why we do this.\n>\n>> Signed-off-by: Caleb White <cdwhite3@pm.me>\n>> ---\n>>  worktree.c | 26 ++++++++++++++------------\n>>  1 file changed, 14 insertions(+), 12 deletions(-)\n>>\n>> diff --git a/worktree.c b/worktree.c\n>> index 0f032cc..c6d2ede 100644\n>> --- a/worktree.c\n>> +++ b/worktree.c\n>> @@ -642,10 +642,9 @@ static int is_main_worktree_path(const char *path)\n>>   * be able to infer the gitdir by manually reading /path/to/worktree/.git,\n>>   * extracting the <id>, and checking if <repo>/worktrees/<id> exists.\n>>   */\n>> -static char *infer_backlink(const char *gitfile)\n>> +static int infer_backlink(st\n>> ruct strbuf *inferred, const char *gitfile)\n>\n> This line is so strange. Why it generates a newline here?\n\nThe patches got corrupted by something.  See the emails from Eric\nSunshine.\n\nThis resubmit didn’t fix the issue.\n\nhttps://lore.kernel.org/git/CAPig+cTB-sA-g4cdhfEjWwY1mnbWJ41e=bOCNwp=Y8JKvpmpRg@mail.gmail.com/\n\n-- \nKristoffer Haugsbakk\n"},{"id":"504193","messageId":"ZwKuptTMf9ECd-kf@ArchLinux","threadId":"62260","inReplyTo":"20241006060017.171788-3-cdwhite3@pm.me","subject":"Re: [PATCH v2 2/4] worktree: link worktrees with relative paths","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-10-06T15:37:10Z","receivedAt":"2024-10-06T15:37:05Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Sun, Oct 06, 2024 at 06:01:17AM +0000, Caleb White wrote:\n> This modifies Git’s handling of worktree linking to use relative\n> paths instead of absolute paths. Previously, when creating a worktree,\n> Git would store the absolute paths to both the main repository and the\n> linked worktrees. These hardcoded absolute paths cause breakages such\n> as when the repository is moved to a different directory or during\n> containerized development where the absolute differs between systems.\n> \n> By switching to relative paths, we help ensure that worktree links are\n> more resilient when the repository is moved. While links external to the\n> repository may still break, Git still automatically handles many common\n> scenarios, reducing the need for manual repair. This is particularly\n> useful in containerized or portable development environments, where the\n> absolute path to the repository can differ between systems. Developers\n> no longer need to reinitialize or repair worktrees after relocating the\n> repository, improving workflow efficiency and reducing breakages.\n> \n> For self-contained repositories (such as using a bare repository with\n> worktrees), where both the repository and its worktrees are located\n> within the same directory structure, using relative paths guarantees all\n> links remain functional regardless of where the directory is located.\n> \n\nEric has already commented on this commit message. I think this commit\nhas done a lot of things which make the review painful.\n\n> Signed-off-by: Caleb White <cdwhite3@pm.me>\n> ---\n>  builtin/worktree.c           |  17 ++--\n>  t/t2408-worktree-relative.sh |  39 +++++++++\n>  worktree.c                   | 152 +++++++++++++++++++++++++----------\n>  3 files changed, 159 insertions(+), 49 deletions(-)\n>  create mode 100755 t/t2408-worktree-relative.sh\n> \n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index fc31d07..99cee56 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -414,7 +414,8 @@ static int add_worktree(const char *path, const char *refname,\n>  \t\t\tconst struct add_opts *opts)\n>  {\n>  \tstruct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT;\n> -\tstruct strbuf sb = STRBUF_INIT, realpath = STRBUF_INIT;\n> +\tstruct strbuf sb = STRBUF_INIT, sb_tmp = STRBUF_INIT;\n> +\tstruct strbuf sb_path_realpath = STRBUF_INIT, sb_repo_realpath = STRBUF_INIT;\n>  \tconst char *name;\n>  \tstruct strvec child_env = STRVEC_INIT;\n>  \tunsigned int counter = 0;\n> @@ -490,11 +491,11 @@ static int add_worktree(const char *path, const char *refname,\n>  \n>  \tstrbuf_reset(&sb);\n>  \tstrbuf_addf(&sb, \"%s/gitdir\", sb_repo.buf);\n> -\tstrbuf_realpath(&realpath, sb_git.buf, 1);\n> -\twrite_file(sb.buf, \"%s\", realpath.buf);\n> -\tstrbuf_realpath(&realpath, repo_get_common_dir(the_repository), 1);\n> -\twrite_file(sb_git.buf, \"gitdir: %s/worktrees/%s\",\n> -\t\t   realpath.buf, name);\n> +\tstrbuf_realpath(&sb_path_realpath, path, 1);\n> +\tstrbuf_realpath(&sb_repo_realpath, sb_repo.buf, 1);\n> +\twrite_file(sb.buf, \"%s/.git\", relative_path(sb_path_realpath.buf, sb_repo_realpath.buf, &sb_tmp));\n> +\tstrbuf_reset(&sb_tmp);\n\nDo we need reset the \"sb_tmp\"? I guess we do not need, \"relative_path\"\ndoes not care about \"sb_tmp\". It will always reset the value of the\n\"sb_tmp\". So, we may delete this line.\n\n[snip]\n\n> diff --git a/worktree.c b/worktree.c\n> index c6d2ede..fc14e9a 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -373,18 +379,30 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,\n>  void update_worktree_location(struct worktree *wt, const char *path_)\n>  {\n>  \tstruct strbuf path = STRBUF_INIT;\n> +\tstruct strbuf repo = STRBUF_INIT;\n> +\tstruct strbuf tmp = STRBUF_INIT;\n> +\tchar *file = NULL;\n>  \n>  \tif (is_main_worktree(wt))\n>  \t\tBUG(\"can't relocate main worktree\");\n>  \n> +\tstrbuf_realpath(&repo, git_common_path(\"worktrees/%s\", wt->id), 1);\n>  \tstrbuf_realpath(&path, path_, 1);\n>  \tif (fspathcmp(wt->path, path.buf)) {\n> -\t\twrite_file(git_common_path(\"worktrees/%s/gitdir\", wt->id),\n> -\t\t\t   \"%s/.git\", path.buf);\n> +\t\tfile = xstrfmt(\"%s/gitdir\", repo.buf);\n> +\t\twrite_file(file, \"%s/.git\", relative_path(path.buf, repo.buf, &tmp));\n> +\t\tfree(file);\n> +\t\tstrbuf_reset(&tmp);\n> +\t\tfile = xstrfmt(\"%s/.git\", path.buf);\n\nStill, we do not need to call \"strbuf_reset\" again for \"tmp\". But there\nis another question here. Should we define the \"file\" just in this \"if\"\nblock and free \"file\" also in the block?\n\nAnd I don't think it's a good idea to use \"xstrfmt\". Here, we need to\nallocate two times and free two times. Why not just define a \"struct\nstrbuf\" and the use \"strbuf_*\" method here?\n\n> +\t\twrite_file(file, \"gitdir: %s\", relative_path(repo.buf, path.buf, &tmp));\n> +\n>  \t\tfree(wt->path);\n>  \t\twt->path = strbuf_detach(&path, NULL);\n>  \t}\n> +\tfree(file);\n>  \tstrbuf_release(&path);\n> +\tstrbuf_release(&repo);\n> +\tstrbuf_release(&tmp);\n>  }\n>  \n> @@ -564,38 +582,52 @@ static void repair_gitfile(struct worktree *wt,\n>  {\n>  \n\n[snip]\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> +\tgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));\n\nWhy here we need to use \"xstrdup_or_null\". The life cycle of the\n\"git_contents\" variable is in the \"repair_gitfile\" function.\n\n[snip]\n\nI omit the review for the later code, there are too many codes to\nreview. And due to my limited knowledge about worktree, the overhead\nwill be too big for me.\n\nThanks,\nJialuo\n"},{"id":"504202","messageId":"CAPig+cSe0ykN=Mmb8Nwp112ROOCuhzzP0jofvL91PsVi8W4dHg@mail.gmail.com","threadId":"62260","inReplyTo":"20241006060017.171788-2-cdwhite3@pm.me","subject":"Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-06T18:16:22Z","receivedAt":"2024-10-06T18:16:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Oct 6, 2024 at 2:01 AM Caleb White <cdwhite3@pm.me> wrote:\n> This refactors the `infer_backlink` function to return an integer\n> result and use a pre-allocated `strbuf` for the inferred backlink\n> path, replacing the previous `char*` return type.\n>\n> This lays the groundwork for the next patch, which needs the\n> resultant backlink as a `strbuf`. There was no need to go from\n> `strbuf -> char* -> strbuf` again. This change also aligns the\n> function's signature with other `strbuf`-based functions.\n\nRegarding aligning the signature with other strbuf-based functions...\n\n> Signed-off-by: Caleb White <cdwhite3@pm.me>\n> ---\n> diff --git a/worktree.c b/worktree.c\n> @@ -642,10 +642,9 @@ static int is_main_worktree_path(const char *path)\n> -static char *infer_backlink(const char *gitfile)\n> +static int infer_backlink(struct strbuf *inferred, const char *gitfile)\n\n... it's subjective, but I find that placing the strbuf as the first\nargument makes the purpose of the function less clear. The direct\npurpose of all (or the majority of) functions in strbuf.h is to\noperate on strbufs, thus it makes sense for the strbuf to be the first\nargument (just like `this` is the invisible first argument of instance\nmethods in C++ which operate on an instance of the class). However,\ninfer_backlink()'s purpose isn't to operate on a strbuf; the fact that\nthe original signature neither accepted nor returned a strbuf bears\nthat out. The really important input to this function is `gitfile`,\nthus it makes sense for this argument to come first. The strbuf which\nthis patch makes it use is a mere implementation detail (a\nmicro-optimization) which doesn't otherwise change the function's\noriginal purpose, thus it belongs in a less prominent position in the\nargument list.\n\n> @@ -658,17 +657,16 @@ static char *infer_backlink(const char *gitfile)\n> -       strbuf_git_common_path(&inferred, the_repository, \"worktrees/%s\", id);\n> -       if (!is_directory(inferred.buf))\n> +       strbuf_git_common_path(inferred, the_repository, \"worktrees/%s\", id);\n> +       if (!is_directory(inferred->buf))\n>                 goto error;\n>         strbuf_release(&actual);\n> -       return strbuf_detach(&inferred, NULL);\n> +       return 0;\n\nOn success, we now return zero...\n\n>  error:\n>         strbuf_release(&actual);\n> -       strbuf_release(&inferred);\n> -       return NULL;\n> +       return 1;\n\n... and on failure we return 1. To avoid confusing readers who are\nfamiliar with project idioms, it would probably be better to instead\nfollow the convention used in most of the rest of the project (and in\nUnix system calls in general) of returning -1.\n\nHowever...\n\n> @@ -698,21 +697,23 @@ void repair_worktree_at_path(const char *path,\n> -               if (!(backlink = infer_backlink(realdotgit.buf))) {\n> +               if (infer_backlink(&backlink, realdotgit.buf)) {\n>                         fn(1, realdotgit.buf, _(\"unable to locate repository; .git file does not reference a repository\"), cb_data);\n>                         goto done;\n>                 }\n\n... this code now becomes more than a little confusing to read. It\nsays \"if infer_backlink then signal error\", which sounds rather\nbackward and leaves the reader scratching his or her head. (\"Why\nsignal error if the function succeeded?\"). Hence, infer_backlink()\nshould probably return 1 on success and 0 on failure, which will make\nthis code read more idiomatically:\n\n    if (!infer_backlink(realdotgit.buf, &backlink)) {\n        ...signal error...\n"},{"id":"504203","messageId":"CAPig+cQC45_Pofw31h2RSydj_CjqpfajLLa5O_sR9m0AQh5OVw@mail.gmail.com","threadId":"62260","inReplyTo":"ZwKoNg-HYGaohvCd@ArchLinux","subject":"Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-06T18:41:22Z","receivedAt":"2024-10-06T18:41:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Oct 6, 2024 at 11:09 AM shejialuo <shejialuo@gmail.com> wrote:\n> On Sun, Oct 06, 2024 at 06:00:57AM +0000, Caleb White wrote:\n> > -     backlink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));\n> > +     git_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));\n>\n> So, we create a new variable \"git_contents\" here. I suspect this is a\n> poor design. In the original logic, we will do the following things for\n> \"backlink\".\n>\n>   1. Call the \"read_gitfile_gently\" function. If it encounters error, it\n>      will return NULL and the \"err\" variable will be set to NON-zero.\n>   2. If the value of \"err\" is 0, we would simply execute the\n>      \"strbuf_addstr(&gitdir, \"%s/gitdir\", backlink)\".\n>   3. If not, and the \"err\" is \"READ_GITFILE_ERR_NOT_A_REPO\". We will\n>      call \"infer_backlink\" to set the \"backlink\".\n>\n> Because now \"backlink\" is \"struct strbuf\", we cannot just assign it by\n> using \"xstrdup_or_null(read_gitfile_gently(...))\". So, we create a new\n> variable \"git_contents\" here.\n>\n> And we will check whether \"git_contents\" is NULL to set the value for\n> the \"backlink\".\n\nThanks for thinking through this logic. I have a few additional comments...\n\n> Why not simply do the following things here (I don't think\n> \"git_contents\" is a good name, however I am not familiar with the\n> worktree, I cannot give some advice here).\n\nI found the name \"git_contents\" clear enough and understood its\npurpose at-a-glance, so I think it's a reasonably good choice. A\nslightly better name might be \"gitfile_contents\" or perhaps\n\"dotgit_contents\" for consistency with other similarly-named variables\nin this function.\n\n>     const char *git_contents;\n>     git_contents = read_gitfile_gently(...);\n>     if (git_contents)\n>         strbuf_addstr(&backlink, git_contents);\n>\n> Even more, we could enhance the logic here.\n\nUpon reading this patch, I had a similar thought about this, that it\nwould be more reflective of the original code to set \"backlink\" early\nhere rather than waiting until the end of the if-else-cascade.\nHowever, upon reflection, I don't mind that setting \"backlink\" is\ndelayed until after the if-else-chain, though I think it deserves at\nleast one change which I will explain below.\n\n> If \"git_contents\" is not\n> NULL, there is no need for us to check the \"err\" variable.\n\nI'm not sure we would want to change this; the existing logic seems\nclear enough.\n\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 (infer_backlink(&backlink, realdotgit.buf)) {\n> >                       fn(1, realdotgit.buf, _(\"unable to locate repository; .git file does not reference a repository\"), cb_data);\n> >                       goto done;\n> >               }\n> >       } else if (err) {\n> >               fn(1, realdotgit.buf, _(\"unable to locate repository; .git file broken\"), cb_data);\n> >               goto done;\n> > +     } else if (git_contents) {\n> > +             strbuf_addstr(&backlink, git_contents);\n> >       }\n\nIt certainly makes sense to check whether \"git_contents\" is NULL\nbefore trying to copy it into the \"backlink\" strbuf, however, if\n\"git_contents\" is NULL here, then what does that mean? What does it\nmean to leave \"backlink\" empty? The only way (presumably) we get this\nfar is if read_gitfile_gently() succeeded, so (presumably)\n\"git_contents\" should not be NULL. Thus, rather than conditionally\ncopying into \"backlink\", we should instead indicate clearly via BUG()\nthat it should be impossible for \"git_contents\" to be NULL. So, rather\nthan making this part of the existing if-else-cascade, we should do\nthis as a standalone `if`:\n\n    if (!git_contents)\n        BUG(...);\n    strbuf_addstr(&backlink, git_contents);\n"},{"id":"504212","messageId":"Iucjq772bnnbBMdH1gDVwBhFAV3BVYQ7ZUdnWArbWM1wq_9x3-OTG8DsOUatJh7GORNRYWDDZaVVlSj4VQy1y4RysqlFKvdpNiVswDdfQ5A=@pm.me","threadId":"62260","inReplyTo":"CAPig+cT5BHuWMrjefP9BMocE7EFXgzRyHMFbUmEZv73F4g+XEQ@mail.gmail.com","subject":"Re: [PATCH v2 2/4] worktree: link worktrees with relative paths","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-06T22:37:10Z","receivedAt":"2024-10-06T22:37:15Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Sunday, October 6th, 2024 at 06:05, Eric Sunshine <sunshine@sunshineco.com> wrote:\n\n> When you reroll, please extend the commit message to give a more\n> detailed overview of how this patch actually changes the behavior both\n> at a high level and at a low level. This is especially important since\n> this patch is sufficiently long and involved that it's not easy to\n> glean these details at-a-glance from the code changes themselves.\n\nI will do that, I was not sure how much low level detail I should dive into.\n\n> Regarding what you wrote above, there seems to be a good deal of\n> redundancy between the first two paragraphs; combining the paragraphs\n> and folding out the duplication might make the message more\n> streamlined. I do like the discussion about containerized environments\n> being used as (at least one) justification for employing relative\n> paths, and think that may be a good lead-in for the commit message.\n> Please see [1] for some helpful hints for composing a good commit\n> message.\n\nThanks, I will clean up the redundancy and add more detail to the commit."},{"id":"504213","messageId":"YYay6Jp5c84h3Tx-LViX4teN5KSBX5PL3_WcOFRzUooFrpJpcBD2SRWf1EQjvrMZsI6zkKyPd7JbWt3Ed3OIxCdkg17fY2qvmT4-B6a0RAE=@pm.me","threadId":"62260","inReplyTo":"CAPig+cQApvzpuCBfViPD_hJjqe_poFO8uB1GapeVpCd2EWvEug@mail.gmail.com","subject":"Re: [PATCH v2 3/4] worktree: sync worktree paths after gitdir move","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-06T22:41:33Z","receivedAt":"2024-10-06T22:41:38Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Sunday, October 6th, 2024 at 06:12, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> If I understand correctly, this patch is fixing breakage resulting\n> from the preceding patch. Unfortunately, this approach is problematic\n> since it breaks bisectability of the project. In particular, it's not\n> sufficient for the full test suite to pass only at the end of the\n> patch series; rather, the entire test suite should continue to pass\n> after application of each patch in a series; we don't want one patch\n> to break the test suite and a subsequent patch to fix it again.\n>\n> So, if my understanding is correct, please put some thought into how\n> to reorganize this patch series to ensure that the full test suite\n> passes for each patch.\n\nYes, there was one edge case that broke and this patch fixed. But I\nunderstand what you mean about the bisectability. I was trying to come\nup with ways to split up the commits and this seemed like a good spot as\nit just introduced new functions with minimal changes elsewhere. But\nthis can be squashed into the previous patch."},{"id":"504215","messageId":"CAPig+cTKjebbpuy-SCLWd0QMKfHhRyJ81oESNjsUoPDLLNpNWQ@mail.gmail.com","threadId":"62260","inReplyTo":"YYay6Jp5c84h3Tx-LViX4teN5KSBX5PL3_WcOFRzUooFrpJpcBD2SRWf1EQjvrMZsI6zkKyPd7JbWt3Ed3OIxCdkg17fY2qvmT4-B6a0RAE=@pm.me","subject":"Re: [PATCH v2 3/4] worktree: sync worktree paths after gitdir move","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-06T22:48:16Z","receivedAt":"2024-10-06T22:48:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Oct 6, 2024 at 6:41 PM Caleb White <cdwhite3@pm.me> wrote:\n> On Sunday, October 6th, 2024 at 06:12, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > So, if my understanding is correct, please put some thought into how\n> > to reorganize this patch series to ensure that the full test suite\n> > passes for each patch.\n>\n> Yes, there was one edge case that broke and this patch fixed. But I\n> understand what you mean about the bisectability. I was trying to come\n> up with ways to split up the commits and this seemed like a good spot as\n> it just introduced new functions with minimal changes elsewhere. But\n> this can be squashed into the previous patch.\n\nI haven't yet pored over the code in-depth, so I don't know if it is\neven possible, but it's typically very much preferred by reviewers if\nyou can present a series as smaller, simpler, easier-to-digest patches\nthan large monolithic ones. So, it would be ideal if you could figure\nout some good split points (especially since patch [2/4] is already\nuncomfortably large for a reviewer). But sometimes it's just not\npossible to find good splits, so a large patch may be the only choice.\n"},{"id":"504216","messageId":"VKrLGkXtD_CWHlQJRx_JqPn9sFsjKL88fRnGvA2UcV_gvQxyv8kmIx9M6BkPRfAx9paB7KrjkL9XAE8P2P2EPJnciBN4F6LAAc176NzLh6U=@pm.me","threadId":"62260","inReplyTo":"CAPig+cSdVVy4huueVQpiO_Gvn4SAXAiQj-uVnuScgfOOFJ6h0g@mail.gmail.com","subject":"Re: [PATCH v2 4/4] worktree: prevent null pointer dereference","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-06T23:03:24Z","receivedAt":"2024-10-06T23:03:31Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Sunday, October 6th, 2024 at 06:24, Eric Sunshine <sunshine@sunshineco.com> wrote: \n\n> Critical questions: It is not clear why this patch is needed,\n> especially coming at the end of the series. Is there some code in a\n> patch earlier in the series which might call free_worktrees() with\n> NULL? If so, then this patch should come before that patch. If not,\n> then why do we need this patch at all?\n\nWhen I was working through different solution for the 3rd patch, I\nencountered this issue and this was the fix for that (granted, I\ncould've handled this on the caller side). It turned out that I had\nto go a different direction and this was no longer needed, but I\nfigured it wouldn't hurt to leave this in which is why it's the\nlast patch. However, I can just drop this if you want.\n\n> Devil's advocate question: Why is it the responsibility of\n> free_worktrees() to check this condition as opposed to being the\n> caller's responsibility?\n\nSometimes it's cleaner to write a check once on the callee side\ninstead of all callers having to duplicate the same check. This\nalso follows the same pattern as free_worktree():\n\n```\nvoid free_worktree(struct worktree *worktree)\n{\n\tif (!worktree)\n\t\treturn;\n```\n\n\n> The commit message should explain the need for this patch and answer\n> these questions, not just say what change is being made.\n\nWill do (unless we decide to drop this).\n\n> Although it's true that this project has recently started allowing\n> declaration of the variable in the `for` statement, that change is\n> unrelated to the stated purpose in the commit message. True, it's a\n> minor thing in this case, but it causes a hiccup for reviewers when\n> unrelated changes are piggybacked like this with the \"real\" change\n> since it adds noise which obscures what the reviewer should really be\n> focusing on.\n\nI didn't change this originally, but then the build process threw errors\nthat I had code before declarations (because I placed the if condition at\nthe start of the function). I could've moved the if condition past the\ndeclaration, but it seemed weird to need to declare a variable if the\nfunction is just going to immediately return due to a NULL pointer.\n\nIf we decide to keep this patch then I can keep the separate declaration."},{"id":"504217","messageId":"Gs20QHTpaeb8S4LJC8MapFaP1PN9dJQFgv2XxFKSMDqJV6NvYQ_2aD79u-Yls5ddXmLECCbljzYUdzpPr10QbGKUu_BdP2ui7xo7m-UY8HM=@pm.me","threadId":"62260","inReplyTo":"CAPig+cTKjebbpuy-SCLWd0QMKfHhRyJ81oESNjsUoPDLLNpNWQ@mail.gmail.com","subject":"Re: [PATCH v2 3/4] worktree: sync worktree paths after gitdir move","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-06T23:13:44Z","receivedAt":"2024-10-06T23:13:49Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Sunday, October 6th, 2024 at 17:48, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> I haven't yet pored over the code in-depth, so I don't know if it is\n> even possible, but it's typically very much preferred by reviewers if\n> you can present a series as smaller, simpler, easier-to-digest patches\n> than large monolithic ones. So, it would be ideal if you could figure\n> out some good split points (especially since patch [2/4] is already\n> uncomfortably large for a reviewer). But sometimes it's just not\n> possible to find good splits, so a large patch may be the only choice.\n\nThere's really not any other good split points because it's\nan all or nothing kind of thing. All of these changes need to be in place\nat the same time or there's some edge cases that are going to fail.\n\nI suppose I could try to split the *reading* of the absolute/relative paths\nseparate from the *writing* of the relative paths. However, I'm not\nsure if this would be worth the trouble as most places that read from\nthe files also write to the files."},{"id":"504218","messageId":"CAPig+cSqWomvpgAGUCCaxHG9UTy+JecRndM9ynwdBgUoLYAXTw@mail.gmail.com","threadId":"62260","inReplyTo":"VKrLGkXtD_CWHlQJRx_JqPn9sFsjKL88fRnGvA2UcV_gvQxyv8kmIx9M6BkPRfAx9paB7KrjkL9XAE8P2P2EPJnciBN4F6LAAc176NzLh6U=@pm.me","subject":"Re: [PATCH v2 4/4] worktree: prevent null pointer dereference","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-06T23:24:17Z","receivedAt":"2024-10-06T23:24:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Oct 6, 2024 at 7:03 PM Caleb White <cdwhite3@pm.me> wrote:\n> On Sunday, October 6th, 2024 at 06:24, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > Critical questions: It is not clear why this patch is needed,\n> > especially coming at the end of the series. Is there some code in a\n> > patch earlier in the series which might call free_worktrees() with\n> > NULL? If so, then this patch should come before that patch. If not,\n> > then why do we need this patch at all?\n>\n> When I was working through different solution for the 3rd patch, I\n> encountered this issue and this was the fix for that (granted, I\n> could've handled this on the caller side). It turned out that I had\n> to go a different direction and this was no longer needed, but I\n> figured it wouldn't hurt to leave this in which is why it's the\n> last patch. However, I can just drop this if you want.\n\nReviewers are a limited resource on this project[1], so it's ideal if\nsubmissions can be as reviewer-friendly as possible. Extraneous\npatches, unnecessary or unrelated changes to surrounding code, etc.\nall make a patch series more onerous to review, thus are best avoided.\n(This concern prompted all the review comments I left on this patch.)\nSo, let's drop this patch since it adds no value to either this series\nor to the existing codebase. If someone needs such a change later on,\nthey can resurrect the change.\n\n[1] There are far more people submitting patches to the project than\nreviewing them. For instance, according to Junio's latest \"What's\nCooking\" report[2], the patch I submitted[3] a couple weeks ago to fix\n\"git worktree repair\" to handle manual copy operations is still\nawaiting review. (Since you've now been living in the worktree code a\nbit and have had to digest the \"repair\" logic, perhaps you'd be\ninterested in reviewing that patch?)\n\n[2]: see the \"es/worktree-repair-copied\" entry at\nhttps://lore.kernel.org/git/xmqq7cancmoj.fsf@gitster.g/\n\n[3]: https://lore.kernel.org/git/20240923075416.54289-1-ericsunshine@charter.net/\n\n> > Although it's true that this project has recently started allowing\n> > declaration of the variable in the `for` statement, that change is\n> > unrelated to the stated purpose in the commit message. True, it's a\n> > minor thing in this case, but it causes a hiccup for reviewers when\n> > unrelated changes are piggybacked like this with the \"real\" change\n> > since it adds noise which obscures what the reviewer should really be\n> > focusing on.\n>\n> I didn't change this originally, but then the build process threw errors\n> that I had code before declarations (because I placed the if condition at\n> the start of the function). I could've moved the if condition past the\n> declaration, but it seemed weird to need to declare a variable if the\n> function is just going to immediately return due to a NULL pointer.\n\nSince this project is still following older style rules about\ndeclaring all variables up-front, it's not unusual to declare\nvariables which don't necessarily get used in some code path (even\nthough it might feel weird for someone who habitually declares\nvariables only as and where needed).\n"},{"id":"504219","messageId":"CAPig+cTvi18WmEM=wGNfoB6nHDh3zSpoSXWxQLBYwWvrOF3N+w@mail.gmail.com","threadId":"62260","inReplyTo":"Gs20QHTpaeb8S4LJC8MapFaP1PN9dJQFgv2XxFKSMDqJV6NvYQ_2aD79u-Yls5ddXmLECCbljzYUdzpPr10QbGKUu_BdP2ui7xo7m-UY8HM=@pm.me","subject":"Re: [PATCH v2 3/4] worktree: sync worktree paths after gitdir move","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-06T23:27:54Z","receivedAt":"2024-10-06T23:28:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Oct 6, 2024 at 7:13 PM Caleb White <cdwhite3@pm.me> wrote:\n> On Sunday, October 6th, 2024 at 17:48, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > I haven't yet pored over the code in-depth, so I don't know if it is\n> > even possible, but it's typically very much preferred by reviewers if\n> > you can present a series as smaller, simpler, easier-to-digest patches\n> > than large monolithic ones. So, it would be ideal if you could figure\n> > out some good split points (especially since patch [2/4] is already\n> > uncomfortably large for a reviewer). But sometimes it's just not\n> > possible to find good splits, so a large patch may be the only choice.\n>\n> There's really not any other good split points because it's\n> an all or nothing kind of thing. All of these changes need to be in place\n> at the same time or there's some edge cases that are going to fail.\n>\n> I suppose I could try to split the *reading* of the absolute/relative paths\n> separate from the *writing* of the relative paths. However, I'm not\n> sure if this would be worth the trouble as most places that read from\n> the files also write to the files.\n\nIf that's the case, then it probably wouldn't help to split it up in\nthat fashion. Artificial splits like the one you describe are very\nlikely to be more confusing for reviewers than helpful.\n"},{"id":"504220","messageId":"Exjb4s8fML6YqFE6Nkf7SQoyvOJxinkwuk-V_TRJvjl5yRxDrrn3rRsPHsnmXqfuu2zFasuP26eO1A8MfNo7shKbIlj_YsPx-mzvelinkPw=@pm.me","threadId":"62260","inReplyTo":"ZwKoNg-HYGaohvCd@ArchLinux","subject":"Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-06T23:47:31Z","receivedAt":"2024-10-06T23:47:37Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Sunday, October 6th, 2024 at 10:09, shejialuo <shejialuo@gmail.com> wrote:\n> I think we should first say why we need to add the change in the commit\n> message which means we should express our motivation in the first. It's\n> wired to say \"I have done something\" and then talk about the motivation\n> why we do this.\n\nI can reword this commit message.\n\n> Because we leave the life cycle of the \"inferred\" to be outside of this\n> function, we should not use \"strbuf_release\" to release the memory here.\n> Make sense.\n\nYes, however, I just realized that I should likely reset the strbuf when it\nenters the function (or before it is written to) which is similar to what\nthe relative_path() function does.\n\n> So, we create a new variable \"git_contents\" here. I suspect this is a\n> poor design. In the original logic, we will do the following things for\n> \"backlink\".\n>\n> 1. Call the \"read_gitfile_gently\" function. If it encounters error, it\n> will return NULL and the \"err\" variable will be set to NON-zero.\n> 2. If the value of \"err\" is 0, we would simply execute the\n> \"strbuf_addstr(&gitdir, \"%s/gitdir\", backlink)\".\n> 3. If not, and the \"err\" is \"READ_GITFILE_ERR_NOT_A_REPO\". We will\n> call \"infer_backlink\" to set the \"backlink\".\n>\n> Because now \"backlink\" is \"struct strbuf\", we cannot just assign it by\n> using \"xstrdup_or_null(read_gitfile_gently(...))\". So, we create a new\n> variable \"git_contents\" here.\n>\n> And we will check whether \"git_contents\" is NULL to set the value for\n> the \"backlink\".\n>\n> Why not simply do the following things here (I don't think\n> \"git_contents\" is a good name, however I am not familiar with the\n> worktree, I cannot give some advice here).\n>\n> const char *git_contents;\n> git_contents = read_gitfile_gently(...);\n> if (git_contents)\n> strbuf_addstr(&backlink, git_contents);\n>\n> Even more, we could enhance the logic here. If \"git_contents\" is not\n> NULL, there is no need for us to check the \"err\" variable.\n\nI was trying to not make huge changes to the logic flow, but I suppose I\ncould revisit this if desired. I can likely move the `if(git_contents)`\nto the start instead of being at the end. I was not aware that if an err\noccurred that the function returned NULL, I thought that perhaps there was\nthe possibility of git_contents being filled with something and an err still\noccurring."},{"id":"504221","messageId":"I3BmSHTyOELt2DzfSaLhRYLouu5iAPZIZGZ2Ne73AygO35CM0mq44INa68t6VD4XV61DgzbrfUW0m8fivd3N9Rejgm-tecXQHXQRs1BP9CQ=@pm.me","threadId":"62260","inReplyTo":"ZwKuptTMf9ECd-kf@ArchLinux","subject":"Re: [PATCH v2 2/4] worktree: link worktrees with relative paths","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-06T23:57:22Z","receivedAt":"2024-10-06T23:57:27Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Sunday, October 6th, 2024 at 10:37, shejialuo <shejialuo@gmail.com> wrote\n> Eric has already commented on this commit message. I think this commit\n> has done a lot of things which make the review painful.\n\nMy apologies, I will do better on this commit message.\n\n> Do we need reset the \"sb_tmp\"? I guess we do not need, \"relative_path\"\n> does not care about \"sb_tmp\". It will always reset the value of the\n> \"sb_tmp\". So, we may delete this line.\n\nYou are right, this is reset by relative_path(). I originally encountered\na bug and I thought not resetting this strbuf between relative_path() calls\nwas the cause but it must have been something else that I fixed.\n\n> Still, we do not need to call \"strbuf_reset\" again for \"tmp\". But there\n> is another question here. Should we define the \"file\" just in this \"if\"\n> block and free \"file\" also in the block?\n\nThe style this code uses seems to place most / all of the declarations at\nthe top of the function and frees at the bottom so I think this fits in.\n\n> And I don't think it's a good idea to use \"xstrfmt\". Here, we need to\n> allocate two times and free two times. Why not just define a \"struct\n> strbuf\" and the use \"strbuf_*\" method here?\n\nI can use strbufs, I just wasn't sure if I really needed a strbuf for\neach of the paths and was just trying to reuse a var.\n\n> > strbuf_realpath(&repo, git_common_path(\"worktrees/%s\", wt->id), 1);\n> > strbuf_addf(&dotgit, \"%s/.git\", wt->path);\n> > - backlink = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));\n> > + git_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));\n> \n\n> \n\n> Why here we need to use \"xstrdup_or_null\". The life cycle of the\n> \"git_contents\" variable is in the \"repair_gitfile\" function.\n\nThis what the existing code used and I saw no reason to change it...\n"},{"id":"504225","messageId":"o7jQV4JUYr2iQtErKFhCrjCySiGn7_I18gmdInKbOC9TAsvqazG0O_fPUtiWZ5UfiGLr25OZ5xvqIhfOIwCG-s1RGgri-BJIdHqiokIw4z0=@pm.me","threadId":"62260","inReplyTo":"CAPig+cQC45_Pofw31h2RSydj_CjqpfajLLa5O_sR9m0AQh5OVw@mail.gmail.com","subject":"Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-07T02:26:51Z","receivedAt":"2024-10-07T02:26:55Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Sunday, October 6th, 2024 at 13:41, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> I found the name \"git_contents\" clear enough and understood its\n> purpose at-a-glance, so I think it's a reasonably good choice. A\n> slightly better name might be \"gitfile_contents\" or perhaps\n> \"dotgit_contents\" for consistency with other similarly-named variables\n> in this function.\n\nI will rename to `dotgit_contents`.\n\n> It certainly makes sense to check whether \"git_contents\" is NULL\n> before trying to copy it into the \"backlink\" strbuf, however, if\n> \"git_contents\" is NULL here, then what does that mean? What does it\n> mean to leave \"backlink\" empty? The only way (presumably) we get this\n> far is if read_gitfile_gently() succeeded, so (presumably)\n> \"git_contents\" should not be NULL. Thus, rather than conditionally\n> copying into \"backlink\", we should instead indicate clearly via BUG()\n> that it should be impossible for \"git_contents\" to be NULL. So, rather\n> than making this part of the existing if-else-cascade, we should do\n> this as a standalone `if`:\n> \n\n> if (!git_contents)\n> BUG(...);\n> strbuf_addstr(&backlink, git_contents);\n\nWe can't use BUG because this is handled as part of the err\nconditions. The contents can be NULL and `backlink` could be\nfilled with the inferred backlink. I moved this to the top\nand I think it reads better."},{"id":"504226","messageId":"LyrqJt-X7sHVSKc-97V4-hTsDiIuz8ucEUMDtLCPWAN15k31EBjZnbeuJ2PldcCVvZaPyudvgb3H00T6uZQsI-XON7Ku8jNFjkfRCgrdr8U=@pm.me","threadId":"62260","inReplyTo":"CAPig+cSe0ykN=Mmb8Nwp112ROOCuhzzP0jofvL91PsVi8W4dHg@mail.gmail.com","subject":"Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-07T02:42:11Z","receivedAt":"2024-10-07T02:42:18Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Sunday, October 6th, 2024 at 13:16, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> ... it's subjective, but I find that placing the strbuf as the first\n> argument makes the purpose of the function less clear. The direct\n> purpose of all (or the majority of) functions in strbuf.h is to\n> operate on strbufs, thus it makes sense for the strbuf to be the first\n> argument (just like `this` is the invisible first argument of instance\n> methods in C++ which operate on an instance of the class). However,\n> infer_backlink()'s purpose isn't to operate on a strbuf; the fact that\n> the original signature neither accepted nor returned a strbuf bears\n> that out. The really important input to this function is `gitfile`,\n> thus it makes sense for this argument to come first. The strbuf which\n> this patch makes it use is a mere implementation detail (a\n> micro-optimization) which doesn't otherwise change the function's\n> original purpose, thus it belongs in a less prominent position in the\n> argument list.\n\nI can reverse the arguments.\n\n> ... this code now becomes more than a little confusing to read. It\n> says \"if infer_backlink then signal error\", which sounds rather\n> backward and leaves the reader scratching his or her head. (\"Why\n> signal error if the function succeeded?\"). Hence, infer_backlink()\n> should probably return 1 on success and 0 on failure, which will make\n> this code read more idiomatically:\n> \n\n> if (!infer_backlink(realdotgit.buf, &backlink)) {\n> ...signal error...\n\nThis was my first thought, however, on unix it is fairly standard\nto return 0 if successful and a non-zero int if there's an error.\nI don't mind updating, but I want to follow what makes the most\nsense and would be most expected."},{"id":"504227","messageId":"5W8yoVL8G4LGoKUVNQNecXZFYtWnQeA71z8hQI4C15XzaPjvJFPzMA2x5DDmTRj4P-7WFYXaJ9F_tLIpG2N8JNiHlWm_cbMHC_KNjO0-lnM=@pm.me","threadId":"62260","inReplyTo":"CAPig+cSqWomvpgAGUCCaxHG9UTy+JecRndM9ynwdBgUoLYAXTw@mail.gmail.com","subject":"Re: [PATCH v2 4/4] worktree: prevent null pointer dereference","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-07T03:09:42Z","receivedAt":"2024-10-07T03:09:47Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Sunday, October 6th, 2024 at 18:24, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> Reviewers are a limited resource on this project[1], so it's ideal if\n> submissions can be as reviewer-friendly as possible. Extraneous\n> patches, unnecessary or unrelated changes to surrounding code, etc.\n> all make a patch series more onerous to review, thus are best avoided.\n> (This concern prompted all the review comments I left on this patch.)\n> So, let's drop this patch since it adds no value to either this series\n> or to the existing codebase. If someone needs such a change later on,\n> they can resurrect the change.\n\nSounds good, dropped.\n\n> [1] There are far more people submitting patches to the project than\n> reviewing them. For instance, according to Junio's latest \"What's\n> Cooking\" report[2], the patch I submitted[3] a couple weeks ago to fix\n> \"git worktree repair\" to handle manual copy operations is still\n> awaiting review. (Since you've now been living in the worktree code a\n> bit and have had to digest the \"repair\" logic, perhaps you'd be\n> interested in reviewing that patch?)\n\nI'd be happy to take a look!"},{"id":"504228","messageId":"CAPig+cTDK7dk-BNgidQnyTBXi7dqQ1uZ91geerzN4aLtJyHkgw@mail.gmail.com","threadId":"62260","inReplyTo":"LyrqJt-X7sHVSKc-97V4-hTsDiIuz8ucEUMDtLCPWAN15k31EBjZnbeuJ2PldcCVvZaPyudvgb3H00T6uZQsI-XON7Ku8jNFjkfRCgrdr8U=@pm.me","subject":"Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-07T03:26:24Z","receivedAt":"2024-10-07T03:26:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Oct 6, 2024 at 10:42 PM Caleb White <cdwhite3@pm.me> wrote:\n> On Sunday, October 6th, 2024 at 13:16, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > ... this code now becomes more than a little confusing to read. It\n> > says \"if infer_backlink then signal error\", which sounds rather\n> > backward and leaves the reader scratching his or her head. (\"Why\n> > signal error if the function succeeded?\"). Hence, infer_backlink()\n> > should probably return 1 on success and 0 on failure, which will make\n> > this code read more idiomatically:\n> >\n> > if (!infer_backlink(realdotgit.buf, &backlink)) {\n> > ...signal error...\n>\n> This was my first thought, however, on unix it is fairly standard\n> to return 0 if successful and a non-zero int if there's an error.\n> I don't mind updating, but I want to follow what makes the most\n> sense and would be most expected.\n\nI mentioned it because it made my reading hiccup, but I don't feel too\nstrongly about it one way or the other considering that this is an\ninternal function.\n"},{"id":"504229","messageId":"Mx9TmTS1dWiDxvYmluzhFOd7JikyhHmhluVLSYToYgzJhIWKsKUT2TV-FJIjUGKU6zp2w7KJJ2Tiz4kHXbi6mKg78FujZGel4Xx8iEEWgME=@pm.me","threadId":"62260","inReplyTo":"CAPig+cTDK7dk-BNgidQnyTBXi7dqQ1uZ91geerzN4aLtJyHkgw@mail.gmail.com","subject":"Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-07T03:28:56Z","receivedAt":"2024-10-07T03:29:00Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Sunday, October 6th, 2024 at 22:26, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> I mentioned it because it made my reading hiccup, but I don't feel too\n> strongly about it one way or the other considering that this is an\n> internal function.\n\nI reversed the flags :thumsup:"},{"id":"504230","messageId":"ZwNZRX1LHlxawJJc@ArchLinux","threadId":"62260","inReplyTo":"I3BmSHTyOELt2DzfSaLhRYLouu5iAPZIZGZ2Ne73AygO35CM0mq44INa68t6VD4XV61DgzbrfUW0m8fivd3N9Rejgm-tecXQHXQRs1BP9CQ=@pm.me","subject":"Re: [PATCH v2 2/4] worktree: link worktrees with relative paths","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-10-07T03:45:09Z","receivedAt":"2024-10-07T03:45:04Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Sun, Oct 06, 2024 at 11:57:22PM +0000, Caleb White wrote:\n\n[snip]\n\n> > Still, we do not need to call \"strbuf_reset\" again for \"tmp\". But there\n> > is another question here. Should we define the \"file\" just in this \"if\"\n> > block and free \"file\" also in the block?\n> \n> The style this code uses seems to place most / all of the declarations at\n> the top of the function and frees at the bottom so I think this fits in.\n> \n\nYes, as you have said, the code style places most / all of the\ndeclarations at the top and free at the bottom. But the trouble here is\nwe will free the \"file\" in the \"if\" block.\n\n    char *file = NULL;\n\n    if (...) {\n        file = xstrfmt(...);\n        free(file);\n        file = xstrfmt(...);\n    }\n\n    free(file);\n\nIf we want to follow the original code style, should we create two\nvariables at the top and free them at the bottom?\n\n> > And I don't think it's a good idea to use \"xstrfmt\". Here, we need to\n> > allocate two times and free two times. Why not just define a \"struct\n> > strbuf\" and the use \"strbuf_*\" method here?\n> \n> I can use strbufs, I just wasn't sure if I really needed a strbuf for\n> each of the paths and was just trying to reuse a var.\n> \n\nYou don't need to create a new \"strbuf\" for each of the paths. You could\njust use \"strbuf_reset\" for only one \"struct strbuf\".\n\n    struct strbuf file = STRBUF_INIT;\n\n    if (...) {\n        strbuf_addf(...);\n        strbuf_reset(&file);\n        strbuf_addf(...);\n\n    }\n\n    strbuf_release(&file);\n\n> > > strbuf_realpath(&repo, git_common_path(\"worktrees/%s\", wt->id), 1);\n> > > strbuf_addf(&dotgit, \"%s/.git\", wt->path);\n> > > - backlink = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));\n> > > + git_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));\n> > \n> \n> > \n> \n> > Why here we need to use \"xstrdup_or_null\". The life cycle of the\n> > \"git_contents\" variable is in the \"repair_gitfile\" function.\n> \n> This what the existing code used and I saw no reason to change it...\n\nActually, I somehow understand why in the original code, it will use\n\"xstrdup_or_null\" to initialize the \"backlink\". Because in\n\"read_gitfile_gently\", we will use a static \"struct strbuf\" as the\nbuffer.\n\nI guess the intention of the original code is that if we call again\n\"read_gitfile_gently\" in this function or we have another thread which\ncalls this function, the content of the buffer will be changed. So we\nexplicitly use \"xstrdup_or_null\" to create a copy here to avoid this.\n\nBut I wonder whether we really need to consider above problem?\n"},{"id":"504231","messageId":"ZwNb5Aabrub4tW5O@ArchLinux","threadId":"62260","inReplyTo":"CAPig+cQC45_Pofw31h2RSydj_CjqpfajLLa5O_sR9m0AQh5OVw@mail.gmail.com","subject":"Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-10-07T03:56:20Z","receivedAt":"2024-10-07T03:56:14Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Sun, Oct 06, 2024 at 02:41:22PM -0400, Eric Sunshine wrote:\n\n[snip]\n\n> > Why not simply do the following things here (I don't think\n> > \"git_contents\" is a good name, however I am not familiar with the\n> > worktree, I cannot give some advice here).\n> \n> I found the name \"git_contents\" clear enough and understood its\n> purpose at-a-glance, so I think it's a reasonably good choice. A\n> slightly better name might be \"gitfile_contents\" or perhaps\n> \"dotgit_contents\" for consistency with other similarly-named variables\n> in this function.\n> \n\nThanks, I don't know the context here.\n\n> > If \"git_contents\" is not\n> > NULL, there is no need for us to check the \"err\" variable.\n> \n> I'm not sure we would want to change this; the existing logic seems\n> clear enough.\n> \n\nThe reason why I don't think we need to check the \"err\" variable is that\nthe \"git_contents\" and \"err\" is relevant. If \"git_contents\" is not NULL,\nthe \"err\" must be zero unless there are bugs in \"read_gitfile_gently\".\nSo, if we already check \"git_contents\", why do we need to check again\nfor \"err\"?\n\nBut, I agree with you that the existing logic is clear enough. I just\nexplain further what I mean here.\n\nThanks,\nJialuo\n"},{"id":"504232","messageId":"7HnZc9JpxZXOAZFD6AO0MdhWccomQBZJmS_IzwiA_-YpmFlQPAjeMJBvP9uBI_6s6-crP3SXWZTrErwj918xIuOAo7TmzqEZek8FV0IMhrg=@pm.me","threadId":"62260","inReplyTo":"ZwNb5Aabrub4tW5O@ArchLinux","subject":"Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-07T04:01:43Z","receivedAt":"2024-10-07T04:01:49Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Sunday, October 6th, 2024 at 22:56, shejialuo <shejialuo@gmail.com> wrote:\n> The reason why I don't think we need to check the \"err\" variable is that\n> the \"git_contents\" and \"err\" is relevant. If \"git_contents\" is not NULL,\n> the \"err\" must be zero unless there are bugs in \"read_gitfile_gently\".\n> So, if we already check \"git_contents\", why do we need to check again\n> for \"err\"?\n\nThere are two other error conditions we check, and one of them we try\nto find the inferred backlink (so it is not a failure path):\n\n```\n} else if (err == READ_GITFILE_ERR_NOT_A_FILE) {\n\tfn(1, realdotgit.buf, _(\"unable to locate repository; .git is not a file\"), cb_data);\n\tgoto done;\n} else if (err == READ_GITFILE_ERR_NOT_A_REPO) {\n\tif (!infer_backlink(realdotgit.buf, &backlink)) {\n\t\tfn(1, realdotgit.buf, _(\"unable to locate repository; .git file does not reference a repository\"), cb_data);\n\t\tgoto done;\n\t}\n} else if (err) {\n\tfn(1, realdotgit.buf, _(\"unable to locate repository; .git file broken\"), cb_data);\n\tgoto done;\n}\n\n```"},{"id":"504233","messageId":"CAPig+cQjHCZChToM_nEhmQsXedK4nEObs7YV2Q2PzrZqcHB4fQ@mail.gmail.com","threadId":"62260","inReplyTo":"ZwNZRX1LHlxawJJc@ArchLinux","subject":"Re: [PATCH v2 2/4] worktree: link worktrees with relative paths","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-07T04:02:59Z","receivedAt":"2024-10-07T04:03:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Oct 6, 2024 at 11:45 PM shejialuo <shejialuo@gmail.com> wrote:\n> Actually, I somehow understand why in the original code, it will use\n> \"xstrdup_or_null\" to initialize the \"backlink\". Because in\n> \"read_gitfile_gently\", we will use a static \"struct strbuf\" as the\n> buffer.\n>\n> I guess the intention of the original code is that if we call again\n> \"read_gitfile_gently\" in this function or we have another thread which\n> calls this function, the content of the buffer will be changed. So we\n> explicitly use \"xstrdup_or_null\" to create a copy here to avoid this.\n\nThat's correct. The code is being defensive in case the static strbuf\ninside read_gitfile_gently() gets overwritten.\n\n> But I wonder whether we really need to consider above problem?\n\nIt's easier to reason about the code in this function if we don't have\nto worry about the underlying static strbuf. Also, this is not a hot\npath so the extra allocation and string copy is not a concern. As\nsuch, it makes sense for the code to remain robust (by creating the\ncopy) rather than becoming potentially more fragile by eliminating the\nstring copy.\n"},{"id":"504234","messageId":"ZwNfuLoVn4aU4mgQ@ArchLinux","threadId":"62260","inReplyTo":"o7jQV4JUYr2iQtErKFhCrjCySiGn7_I18gmdInKbOC9TAsvqazG0O_fPUtiWZ5UfiGLr25OZ5xvqIhfOIwCG-s1RGgri-BJIdHqiokIw4z0=@pm.me","subject":"Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-10-07T04:12:40Z","receivedAt":"2024-10-07T04:12:34Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Mon, Oct 07, 2024 at 02:26:51AM +0000, Caleb White wrote:\n> On Sunday, October 6th, 2024 at 13:41, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > I found the name \"git_contents\" clear enough and understood its\n> > purpose at-a-glance, so I think it's a reasonably good choice. A\n> > slightly better name might be \"gitfile_contents\" or perhaps\n> > \"dotgit_contents\" for consistency with other similarly-named variables\n> > in this function.\n> \n> I will rename to `dotgit_contents`.\n> \n> > It certainly makes sense to check whether \"git_contents\" is NULL\n> > before trying to copy it into the \"backlink\" strbuf, however, if\n> > \"git_contents\" is NULL here, then what does that mean? What does it\n> > mean to leave \"backlink\" empty? The only way (presumably) we get this\n> > far is if read_gitfile_gently() succeeded, so (presumably)\n> > \"git_contents\" should not be NULL. Thus, rather than conditionally\n> > copying into \"backlink\", we should instead indicate clearly via BUG()\n> > that it should be impossible for \"git_contents\" to be NULL. So, rather\n> > than making this part of the existing if-else-cascade, we should do\n> > this as a standalone `if`:\n> > \n> \n> > if (!git_contents)\n> > BUG(...);\n> > strbuf_addstr(&backlink, git_contents);\n\nI agree with the idea that Eric proposed. It's truly we should raise a\nbug here. That's would be much more clear.\n\n> We can't use BUG because this is handled as part of the err\n> conditions. The contents can be NULL and `backlink` could be\n> filled with the inferred backlink. I moved this to the top\n> and I think it reads better.\n\nThat's not correct. It is true that the contents can be NULL and\n`backlink` could be filled with the infer_backlink. But do you notice\nthat if `backlink` was filled with the infer_backlink, it will jump\nto the \"done\" label.\n\nWhat Erie means is the following:\n\n    git_contents = read_gitfile_gently(...);\n    if (err) {\n        if (...) {\n\n        } else if (err == RAED_GITFILE_ERR_NOT_A_REPO) {\n            // handle backlink\n            goto done;\n\n        }\n    }\n\n    if (!git_contents)\n        BUG(...);\n    strbuf_addstr(&backlink, git_contents);\n\n    done:\n        // cleanup operations\n\nThanks,\nJialuo\n"},{"id":"504235","messageId":"ZwNhTdAhJb-KQBav@ArchLinux","threadId":"62260","inReplyTo":"7HnZc9JpxZXOAZFD6AO0MdhWccomQBZJmS_IzwiA_-YpmFlQPAjeMJBvP9uBI_6s6-crP3SXWZTrErwj918xIuOAo7TmzqEZek8FV0IMhrg=@pm.me","subject":"Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-10-07T04:19:25Z","receivedAt":"2024-10-07T04:19:19Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Mon, Oct 07, 2024 at 04:01:43AM +0000, Caleb White wrote:\n> On Sunday, October 6th, 2024 at 22:56, shejialuo <shejialuo@gmail.com> wrote:\n> > The reason why I don't think we need to check the \"err\" variable is that\n> > the \"git_contents\" and \"err\" is relevant. If \"git_contents\" is not NULL,\n> > the \"err\" must be zero unless there are bugs in \"read_gitfile_gently\".\n> > So, if we already check \"git_contents\", why do we need to check again\n> > for \"err\"?\n> \n> There are two other error conditions we check, and one of them we try\n> to find the inferred backlink (so it is not a failure path):\n> \n> ```\n> } else if (err == READ_GITFILE_ERR_NOT_A_FILE) {\n> \tfn(1, realdotgit.buf, _(\"unable to locate repository; .git is not a file\"), cb_data);\n> \tgoto done;\n> } else if (err == READ_GITFILE_ERR_NOT_A_REPO) {\n> \tif (!infer_backlink(realdotgit.buf, &backlink)) {\n> \t\tfn(1, realdotgit.buf, _(\"unable to locate repository; .git file does not reference a repository\"), cb_data);\n> \t\tgoto done;\n> \t}\n> } else if (err) {\n> \tfn(1, realdotgit.buf, _(\"unable to locate repository; .git file broken\"), cb_data);\n> \tgoto done;\n> }\n> \n> ```\n\nI am sorry that my words make you not clear here. I want to express that\nif \"git_contents\" is not NULL and there is no need for us to check\n\"err\". This means we could use the following flows:\n\n    if (git_contents && !err) {\n        ...\n    } else if (err == xxx) {\n        ...\n    }\n\nHowever, from my perspective, the way proposed by Eric where we could\nuse \"BUG\" is more robust. Because the current method assumes that\n\"read_gitfile_gently\" works as we want.\n\nThanks,\nJialuo\n"},{"id":"504236","messageId":"XzNC1BXk2OaIGmgas_MWKZ_ubpeR_kA19hNxmF76FLtgI_WfYrMOGEDEr5uqOETNPHTI_bA3qJE0R0eah4UTPXZUgeRjG7psN6ncDOdPXNA=@pm.me","threadId":"62260","inReplyTo":"ZwNfuLoVn4aU4mgQ@ArchLinux","subject":"Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-07T04:19:22Z","receivedAt":"2024-10-07T04:19:27Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Sunday, October 6th, 2024 at 23:12, shejialuo <shejialuo@gmail.com> wrote:\n> That's not correct. It is true that the contents can be NULL and\n> `backlink` could be filled with the infer_backlink. But do you notice\n> that if `backlink` was filled with the infer_backlink, it will jump\n> to the \"done\" label.\n\nThis is not correct, if backlink is filled with `infer_backlink` it\ncontinues on with the processing. I have made this more clear:\n\n    dotgit_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));\n    if (dotgit_contents) {\n\tif (is_absolute_path(dotgit_contents)) {\n\t    strbuf_addstr(&backlink, dotgit_contents);\n        } else {\n\t    strbuf_addbuf(&backlink, &realdotgit);\n\t    strbuf_strip_suffix(&backlink, \".git\");\n\t    strbuf_addstr(&backlink, dotgit_contents);\n        }\n    } else if (err == READ_GITFILE_ERR_NOT_A_FILE) {\n\tfn(1, realdotgit.buf, _(\"unable to locate repository; .git is not a file\"), cb_data);\n\tgoto done;\n    } else if (err == READ_GITFILE_ERR_NOT_A_REPO) {\n\tif (!infer_backlink(realdotgit.buf, &backlink)) {\n\t    fn(1, realdotgit.buf, _(\"unable to locate repository; .git file does not reference a repository\"), cb_data);\n\t    goto done;\n        }\n    } else if (err) {\n        fn(1, realdotgit.buf, _(\"unable to locate repository; .git file broken\"), cb_data);\n        goto done;\n    }\n\n\n"},{"id":"504239","messageId":"ZwNjgvMbUKgpbP7O@ArchLinux","threadId":"62260","inReplyTo":"XzNC1BXk2OaIGmgas_MWKZ_ubpeR_kA19hNxmF76FLtgI_WfYrMOGEDEr5uqOETNPHTI_bA3qJE0R0eah4UTPXZUgeRjG7psN6ncDOdPXNA=@pm.me","subject":"Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-10-07T04:28:50Z","receivedAt":"2024-10-07T04:28:45Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Mon, Oct 07, 2024 at 04:19:22AM +0000, Caleb White wrote:\n> On Sunday, October 6th, 2024 at 23:12, shejialuo <shejialuo@gmail.com> wrote:\n> > That's not correct. It is true that the contents can be NULL and\n> > `backlink` could be filled with the infer_backlink. But do you notice\n> > that if `backlink` was filled with the infer_backlink, it will jump\n> > to the \"done\" label.\n> \n> This is not correct, if backlink is filled with `infer_backlink` it\n> continues on with the processing. I have made this more clear:\n> \n>     dotgit_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));\n>     if (dotgit_contents) {\n> \tif (is_absolute_path(dotgit_contents)) {\n> \t    strbuf_addstr(&backlink, dotgit_contents);\n>         } else {\n> \t    strbuf_addbuf(&backlink, &realdotgit);\n> \t    strbuf_strip_suffix(&backlink, \".git\");\n> \t    strbuf_addstr(&backlink, dotgit_contents);\n>         }\n>     } else if (err == READ_GITFILE_ERR_NOT_A_FILE) {\n> \tfn(1, realdotgit.buf, _(\"unable to locate repository; .git is not a file\"), cb_data);\n> \tgoto done;\n>     } else if (err == READ_GITFILE_ERR_NOT_A_REPO) {\n> \tif (!infer_backlink(realdotgit.buf, &backlink)) {\n> \t    fn(1, realdotgit.buf, _(\"unable to locate repository; .git file does not reference a repository\"), cb_data);\n> \t    goto done;\n>         }\n>     } else if (err) {\n>         fn(1, realdotgit.buf, _(\"unable to locate repository; .git file broken\"), cb_data);\n>         goto done;\n>     }\n> \n> \n\nThanks, you are correct here. We cannot use \"BUG\" here. And I think\ncurrently this is OK.\n\n"},{"id":"504240","messageId":"vkneUltI5q9qajOYcG8GYE8FHCRIJbH1ExR58YoEm9pam2_L6LlchL4otU6jk6l7iIN0kjOWtknZx4aCxRjGTVicQ69RmOYt_RUghGlV4hA=@pm.me","threadId":"62260","inReplyTo":"ZwNjgvMbUKgpbP7O@ArchLinux","subject":"Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-07T04:31:04Z","receivedAt":"2024-10-07T04:31:09Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Sunday, October 6th, 2024 at 23:28, shejialuo <shejialuo@gmail.com> wrote:\n> > } else if (err) {\n> > fn(1, realdotgit.buf, _(\"unable to locate repository; .git file broken\"), cb_data);\n> > goto done;\n> > }\n> \n\n> \n\n> Thanks, you are correct here. We cannot use \"BUG\" here. And I think\n> currently this is OK.\n\nGranted, that last `else if` could just become an `else` statement now,\nI'll make that change."},{"id":"504347","messageId":"XhMvjO_NVmnkqWATQ8dMn0D7igePOHd7t0zfhKOr8ZTzd90-zslpej19uaEhtjpZi0nT1JalpyViubAEBBc5VRq6YI-AsrTftI180UJWc7c=@pm.me","threadId":"62260","inReplyTo":"ZwNZRX1LHlxawJJc@ArchLinux","subject":"Re: [PATCH v2 2/4] worktree: link worktrees with relative paths","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-07T16:59:21Z","receivedAt":"2024-10-07T16:59:32Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Sunday, October 6th, 2024 at 22:45, shejialuo <shejialuo@gmail.com> wrote:\n> You don't need to create a new \"strbuf\" for each of the paths. You could\n> just use \"strbuf_reset\" for only one \"struct strbuf\".\n> \n\n> struct strbuf file = STRBUF_INIT;\n> \n\n> if (...) {\n> strbuf_addf(...);\n> strbuf_reset(&file);\n> strbuf_addf(...);\n> \n\n> }\n> \n\n> strbuf_release(&file);\n\nAh, this is a better design---I've updated this to use a single strbuf, thanks!"}]}