| Message ID | 20241006060017.171788-4-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: > When re-initializing a repository with a separate gitdir (the original > gitdir is moved to a new location), any linked worktrees become broken > and must be repaired to reflect the new gitdir location. For absolute > paths, this breakage is one-way, but is both ways for relative paths > (the `<worktree>/.git` and the `<repo>/worktrees/<id>/gitdir`). > > Previously, `repair_worktrees` was being called which loops through all > the worktrees in the repository and updates the `<worktree>/.git` files > to point to the new gitdir. However, when both sides of the worktrees > are broken, the previous gitdir location is required to reestablish the > link. > > To fix this, the function `repair_worktrees_after_gitdir_move` is > introduced. It takes the old gitdir path as an argument and repairs both > sides of the worktree. > > This change fixes the following test cases in t0001-init.sh: > - re-init to move gitdir with linked worktrees > - re-init to move gitdir within linked worktree If I understand correctly, this patch is fixing breakage resulting from the preceding patch. Unfortunately, this approach is problematic since it breaks bisectability of the project. In particular, it's not sufficient for the full test suite to pass only at the end of the patch series; rather, the entire test suite should continue to pass after application of *each* patch in a series; we don't want one patch to break the test suite and a subsequent patch to fix it again. So, if my understanding is correct, please put some thought into how to reorganize this patch series to ensure that the full test suite passes for each patch.
On Sunday, October 6th, 2024 at 06:12, Eric Sunshine <sunshine@sunshineco.com> wrote: > If I understand correctly, this patch is fixing breakage resulting > from the preceding patch. Unfortunately, this approach is problematic > since it breaks bisectability of the project. In particular, it's not > sufficient for the full test suite to pass only at the end of the > patch series; rather, the entire test suite should continue to pass > after application of each patch in a series; we don't want one patch > to break the test suite and a subsequent patch to fix it again. > > So, if my understanding is correct, please put some thought into how > to reorganize this patch series to ensure that the full test suite > passes for each patch. Yes, there was one edge case that broke and this patch fixed. But I understand what you mean about the bisectability. I was trying to come up with ways to split up the commits and this seemed like a good spot as it just introduced new functions with minimal changes elsewhere. But this can be squashed into the previous patch.
On Sun, Oct 6, 2024 at 6:41 PM Caleb White <cdwhite3@pm.me> wrote: > On Sunday, October 6th, 2024 at 06:12, Eric Sunshine <sunshine@sunshineco.com> wrote: > > So, if my understanding is correct, please put some thought into how > > to reorganize this patch series to ensure that the full test suite > > passes for each patch. > > Yes, there was one edge case that broke and this patch fixed. But I > understand what you mean about the bisectability. I was trying to come > up with ways to split up the commits and this seemed like a good spot as > it just introduced new functions with minimal changes elsewhere. But > this can be squashed into the previous patch. I haven't yet pored over the code in-depth, so I don't know if it is even possible, but it's typically very much preferred by reviewers if you can present a series as smaller, simpler, easier-to-digest patches than large monolithic ones. So, it would be ideal if you could figure out some good split points (especially since patch [2/4] is already uncomfortably large for a reviewer). But sometimes it's just not possible to find good splits, so a large patch may be the only choice.
On Sunday, October 6th, 2024 at 17:48, Eric Sunshine <sunshine@sunshineco.com> wrote: > I haven't yet pored over the code in-depth, so I don't know if it is > even possible, but it's typically very much preferred by reviewers if > you can present a series as smaller, simpler, easier-to-digest patches > than large monolithic ones. So, it would be ideal if you could figure > out some good split points (especially since patch [2/4] is already > uncomfortably large for a reviewer). But sometimes it's just not > possible to find good splits, so a large patch may be the only choice. There's really not any other good split points because it's an all or nothing kind of thing. All of these changes need to be in place at the same time or there's some edge cases that are going to fail. I suppose I could try to split the *reading* of the absolute/relative paths separate from the *writing* of the relative paths. However, I'm not sure if this would be worth the trouble as most places that read from the files also write to the files.
On Sun, Oct 6, 2024 at 7:13 PM Caleb White <cdwhite3@pm.me> wrote: > On Sunday, October 6th, 2024 at 17:48, Eric Sunshine <sunshine@sunshineco.com> wrote: > > I haven't yet pored over the code in-depth, so I don't know if it is > > even possible, but it's typically very much preferred by reviewers if > > you can present a series as smaller, simpler, easier-to-digest patches > > than large monolithic ones. So, it would be ideal if you could figure > > out some good split points (especially since patch [2/4] is already > > uncomfortably large for a reviewer). But sometimes it's just not > > possible to find good splits, so a large patch may be the only choice. > > There's really not any other good split points because it's > an all or nothing kind of thing. All of these changes need to be in place > at the same time or there's some edge cases that are going to fail. > > I suppose I could try to split the *reading* of the absolute/relative paths > separate from the *writing* of the relative paths. However, I'm not > sure if this would be worth the trouble as most places that read from > the files also write to the files. If that's the case, then it probably wouldn't help to split it up in that fashion. Artificial splits like the one you describe are very likely to be more confusing for reviewers than helpful.
diff --git a/setup.c b/setup.c index 94e79b2..7b648de 100644 --- a/setup.c +++ b/setup.c @@ -2420,7 +2420,7 @@ static void separate_git_dir(const char *git_dir, const char *git_link) if (rename(src, git_dir)) die_errno(_("unable to move %s to %s"), src, git_dir); - repair_worktrees(NULL, NULL); + repair_worktrees_after_gitdir_move(src); } write_file(git_link, "gitdir: %s", git_dir); diff --git a/worktree.c b/worktree.c index fc14e9a..b08ecce 100644 --- a/worktree.c +++ b/worktree.c @@ -650,6 +650,60 @@ void repair_worktrees(worktree_repair_fn fn, void *cb_data) free_worktrees(worktrees); } +void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path) +{ + struct strbuf path = STRBUF_INIT; + struct strbuf repo = STRBUF_ INIT; + struct strbuf gitdir = STRBUF_INIT; + struct strbuf dotgit = STRBUF_INIT; + struct strbuf olddotgit = STRBUF_INIT; + struct strbuf tmp = STRBUF_INIT; + + if (is_main_worktree(wt)) + goto done; + + strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1); + strbuf_addf(&gitdir, "%s/gitdir", repo.buf); + + if (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0) + goto done; + + strbuf_rtrim(&olddotgit); + if (is_absolute_path(olddotgit.buf)) { + strbuf_addbuf(&dotgit, &olddotgit); + } else { + strbuf_addf(&dotgit, "%s/worktrees/%s/%s", old_path, wt->id, olddotgit.buf); + strbuf_realpath_forgiving(&dotgit, dotgit.buf, 0); + } + + if (!file_exists(dotgit.buf)) + goto done; + + strbuf_addbuf(&path, &dotgit); + strbuf_strip_suffix(&path, "/.git"); + + write_file(dotgit.buf, "gitdir: %s", relative_path(repo.buf, path.buf, &tmp)); + strbuf_reset(&tmp); + write_file(gitdir.buf, "%s", relative_path(dotgit.buf, repo.buf, &tmp)); +done: + strbu f_release(&path); + strbuf_release(&repo); + strbuf_release(&gitdir); + strbuf_release(&dotgit); + strbuf_release(&olddotgit); + strbuf_release(&tmp); +} + +void repair_worktrees_after_gitdir_move(const char *old_path) +{ + struct worktree **worktrees = get_worktrees_internal(1); + struct worktree **wt = worktrees + 1; /* +1 skips main worktree */ + + for (; *wt; wt++) + repair_worktree_after_gitdir_move(*wt, old_path); + free_worktrees(worktrees); +} + static int is_main_worktree_path(const char *path) { struct strbuf target = STRBUF_INIT; diff --git a/worktree.h b/worktree.h index 11279d0..e961186 100644 --- a/worktree.h +++ b/worktree.h @@ -131,6 +131,16 @@ typedef void (* worktree_repair_fn)(int iserr, const char *path, */ void repair_worktrees(worktree_repair_fn, void *cb_data); +/* + * Repair the linked worktrees after the gitdir has been moved. + */ +void repair_worktrees_after_gitdir_move(const char *old_path); + +/* + * Repair the l inked worktree after the gitdir has been moved. + */ +void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path); + /* * Repair administrative files corresponding to the worktree at the given path.
When re-initializing a repository with a separate gitdir (the original gitdir is moved to a new location), any linked worktrees become broken and must be repaired to reflect the new gitdir location. For absolute paths, this breakage is one-way, but is both ways for relative paths (the `<worktree>/.git` and the `<repo>/worktrees/<id>/gitdir`). Previously, `repair_worktrees` was being called which loops through all the worktrees in the repository and updates the `<worktree>/.git` files to point to the new gitdir. However, when both sides of the worktrees are broken, the previous gitdir location is required to reestablish the link. To fix this, the function `repair_worktrees_after_gitdir_move` is introduced. It takes the old gitdir path as an argument and repairs both sides of the worktree. This change fixes the following test cases in t0001-init.sh: - re-init to move gitdir with linked worktrees - re-init to move gitdir within linked worktree Signed-off-by: Caleb White <cdwhite3@pm.me> --- setup.c | 2 +- worktree.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ worktree.h | 10 ++++++++++ 3 files changed, 65 insertions(+), 1 deletion(-) * The worktree's .git file pointing at the repository must be intact for the