Eric Sunshine <ericsunshine@xxxxxxxxxxx> writes:
> + inferred_backlink = infer_backlink(realdotgit.buf);
> backlink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
> if (err == READ_GITFILE_ERR_NOT_A_FILE) {
> fn(1, realdotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
> goto done;
> } else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
> - if (!(backlink = infer_backlink(realdotgit.buf))) {
> + if (inferred_backlink) {
> + /*
> + * Worktree's .git file does not point at a repository
> + * but we found a .git/worktrees/<id> in this
> + * repository with the same <id> as recorded in the
> + * worktree's .git file so make the worktree point at
> + * the discovered .git/worktrees/<id>. (Note: backlink
> + * is already NULL, so no need to free it first.)
> + */
> + backlink = inferred_backlink;
> + inferred_backlink = NULL;
> + } else {
> fn(1, realdotgit.buf, _("unable to locate reposito
ry; .git file does not reference a repository"), cb_data);
> goto done;
> }
Moving the `infer_backlink()` call outside of the error condition caused me to
discover a bug in my own refactor of infer_backlink() on my relative paths series.
Turns out I needed to clear the `inferred` path if an error occurred.
This is OK.
> + /*
> + * If we got this far, either the worktree's .git file pointed at a
> + * valid repository (i.e. read_gitfile_gently() returned success) or
> + * the .git file did not point at a repository but we were able to
> + * infer a suitable new value for the .git file by locating a
> + * .git/worktrees/<id> in *this* repository corresponding to the <id>
> + * recorded in the worktree's .git file.
> + *
> + * However, if, at this point, inferred_backlink is non-NULL (i.e. we
> + * found a suitable .git/worktrees/<id> in *this* repository) *and* the
> + * worktree's .git file points at a valid repository *and* those two
> + * paths
differ, then that indicates that the user probably *copied*
> + * the main and linked worktrees to a new location as a unit rather
> + * than *moving* them. Thus, the copied worktree's .git file actually
> + * points at the .git/worktrees/<id> in the *original* repository, not
> + * in the "copy" repository. In this case, point the "copy" worktree's
> + * .git file at the "copy" repository.
> + */
> + if (inferred_backlink && fspathcmp(backlink, inferred_backlink)) {
> + free(backlink);
> + backlink = inferred_backlink;
> + inferred_backlink = NULL;
> + }
This edge case seems to primarily be an artifact of using absolute paths to link
the worktrees. The test case you provided passes on my relative path series even
with this condition commented out. However, we do still want to support absolute
paths to maintain backwards compatibility so I think we should try to detect
this edge case if we can.
Unfortunately, this change now "repairs" (i.e., breaks) worktr
ees from other
repositories if the worktree_id happens to be identical between the repositories.
For example, the following test passes on `master` (with absolute paths) and on
my relative path series (relative paths without your changes integrated), but it
fails on your patch (and my relative patch series with your changes integrated):
test_expect_success 'does not repair worktrees from another repo' '
test_when_finished "rm -rf repo1 repo2" &&
mkdir -p repo1 &&
git -C repo1 init main &&
test_commit -C repo1/main nothing &&
git -C repo1/main worktree add ../linked &&
cp repo1/main/.git/worktrees/linked/gitdir repo1/main.expect &&
cp repo1/linked/.git repo1/linked.expect &&
mkdir -p repo2 &&
git -C repo2 init main &&
test_commit -C repo2/main nothing &&
git -C repo2/main worktree add ../linked &&
cp repo2/main/.git/worktrees/linked/gitdir repo2/main.expect &&
cp repo2/linked/.git repo2/linked.expect &&
git -C repo1/main worktree repair ../.. &&
test_cmp repo1/main.expect repo1/main/.git/worktrees/linked/gitdir &&
test_cmp repo1/linked.expect repo1/linked/.git &&
test_cmp repo2/main.expect repo2/main/.git/worktrees/linked/gitdir &&
test_cmp repo2/linked.expect repo2/linked/.git &&
git -C repo2/main worktree repair ../../repo1/linked &&
test_cmp repo1/main.expect repo1/main/.git/worktrees/linked/gitdir &&
test_cmp repo1/linked.expect repo1/linked/.git &&
test_cmp repo2/main.expect repo2/main/.git/worktrees/linked/gitdir &&
test_cmp repo2/linked.expect repo2/linked/.git
'
Granted, this is likely a rare edge case, but I would not expect trying to repair
a valid worktree from another repository to change the pointers. I think a good
solution to this would be to introduce a unique hash on the worktree_id when
creating a worktree to guarantee uniqu
eness across repositories, e.g.,:
repo1/main/.git/worktrees/linked-6a4b5d/gitdir:
/path/to/repo1/linked/.git
repo1/linked/.git:
gitdir: /path/to/repo1/main/worktrees/linked-6a4b5d
repo2/main/.git/worktrees/linked-9ab26fe/gitdir:
/path/to/repo2/linked/.git
repo2/linked/.git:
gitdir: /path/to/repo2/main/worktrees/linked-9ab26fe
This would allow us to detect this copied case while not "repairing" valid
worktrees from other repositories with the same linked worktree directory name.
Best,
- Caleb
Attachment:
signature.asc
Description: OpenPGP digital signature
