| Message ID | 20241006060017.171788-3-cdwhite3@pm.me (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Link worktrees with relative paths | expand |
On Sun, Oct 6, 2024 at 2:01 AM Caleb White <cdwhite3@pm.me> wrote: > This modifies Git’s handling of worktree linking to use relative > paths instead of absolute paths. Previously, when creating a worktree, > Git would store the absolute paths to both the main repository and the > linked worktrees. These hardcoded absolute paths cause breakages such > as when the repository is moved to a different directory or during > containerized development where the absolute differs between systems. > > By switching to relative paths, we help ensure that worktree links are > more resilient when the repository is moved. While links external to the > repository may still break, Git still automatically handles many common > scenarios, reducing the need for manual repair. This is particularly > useful in containerized or portable development environments, where the > absolute path to the repository can differ between systems. Developers > no longer need to reinitialize or repair worktrees after relocating the > repository, improving workflow efficiency and reducing breakages. > > For self-contained repositories (such as using a bare repository with > worktrees), where both the repository and its worktrees are located > within the same directory structure, using relative paths guarantees all > links remain functional regardless of where the directory is located. > > Signed-off-by: Caleb White <cdwhite3@pm.me> When you reroll, please extend the commit message to give a more detailed overview of how this patch actually changes the behavior both at a high level and at a low level. This is especially important since this patch is sufficiently long and involved that it's not easy to glean these details at-a-glance from the code changes themselves. For instance, the cover letter talked about retaining correct behavior for both absolute and relative paths, but this commit message mentions only relative paths, so it's not obvious just from reading the commit message whether absolute paths are still supported or, if they are, what exactly that support looks like or how the user controls the choice between the two path types. This sort of information should be present in the commit message. Similarly, if there is a chance that this change may break existing tooling and workflows, then the commit message should talk about those risks, as well, and what if any remediations are available. Also, if there are such risks, then Documentation/git-worktree.txt may need to be updated to warn users. Regarding what you wrote above, there seems to be a good deal of redundancy between the first two paragraphs; combining the paragraphs and folding out the duplication might make the message more streamlined. I do like the discussion about containerized environments being used as (at least one) justification for employing relative paths, and think that may be a good lead-in for the commit message. Please see [1] for some helpful hints for composing a good commit message. I'm not going to review the code itself right now since I haven't been able to apply the patches or play around with the functionality, but I wanted to get the above written up since I think this series is going to need to be rerolled anyhow. [1]: https://lore.kernel.org/git/xmqqmsm6sc0q.fsf@gitster.g/
On Sun, Oct 06, 2024 at 06:01:17AM +0000, Caleb White wrote: > This modifies Git’s handling of worktree linking to use relative > paths instead of absolute paths. Previously, when creating a worktree, > Git would store the absolute paths to both the main repository and the > linked worktrees. These hardcoded absolute paths cause breakages such > as when the repository is moved to a different directory or during > containerized development where the absolute differs between systems. > > By switching to relative paths, we help ensure that worktree links are > more resilient when the repository is moved. While links external to the > repository may still break, Git still automatically handles many common > scenarios, reducing the need for manual repair. This is particularly > useful in containerized or portable development environments, where the > absolute path to the repository can differ between systems. Developers > no longer need to reinitialize or repair worktrees after relocating the > repository, improving workflow efficiency and reducing breakages. > > For self-contained repositories (such as using a bare repository with > worktrees), where both the repository and its worktrees are located > within the same directory structure, using relative paths guarantees all > links remain functional regardless of where the directory is located. > Eric has already commented on this commit message. I think this commit has done a lot of things which make the review painful. > Signed-off-by: Caleb White <cdwhite3@pm.me> > --- > builtin/worktree.c | 17 ++-- > t/t2408-worktree-relative.sh | 39 +++++++++ > worktree.c | 152 +++++++++++++++++++++++++---------- > 3 files changed, 159 insertions(+), 49 deletions(-) > create mode 100755 t/t2408-worktree-relative.sh > > diff --git a/builtin/worktree.c b/builtin/worktree.c > index fc31d07..99cee56 100644 > --- a/builtin/worktree.c > +++ b/builtin/worktree.c > @@ -414,7 +414,8 @@ static int add_worktree(const char *path, const char *refname, > const struct add_opts *opts) > { > struct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT; > - struct strbuf sb = STRBUF_INIT, realpath = STRBUF_INIT; > + struct strbuf sb = STRBUF_INIT, sb_tmp = STRBUF_INIT; > + struct strbuf sb_path_realpath = STRBUF_INIT, sb_repo_realpath = STRBUF_INIT; > const char *name; > struct strvec child_env = STRVEC_INIT; > unsigned int counter = 0; > @@ -490,11 +491,11 @@ static int add_worktree(const char *path, const char *refname, > > strbuf_reset(&sb); > strbuf_addf(&sb, "%s/gitdir", sb_repo.buf); > - strbuf_realpath(&realpath, sb_git.buf, 1); > - write_file(sb.buf, "%s", realpath.buf); > - strbuf_realpath(&realpath, repo_get_common_dir(the_repository), 1); > - write_file(sb_git.buf, "gitdir: %s/worktrees/%s", > - realpath.buf, name); > + strbuf_realpath(&sb_path_realpath, path, 1); > + strbuf_realpath(&sb_repo_realpath, sb_repo.buf, 1); > + write_file(sb.buf, "%s/.git", relative_path(sb_path_realpath.buf, sb_repo_realpath.buf, &sb_tmp)); > + strbuf_reset(&sb_tmp); Do we need reset the "sb_tmp"? I guess we do not need, "relative_path" does not care about "sb_tmp". It will always reset the value of the "sb_tmp". So, we may delete this line. [snip] > diff --git a/worktree.c b/worktree.c > index c6d2ede..fc14e9a 100644 > --- a/worktree.c > +++ b/worktree.c > @@ -373,18 +379,30 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg, > void update_worktree_location(struct worktree *wt, const char *path_) > { > struct strbuf path = STRBUF_INIT; > + struct strbuf repo = STRBUF_INIT; > + struct strbuf tmp = STRBUF_INIT; > + char *file = NULL; > > if (is_main_worktree(wt)) > BUG("can't relocate main worktree"); > > + strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1); > strbuf_realpath(&path, path_, 1); > if (fspathcmp(wt->path, path.buf)) { > - write_file(git_common_path("worktrees/%s/gitdir", wt->id), > - "%s/.git", path.buf); > + file = xstrfmt("%s/gitdir", repo.buf); > + write_file(file, "%s/.git", relative_path(path.buf, repo.buf, &tmp)); > + free(file); > + strbuf_reset(&tmp); > + file = xstrfmt("%s/.git", path.buf); Still, we do not need to call "strbuf_reset" again for "tmp". But there is another question here. Should we define the "file" just in this "if" block and free "file" also in the block? And I don't think it's a good idea to use "xstrfmt". Here, we need to allocate two times and free two times. Why not just define a "struct strbuf" and the use "strbuf_*" method here? > + write_file(file, "gitdir: %s", relative_path(repo.buf, path.buf, &tmp)); > + > free(wt->path); > wt->path = strbuf_detach(&path, NULL); > } > + free(file); > strbuf_release(&path); > + strbuf_release(&repo); > + strbuf_release(&tmp); > } > > @@ -564,38 +582,52 @@ static void repair_gitfile(struct worktree *wt, > { > [snip] > strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1); > strbuf_addf(&dotgit, "%s/.git", wt->path); > - backlink = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err)); > + git_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err)); Why here we need to use "xstrdup_or_null". The life cycle of the "git_contents" variable is in the "repair_gitfile" function. [snip] I omit the review for the later code, there are too many codes to review. And due to my limited knowledge about worktree, the overhead will be too big for me. Thanks, Jialuo
On Sunday, October 6th, 2024 at 06:05, Eric Sunshine <sunshine@sunshineco.com> wrote: > When you reroll, please extend the commit message to give a more > detailed overview of how this patch actually changes the behavior both > at a high level and at a low level. This is especially important since > this patch is sufficiently long and involved that it's not easy to > glean these details at-a-glance from the code changes themselves. I will do that, I was not sure how much low level detail I should dive into. > Regarding what you wrote above, there seems to be a good deal of > redundancy between the first two paragraphs; combining the paragraphs > and folding out the duplication might make the message more > streamlined. I do like the discussion about containerized environments > being used as (at least one) justification for employing relative > paths, and think that may be a good lead-in for the commit message. > Please see [1] for some helpful hints for composing a good commit > message. Thanks, I will clean up the redundancy and add more detail to the commit.
On Sunday, October 6th, 2024 at 10:37, shejialuo <shejialuo@gmail.com> wrote > Eric has already commented on this commit message. I think this commit > has done a lot of things which make the review painful. My apologies, I will do better on this commit message. > Do we need reset the "sb_tmp"? I guess we do not need, "relative_path" > does not care about "sb_tmp". It will always reset the value of the > "sb_tmp". So, we may delete this line. You are right, this is reset by relative_path(). I originally encountered a bug and I thought not resetting this strbuf between relative_path() calls was the cause but it must have been something else that I fixed. > Still, we do not need to call "strbuf_reset" again for "tmp". But there > is another question here. Should we define the "file" just in this "if" > block and free "file" also in the block? The style this code uses seems to place most / all of the declarations at the top of the function and frees at the bottom so I think this fits in. > And I don't think it's a good idea to use "xstrfmt". Here, we need to > allocate two times and free two times. Why not just define a "struct > strbuf" and the use "strbuf_*" method here? I can use strbufs, I just wasn't sure if I really needed a strbuf for each of the paths and was just trying to reuse a var. > > strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1); > > strbuf_addf(&dotgit, "%s/.git", wt->path); > > - backlink = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err)); > > + git_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err)); > > > Why here we need to use "xstrdup_or_null". The life cycle of the > "git_contents" variable is in the "repair_gitfile" function. This what the existing code used and I saw no reason to change it...
On Sun, Oct 06, 2024 at 11:57:22PM +0000, Caleb White wrote: [snip] > > Still, we do not need to call "strbuf_reset" again for "tmp". But there > > is another question here. Should we define the "file" just in this "if" > > block and free "file" also in the block? > > The style this code uses seems to place most / all of the declarations at > the top of the function and frees at the bottom so I think this fits in. > Yes, as you have said, the code style places most / all of the declarations at the top and free at the bottom. But the trouble here is we will free the "file" in the "if" block. char *file = NULL; if (...) { file = xstrfmt(...); free(file); file = xstrfmt(...); } free(file); If we want to follow the original code style, should we create two variables at the top and free them at the bottom? > > And I don't think it's a good idea to use "xstrfmt". Here, we need to > > allocate two times and free two times. Why not just define a "struct > > strbuf" and the use "strbuf_*" method here? > > I can use strbufs, I just wasn't sure if I really needed a strbuf for > each of the paths and was just trying to reuse a var. > You don't need to create a new "strbuf" for each of the paths. You could just use "strbuf_reset" for only one "struct strbuf". struct strbuf file = STRBUF_INIT; if (...) { strbuf_addf(...); strbuf_reset(&file); strbuf_addf(...); } strbuf_release(&file); > > > strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1); > > > strbuf_addf(&dotgit, "%s/.git", wt->path); > > > - backlink = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err)); > > > + git_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err)); > > > > > > > > Why here we need to use "xstrdup_or_null". The life cycle of the > > "git_contents" variable is in the "repair_gitfile" function. > > This what the existing code used and I saw no reason to change it... Actually, I somehow understand why in the original code, it will use "xstrdup_or_null" to initialize the "backlink". Because in "read_gitfile_gently", we will use a static "struct strbuf" as the buffer. I guess the intention of the original code is that if we call again "read_gitfile_gently" in this function or we have another thread which calls this function, the content of the buffer will be changed. So we explicitly use "xstrdup_or_null" to create a copy here to avoid this. But I wonder whether we really need to consider above problem?
On Sun, Oct 6, 2024 at 11:45 PM shejialuo <shejialuo@gmail.com> wrote: > Actually, I somehow understand why in the original code, it will use > "xstrdup_or_null" to initialize the "backlink". Because in > "read_gitfile_gently", we will use a static "struct strbuf" as the > buffer. > > I guess the intention of the original code is that if we call again > "read_gitfile_gently" in this function or we have another thread which > calls this function, the content of the buffer will be changed. So we > explicitly use "xstrdup_or_null" to create a copy here to avoid this. That's correct. The code is being defensive in case the static strbuf inside read_gitfile_gently() gets overwritten. > But I wonder whether we really need to consider above problem? It's easier to reason about the code in this function if we don't have to worry about the underlying static strbuf. Also, this is not a hot path so the extra allocation and string copy is not a concern. As such, it makes sense for the code to remain robust (by creating the copy) rather than becoming potentially more fragile by eliminating the string copy.
On Sunday, October 6th, 2024 at 22:45, shejialuo <shejialuo@gmail.com> wrote: > You don't need to create a new "strbuf" for each of the paths. You could > just use "strbuf_reset" for only one "struct strbuf". > > struct strbuf file = STRBUF_INIT; > > if (...) { > strbuf_addf(...); > strbuf_reset(&file); > strbuf_addf(...); > > } > > strbuf_release(&file); Ah, this is a better design---I've updated this to use a single strbuf, thanks!
diff --git a/builtin/worktree.c b/builtin/worktree.c index fc31d07..99cee56 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -414,7 +414,8 @@ static int add_worktree(const char *path, const char *refname, const struct add_opts *opts) { struct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT; - struct strbuf sb = STRBUF_INIT, realpath = STRBUF_INIT; + struct strbuf sb = STRBUF_INIT, sb_tmp = STRBUF_INIT; + struct strbuf sb_path_realpath = STRBUF_INIT, sb_repo_realpath = STRBUF_INIT; const char *name; struct strvec child_env = STRVEC_INIT; unsigned int counter = 0; @@ -490,11 +491,11 @@ static int add_worktree(const char *path, const char *refname, strbuf_reset(&sb); strbuf_addf(&sb, "%s/gitdir", sb_repo.buf); - strbuf_realpath(&realpath, sb_git.buf, 1); - write_file(sb.buf, "%s", realpath.buf); - strbuf_realpath(&realpath, repo_get_common_dir(the_repository), 1); - write_file(sb_git.buf, "gitdir: %s/worktrees/%s", - realpath.buf, name); + strbuf_realpath(&sb_path_realpath, path, 1); + strbuf_realpath(&sb_repo_realpath, sb_repo.buf, 1); + write_file(sb.buf, "%s/.git", relative_path(sb_path_realpath.buf, sb_repo_realpath.buf, &sb_tmp)); + strbuf_reset(&sb_tmp); + write_file(sb_git.buf, "gitdir: %s", relative_path(sb_repo_realpath.buf, sb_path_realpath.buf, &sb_tmp)); strbuf_reset(&sb); strbuf_addf(&sb, "%s/commondir", sb_repo.buf); write_file(sb.buf, "../.."); @@ -578,11 +579,13 @@ static int add_worktree(const char *path, const char *refname, strvec_clear(&child_env); strbuf_release(&sb); + strbuf_release(&sb_tmp); strbuf_release(&symref); strbuf_release(&sb_repo); + strbuf_release(&sb_repo_realpath); strbuf_release(&sb_git); + strbuf_release(&sb_path_realpath); strbuf_release(&sb_name); - strbuf_release(&realpath); free_worktree(wt); return ret; } diff --git a/t/t2408-worktree-relative.sh b/t/t2408-worktree-relative.sh new file mode 100755 index 0000000..a3136db --- /dev/null +++ b/t/t2408-worktree-relative.sh @@ -0,0 +1,39 @@ +#!/bin/sh + +test_description='test worktrees linked with relative paths' + +TEST_PASSES_SANITIZE_LEAK=true +. ./test-lib.sh + +test_expect_success 'links worktrees with relative paths' ' + test_when_finished rm -rf repo && + git init repo && + ( + cd repo && + test_commit initial && + git worktree add wt1 && + echo "../../../wt1/.git" >expected_gitdir && + cat .git/worktrees/wt1/gitdir >actual_gitdir && + echo "gitdir: ../.git/worktrees/wt1" >expected_git && + cat wt1/.git >actual_git && + test_cmp expected_gitdir actual_gitdir && + test_cmp expected_git actual_git + ) +' + +test_expect_success 'move repo without breaking relative internal links' ' + test_when_finished rm -rf repo moved && + git init repo && + ( + cd repo && + test_commit initial && + git worktree add wt1 && + cd .. && + mv repo moved && + cd moved/wt1 && + git status >out 2>err && + test_must_be_empty err + ) +' + +test_done diff --git a/worktree.c b/worktree.c index c6d2ede..fc14e9a 100644 --- a/worktree.c +++ b/worktree.c @@ -110,6 +110,12 @@ struct worktree *get_linked_worktree(const char *id, strbuf_rtrim(&worktree_path); strbuf_strip_suffix(&worktree_path, "/.git"); + if (!is_absolute_path(worktree_path.buf)) { + strbuf_strip_suffix(&path, "gitdir"); + strbuf_addbuf(&path, &worktree_path); + strbuf_realpath_forgiving(&worktree_path, path.buf, 0); + } + CALLOC_ARRAY(worktree, 1); worktree->repo = the_repository; worktree->path = strbuf_detach(&worktree_path, NULL); @@ -373,18 +379,30 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg, void update_worktree_location(struct worktree *wt, const char *path_) { struct strbuf path = STRBUF_INIT; + struct strbuf repo = STRBUF_INIT; + struct strbuf tmp = STRBUF_INIT; + char *file = NULL; if (is_main_worktree(wt)) BUG("can't relocate main worktree"); + strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1); strbuf_realpath(&path, path_, 1); if (fspathcmp(wt->path, path.buf)) { - write_file(git_common_path("worktrees/%s/gitdir", wt->id), - "%s/.git", path.buf); + file = xstrfmt("%s/gitdir", repo.buf); + write_file(file, "%s/.git", relative_path(path.buf, repo.buf, &tmp)); + free(file); + strbuf_reset(&tmp); + file = xstrfmt("%s/.git", path.buf); + write_file(file, "gitdir: %s", relative_path(repo.buf, path.buf, &tmp)); + free(wt->path); wt->path = strbuf_detach(&path, NULL); } + free(file); strbuf_release(&path); + strbuf_release(&repo); + strbuf_release(&tmp); } int is_worktree_being_rebased(const struct worktree *wt, @@ -564,38 +582,52 @@ static void repair_gitfile(struct worktree *wt, { struct strbuf dotgit = STRBUF_INIT; struct strbuf repo = STRBUF_INIT; - char *backlink; + struct strbuf backlink = STRBUF_INIT; + struct strbuf tmp = STRBUF_INIT; + char *git_contents = NULL; const char *repair = NULL; int err; /* missing worktree can't be repaired */ if (!file_exists(wt->path)) - return; + goto done; if (!is_directory(wt->path)) { fn(1, wt->path, _("not a directory"), cb_data); - return; + goto done; } strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1); strbuf_addf(&dotgit, "%s/.git", wt->path); - backlink = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err)); + git_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err)); + + if (git_contents) { + if (is_absolute_path(git_contents)) { + strbuf_addstr(&backlink, git_contents); + } else { + strbuf_addf(&backlink, "%s/%s", wt->path, git_contents); + strbuf_realpath_forgiving(&backlink, backlink.buf, 0); + } + } if (err == READ_GITFILE_ERR_NOT_A_FILE) fn(1, wt->path, _(".git is not a file"), cb_data); else if (err) repair = _(".git file broken"); - else if (fspathcmp(backlink, repo.buf)) + else if (fspathcmp(backlink.buf, repo.buf)) repair = _(".git file incorrect"); if (repair) { fn(0, wt->path, repair, cb_data); - write_file(dotgit.buf, "gitdir: %s", repo.buf); + write_file(dotgit.buf, "gitdir: %s", relative_path(repo.buf, wt->path, &tmp)); } - free(backlink); +done: + free(git_contents); strbuf_release(&repo); strbuf_release(&dotgit); + strbuf_release(&backlink); + strbuf_release(&tmp); } static void repair_noop(int iserr UNUSED, @@ -681,6 +713,8 @@ void repair_worktree_at_path(const char *path, struct strbuf backlink = STRBUF_INIT; struct strbuf gitdir = STRBUF_INIT; struct strbuf olddotgit = STRBUF_INIT; + struct strbuf realolddotgit = STRBUF_INIT; + struct strbuf tmp = STRBUF_INIT; char *git_contents = NULL; const char *repair = NULL; int err; @@ -710,97 +744,131 @@ void repair_worktree_at_path(const char *path, fn(1, realdotgit.buf, _("unable to locate repository; .git file broken"), cb_data); goto done; } else if (git_contents) { - strbuf_addstr(&backlink, git_contents); + if (is_absolute_path(git_contents)) { + strbuf_addstr(&backlink, git_contents); + } else { + strbuf_addbuf(&backlink, &realdotgit); + strbuf_strip_suffix(&backlink, ".git"); + strbuf_addstr(&backlink, git_contents); + } } + strbuf_realpath_forgiving(&backlink, backlink.buf, 0); strbuf_addf(&gitdir, "%s/gitdir", backlink.buf); if (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0) repair = _("gitdir unreadable"); else { strbuf_rtrim(&olddotgit); - if (fspathcmp(olddotgit.buf, realdotgit.buf)) + if (is_absolute_path(olddotgit.buf)) { + strbuf_addbuf(&realolddotgit, &olddotgit); + } else { + strbuf_addf(&realolddotgit, "%s/%s", backlink.buf, olddotgit.buf); + strbuf_realpath_forgiving(&realolddotgit, realolddotgit.buf, 0); + } + if (fspathcmp(realolddotgit.buf, realdotgit.buf)) repair = _("gitdir incorrect"); } if (repair) { fn(0, gitdir.buf, repair, cb_data); - write_file(gitdir.buf, "%s", realdotgit.buf); + write_file(gitdir.buf, "%s", relative_path(realdotgit.buf, backlink.buf, &tmp)); } done: free(git_contents); strbuf_release(&olddotgit); + strbuf_release(&realolddotgit); strbuf_release(&backlink); strbuf_release(&gitdir); strbuf_release(&realdotgit); strbuf_release(&dotgit); + strbuf_release(&tmp); } int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, timestamp_t expire) { struct stat st; - char *path; + struct strbuf dotgit = STRBUF_INIT; + struct strbuf gitdir = STRBUF_INIT; + struct strbuf repo = STRBUF_INIT; + char *path = NULL; + char *file = NULL; + int rc = 0; int fd; size_t len; ssize_t read_result; *wtpath = NULL; - if (!is_directory(git_path("worktrees/%s", id))) { + strbuf_realpath(&repo, git_common_path("worktrees/%s", id), 1); + strbuf_addf(&gitdir, "%s/gitdir", repo.buf); + if (!is_directory(repo.buf)) { strbuf_addstr(reason, _("not a valid directory")); - return 1; + rc = 1; + goto done; } - if (file_exists(git_path("worktrees/%s/locked", id))) - return 0; - if (stat(git_path("worktrees/%s/gitdir", id), &st)) { + file = xstrfmt("%s/locked", repo.buf); + if (file_exists(file)) { + goto done; + } + if (stat(gitdir.buf, &st)) { strbuf_addstr(reason, _("gitdir file does not exist")); - return 1; + rc = 1; + goto done; } - fd = open(git_path("worktrees/%s/gitdir", id), O_RDONLY); + fd = open(gitdir.buf, O_RDONLY); if (fd < 0) { strbuf_addf(reason, _("unable to read gitdir file (%s)"), strerror(errno)); - return 1; + rc = 1; + goto done; } len = xsize_t(st.st_size); path = xmallocz(len); read_result = read_in_full(fd, path, len); + close(fd); if (read_result < 0) { strbuf_addf(reason, _("unable to read gitdir file (%s)"), strerror(errno)); - close(fd); - free(path); - return 1; - } - close(fd); - - if (read_result != len) { + rc = 1; + goto done; + } else if (read_result != len) { strbuf_addf(reason, _("short read (expected %"PRIuMAX" bytes, read %"PRIuMAX")"), (uintmax_t)len, (uintmax_t)read_result); - free(path); - return 1; + rc = 1; + goto done; } while (len && (path[len - 1] == '\n' || path[len - 1] == '\r')) len--; if (!len) { strbuf_addstr(reason, _("invalid gitdir file")); - free(path); - return 1; + rc = 1; + goto done; } path[len] = '\0'; - if (!file_exists(path)) { - if (stat(git_path("worktrees/%s/index", id), &st) || - st.st_mtime <= expire) { + if (is_absolute_path(path)) { + strbuf_addstr(&dotgit, path); + } else { + strbuf_addf(&dotgit, "%s/%s", repo.buf, path); + strbuf_realpath_forgiving(&dotgit, dotgit.buf, 0); + } + if (!file_exists(dotgit.buf)) { + free(file); + file = xstrfmt("%s/index", repo.buf); + if (stat(file, &st) || st.st_mtime <= expire) { strbuf_addstr(reason, _("gitdir file points to non-existent location")); - free(path); - return 1; - } else { - *wtpath = path; - return 0; + rc = 1; + goto done; } } - *wtpath = path; - return 0; + *wtpath = strbuf_detach(&dotgit, NULL); +done: + free(path); + free(file); + strbuf_release(&dotgit); + strbuf_release(&gitdir); + strbuf_release(&repo); + return rc; } static int move_config_setting(const char *key, const char *value,
This modifies Git’s handling of worktree linking to use relative paths instead of absolute paths. Previously, when creating a worktree, Git would store the absolute paths to both the main repository and the linked worktrees. These hardcoded absolute paths cause breakages such as when the repository is moved to a different directory or during containerized development where the absolute differs between systems. By switching to relative paths, we help ensure that worktree links are more resilient when the repository is moved. While links external to the repository may still break, Git still automatically handles many common scenarios, reducing the need for manual repair. This is particularly useful in containerized or portable development environments, where the absolute path to the repository can differ between systems. Developers no longer need to reinitialize or repair worktrees after relocating the repository, improving workflow efficiency and reducing breakages. For self-contained repositories (such as using a bare repository with worktrees), where both the repository and its worktrees are located within the same directory structure, using relative paths guarantees all links remain functional regardless of where the directory is located. Signed-off-by: Caleb White <cdwhite3@pm.me> --- builtin/worktree.c | 17 ++-- t/t2408-worktree-relative.sh | 39 +++++++++ worktree.c | 152 +++++++++++++++++++++++++---------- 3 files changed, 159 insertions(+), 49 deletions(-) create mode 100755 t/t2408-worktree-relative.sh