{"thread":{"id":"62276","subject":"[PATCH v3 0/3] Link worktrees with relative paths","startedAt":"2024-10-08T03:12:44Z","lastAt":"2024-10-22T04:32:29Z","messageCount":24,"participants":["Caleb White via B4 Relay","Caleb White","Junio C Hamano","Phillip Wood","shejialuo"],"isPatch":true,"patchVersion":3,"patchTotal":3},"messages":[{"id":"504374","messageId":"20241007-wt_relative_paths-v3-0-622cf18c45eb@pm.me","threadId":"62276","inReplyTo":null,"subject":"[PATCH v3 0/3] Link worktrees with relative paths","fromName":"Caleb White via B4 Relay","fromEmail":"devnull+cdwhite3.pm.me@kernel.org","sentAt":"2024-10-08T03:12:29Z","receivedAt":"2024-10-08T03:12:44Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"Git currently stores absolute paths to both the main repository and\nlinked worktrees. However, this causes problems when moving repositories\nor working in containerized environments where absolute paths differ\nbetween systems. The worktree links break, and users are required to\nmanually execute `worktree repair` to repair them, leading to workflow\ndisruptions. Additionally, mapping repositories inside of containerized\nenvironments renders the repository unusable inside the containers, and\nthis is not repairable as repairing the worktrees inside the containers\nwill result in them being broken outside the containers.\n\nTo address this, this patch series makes Git always write relative paths\nwhen linking worktrees. Relative paths increase the resilience of the\nworktree links across various systems and environments, particularly\nwhen the worktrees are self-contained inside the main repository (such\nas when using a bare repository with worktrees). This improves\nportability, workflow efficiency, and reduces overall breakages.\n\nAlthough Git now writes relative paths, existing repositories with\nabsolute paths are still supported. There are no breaking changes\nto workflows based on absolute paths, ensuring backward compatibility.\n\nSigned-off-by: Caleb White <cdwhite3@pm.me>\n---\nChanges in v3:\n- Squashed patch [3/4] into patch [2/4] to streamline changes.\n- Dropped patch [4/4] as it was no longer necessary.\n- Rebased onto 20240923075416.54289-1-ericsunshine@charter.net\n- Updated `infer_backlink()` to return 1 on success for consistency.\n- Swapped the order of `infer_backlink()` arguments for clarity.\n- Clear `inferred` if error occurs in `infer_backlink()`.\n- Renamed `git_contents` to `dotgit_contents` for clearer semantics.\n- Cleaned up `dotgit_contents` logic in `repair_worktree_at_path()`.\n- Replaced multiple `xstrfmt/free` calls with a single `strbuf`.\n- Added a test case covering a failure noted in a separate patch series.\n- Improved commit messages.\n- Link to v2: https://lore.kernel.org/r/20241006060017.171788-1-cdwhite3@pm.me\nChanges in v2:\n- No changes, just a resubmission\n- Link to v1: https://lore.kernel.org/git/20241006045847.159937-1-cdwhite3@pm.me/\n\nRange-diff against v2:\n1:  735a90355e ! 1:  b5060dba05 worktree: refactor infer_backlink() to use *strbuf\n    @@ Metadata\n      ## Commit message ##\n         worktree: refactor infer_backlink() to use *strbuf\n     \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    +    This lays the groundwork for the next patch, which needs the backlink\n    +    returned from infer_backlink() as a `strbuf`. It seemed inefficient to\n    +    convert from `strbuf` to `char*` and back to `strbuf` again.\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    +    This refactors infer_backlink() to return an integer result and use a\n    +    pre-allocated `strbuf` for the inferred backlink path, replacing the\n    +    previous `char*` return type and improving efficiency.\n     \n         Signed-off-by: Caleb White <cdwhite3@pm.me>\n     \n    @@ worktree.c: static int is_main_worktree_path(const char *path)\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(struct strbuf *inferred, const char *gitfile)\n    ++static int infer_backlink(const char *gitfile, struct strbuf *inferred)\n      {\n        struct strbuf actual = STRBUF_INIT;\n     -\tstruct strbuf inferred = STRBUF_INIT;\n    @@ worktree.c: static char *infer_backlink(const char *gitfile)\n                goto error;\n     -\tstrbuf_git_common_path(&inferred, the_repository, \"worktrees/%s\", id);\n     -\tif (!is_directory(inferred.buf))\n    ++\tstrbuf_reset(inferred);\n     +\tstrbuf_git_common_path(inferred, the_repository, \"worktrees/%s\", id);\n     +\tif (!is_directory(inferred->buf))\n                goto error;\n      \n        strbuf_release(&actual);\n     -\treturn strbuf_detach(&inferred, NULL);\n    -+\treturn 0;\n    ++\treturn 1;\n      \n      error:\n        strbuf_release(&actual);\n     -\tstrbuf_release(&inferred);\n     -\treturn NULL;\n    -+\treturn 1;\n    ++\tstrbuf_reset(inferred); /* clear invalid path */\n    ++\treturn 0;\n      }\n      \n      /*\n    @@ worktree.c: void repair_worktree_at_path(const char *path,\n        struct strbuf dotgit = STRBUF_INIT;\n        struct strbuf realdotgit = STRBUF_INIT;\n     +\tstruct strbuf backlink = STRBUF_INIT;\n    ++\tstruct strbuf inferred_backlink = STRBUF_INIT;\n        struct strbuf gitdir = STRBUF_INIT;\n        struct strbuf olddotgit = STRBUF_INIT;\n     -\tchar *backlink = NULL;\n    -+\tchar *git_contents = NULL;\n    +-\tchar *inferred_backlink = NULL;\n    ++\tchar *dotgit_contents = NULL;\n        const char *repair = NULL;\n        int err;\n      \n    @@ worktree.c: void repair_worktree_at_path(const char *path,\n                goto done;\n        }\n      \n    +-\tinferred_backlink = infer_backlink(realdotgit.buf);\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    +-\tif (err == READ_GITFILE_ERR_NOT_A_FILE) {\n    ++\tinfer_backlink(realdotgit.buf, &inferred_backlink);\n    ++\tdotgit_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));\n    ++\tif (dotgit_contents) {\n    ++\t\tstrbuf_addstr(&backlink, dotgit_contents);\n    ++\t} else 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    --\t\tif (!(backlink = infer_backlink(realdotgit.buf))) {\n    -+\t\tif (infer_backlink(&backlink, realdotgit.buf)) {\n    +-\t\tif (inferred_backlink) {\n    ++\t\tif (inferred_backlink.len) {\n    + \t\t\t/*\n    + \t\t\t * Worktree's .git file does not point at a repository\n    + \t\t\t * but we found a .git/worktrees/<id> in this\n    + \t\t\t * repository with the same <id> as recorded in the\n    + \t\t\t * worktree's .git file so make the worktree point at\n    +-\t\t\t * the discovered .git/worktrees/<id>. (Note: backlink\n    +-\t\t\t * is already NULL, so no need to free it first.)\n    ++\t\t\t * the discovered .git/worktrees/<id>.\n    + \t\t\t */\n    +-\t\t\tbacklink = inferred_backlink;\n    +-\t\t\tinferred_backlink = NULL;\n    ++\t\t\tstrbuf_swap(&backlink, &inferred_backlink);\n    + \t\t} else {\n                        fn(1, realdotgit.buf, _(\"unable to locate repository; .git file does not reference a repository\"), cb_data);\n                        goto 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_contents) {\n    -+\t\tstrbuf_addstr(&backlink, git_contents);\n    +@@ worktree.c: void repair_worktree_at_path(const char *path,\n    + \t * in the \"copy\" repository. In this case, point the \"copy\" worktree's\n    + \t * .git file at the \"copy\" repository.\n    + \t */\n    +-\tif (inferred_backlink && fspathcmp(backlink, inferred_backlink)) {\n    +-\t\tfree(backlink);\n    +-\t\tbacklink = inferred_backlink;\n    +-\t\tinferred_backlink = NULL;\n    ++\tif (inferred_backlink.len && fspathcmp(backlink.buf, inferred_backlink.buf)) {\n    ++\t\tstrbuf_swap(&backlink, &inferred_backlink);\n        }\n      \n     -\tstrbuf_addf(&gitdir, \"%s/gitdir\", backlink);\n    @@ worktree.c: void repair_worktree_at_path(const char *path,\n        }\n      done:\n     -\tfree(backlink);\n    -+\tfree(git_contents);\n    +-\tfree(inferred_backlink);\n    ++\tfree(dotgit_contents);\n        strbuf_release(&olddotgit);\n     +\tstrbuf_release(&backlink);\n    ++\tstrbuf_release(&inferred_backlink);\n        strbuf_release(&gitdir);\n        strbuf_release(&realdotgit);\n        strbuf_release(&dotgit);\n2:  f128dfa53c ! 2:  017f199988 worktree: link worktrees with relative paths\n    @@ Metadata\n      ## Commit message ##\n         worktree: link worktrees with relative paths\n     \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    +    Git currently stores absolute paths to both the main repository and\n    +    linked worktrees. However, this causes problems when moving repositories\n    +    or working in containerized environments where absolute paths differ\n    +    between systems. The worktree links break, and users are required to\n    +    manually execute `worktree repair` to repair them, leading to workflow\n    +    disruptions. Additionally, mapping repositories inside of containerized\n    +    environments renders the repository unusable inside the containers, and\n    +    this is not repairable as repairing the worktrees inside the containers\n    +    will result in them being broken outside the containers.\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    +    To address this, this patch makes Git always write relative paths when\n    +    linking worktrees. Relative paths increase the resilience of the\n    +    worktree links across various systems and environments, particularly\n    +    when the worktrees are self-contained inside the main repository (such\n    +    as when using a bare repository with worktrees). This improves\n    +    portability, workflow efficiency, and reduces overall 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    +    Although Git now writes relative paths, existing repositories with\n    +    absolute paths are still supported. There are no breaking changes\n    +    to workflows based on absolute paths, ensuring backward compatibility.\n    +\n    +    At a low level, the changes involve modifying functions in `worktree.c`\n    +    and `builtin/worktree.c` to use `relative_path()` when writing the\n    +    worktree’s `.git` file and the main repository’s `gitdir` reference.\n    +    Instead of hardcoding absolute paths, Git now computes the relative path\n    +    between the worktree and the repository, ensuring that these links are\n    +    portable. Locations where these respective file are read have also been\n    +    updated to properly handle both absolute and relative paths. Generally,\n    +    relative paths are always resolved into absolute paths before any\n    +    operations or comparisons are performed.\n    +\n    +    Additionally, `repair_worktrees_after_gitdir_move()` has been introduced\n    +    to address the case where both the `<worktree>/.git` and\n    +    `<repo>/worktrees/<id>/gitdir` links are broken after the gitdir is\n    +    moved (such as during a re-initialization). This function repairs both\n    +    sides of the worktree link using the old gitdir path to reestablish the\n    +    correct paths after a move.\n    +\n    +    The `worktree.path` struct member has also been updated to always store\n    +    the absolute path of a worktree. This ensures that worktree consumers\n    +    never have to worry about trying to resolve the absolute path themselves.\n     \n         Signed-off-by: Caleb White <cdwhite3@pm.me>\n     \n    @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam\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        strbuf_reset(&sb);\n        strbuf_addf(&sb, \"%s/commondir\", sb_repo.buf);\n    @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam\n        return ret;\n      }\n     \n    + ## setup.c ##\n    +@@ setup.c: 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);\n    +\n      ## t/t2408-worktree-relative.sh (new) ##\n     @@\n     +#!/bin/sh\n    @@ worktree.c: int validate_worktree(const struct worktree *wt, struct strbuf *errm\n      {\n        struct strbuf path = STRBUF_INIT;\n     +\tstruct strbuf repo = STRBUF_INIT;\n    ++\tstruct strbuf file = STRBUF_INIT;\n     +\tstruct strbuf tmp = STRBUF_INIT;\n    -+\tchar *file = NULL;\n      \n        if (is_main_worktree(wt))\n                BUG(\"can't relocate main worktree\");\n    @@ worktree.c: int validate_worktree(const struct worktree *wt, struct strbuf *errm\n        if (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    ++\t\tstrbuf_addf(&file, \"%s/gitdir\", repo.buf);\n    ++\t\twrite_file(file.buf, \"%s/.git\", relative_path(path.buf, repo.buf, &tmp));\n    ++\t\tstrbuf_reset(&file);\n    ++\t\tstrbuf_addf(&file, \"%s/.git\", path.buf);\n    ++\t\twrite_file(file.buf, \"gitdir: %s\", relative_path(repo.buf, path.buf, &tmp));\n     +\n                free(wt->path);\n                wt->path = strbuf_detach(&path, NULL);\n        }\n    -+\tfree(file);\n        strbuf_release(&path);\n     +\tstrbuf_release(&repo);\n    ++\tstrbuf_release(&file);\n     +\tstrbuf_release(&tmp);\n      }\n      \n    @@ worktree.c: static void repair_gitfile(struct worktree *wt,\n     -\tchar *backlink;\n     +\tstruct strbuf backlink = STRBUF_INIT;\n     +\tstruct strbuf tmp = STRBUF_INIT;\n    -+\tchar *git_contents = NULL;\n    ++\tchar *dotgit_contents = NULL;\n        const char *repair = NULL;\n        int err;\n      \n    @@ worktree.c: static void repair_gitfile(struct worktree *wt,\n        strbuf_realpath(&repo, git_common_path(\"worktrees/%s\", wt->id), 1);\n        strbuf_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    ++\tdotgit_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    ++\tif (dotgit_contents) {\n    ++\t\tif (is_absolute_path(dotgit_contents)) {\n    ++\t\t\tstrbuf_addstr(&backlink, dotgit_contents);\n     +\t\t} else {\n    -+\t\t\tstrbuf_addf(&backlink, \"%s/%s\", wt->path, git_contents);\n    ++\t\t\tstrbuf_addf(&backlink, \"%s/%s\", wt->path, dotgit_contents);\n     +\t\t\tstrbuf_realpath_forgiving(&backlink, backlink.buf, 0);\n     +\t\t}\n     +\t}\n    @@ worktree.c: static void repair_gitfile(struct worktree *wt,\n      \n     -\tfree(backlink);\n     +done:\n    -+\tfree(git_contents);\n    ++\tfree(dotgit_contents);\n        strbuf_release(&repo);\n        strbuf_release(&dotgit);\n     +\tstrbuf_release(&backlink);\n    @@ worktree.c: static void repair_gitfile(struct worktree *wt,\n      }\n      \n      static void repair_noop(int iserr UNUSED,\n    +@@ worktree.c: 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_INIT;\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    ++\twrite_file(gitdir.buf, \"%s\", relative_path(dotgit.buf, repo.buf, &tmp));\n    ++done:\n    ++\tstrbuf_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;\n     @@ worktree.c: void repair_worktree_at_path(const char *path,\n    - \tstruct strbuf backlink = STRBUF_INIT;\n    + \tstruct strbuf inferred_backlink = STRBUF_INIT;\n        struct strbuf gitdir = STRBUF_INIT;\n        struct strbuf olddotgit = STRBUF_INIT;\n     +\tstruct strbuf realolddotgit = STRBUF_INIT;\n     +\tstruct strbuf tmp = STRBUF_INIT;\n    - \tchar *git_contents = NULL;\n    + \tchar *dotgit_contents = NULL;\n        const char *repair = NULL;\n        int err;\n     @@ worktree.c: 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}\n    + \n    + \tinfer_backlink(realdotgit.buf, &inferred_backlink);\n    ++\tstrbuf_realpath_forgiving(&inferred_backlink, inferred_backlink.buf, 0);\n    + \tdotgit_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));\n    + \tif (dotgit_contents) {\n    +-\t\tstrbuf_addstr(&backlink, dotgit_contents);\n    ++\t\tif (is_absolute_path(dotgit_contents)) {\n    ++\t\t\tstrbuf_addstr(&backlink, dotgit_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\tstrbuf_addstr(&backlink, dotgit_contents);\n    ++\t\t\tstrbuf_realpath_forgiving(&backlink, backlink.buf, 0);\n     +\t\t}\n    + \t} else if (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    +@@ worktree.c: void repair_worktree_at_path(const char *path,\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} else {\n    + \t\tfn(1, realdotgit.buf, _(\"unable to locate repository; .git file broken\"), cb_data);\n    + \t\tgoto done;\n        }\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    +@@ worktree.c: void repair_worktree_at_path(const char *path,\n                repair = _(\"gitdir unreadable\");\n        else {\n                strbuf_rtrim(&olddotgit);\n    @@ worktree.c: void repair_worktree_at_path(const char *path,\n     +\t\twrite_file(gitdir.buf, \"%s\", relative_path(realdotgit.buf, backlink.buf, &tmp));\n        }\n      done:\n    - \tfree(git_contents);\n    + \tfree(dotgit_contents);\n        strbuf_release(&olddotgit);\n     +\tstrbuf_release(&realolddotgit);\n        strbuf_release(&backlink);\n    + \tstrbuf_release(&inferred_backlink);\n        strbuf_release(&gitdir);\n        strbuf_release(&realdotgit);\n        strbuf_release(&dotgit);\n    @@ worktree.c: void repair_worktree_at_path(const char *path,\n     +\tstruct strbuf dotgit = STRBUF_INIT;\n     +\tstruct strbuf gitdir = STRBUF_INIT;\n     +\tstruct strbuf repo = STRBUF_INIT;\n    ++\tstruct strbuf file = STRBUF_INIT;\n     +\tchar *path = NULL;\n    -+\tchar *file = NULL;\n     +\tint rc = 0;\n        int fd;\n        size_t len;\n    @@ worktree.c: void repair_worktree_at_path(const char *path,\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    ++\tstrbuf_addf(&file, \"%s/locked\", repo.buf);\n    ++\tif (file_exists(file.buf)) {\n     +\t\tgoto done;\n     +\t}\n     +\tif (stat(gitdir.buf, &st)) {\n    @@ worktree.c: void repair_worktree_at_path(const char *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\tstrbuf_reset(&file);\n    ++\t\tstrbuf_addf(&file, \"%s/index\", repo.buf);\n    ++\t\tif (stat(file.buf, &st) || st.st_mtime <= expire) {\n                        strbuf_addstr(reason, _(\"gitdir file points to non-existent location\"));\n     -\t\t\tfree(path);\n     -\t\t\treturn 1;\n    @@ worktree.c: void repair_worktree_at_path(const char *path,\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    ++\tstrbuf_release(&file);\n     +\treturn rc;\n      }\n      \n      static int move_config_setting(const char *key, const char *value,\n    +\n    + ## worktree.h ##\n    +@@ worktree.h: 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 linked 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\n3:  2347affc96 < -:  ---------- worktree: sync worktree paths after gitdir move\n4:  64638e39c3 < -:  ---------- worktree: prevent null pointer dereference\n-:  ---------- > 3:  a3ea6625f2 worktree: add test for path handling in linked worktrees\n\n---\nCaleb White (3):\n      worktree: refactor infer_backlink() to use *strbuf\n      worktree: link worktrees with relative paths\n      worktree: add test for path handling in linked worktrees\n\n builtin/worktree.c           |  16 +--\n setup.c                      |   2 +-\n t/t2401-worktree-prune.sh    |  19 ++++\n t/t2408-worktree-relative.sh |  39 +++++++\n worktree.c                   | 253 ++++++++++++++++++++++++++++++++-----------\n worktree.h                   |  10 ++\n 6 files changed, 265 insertions(+), 74 deletions(-)\n---\nbase-commit: 4ec4435df758668055cc904ef64c275bc8d1089b\nchange-id: 20241007-wt_relative_paths-88f9cf5a070c\nprerequisite-message-id: <20240923075416.54289-1-ericsunshine@charter.net>\nprerequisite-patch-id: 78371f4dbb635e6edab8c51ee7857b903e60df8f\n\nBest regards,\n-- \nCaleb White <cdwhite3@pm.me>\n\n\n"},{"id":"504375","messageId":"20241007-wt_relative_paths-v3-1-622cf18c45eb@pm.me","threadId":"62276","inReplyTo":"20241007-wt_relative_paths-v3-0-622cf18c45eb@pm.me","subject":"[PATCH v3 1/3] worktree: refactor infer_backlink() to use *strbuf","fromName":"Caleb White via B4 Relay","fromEmail":"devnull+cdwhite3.pm.me@kernel.org","sentAt":"2024-10-08T03:12:30Z","receivedAt":"2024-10-08T03:12:44Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"From: Caleb White <cdwhite3@pm.me>\n\nThis lays the groundwork for the next patch, which needs the backlink\nreturned from infer_backlink() as a `strbuf`. It seemed inefficient to\nconvert from `strbuf` to `char*` and back to `strbuf` again.\n\nThis refactors infer_backlink() to return an integer result and use a\npre-allocated `strbuf` for the inferred backlink path, replacing the\nprevious `char*` return type and improving efficiency.\n\nSigned-off-by: Caleb White <cdwhite3@pm.me>\n---\n worktree.c | 48 ++++++++++++++++++++++++------------------------\n 1 file changed, 24 insertions(+), 24 deletions(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex ec95ea2986107b3bc12d38b0825d7c6e87402bc6..0cba0d6e6e9ad02ace04a0301104a04a07cbef65 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(const char *gitfile, struct strbuf *inferred)\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,18 @@ 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_reset(inferred);\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 1;\n \n error:\n \tstrbuf_release(&actual);\n-\tstrbuf_release(&inferred);\n-\treturn NULL;\n+\tstrbuf_reset(inferred); /* clear invalid path */\n+\treturn 0;\n }\n \n /*\n@@ -680,10 +680,11 @@ 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 inferred_backlink = STRBUF_INIT;\n \tstruct strbuf gitdir = STRBUF_INIT;\n \tstruct strbuf olddotgit = STRBUF_INIT;\n-\tchar *backlink = NULL;\n-\tchar *inferred_backlink = NULL;\n+\tchar *dotgit_contents = NULL;\n \tconst char *repair = NULL;\n \tint err;\n \n@@ -699,23 +700,23 @@ void repair_worktree_at_path(const char *path,\n \t\tgoto done;\n \t}\n \n-\tinferred_backlink = infer_backlink(realdotgit.buf);\n-\tbacklink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));\n-\tif (err == READ_GITFILE_ERR_NOT_A_FILE) {\n+\tinfer_backlink(realdotgit.buf, &inferred_backlink);\n+\tdotgit_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));\n+\tif (dotgit_contents) {\n+\t\tstrbuf_addstr(&backlink, dotgit_contents);\n+\t} else if (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 (inferred_backlink) {\n+\t\tif (inferred_backlink.len) {\n \t\t\t/*\n \t\t\t * Worktree's .git file does not point at a repository\n \t\t\t * but we found a .git/worktrees/<id> in this\n \t\t\t * repository with the same <id> as recorded in the\n \t\t\t * worktree's .git file so make the worktree point at\n-\t\t\t * the discovered .git/worktrees/<id>. (Note: backlink\n-\t\t\t * is already NULL, so no need to free it first.)\n+\t\t\t * the discovered .git/worktrees/<id>.\n \t\t\t */\n-\t\t\tbacklink = inferred_backlink;\n-\t\t\tinferred_backlink = NULL;\n+\t\t\tstrbuf_swap(&backlink, &inferred_backlink);\n \t\t} else {\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@@ -743,13 +744,11 @@ void repair_worktree_at_path(const char *path,\n \t * in the \"copy\" repository. In this case, point the \"copy\" worktree's\n \t * .git file at the \"copy\" repository.\n \t */\n-\tif (inferred_backlink && fspathcmp(backlink, inferred_backlink)) {\n-\t\tfree(backlink);\n-\t\tbacklink = inferred_backlink;\n-\t\tinferred_backlink = NULL;\n+\tif (inferred_backlink.len && fspathcmp(backlink.buf, inferred_backlink.buf)) {\n+\t\tstrbuf_swap(&backlink, &inferred_backlink);\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@@ -763,9 +762,10 @@ 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(inferred_backlink);\n+\tfree(dotgit_contents);\n \tstrbuf_release(&olddotgit);\n+\tstrbuf_release(&backlink);\n+\tstrbuf_release(&inferred_backlink);\n \tstrbuf_release(&gitdir);\n \tstrbuf_release(&realdotgit);\n \tstrbuf_release(&dotgit);\n\n-- \n2.46.2\n\n\n"},{"id":"504376","messageId":"20241007-wt_relative_paths-v3-3-622cf18c45eb@pm.me","threadId":"62276","inReplyTo":"20241007-wt_relative_paths-v3-0-622cf18c45eb@pm.me","subject":"[PATCH v3 3/3] worktree: add test for path handling in linked worktrees","fromName":"Caleb White via B4 Relay","fromEmail":"devnull+cdwhite3.pm.me@kernel.org","sentAt":"2024-10-08T03:12:32Z","receivedAt":"2024-10-08T03:12:44Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"From: Caleb White <cdwhite3@pm.me>\n\nA failure scenario reported in an earlier patch series[1] that several\n`git worktree` subcommands failed or misbehaved when invoked from within\nlinked worktrees that used relative paths.\n\nThis adds a test that executes a `worktree prune` command inside both an\ninternally and an externally linked worktree and asserts that the other\nworktree was not pruned.\n\n[1]: https://lore.kernel.org/git/CAPig+cQXFy=xPVpoSq6Wq0pxMRCjS=WbkgdO+3LySPX=q0nPCw@mail.gmail.com/\n\nReported-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Caleb White <cdwhite3@pm.me>\n---\n t/t2401-worktree-prune.sh | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/t/t2401-worktree-prune.sh b/t/t2401-worktree-prune.sh\nindex 71aa9bcd620f8a504fe628a2d7332d8b47fd4701..976d048e3efc74be9cd909ce76d552b3944d2e10 100755\n--- a/t/t2401-worktree-prune.sh\n+++ b/t/t2401-worktree-prune.sh\n@@ -120,4 +120,23 @@ test_expect_success 'prune duplicate (main/linked)' '\n \t! test -d .git/worktrees/wt\n '\n \n+test_expect_success 'not prune proper worktrees when run inside linked worktree' '\n+\ttest_when_finished rm -rf repo wt_ext &&\n+\tgit init repo &&\n+\t(\n+\t    cd repo &&\n+\t    echo content >file &&\n+\t    git add file &&\n+\t    git commit -m msg &&\n+\t    git worktree add ../wt_ext &&\n+\t    git worktree add wt_int &&\n+\t    cd wt_int &&\n+\t    git worktree prune -v >out &&\n+\t    test_must_be_empty out &&\n+\t    cd ../../wt_ext &&\n+\t    git worktree prune -v >out &&\n+\t    test_must_be_empty out\n+\t)\n+'\n+\n test_done\n\n-- \n2.46.2\n\n\n"},{"id":"504377","messageId":"20241007-wt_relative_paths-v3-2-622cf18c45eb@pm.me","threadId":"62276","inReplyTo":"20241007-wt_relative_paths-v3-0-622cf18c45eb@pm.me","subject":"[PATCH v3 2/3] worktree: link worktrees with relative paths","fromName":"Caleb White via B4 Relay","fromEmail":"devnull+cdwhite3.pm.me@kernel.org","sentAt":"2024-10-08T03:12:31Z","receivedAt":"2024-10-08T03:12:44Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"From: Caleb White <cdwhite3@pm.me>\n\nGit currently stores absolute paths to both the main repository and\nlinked worktrees. However, this causes problems when moving repositories\nor working in containerized environments where absolute paths differ\nbetween systems. The worktree links break, and users are required to\nmanually execute `worktree repair` to repair them, leading to workflow\ndisruptions. Additionally, mapping repositories inside of containerized\nenvironments renders the repository unusable inside the containers, and\nthis is not repairable as repairing the worktrees inside the containers\nwill result in them being broken outside the containers.\n\nTo address this, this patch makes Git always write relative paths when\nlinking worktrees. Relative paths increase the resilience of the\nworktree links across various systems and environments, particularly\nwhen the worktrees are self-contained inside the main repository (such\nas when using a bare repository with worktrees). This improves\nportability, workflow efficiency, and reduces overall breakages.\n\nAlthough Git now writes relative paths, existing repositories with\nabsolute paths are still supported. There are no breaking changes\nto workflows based on absolute paths, ensuring backward compatibility.\n\nAt a low level, the changes involve modifying functions in `worktree.c`\nand `builtin/worktree.c` to use `relative_path()` when writing the\nworktree’s `.git` file and the main repository’s `gitdir` reference.\nInstead of hardcoding absolute paths, Git now computes the relative path\nbetween the worktree and the repository, ensuring that these links are\nportable. Locations where these respective file are read have also been\nupdated to properly handle both absolute and relative paths. Generally,\nrelative paths are always resolved into absolute paths before any\noperations or comparisons are performed.\n\nAdditionally, `repair_worktrees_after_gitdir_move()` has been introduced\nto address the case where both the `<worktree>/.git` and\n`<repo>/worktrees/<id>/gitdir` links are broken after the gitdir is\nmoved (such as during a re-initialization). This function repairs both\nsides of the worktree link using the old gitdir path to reestablish the\ncorrect paths after a move.\n\nThe `worktree.path` struct member has also been updated to always store\nthe absolute path of a worktree. This ensures that worktree consumers\nnever have to worry about trying to resolve the absolute path themselves.\n\nSigned-off-by: Caleb White <cdwhite3@pm.me>\n---\n builtin/worktree.c           |  16 ++--\n setup.c                      |   2 +-\n t/t2408-worktree-relative.sh |  39 ++++++++\n worktree.c                   | 207 ++++++++++++++++++++++++++++++++++---------\n worktree.h                   |  10 +++\n 5 files changed, 223 insertions(+), 51 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex fc31d072a620d7b455d7f150bd3a9e773ee9d4ed..dae63dedf4cac2621f51f95a39aa456b33acd894 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,10 @@ 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+\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 +578,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/setup.c b/setup.c\nindex 94e79b2e487f3faa537547e190acf9b7ea0be3b5..7b648de0279116b381eea46800ad130606926103 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/t/t2408-worktree-relative.sh b/t/t2408-worktree-relative.sh\nnew file mode 100755\nindex 0000000000000000000000000000000000000000..a3136db7e28cb20926ff44211e246ce625a6e51a\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 0cba0d6e6e9ad02ace04a0301104a04a07cbef65..77ff484d3ec48c547ee4e3d958dfa28a52c1eaa7 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,29 @@ 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 file = STRBUF_INIT;\n+\tstruct strbuf tmp = STRBUF_INIT;\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\tstrbuf_addf(&file, \"%s/gitdir\", repo.buf);\n+\t\twrite_file(file.buf, \"%s/.git\", relative_path(path.buf, repo.buf, &tmp));\n+\t\tstrbuf_reset(&file);\n+\t\tstrbuf_addf(&file, \"%s/.git\", path.buf);\n+\t\twrite_file(file.buf, \"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 \tstrbuf_release(&path);\n+\tstrbuf_release(&repo);\n+\tstrbuf_release(&file);\n+\tstrbuf_release(&tmp);\n }\n \n int is_worktree_being_rebased(const struct worktree *wt,\n@@ -564,38 +581,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 *dotgit_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+\tdotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));\n+\n+\tif (dotgit_contents) {\n+\t\tif (is_absolute_path(dotgit_contents)) {\n+\t\t\tstrbuf_addstr(&backlink, dotgit_contents);\n+\t\t} else {\n+\t\t\tstrbuf_addf(&backlink, \"%s/%s\", wt->path, dotgit_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(dotgit_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@@ -618,6 +649,59 @@ 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_INIT;\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+\twrite_file(gitdir.buf, \"%s\", relative_path(dotgit.buf, repo.buf, &tmp));\n+done:\n+\tstrbuf_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;\n@@ -684,6 +768,8 @@ void repair_worktree_at_path(const char *path,\n \tstruct strbuf inferred_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 *dotgit_contents = NULL;\n \tconst char *repair = NULL;\n \tint err;\n@@ -701,9 +787,17 @@ void repair_worktree_at_path(const char *path,\n \t}\n \n \tinfer_backlink(realdotgit.buf, &inferred_backlink);\n+\tstrbuf_realpath_forgiving(&inferred_backlink, inferred_backlink.buf, 0);\n \tdotgit_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));\n \tif (dotgit_contents) {\n-\t\tstrbuf_addstr(&backlink, dotgit_contents);\n+\t\tif (is_absolute_path(dotgit_contents)) {\n+\t\t\tstrbuf_addstr(&backlink, dotgit_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, dotgit_contents);\n+\t\t\tstrbuf_realpath_forgiving(&backlink, backlink.buf, 0);\n+\t\t}\n \t} else if (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@@ -721,7 +815,7 @@ void repair_worktree_at_path(const char *path,\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} else {\n \t\tfn(1, realdotgit.buf, _(\"unable to locate repository; .git file broken\"), cb_data);\n \t\tgoto done;\n \t}\n@@ -753,90 +847,117 @@ void repair_worktree_at_path(const char *path,\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(dotgit_contents);\n \tstrbuf_release(&olddotgit);\n+\tstrbuf_release(&realolddotgit);\n \tstrbuf_release(&backlink);\n \tstrbuf_release(&inferred_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+\tstruct strbuf file = STRBUF_INIT;\n+\tchar *path = 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+\tstrbuf_addf(&file, \"%s/locked\", repo.buf);\n+\tif (file_exists(file.buf)) {\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\tstrbuf_reset(&file);\n+\t\tstrbuf_addf(&file, \"%s/index\", repo.buf);\n+\t\tif (stat(file.buf, &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+\tstrbuf_release(&dotgit);\n+\tstrbuf_release(&gitdir);\n+\tstrbuf_release(&repo);\n+\tstrbuf_release(&file);\n+\treturn rc;\n }\n \n static int move_config_setting(const char *key, const char *value,\ndiff --git a/worktree.h b/worktree.h\nindex 11279d0c8fe249bb30642563bf221a8de7f3b0a3..e96118621638667d87c5d7e0452ed10bd1ddf606 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 linked 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\n-- \n2.46.2\n\n\n"},{"id":"504390","messageId":"u6rg54FdIvyFm4NUV46fJMur4T3aUYUpz2Pim2cw3gHd1966FysdkWjm_awEmjoWrFo8KNmC3_IL1nLmFquHNNtMc106AyMmUvNgZScK84o=@pm.me","threadId":"62276","inReplyTo":"20241007-wt_relative_paths-v3-0-622cf18c45eb@pm.me","subject":"Re: [PATCH v3 0/3] Link worktrees with relative paths","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-08T04:48:34Z","receivedAt":"2024-10-08T04:48:40Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Monday, October 7th, 2024 at 22:12, Caleb White via B4 Relay <devnull+cdwhite3.pm.me@kernel.org> wrote:\n\n> Changes in v3:\n> - Squashed patch [3/4] into patch [2/4] to streamline changes.\n> - Dropped patch [4/4] as it was no longer necessary.\n> - Rebased onto 20240923075416.54289-1-ericsunshine@charter.net\n> - Updated `infer_backlink()` to return 1 on success for consistency.\n> - Swapped the order of `infer_backlink()` arguments for clarity.\n> - Clear `inferred` if error occurs in `infer_backlink()`.\n> - Renamed `git_contents` to `dotgit_contents` for clearer semantics.\n> - Cleaned up `dotgit_contents` logic in `repair_worktree_at_path()`.\n> - Replaced multiple `xstrfmt/free` calls with a single `strbuf`.\n> - Added a test case covering a failure noted in a separate patch series.\n> - Improved commit messages.\n> - Link to v2: https://lore.kernel.org/r/20241006060017.171788-1-cdwhite3@pm.me\n\nAll, my apologies for the patch corruption in the first two revisions (I suspect\nit was due to my SMTP server). I've submitted this v3 series with b4 using the\npublic submission endpoint and I think it turned out much better. But please let\nme know if you have any issues.\n\nThanks!"},{"id":"504474","messageId":"xmqq5xq21kub.fsf@gitster.g","threadId":"62276","inReplyTo":"20241007-wt_relative_paths-v3-0-622cf18c45eb@pm.me","subject":"Re: [PATCH v3 0/3] Link worktrees with relative paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-08T19:00:12Z","receivedAt":"2024-10-08T19:00:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Caleb White via B4 Relay <devnull+cdwhite3.pm.me@kernel.org> writes:\n\n> - Rebased onto 20240923075416.54289-1-ericsunshine@charter.net\n> ...\n> base-commit: 4ec4435df758668055cc904ef64c275bc8d1089b\n> change-id: 20241007-wt_relative_paths-88f9cf5a070c\n> prerequisite-message-id: <20240923075416.54289-1-ericsunshine@charter.net>\n> prerequisite-patch-id: 78371f4dbb635e6edab8c51ee7857b903e60df8f\n\nIt is more common on this list to explain how the base was prepared\nin the cover letter, something like:\n\n    The patch was built on the result of merging the\n    es/worktree-repair-copied topic, i.e. 992f7a4f (worktree: repair\n    copied repository and linked worktrees, 2024-09-23), to v2.47.0.\n\nTo help those who are reading from sidelines, the above may need\nsome deciphering, as the patches as posted do not cleanly apply to\nthe base-commit 4ec4435df, which is 777489f9 (Git 2.47, 2024-10-06).\n\n    $ git grep '<2024092307...charter.net>' notes/amlog\n\nfinds that the message resulted in 992f7a4f (worktree: repair copied\nrepository and linked worktrees, 2024-09-23).  So the right base to\napply them is obtained by checking out the v2.47.0 and then merging\nthe es/worktree-repair-copied topic (which is a single commit topic\nwith the 992f7a4f on it).\n\n    $ git checkout --detach 4ec4435df758^0\n    $ git merge --into cw/worktree-relative es/worktree-repair-copied\n    $ git am -s ./+cw-worktree-relative-3-patch-series\n\nAnd of course the series applied cleanly ;-)  I haven't read them\nyet, though.\n\nThanks.\n\n"},{"id":"504493","messageId":"qBPeXsmRWeJG-VRJyU_75S-f6dZWTlXQx0PjXmc9FMOrciammmLNivZ6kRt3mWuWC9x9W65FeU2Qdc99VvXGccLtdxnMctxeJqN92g41l40=@pm.me","threadId":"62276","inReplyTo":"xmqq5xq21kub.fsf@gitster.g","subject":"Re: [PATCH v3 0/3] Link worktrees with relative paths","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-08T21:55:45Z","receivedAt":"2024-10-08T21:55:56Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Tuesday, October 8th, 2024 at 14:00, Junio C Hamano <gitster@pobox.com> wrote:\n\n> Caleb White via B4 Relay devnull+cdwhite3.pm.me@kernel.org writes:\n> \n\n> > - Rebased onto 20240923075416.54289-1-ericsunshine@charter.net\n> > ...\n> > base-commit: 4ec4435df758668055cc904ef64c275bc8d1089b\n> > change-id: 20241007-wt_relative_paths-88f9cf5a070c\n> > prerequisite-message-id: 20240923075416.54289-1-ericsunshine@charter.net\n> > prerequisite-patch-id: 78371f4dbb635e6edab8c51ee7857b903e60df8f\n> \n\n> \n\n> It is more common on this list to explain how the base was prepared\n> in the cover letter, something like:\n> \n\n> The patch was built on the result of merging the\n> es/worktree-repair-copied topic, i.e. 992f7a4f (worktree: repair\n> copied repository and linked worktrees, 2024-09-23), to v2.47.0.\n> \n\n> To help those who are reading from sidelines, the above may need\n> some deciphering, as the patches as posted do not cleanly apply to\n> the base-commit 4ec4435df, which is 777489f9 (Git 2.47, 2024-10-06).\n> \n\n> $ git grep '<2024092307...charter.net>' notes/amlog\n> \n\n> \n\n> finds that the message resulted in 992f7a4f (worktree: repair copied\n> repository and linked worktrees, 2024-09-23). So the right base to\n> apply them is obtained by checking out the v2.47.0 and then merging\n> the es/worktree-repair-copied topic (which is a single commit topic\n> with the 992f7a4f on it).\n> \n\n> $ git checkout --detach 4ec4435df758^0\n> $ git merge --into cw/worktree-relative es/worktree-repair-copied\n> $ git am -s ./+cw-worktree-relative-3-patch-series\n\nAh, thank you, I'll make preparing the base more clear in the next revision.\nThis is just how b4 tracks things so it knows how to apply the deps.\n\n> And of course the series applied cleanly ;-) I haven't read them\n> yet, though.\n\nThat's good to hear :)\n\n"},{"id":"504497","messageId":"xmqqmsjexkcg.fsf@gitster.g","threadId":"62276","inReplyTo":"20241007-wt_relative_paths-v3-2-622cf18c45eb@pm.me","subject":"Re: [PATCH v3 2/3] worktree: link worktrees with relative paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-08T23:09:51Z","receivedAt":"2024-10-08T23:09:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Caleb White via B4 Relay <devnull+cdwhite3.pm.me@kernel.org> writes:\n\n> From: Caleb White <cdwhite3@pm.me>\n>\n> Git currently stores absolute paths to both the main repository and\n\nAs always, drop \"currently\".\n\nThe usual way to compose a log message of this project is to\n\n - Give an observation on how the current system work in the present\n   tense (so no need to say \"Currently X is Y\", just \"X is Y\"), and\n   discuss what you perceive as a problem in it.\n\n - Propose a solution (optional---often, problem description\n   trivially leads to an obvious solution in reader's minds).\n\n - Give commands to the codebase to \"become like so\".\n\nin this order.\n\n> linked worktrees. However, this causes problems when moving repositories\n\nThe above claim is way too strong.  When relative references are\nused, you can move directories without problems only if you move all\nthe worktrees and the repository in unison without breaking their\nrelative locations, which is an exception and not the norm.\n\n    The repository knows where its extra worktrees are by their\n    absolute paths (and vice versa) to allow discovery of each\n    other.  When a directory is moved, \"git worktree repair\" must be\n    used to adjust these references.\n\n    It however becomes possible, in a narrow special case, to do\n    without \"git worktree repair\".  When the repository and the\n    worktrees move in parallel without breaking their relative\n    location, the repair operation becomes unneeded if we made them\n    refer to each other using relative paths (think: tarring up both\n    the reposity and the worktrees at the same time, and then\n    untarring the result at a different place).\n\nor something, perhaps.\n\n> Although Git now writes relative paths, existing repositories with\n> absolute paths are still supported. There are no breaking changes\n> to workflows based on absolute paths, ensuring backward compatibility.\n\nGood.  Being able to work with existing repositories is an absolute\nrequirement.  Are there good test cases?  I would imagine that you\nwould need to force a worktree and its repository to refer to each\nother using absolute paths and then try to access with the updated\ncode, perhaps moving one of such \"old-style\" directory and then\nusing the updated \"git worktree repair\" and observing the result.\nTo allow such a test possible, you might even need an option to \"git\nworktree\" to allow it to create these linkages using absolute paths,\nnot relative (and if you need to keep both possibilities anyway, you\nmight make the newer \"relative\" layout an optional feature,\ntriggered by a command line option given to \"git worktree add\" and\nfriends).\n\nHave we considered how badly existing third-party tools, that has\nlearned to assume that the paths are aboslute, may break with this\nchange, though?  Or is this \"we won't know until we inflict it on\nreal users and see who screams\"?\n\nThanks.\n"},{"id":"504535","messageId":"de748e18-d3c1-4c34-a2c8-086271c208dd@gmail.com","threadId":"62276","inReplyTo":"20241007-wt_relative_paths-v3-2-622cf18c45eb@pm.me","subject":"Re: [PATCH v3 2/3] worktree: link worktrees with relative paths","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-10-09T10:11:44Z","receivedAt":"2024-10-09T10:11:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Caleb\n\nOn 08/10/2024 04:12, Caleb White via B4 Relay wrote:\n> From: Caleb White <cdwhite3@pm.me>\n> \n> Git currently stores absolute paths to both the main repository and\n> linked worktrees. However, this causes problems when moving repositories\n> or working in containerized environments where absolute paths differ\n> between systems. The worktree links break, and users are required to\n> manually execute `worktree repair` to repair them, leading to workflow\n> disruptions. Additionally, mapping repositories inside of containerized\n> environments renders the repository unusable inside the containers, and\n> this is not repairable as repairing the worktrees inside the containers\n> will result in them being broken outside the containers.\n> \n> To address this, this patch makes Git always write relative paths when\n> linking worktrees. Relative paths increase the resilience of the\n> worktree links across various systems and environments, particularly\n> when the worktrees are self-contained inside the main repository (such\n> as when using a bare repository with worktrees). This improves\n> portability, workflow efficiency, and reduces overall breakages.\n\nThis is quite a sweeping claim, it would be helpful to describe the \ntrade off that this patch is making. I've not been following the \ndiscussion closely but as I understand it while there are cases such as \nsharing a drive between Windows and linux or moving all the worktrees \nincluding the main one together where using relative paths prevents \nproblems there are other cases such as moving a single worktree that are \nfixable by running \"git worktree repair\" when using absolute paths but \nnot with relative paths.\n\nIt was suggested in another thread I saw recently that storing both \nwould be more resiliant.\n\nAnother possibility would be to store the an absolute path in the \nworktree's .git file and relative paths in worktrees/*/gitdir. That \nwould enable \"git worktree repair\" run in a moved worktree to find the \nmain worktree if the main worktree has not been moved as it does now. It \nwould also allow \"git worktree repair\" run in the main worktree that has \nbeen moved to find the linked worktrees that were moved in tandam with it.\n\nBest Wishes\n\nPhillip\n\n\n> Although Git now writes relative paths, existing repositories with\n> absolute paths are still supported. There are no breaking changes\n> to workflows based on absolute paths, ensuring backward compatibility.\n> \n> At a low level, the changes involve modifying functions in `worktree.c`\n> and `builtin/worktree.c` to use `relative_path()` when writing the\n> worktree’s `.git` file and the main repository’s `gitdir` reference.\n> Instead of hardcoding absolute paths, Git now computes the relative path\n> between the worktree and the repository, ensuring that these links are\n> portable. Locations where these respective file are read have also been\n> updated to properly handle both absolute and relative paths. Generally,\n> relative paths are always resolved into absolute paths before any\n> operations or comparisons are performed.\n> \n> Additionally, `repair_worktrees_after_gitdir_move()` has been introduced\n> to address the case where both the `<worktree>/.git` and\n> `<repo>/worktrees/<id>/gitdir` links are broken after the gitdir is\n> moved (such as during a re-initialization). This function repairs both\n> sides of the worktree link using the old gitdir path to reestablish the\n> correct paths after a move.\n> \n> The `worktree.path` struct member has also been updated to always store\n> the absolute path of a worktree. This ensures that worktree consumers\n> never have to worry about trying to resolve the absolute path themselves.\n> \n> Signed-off-by: Caleb White <cdwhite3@pm.me>\n> ---\n>   builtin/worktree.c           |  16 ++--\n>   setup.c                      |   2 +-\n>   t/t2408-worktree-relative.sh |  39 ++++++++\n>   worktree.c                   | 207 ++++++++++++++++++++++++++++++++++---------\n>   worktree.h                   |  10 +++\n>   5 files changed, 223 insertions(+), 51 deletions(-)\n> \n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index fc31d072a620d7b455d7f150bd3a9e773ee9d4ed..dae63dedf4cac2621f51f95a39aa456b33acd894 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,10 @@ 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> +\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 +578,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>   }\n> diff --git a/setup.c b/setup.c\n> index 94e79b2e487f3faa537547e190acf9b7ea0be3b5..7b648de0279116b381eea46800ad130606926103 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);\n> diff --git a/t/t2408-worktree-relative.sh b/t/t2408-worktree-relative.sh\n> new file mode 100755\n> index 0000000000000000000000000000000000000000..a3136db7e28cb20926ff44211e246ce625a6e51a\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\n> diff --git a/worktree.c b/worktree.c\n> index 0cba0d6e6e9ad02ace04a0301104a04a07cbef65..77ff484d3ec48c547ee4e3d958dfa28a52c1eaa7 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,29 @@ 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 file = STRBUF_INIT;\n> +\tstruct strbuf tmp = STRBUF_INIT;\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\tstrbuf_addf(&file, \"%s/gitdir\", repo.buf);\n> +\t\twrite_file(file.buf, \"%s/.git\", relative_path(path.buf, repo.buf, &tmp));\n> +\t\tstrbuf_reset(&file);\n> +\t\tstrbuf_addf(&file, \"%s/.git\", path.buf);\n> +\t\twrite_file(file.buf, \"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>   \tstrbuf_release(&path);\n> +\tstrbuf_release(&repo);\n> +\tstrbuf_release(&file);\n> +\tstrbuf_release(&tmp);\n>   }\n>   \n>   int is_worktree_being_rebased(const struct worktree *wt,\n> @@ -564,38 +581,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 *dotgit_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> +\tdotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));\n> +\n> +\tif (dotgit_contents) {\n> +\t\tif (is_absolute_path(dotgit_contents)) {\n> +\t\t\tstrbuf_addstr(&backlink, dotgit_contents);\n> +\t\t} else {\n> +\t\t\tstrbuf_addf(&backlink, \"%s/%s\", wt->path, dotgit_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(dotgit_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> @@ -618,6 +649,59 @@ 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_INIT;\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> +\twrite_file(gitdir.buf, \"%s\", relative_path(dotgit.buf, repo.buf, &tmp));\n> +done:\n> +\tstrbuf_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;\n> @@ -684,6 +768,8 @@ void repair_worktree_at_path(const char *path,\n>   \tstruct strbuf inferred_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 *dotgit_contents = NULL;\n>   \tconst char *repair = NULL;\n>   \tint err;\n> @@ -701,9 +787,17 @@ void repair_worktree_at_path(const char *path,\n>   \t}\n>   \n>   \tinfer_backlink(realdotgit.buf, &inferred_backlink);\n> +\tstrbuf_realpath_forgiving(&inferred_backlink, inferred_backlink.buf, 0);\n>   \tdotgit_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));\n>   \tif (dotgit_contents) {\n> -\t\tstrbuf_addstr(&backlink, dotgit_contents);\n> +\t\tif (is_absolute_path(dotgit_contents)) {\n> +\t\t\tstrbuf_addstr(&backlink, dotgit_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, dotgit_contents);\n> +\t\t\tstrbuf_realpath_forgiving(&backlink, backlink.buf, 0);\n> +\t\t}\n>   \t} else if (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> @@ -721,7 +815,7 @@ void repair_worktree_at_path(const char *path,\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} else {\n>   \t\tfn(1, realdotgit.buf, _(\"unable to locate repository; .git file broken\"), cb_data);\n>   \t\tgoto done;\n>   \t}\n> @@ -753,90 +847,117 @@ void repair_worktree_at_path(const char *path,\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(dotgit_contents);\n>   \tstrbuf_release(&olddotgit);\n> +\tstrbuf_release(&realolddotgit);\n>   \tstrbuf_release(&backlink);\n>   \tstrbuf_release(&inferred_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> +\tstruct strbuf file = STRBUF_INIT;\n> +\tchar *path = 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> +\tstrbuf_addf(&file, \"%s/locked\", repo.buf);\n> +\tif (file_exists(file.buf)) {\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\tstrbuf_reset(&file);\n> +\t\tstrbuf_addf(&file, \"%s/index\", repo.buf);\n> +\t\tif (stat(file.buf, &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> +\tstrbuf_release(&dotgit);\n> +\tstrbuf_release(&gitdir);\n> +\tstrbuf_release(&repo);\n> +\tstrbuf_release(&file);\n> +\treturn rc;\n>   }\n>   \n>   static int move_config_setting(const char *key, const char *value,\n> diff --git a/worktree.h b/worktree.h\n> index 11279d0c8fe249bb30642563bf221a8de7f3b0a3..e96118621638667d87c5d7e0452ed10bd1ddf606 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 linked 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> \n\n"},{"id":"504624","messageId":"k3X5W4US76LBJ_IUq-quVRha2jd-4iWJ9yX6Ukh6-ifZdWC3iajoUJ8VUyTDfkJHSqiD1RJlqIuVlDGIsReR_SDREVWyHGIqsXhazvJu1ek=@pm.me","threadId":"62276","inReplyTo":"xmqqmsjexkcg.fsf@gitster.g","subject":"Re: [PATCH v3 2/3] worktree: link worktrees with relative paths","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-09T18:34:10Z","receivedAt":"2024-10-09T18:34:23Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Tuesday, October 8th, 2024 at 18:09, Junio C Hamano <gitster@pobox.com> wrote:\n\n> Caleb White via B4 Relay devnull+cdwhite3.pm.me@kernel.org writes:\n>\n> > From: Caleb White cdwhite3@pm.me\n> >\n> > Git currently stores absolute paths to both the main repository and\n>\n>\n> As always, drop \"currently\".\n>\n> The usual way to compose a log message of this project is to\n>\n> - Give an observation on how the current system work in the present\n> tense (so no need to say \"Currently X is Y\", just \"X is Y\"), and\n> discuss what you perceive as a problem in it.\n>\n> - Propose a solution (optional---often, problem description\n> trivially leads to an obvious solution in reader's minds).\n>\n> - Give commands to the codebase to \"become like so\".\n>\n> in this order.\n\nThank you for the feedback, I'll reword this. This is my first patch series\nto this project (and my first mailing list patch submission, so I've been\nlearning a bunch), thank you for bearing with me! :)\n\n> > linked worktrees. However, this causes problems when moving repositories\n>\n> The above claim is way too strong. When relative references are\n> used, you can move directories without problems only if you move all\n> the worktrees and the repository in unison without breaking their\n> relative locations, which is an exception and not the norm.\n>\n> The repository knows where its extra worktrees are by their\n> absolute paths (and vice versa) to allow discovery of each\n> other. When a directory is moved, \"git worktree repair\" must be\n> used to adjust these references.\n>\n> It however becomes possible, in a narrow special case, to do\n> without \"git worktree repair\". When the repository and the\n> worktrees move in parallel without breaking their relative\n> location, the repair operation becomes unneeded if we made them\n> refer to each other using relative paths (think: tarring up both\n> the reposity and the worktrees at the same time, and then\n> untarring the result at a different place).\n>\n> or something, perhaps.\n\nI can reduce the strength of the language and add some better examples,\nhowever, I believe this scenario is becoming more common. There's been\na rise in popularity with using worktrees in conjunction with bare\nrepositories that take the following form:\n\n    /path/to/repo/\n        .git/ (bare repo)\n        master/\n        develop/\n        feature1/\n        hotfix/\n\nEssentially, you create a directory to act as the the \"repository\",\nthen you `clone --bare` the actual repository to `.git`, and you\ncreate worktrees (there's some other steps that need to be performed\nsuch as updating the refs to point to origin, but this is essentially\nit) as needed. This pattern has the following advantages:\n1. For high volume projects, this allows you to use worktrees to quickly\n   switch between contexts. I might be working on a feature at work\n   when I quickly need to switch focus to hotfix something that broke\n   in production and I don't have to worry about committing / stashing.\n2. This avoids polluting the parent directory with the different\n   worktrees. I tend to have a directory that houses the various\n   repos I work on, and I don't want to have worktrees intermixed\n   with other repos.\n3. This keeps the `repo` directory clean so you don't have worktrees\n   intermixed with your source code on the main working tree.\n\nIn this pattern, everything is self contained inside the `repo` dir.\nMoving this directory or tarring / untarring it (as you mentioned)\nshould not cause the links to break and require repair. Granted,\nthis is not the only use-case which is why both absolute and\nrelative paths should be supported.\n\n> > Although Git now writes relative paths, existing repositories with\n> > absolute paths are still supported. There are no breaking changes\n> > to workflows based on absolute paths, ensuring backward compatibility.\n>\n> Good. Being able to work with existing repositories is an absolute\n> requirement. Are there good test cases? I would imagine that you\n> would need to force a worktree and its repository to refer to each\n> other using absolute paths and then try to access with the updated\n> code, perhaps moving one of such \"old-style\" directory and then\n> using the updated \"git worktree repair\" and observing the result.\n> To allow such a test possible, you might even need an option to \"git\n> worktree\" to allow it to create these linkages using absolute paths,\n> not relative (and if you need to keep both possibilities anyway, you\n> might make the newer \"relative\" layout an optional feature,\n> triggered by a command line option given to \"git worktree add\" and\n> friends).\n\nThe tests passed before and after, but there's no tests that explicitly\nuse absolute paths. I can add a `worktree.useRelativePaths` config/cli\noption. I was trying to avoid this, but this may be best to offer users\nthe most flexibility. With the addition of this config value, I might\nsplit patch [2/3] into two patches: the first will handle reading\npotentially relative paths from the files, and the second will add the\nconfig option and handle the writing of relative paths to the files.\n\nWhat's the best way to parameterize the worktree tests? I would like\nto run the same tests for both absolute and relative paths and I'm\nnot particularly a fan of just copying them all into new *-relative.sh\nfiles.\n\n> Have we considered how badly existing third-party tools, that has\n> learned to assume that the paths are aboslute, may break with this\n> change, though? Or is this \"we won't know until we inflict it on\n> real users and see who screams\"?\n\nThe `worktree` struct that's used internally is populated with the\nabsolute path, and `worktree list` and the porcelain options return\nthe absolute paths to the user, so third-party tools should not notice\na difference if they're using the api. The worktree docs even include\nthe following warning:\n\n    The rule of thumb is do not make any assumption about whether a path belongs\n    to $GIT_DIR or $GIT_COMMON_DIR when you need to directly access something\n    inside $GIT_DIR. Use git rev-parse --git-path to get the final path.\n\nso third-party tools really shouldn't be reading the files, and I'm not\naware of any that do, but we can monitor for feedback once users start\nusing the change.\n\n\nBest,"},{"id":"504628","messageId":"18nudmdBF3K-9cMiTynl4-amBudFtwKaaZtIkF4E86BwxmjYNV_ISMihIddKiwYZBTEXSbJ2XFliyXDwaGs94pn3Xj8-V8QtggsTgFXl4to=@pm.me","threadId":"62276","inReplyTo":"de748e18-d3c1-4c34-a2c8-086271c208dd@gmail.com","subject":"Re: [PATCH v3 2/3] worktree: link worktrees with relative paths","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-09T18:49:49Z","receivedAt":"2024-10-09T18:49:56Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Wednesday, October 9th, 2024 at 05:11, Phillip Wood <phillip.wood123@gmail.com> wrote:\n\n> This is quite a sweeping claim, it would be helpful to describe the\n> trade off that this patch is making.\n\nThank you for your feedback, I'll adjust the language and add some\nadditional examples.\n\n> but as I understand it while there are cases such as\n> sharing a drive between Windows and linux or moving all the worktrees\n> including the main one together where using relative paths prevents problems\n\nThis is true, there are several cases where relative paths prevent breakages.\n\n> there are other cases such as moving a single worktree that are\n> fixable by running \"git worktree repair\" when using absolute paths but\n> not with relative paths.\n\nThis is not exactly true, if only the **repository** is moved then yes it\nwill no longer be able to find it's worktrees without the absolute path,\nhowever, `git worktree repair` can still repair the worktree when provided\nthe path to the worktree (or the directory containing the worktrees I believe).\n\n> It was suggested in another thread I saw recently that storing both\n> would be more resiliant.\n\nI'm not too sure what you mean here, but storing both the absolute\nand relative paths in both files seems like an over-complication\n(not sure if that's what you were talking about or not). However,\nthis patch series does support the linking files containing either\nabsolute or relative paths.\n\n> Another possibility would be to store the an absolute path in the\n> worktree's .git file and relative paths in worktrees/*/gitdir. That\n> would enable \"git worktree repair\" run in a moved worktree to find the\n> main worktree if the main worktree has not been moved as it does now. It\n> would also allow \"git worktree repair\" run in the main worktree that has\n> been moved to find the linked worktrees that were moved in tandam with it.\n\nStoring the absolute path in the `.git` and relative paths in the `gitdir`\nwill not work---it will still suffer from the same limitations that we are\ntrying to avoid. And all the test cases related to `worktree repair` are\npassing with relative paths.\n\nBest,"},{"id":"504631","messageId":"xmqqy12xqehd.fsf@gitster.g","threadId":"62276","inReplyTo":"k3X5W4US76LBJ_IUq-quVRha2jd-4iWJ9yX6Ukh6-ifZdWC3iajoUJ8VUyTDfkJHSqiD1RJlqIuVlDGIsReR_SDREVWyHGIqsXhazvJu1ek=@pm.me","subject":"Re: [PATCH v3 2/3] worktree: link worktrees with relative paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-09T19:10:38Z","receivedAt":"2024-10-09T19:10:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Caleb White <cdwhite3@pm.me> writes:\n\n> What's the best way to parameterize the worktree tests? I would like\n> to run the same tests for both absolute and relative paths and I'm\n> not particularly a fan of just copying them all into new *-relative.sh\n> files.\n\nWhat I meant by interoperability tests are a lot smaller scale.\n\nA test that creates worktree/repository pair without the option to\nuse relative, and then tries to use such a worktree/repository pair\nwith the option would simulate \"how well the newer Git handles an\nexisting repository\", and another test that creates with the option\nto use relative and uses the worktree/repository without the option\nwould simulate \"how well existing versions of Git works when seeing\na worktree made with the newer git with the relative option\".\n\nBy \"parameterise\", if you mean running a set of worktree/repository\ntests without the \"relative\" option enabled, and run the same set of\ntests with the option enabled, you could model it after how t8001\nand t8002 (or t5560 and t5561) share a lot of same tests that are in\na file that is included by both of them.  In smaller scale, it is\ncommon to have an ad-hoc construct like:\n\n\tfor conf in relative absolute\n\tdo\n\t\ttest_expect_success ... \n\t\ttest_expect_success ... \n\t\ttest_expect_success ... \n\tdone\n\nthat has bunch of test_expect_success, which may change the\nbehaviour depending on the value of $conf, not &&-chained inside the\nfor loop.  You can use a nested loop (one for preparing, the other\nfor testing the use of worktree) if you want to test the full\nmatrix.\n\nI do not offhand know if such parametralized tests are necessary in\nthe context of this change, though.\n\nThanks.\n"},{"id":"504632","messageId":"oUBKv4JI3Bf1nqAFU_C-HDl5vkZC6mg8mb5F0HB7akXC6n0T1ddV0LiOCqY_AGHCyr9_W7gs2EyTGqIihz89ciBbL61UFwznnHcYRNzfGLI=@pm.me","threadId":"62276","inReplyTo":"xmqqy12xqehd.fsf@gitster.g","subject":"Re: [PATCH v3 2/3] worktree: link worktrees with relative paths","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-09T19:19:40Z","receivedAt":"2024-10-09T19:19:44Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"> Caleb White cdwhite3@pm.me writes:\n> \n\n> > What's the best way to parameterize the worktree tests? I would like\n> > to run the same tests for both absolute and relative paths and I'm\n> > not particularly a fan of just copying them all into new *-relative.sh\n> > files.\n> \n\n> \n\n> What I meant by interoperability tests are a lot smaller scale.\n> \n\n> A test that creates worktree/repository pair without the option to\n> use relative, and then tries to use such a worktree/repository pair\n> with the option would simulate \"how well the newer Git handles an\n> existing repository\", and another test that creates with the option\n> to use relative and uses the worktree/repository without the option\n> would simulate \"how well existing versions of Git works when seeing\n> a worktree made with the newer git with the relative option\".\n> \n\n> By \"parameterise\", if you mean running a set of worktree/repository\n> tests without the \"relative\" option enabled, and run the same set of\n> tests with the option enabled, you could model it after how t8001\n> and t8002 (or t5560 and t5561) share a lot of same tests that are in\n> a file that is included by both of them. In smaller scale, it is\n> common to have an ad-hoc construct like:\n> \n\n> for conf in relative absolute\n> do\n> test_expect_success ...\n> test_expect_success ...\n> test_expect_success ...\n> done\n> \n\n> that has bunch of test_expect_success, which may change the\n> behaviour depending on the value of $conf, not &&-chained inside the\n> for loop. You can use a nested loop (one for preparing, the other\n> for testing the use of worktree) if you want to test the full\n> matrix.\n> \n\n> I do not offhand know if such parametralized tests are necessary in\n> the context of this change, though.\n> \n\n> Thanks.\n\nAh, I see what you mean, thank you for the details! I'll be sure to\nlook at the examples and try to determine which would be the best\npaths forward.\n\n> existing repository\", and another test that creates with the option\n> to use relative and uses the worktree/repository without the option\n> would simulate \"how well existing versions of Git works when seeing\n> a worktree made with the newer git with the relative option\".\n\nI can already tell you that this particular case is not going to work\nbecause existing versions of git expect the path to be absolute. Most\nof the changes in this patch revolve around properly reading/handling\nthe relative paths, not writing the relative paths.\n\nBest,\n"},{"id":"504662","messageId":"xmqqfrp4onjd.fsf@gitster.g","threadId":"62276","inReplyTo":"oUBKv4JI3Bf1nqAFU_C-HDl5vkZC6mg8mb5F0HB7akXC6n0T1ddV0LiOCqY_AGHCyr9_W7gs2EyTGqIihz89ciBbL61UFwznnHcYRNzfGLI=@pm.me","subject":"Re: [PATCH v3 2/3] worktree: link worktrees with relative paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-09T23:37:58Z","receivedAt":"2024-10-09T23:38:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Caleb White <cdwhite3@pm.me> writes:\n\n>> existing repository\", and another test that creates with the option\n>> to use relative and uses the worktree/repository without the option\n>> would simulate \"how well existing versions of Git works when seeing\n>> a worktree made with the newer git with the relative option\".\n>\n> I can already tell you that this particular case is not going to work\n> because existing versions of git expect the path to be absolute. Most\n> of the changes in this patch revolve around properly reading/handling\n> the relative paths, not writing the relative paths.\n\nIf we are talking about making irreversible change to an existing\nrepository, we may need to grab one extensions bit (cf.\nDocumentation/technical/repository-version.txt and then refer also\nto Documentation/config/extensions.txt [*]) and flip it when we\nwrote a relative link to refer to an worktree and repository.\n\n[Footnote]\n\n * The repository-version document claims that any extensions\n   invented must be registered there, but config/extensions.txt that\n   came later ignored it and seems to have acquired a few more than\n   the \"master list\".  We should clean up the mess.\n\n\n"},{"id":"504708","messageId":"Zwf4PT-vaLX35ELL@ArchLinux","threadId":"62276","inReplyTo":"20241007-wt_relative_paths-v3-1-622cf18c45eb@pm.me","subject":"Re: [PATCH v3 1/3] worktree: refactor infer_backlink() to use *strbuf","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-10-10T15:52:29Z","receivedAt":"2024-10-10T15:52:19Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Mon, Oct 07, 2024 at 10:12:30PM -0500, Caleb White via B4 Relay wrote:\n> From: Caleb White <cdwhite3@pm.me>\n> \n> This lays the groundwork for the next patch, which needs the backlink\n> returned from infer_backlink() as a `strbuf`. It seemed inefficient to\n> convert from `strbuf` to `char*` and back to `strbuf` again.\n> \n\nI think here we should first talk about the current behavior of the\n`infer_backlink`:\n\n    `infer_backlink` initializes a \"strbuf\" for the inferred backlink\n    and returns the result with type \"char *\" by detaching the \"strbuf\"\n\nAnd then you should tell your intention like the following (The reader\nlike me does not know what is the intention of the next patch, you should\n__explicitly__ mention this).\n\n    Because we decide to link worktrees with relative paths, we need to\n    convert the returned inferred backlink \"char *\" to \"strbuf\".\n    However, it is a bad idea to convert from `strbuf` to `char*` and\n    back to `strbuf` again. Instead, we should just let the\n    `infer_backlink` to accept the \"struct strbuf *\" parameter to\n    improve efficiency.\n\n> This refactors infer_backlink() to return an integer result and use a\n> pre-allocated `strbuf` for the inferred backlink path, replacing the\n> previous `char*` return type and improving efficiency.\n> \n\nI think you should also talk about that you make the\n`repair_worktree_at_path` function to align with above refactor.\n\n> Signed-off-by: Caleb White <cdwhite3@pm.me>\n> ---\n>  worktree.c | 48 ++++++++++++++++++++++++------------------------\n>  1 file changed, 24 insertions(+), 24 deletions(-)\n> \n> diff --git a/worktree.c b/worktree.c\n> index ec95ea2986107b3bc12d38b0825d7c6e87402bc6..0cba0d6e6e9ad02ace04a0301104a04a07cbef65 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(const char *gitfile, struct strbuf *inferred)\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,18 @@ 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_reset(inferred);\n\nQuestion here: should we use `strbuf_reset` here? I want to know the\nreason why you design this. Is the caller's responsibility to clear the\n\"inferred\" when calling this function?\n\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 1;\n>  \n>  error:\n>  \tstrbuf_release(&actual);\n> -\tstrbuf_release(&inferred);\n> -\treturn NULL;\n> +\tstrbuf_reset(inferred); /* clear invalid path */\n> +\treturn 0;\n>  }\n\nDesign question, when calling `infer_backlink` successfully, we will\nreturn 1, if not, we will return 0. But in the later code, we use the\n\"inferred.buf.len\" to indicate whether we could get the inferred\nbacklink successfully.\n\nWe have two signals to indicate the success. I think it's a bad idea to\nuse \"inferred.buf.len\". Let me give you an example here:\n\n    struct strbuf inferred_backlink = STRBUF_INIT;\n    inferred_backlink = infer_backlink(realdotgit.buf);\n\n    // What if I wrongly use the following statements?\n    strbuf_addstr(&inferred_backlink, \"foo\");\n\n    if (inferred_backlink.buf.len) {\n\n    }\n\nIf you insist using \"inferred_backlink.buf.len\" as the result, this\nfunction should return `void`. And you should add some comments for this\nfunction.\n\n>  \n>  /*\n> @@ -680,10 +680,11 @@ 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 inferred_backlink = STRBUF_INIT;\n>  \tstruct strbuf gitdir = STRBUF_INIT;\n>  \tstruct strbuf olddotgit = STRBUF_INIT;\n> -\tchar *backlink = NULL;\n> -\tchar *inferred_backlink = NULL;\n> +\tchar *dotgit_contents = NULL;\n>  \tconst char *repair = NULL;\n>  \tint err;\n>  \n> @@ -699,23 +700,23 @@ void repair_worktree_at_path(const char *path,\n>  \t\tgoto done;\n>  \t}\n>  \n> -\tinferred_backlink = infer_backlink(realdotgit.buf);\n> -\tbacklink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));\n> -\tif (err == READ_GITFILE_ERR_NOT_A_FILE) {\n> +\tinfer_backlink(realdotgit.buf, &inferred_backlink);\n> +\tdotgit_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));\n> +\tif (dotgit_contents) {\n> +\t\tstrbuf_addstr(&backlink, dotgit_contents);\n> +\t} else if (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 (inferred_backlink) {\n> +\t\tif (inferred_backlink.len) {\n>  \t\t\t/*\n>  \t\t\t * Worktree's .git file does not point at a repository\n>  \t\t\t * but we found a .git/worktrees/<id> in this\n>  \t\t\t * repository with the same <id> as recorded in the\n>  \t\t\t * worktree's .git file so make the worktree point at\n> -\t\t\t * the discovered .git/worktrees/<id>. (Note: backlink\n> -\t\t\t * is already NULL, so no need to free it first.)\n> +\t\t\t * the discovered .git/worktrees/<id>.\n>  \t\t\t */\n> -\t\t\tbacklink = inferred_backlink;\n> -\t\t\tinferred_backlink = NULL;\n> +\t\t\tstrbuf_swap(&backlink, &inferred_backlink);\n>  \t\t} else {\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> @@ -743,13 +744,11 @@ void repair_worktree_at_path(const char *path,\n>  \t * in the \"copy\" repository. In this case, point the \"copy\" worktree's\n>  \t * .git file at the \"copy\" repository.\n>  \t */\n> -\tif (inferred_backlink && fspathcmp(backlink, inferred_backlink)) {\n> -\t\tfree(backlink);\n> -\t\tbacklink = inferred_backlink;\n> -\t\tinferred_backlink = NULL;\n> +\tif (inferred_backlink.len && fspathcmp(backlink.buf, inferred_backlink.buf)) {\n> +\t\tstrbuf_swap(&backlink, &inferred_backlink);\n>  \t}\n\nFor single line statement after \"if\", we should not use `{`.\n\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> @@ -763,9 +762,10 @@ 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(inferred_backlink);\n> +\tfree(dotgit_contents);\n>  \tstrbuf_release(&olddotgit);\n> +\tstrbuf_release(&backlink);\n> +\tstrbuf_release(&inferred_backlink);\n>  \tstrbuf_release(&gitdir);\n>  \tstrbuf_release(&realdotgit);\n>  \tstrbuf_release(&dotgit);\n> \n> -- \n> 2.46.2\n> \n> \n\nSorry, I could not review other patches today (I need to go sleep).\n\nThanks,\nJialuo\n"},{"id":"504712","messageId":"e1RptKNShhPZHXDhBkQBaCNCmKBKik4nQzRShqtgVfjcH7vBWpuLYV60PSHaJ0diX-oG3XiKHc7IebhIZM4eSkeYdQQZ_QYK2ixxsK-XwrE=@pm.me","threadId":"62276","inReplyTo":"Zwf4PT-vaLX35ELL@ArchLinux","subject":"Re: [PATCH v3 1/3] worktree: refactor infer_backlink() to use *strbuf","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-10T16:41:03Z","receivedAt":"2024-10-10T16:41:07Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Thursday, October 10th, 2024 at 10:52, shejialuo <shejialuo@gmail.com> wrote:\n\n> On Mon, Oct 07, 2024 at 10:12:30PM -0500, Caleb White via B4 Relay wrote:\n> \n\n> > From: Caleb White cdwhite3@pm.me\n> > \n\n> > This lays the groundwork for the next patch, which needs the backlink\n> > returned from infer_backlink() as a `strbuf`. It seemed inefficient to\n> > convert from `strbuf` to `char*` and back to `strbuf` again.\n> \n\n> \n\n> I think here we should first talk about the current behavior of the\n> `infer_backlink`:\n> \n\n> `infer_backlink` initializes a \"strbuf\" for the inferred backlink\n> and returns the result with type \"char *\" by detaching the \"strbuf\"\n> \n\n> And then you should tell your intention like the following (The reader\n> like me does not know what is the intention of the next patch, you should\n> explicitly mention this).\n> \n\n> Because we decide to link worktrees with relative paths, we need to\n> convert the returned inferred backlink \"char *\" to \"strbuf\".\n> However, it is a bad idea to convert from `strbuf` to `char*` and\n> back to `strbuf` again. Instead, we should just let the\n> `infer_backlink` to accept the \"struct strbuf *\" parameter to\n> improve efficiency.\n> \n\n> > This refactors infer_backlink() to return an integer result and use a\n> > pre-allocated `strbuf` for the inferred backlink path, replacing the\n> > previous `char*` return type and improving efficiency.\n> \n\n> \n\n> I think you should also talk about that you make the\n> `repair_worktree_at_path` function to align with above refactor.\n\nThank you for the feedback, I'll add these changes.\n\n> > if (strbuf_read_file(&actual, gitfile, 0) < 0)\n> > @@ -658,17 +657,18 @@ static char *infer_backlink(const char gitfile)\n> > id++; / advance past '/' to point at <id> */\n> > if (!*id)\n> > goto error;\n> > - strbuf_git_common_path(&inferred, the_repository, \"worktrees/%s\", id);\n> > - if (!is_directory(inferred.buf))\n> > + strbuf_reset(inferred);\n> \n\n> \n\n> Question here: should we use `strbuf_reset` here? I want to know the\n> reason why you design this. Is the caller's responsibility to clear the\n> \"inferred\" when calling this function?\n\nYes we should, sure it is the caller's responsibility but this just helps\nprevent bugs. There's plenty of functions that reset the strbuf that's\npassed to the function before modifying it.\n\n\n> > + strbuf_git_common_path(inferred, the_repository, \"worktrees/%s\", id);\n> > + if (!is_directory(inferred->buf))\n> > goto error;\n> > \n\n> > strbuf_release(&actual);\n> > - return strbuf_detach(&inferred, NULL);\n> > + return 1;\n> > \n\n> > error:\n> > strbuf_release(&actual);\n> > - strbuf_release(&inferred);\n> > - return NULL;\n> > + strbuf_reset(inferred); /* clear invalid path */\n> > + return 0;\n> > }\n> \n\n> \n\n> Design question, when calling `infer_backlink` successfully, we will\n> return 1, if not, we will return 0. But in the later code, we use the\n> \"inferred.buf.len\" to indicate whether we could get the inferred\n> backlink successfully.\n\nOriginally, this function call was inside an `if` condition, and it made\nsense to keep the boolean return. However, the dependent patch that I later\nrebased on refactored this to be a standalone call. I didn't feel like\nintroducing another variable just to capture the return value when I could\nglean the same information from the existing strbuf.\n\n> We have two signals to indicate the success. I think it's a bad idea to\n> use \"inferred.buf.len\". Let me give you an example here:\n\nI don't see a problem with this---the two \"signals\" are guaranteed to\nalways be in sync (either the return is 1 and len is > 0, or return is\n0 and len is 0). Having the boolean return gives you flexibility in how\nyou can call the function (if it can be placed inside an if condition).\n\n> struct strbuf inferred_backlink = STRBUF_INIT;\n> inferred_backlink = infer_backlink(realdotgit.buf);\n> \n\n> // What if I wrongly use the following statements?\n> strbuf_addstr(&inferred_backlink, \"foo\");\n> \n\n> if (inferred_backlink.buf.len) {\n> \n\n> }\n\nI'm sorry, but this example doesn't make sense. This will fail to compile\nfor several reasons:\n- infer_backlink() requires two args\n- you cannot assign an `int` to a `struct strbuf`\n- even if inferred_backlink became an int then the strbuf_addstr()\n  would fail because you can't pass an `int*` to a `struct strbuf*`\n- `inferred_backlink.buf.len` doesn't exist, it's `inferred_backlink.len`\n  (probably just a typo)\n\n> If you insist using \"inferred_backlink.buf.len\" as the result, this\n> function should return `void`. And you should add some comments for this\n> function.\n\nI can add comments, and I can change the return type to `void` if there's\nconsensus, but I really don't see any issue with leaving it as-is.\n\n> > - if (inferred_backlink && fspathcmp(backlink, inferred_backlink)) {\n> > - free(backlink);\n> > - backlink = inferred_backlink;\n> > - inferred_backlink = NULL;\n> > + if (inferred_backlink.len && fspathcmp(backlink.buf, inferred_backlink.buf)) {\n> > + strbuf_swap(&backlink, &inferred_backlink);\n> > }\n> \n\n> \n\n> For single line statement after \"if\", we should not use `{`.\n\nThe brackets were introduced by the patch that my series depends on.\nI can remove them, but perhaps it would be better to address that\non the dependent patch?\n\nBest,"},{"id":"504719","messageId":"ZwgOURNkNhg8BsaU@ArchLinux","threadId":"62276","inReplyTo":"e1RptKNShhPZHXDhBkQBaCNCmKBKik4nQzRShqtgVfjcH7vBWpuLYV60PSHaJ0diX-oG3XiKHc7IebhIZM4eSkeYdQQZ_QYK2ixxsK-XwrE=@pm.me","subject":"Re: [PATCH v3 1/3] worktree: refactor infer_backlink() to use *strbuf","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-10-10T17:26:41Z","receivedAt":"2024-10-10T17:26:31Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Thu, Oct 10, 2024 at 04:41:03PM +0000, Caleb White wrote:\n\n[snip]\n\n> > > @@ -658,17 +657,18 @@ static char *infer_backlink(const char gitfile)\n> > > id++; / advance past '/' to point at <id> */\n> > > if (!*id)\n> > > goto error;\n> > > - strbuf_git_common_path(&inferred, the_repository, \"worktrees/%s\", id);\n> > > - if (!is_directory(inferred.buf))\n> > > + strbuf_reset(inferred);\n> > \n> \n> > \n> \n> > Question here: should we use `strbuf_reset` here? I want to know the\n> > reason why you design this. Is the caller's responsibility to clear the\n> > \"inferred\" when calling this function?\n> \n> Yes we should, sure it is the caller's responsibility but this just helps\n> prevent bugs. There's plenty of functions that reset the strbuf that's\n> passed to the function before modifying it.\n> \n\nYes, that make senses.\n\n[snip]\n\n> > We have two signals to indicate the success. I think it's a bad idea to\n> > use \"inferred.buf.len\". Let me give you an example here:\n> \n> I don't see a problem with this---the two \"signals\" are guaranteed to\n> always be in sync (either the return is 1 and len is > 0, or return is\n> 0 and len is 0). Having the boolean return gives you flexibility in how\n> you can call the function (if it can be placed inside an if condition).\n> \n\nYes, there is nothing wrong with this. But we also introduce a burden here,\nwhen we need to change/refactor `infer_backlink`, the developer should\nhave the knowledge \"when the return is 1 and len is >0 or return is 0\nand len is 0\".\n\nIf so, why not just return \"infer_backlink.len\"?\n\n> > struct strbuf inferred_backlink = STRBUF_INIT;\n> > inferred_backlink = infer_backlink(realdotgit.buf);\n> > \n> \n> > // What if I wrongly use the following statements?\n> > strbuf_addstr(&inferred_backlink, \"foo\");\n> > \n> \n> > if (inferred_backlink.buf.len) {\n> > \n> \n> > }\n> \n> I'm sorry, but this example doesn't make sense. This will fail to compile\n> for several reasons:\n> - infer_backlink() requires two args\n> - you cannot assign an `int` to a `struct strbuf`\n> - even if inferred_backlink became an int then the strbuf_addstr()\n>   would fail because you can't pass an `int*` to a `struct strbuf*`\n> - `inferred_backlink.buf.len` doesn't exist, it's `inferred_backlink.len`\n>   (probably just a typo)\n> \n\nI am sorry for this, I gave a wrong example here, it should be the\nfollowing (I copied the wrong line in the previous email):\n\n    struct strbuf inferred_backlink = STRBUF_INIT;\n    infer_backlink(realdotgit.buf, &inferred_backlink);\n\n    // What if I wronly use the following statements?\n    strbuf_addstr(&inferred_backlink, \"foo\");\n\n    if (inferred_backlink.len) {\n        ...\n    }\n\nActually, I am not against your way. Instead, you should mention why you\nchoose \"inferred_backlink.len\" as the signal in the commit message.\nThat's the main reason why I think we may add some comments here. The\ncaller may do not know we should use \"inferred_backlink.len\" to indicate\nthat we have successfully found the inferred backlink especially there\nis already a return code in the function.\n\n> > If you insist using \"inferred_backlink.buf.len\" as the result, this\n> > function should return `void`. And you should add some comments for this\n> > function.\n> \n> I can add comments, and I can change the return type to `void` if there's\n> consensus, but I really don't see any issue with leaving it as-is.\n> \n\nI agree with you that this function is NOT harmful. Actually, I do not\nthink using \"void\" as the return type is a good idea. If we decide to\nuse two signals, let's leave it as-is. Some comments should be enough.\n\n> > > - if (inferred_backlink && fspathcmp(backlink, inferred_backlink)) {\n> > > - free(backlink);\n> > > - backlink = inferred_backlink;\n> > > - inferred_backlink = NULL;\n> > > + if (inferred_backlink.len && fspathcmp(backlink.buf, inferred_backlink.buf)) {\n> > > + strbuf_swap(&backlink, &inferred_backlink);\n> > > }\n> > \n> \n> > \n> \n> > For single line statement after \"if\", we should not use `{`.\n> \n> The brackets were introduced by the patch that my series depends on.\n> I can remove them, but perhaps it would be better to address that\n> on the dependent patch?\n> \n\nThe original patch has three lines. So it should use `{`. After your\nchange, it only has one line, isn't it?\n\nYou could refer to this to see the code style.\n\n  https://github.com/git/git/blob/master/Documentation/CodingGuidelines\n\n> Best,\n\nThanks,\nJialuo\n"},{"id":"504720","messageId":"gSEdRZ0rf9yJ68dp1Jgq7CYrXPGMgUxgBji4qs_emJy25eHOG7ULCtH8f2bawTRFSMGoxC_QsB7EN0OJnbQ_Olf66hNk8fEJoVMkkIlyA14=@pm.me","threadId":"62276","inReplyTo":"xmqqfrp4onjd.fsf@gitster.g","subject":"Re: [PATCH v3 2/3] worktree: link worktrees with relative paths","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-10T17:30:22Z","receivedAt":"2024-10-10T17:30:29Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Wednesday, October 9th, 2024 at 18:37, Junio C Hamano <gitster@pobox.com> wrote:\n\n> Caleb White cdwhite3@pm.me writes:\n> \n\n> > > existing repository\", and another test that creates with the option\n> > > to use relative and uses the worktree/repository without the option\n> > > would simulate \"how well existing versions of Git works when seeing\n> > > a worktree made with the newer git with the relative option\".\n> > \n\n> > I can already tell you that this particular case is not going to work\n> > because existing versions of git expect the path to be absolute. Most\n> > of the changes in this patch revolve around properly reading/handling\n> > the relative paths, not writing the relative paths.\n> \n\n> \n\n> If we are talking about making irreversible change to an existing\n> repository, we may need to grab one extensions bit (cf.\n> Documentation/technical/repository-version.txt and then refer also\n> to Documentation/config/extensions.txt [*]) and flip it when we\n> wrote a relative link to refer to an worktree and repository.\n\nThanks, I'll take a look at the references.\n\n> [Footnote]\n> \n\n> * The repository-version document claims that any extensions\n> invented must be registered there, but config/extensions.txt that\n> came later ignored it and seems to have acquired a few more than\n> the \"master list\". We should clean up the mess.\n\nWould you like the contents of config/extensions.txt moved into\nthe repository-version document and then deleted?"},{"id":"504721","messageId":"J1frhnJqftB1IUvIGDnJiz6uYTDNl_Bc92NLj1gXJtgRxapI3Uy-fyFHYtv5_p8e7yzF0pegawV6NlHQTn0GHcXP8MZ_1X8CNkIvO3_MP9c=@pm.me","threadId":"62276","inReplyTo":"ZwgOURNkNhg8BsaU@ArchLinux","subject":"Re: [PATCH v3 1/3] worktree: refactor infer_backlink() to use *strbuf","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-10T17:52:49Z","receivedAt":"2024-10-10T17:52:59Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Thursday, October 10th, 2024 at 12:26, shejialuo <shejialuo@gmail.com> wrote:\n\n> > > We have two signals to indicate the success. I think it's a bad idea to\n> > > use \"inferred.buf.len\". Let me give you an example here:\n> > \n\n> > I don't see a problem with this---the two \"signals\" are guaranteed to\n> > always be in sync (either the return is 1 and len is > 0, or return is\n> > 0 and len is 0). Having the boolean return gives you flexibility in how\n> > you can call the function (if it can be placed inside an if condition).\n> \n\n> Yes, there is nothing wrong with this. But we also introduce a burden here,\n> when we need to change/refactor `infer_backlink`, the developer should\n> have the knowledge \"when the return is 1 and len is >0 or return is 0\n> and len is 0\".\n> \n\n> If so, why not just return \"infer_backlink.len\"?\n\nI would say because the purpose of the return is a boolean,\neither the call was successful or it wasn't. The developer\nknowledge that you speak of should be a given---if the\nfunction returned true then there's obviously a path\nin the strbuf.\n\n> I am sorry for this, I gave a wrong example here, it should be the\n> following (I copied the wrong line in the previous email):\n> \n\n> struct strbuf inferred_backlink = STRBUF_INIT;\n> infer_backlink(realdotgit.buf, &inferred_backlink);\n> \n\n> // What if I wronly use the following statements?\n> strbuf_addstr(&inferred_backlink, \"foo\");\n\nThere's nothing wrong with this, it's on the developer\nto check the error condition before proceeding to use\nthe strbuf.\n\n> Actually, I am not against your way. Instead, you should mention why you\n> choose \"inferred_backlink.len\" as the signal in the commit message.\n> That's the main reason why I think we may add some comments here. The\n> caller may do not know we should use \"inferred_backlink.len\" to indicate\n> that we have successfully found the inferred backlink especially there\n> is already a return code in the function.\n\nI think the intent is fairly self-evident and does not warrant a\ncomment in the commit message? If the strbuf does not have a\nlength then it should be obvious there is no inferred backlink?\n\n> > > If you insist using \"inferred_backlink.buf.len\" as the result, this\n> > > function should return `void`. And you should add some comments for this\n> > > function.\n> > \n\n> > I can add comments, and I can change the return type to `void` if there's\n> > consensus, but I really don't see any issue with leaving it as-is.\n> \n\n> I agree with you that this function is NOT harmful. Actually, I do not\n> think using \"void\" as the return type is a good idea. If we decide to\n> use two signals, let's leave it as-is. Some comments should be enough.\n\nI've added a comment to the function docs.\n\n> The original patch has three lines. So it should use `{`. After your\n> change, it only has one line, isn't it?\n> \n\n> You could refer to this to see the code style.\n> \n\n> https://github.com/git/git/blob/master/Documentation/CodingGuidelines\n\nAh I'm sorry, you're right---I'll adjust.\n\nBest,"},{"id":"504736","messageId":"xmqqmsjbkbww.fsf@gitster.g","threadId":"62276","inReplyTo":"e1RptKNShhPZHXDhBkQBaCNCmKBKik4nQzRShqtgVfjcH7vBWpuLYV60PSHaJ0diX-oG3XiKHc7IebhIZM4eSkeYdQQZ_QYK2ixxsK-XwrE=@pm.me","subject":"Re: [PATCH v3 1/3] worktree: refactor infer_backlink() to use *strbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-10T19:14:55Z","receivedAt":"2024-10-10T19:14:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Caleb White <cdwhite3@pm.me> writes:\n\n>> Question here: should we use `strbuf_reset` here? I want to know the\n>> reason why you design this. Is the caller's responsibility to clear the\n>> \"inferred\" when calling this function?\n>\n> Yes we should, sure it is the caller's responsibility but this just helps\n> prevent bugs. There's plenty of functions that reset the strbuf that's\n> passed to the function before modifying it.\n\nIf the API says that it _appends_ what it found in the supplied\nstrbuf, then it is the caller's responsibility to ensure that the\nstrbuf being passed is empty, if the caller wants the resulting\nstrbuf to hold only what the function found.\n\nIf the API says it _returns_ what it found in the supplied strbuf,\nthen the function is responsible to discard what the supplied strbuf\noriginally has.\n\nAs an API design, if it is rarely useful for callers to be able to\nappend, then the latter is simpler to use, but there are many cases\nwhere the \"append\" semantics is useful (see strbuf.c to find\nexamples).\n\nIn this particular case, it is rarely useful for the caller to get\nthe result appended to a string it already has before calling this\nfunction, so unconditionally discarding what the caller had, without\ntelling the caller that it is responsible for passing an empty buffer,\nis the right thing, I wuld think.\n\nThanks.\n"},{"id":"504739","messageId":"xmqqcyk7k9nx.fsf@gitster.g","threadId":"62276","inReplyTo":"J1frhnJqftB1IUvIGDnJiz6uYTDNl_Bc92NLj1gXJtgRxapI3Uy-fyFHYtv5_p8e7yzF0pegawV6NlHQTn0GHcXP8MZ_1X8CNkIvO3_MP9c=@pm.me","subject":"Re: [PATCH v3 1/3] worktree: refactor infer_backlink() to use *strbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-10T20:03:30Z","receivedAt":"2024-10-10T20:03:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Caleb White <cdwhite3@pm.me> writes:\n\n>> If so, why not just return \"infer_backlink.len\"?\n>\n> I would say because the purpose of the return is a boolean,\n> either the call was successful or it wasn't. The developer\n> knowledge that you speak of should be a given---if the\n> function returned true then there's obviously a path\n> in the strbuf.\n\nThat does not say what should be left in the strbuf when the request\nfailed.  If it is undefined, then using its .len (or .buf) is not\nquite right, without relying on too much implementation details of\nthe called function.\n\nUsually, this project uses 0 to signal a success, while errors are\nsignalled by returning a negative value (and if there can be\nmultiple ways to fail, such a design leaves different negative\nvalues as a possibility).\n\nIf the result the caller finds useful is always positive value (or\nnon-negative value), however, it is perfectly fine and often more\nconvenient if you used the \"positive (or non-negative) value gives\nthe intended result and signals a success, negative (or\nnon-positive) value signals an error\" convention.  The caller can\n\n    if (0 < do_the_thing())\n\t; /* success */\n\njust fine, instead of the boolean \"0 is success, negative values are\nvarious errors\",\n\n    if (!do_the_thing())\n\t; /* success */\n\nwhen it does not care how/why the thing failed.\n\n"},{"id":"504742","messageId":"XUb4_NFTymdqM_NMxxIfI3YZdQXYu0zViporhB_GbnDFY1pjJNaM8usiQB68mu4RIM_ud9bt8lqq28fdeJhYs-J09YKma67O8kuC8rOGG-8=@pm.me","threadId":"62276","inReplyTo":"xmqqcyk7k9nx.fsf@gitster.g","subject":"Re: [PATCH v3 1/3] worktree: refactor infer_backlink() to use *strbuf","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-10T20:24:07Z","receivedAt":"2024-10-10T20:24:14Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Thursday, October 10th, 2024 at 15:03, Junio C Hamano <gitster@pobox.com> wrote:\n\n> That does not say what should be left in the strbuf when the request\n> failed. If it is undefined, then using its .len (or .buf) is not\n> quite right, without relying on too much implementation details of\n> the called function.\n> \n\n> Usually, this project uses 0 to signal a success, while errors are\n> signalled by returning a negative value (and if there can be\n> multiple ways to fail, such a design leaves different negative\n> values as a possibility).\n> \n\n> If the result the caller finds useful is always positive value (or\n> non-negative value), however, it is perfectly fine and often more\n> convenient if you used the \"positive (or non-negative) value gives\n> the intended result and signals a success, negative (or\n> non-positive) value signals an error\" convention. The caller can\n> \n\n> if (0 < do_the_thing())\n> ; /* success /\n> \n\n> just fine, instead of the boolean \"0 is success, negative values are\n> various errors\",\n> \n\n> if (!do_the_thing())\n> ; / success */\n> \n\n> when it does not care how/why the thing failed.\n\nThat makes sense. I can update the function to return -1 on error\nand the strbuf.len on success."},{"id":"504776","messageId":"D4SOG00CK1L8.E3J8BLFR3ACE@pm.me","threadId":"62276","inReplyTo":"xmqqfrp4onjd.fsf@gitster.g","subject":"Re: [PATCH v3 2/3] worktree: link worktrees with relative paths","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-11T04:03:46Z","receivedAt":"2024-10-11T04:03:57Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"Empty Message\n"},{"id":"505799","messageId":"D521XSJ9N80C.19U6V5T7LCRSB@pm.me","threadId":"62276","inReplyTo":"xmqqfrp4onjd.fsf@gitster.g","subject":"Re: [PATCH v3 2/3] worktree: link worktrees with relative paths","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-22T04:32:17Z","receivedAt":"2024-10-22T04:32:29Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Wed Oct 9, 2024 at 6:37 PM CDT, Junio C Hamano wrote:\n> If we are talking about making irreversible change to an existing\n> repository, we may need to grab one extensions bit (cf.\n> Documentation/technical/repository-version.txt and then refer also\n> to Documentation/config/extensions.txt [*]) and flip it when we\n> wrote a relative link to refer to an worktree and repository.\n\nThis has been implemented.\n\n> [Footnote]\n>\n>  * The repository-version document claims that any extensions\n>    invented must be registered there, but config/extensions.txt that\n>    came later ignored it and seems to have acquired a few more than\n>    the \"master list\".  We should clean up the mess.\n\nI cleaned up the mess in a separate patch[1].\n\n\n[1]: https://lore.kernel.org/git/20241021-cleanup-extension-docs-v1-1-ab02cece3132@pm.me\n\nBest,\n\n"}]}