Skip to content

Commit b2f602b

Browse files
committed
Merge branch 'yn/worktree-repair-fix' into seen
The 'git worktree repair' command learned to thoroughly validate the worktree ID and the inferred gitdir pathway, preventing it from corrupting unrelated '.git' files and gitdirs when repositories and their linked worktrees are moved or swapped. * yn/worktree-repair-fix: worktree repair: avoid breaking unrelated .git file and gitdir worktree repair: reduce .git file reads worktree repair: use read_gitfile_raw() in infer_backlink() worktree repair: centralize inferred backlink selection worktree repair: align .git file handling with repair_gitfile()
2 parents fdc0f33 + e1afa01 commit b2f602b

2 files changed

Lines changed: 91 additions & 55 deletions

File tree

‎t/t2406-worktree-repair.sh‎

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -56,15 +56,12 @@ test_expect_success 'repair missing .git file' '
5656
'
5757

5858
test_expect_success 'repair bogus .git file' '
59-
test_corrupt_gitfile "echo \"gitdir: /nowhere\" >corrupt/.git" \
59+
test_corrupt_gitfile "echo \"contents not started with gitdir:\" >corrupt/.git" \
6060
".git file broken"
6161
'
6262

63-
test_expect_success 'repair incorrect .git file' '
64-
test_when_finished "rm -rf other && git worktree prune" &&
65-
test_create_repo other &&
66-
other=$(git -C other rev-parse --absolute-git-dir) &&
67-
test_corrupt_gitfile "echo \"gitdir: $other\" >corrupt/.git" \
63+
test_expect_success 'repair unlinked .git file' '
64+
test_corrupt_gitfile "echo \"gitdir: /nowhere/worktrees/corrupt\" >corrupt/.git" \
6865
".git file incorrect"
6966
'
7067

@@ -89,6 +86,18 @@ test_expect_success 'repair .git file from bare.git' '
8986
test_cmp expect actual
9087
'
9188

89+
test_expect_success 'skip unrelated .git file' '
90+
test_when_finished "rm -rf corrupt other && git worktree prune" &&
91+
git worktree add --detach corrupt &&
92+
rm -rf corrupt &&
93+
git worktree add --detach other &&
94+
mv other corrupt &&
95+
cat corrupt/.git >expect &&
96+
test_must_fail git worktree repair 2>err &&
97+
test_cmp expect corrupt/.git &&
98+
test_grep "unrelated .git file" err
99+
'
100+
92101
test_expect_success 'invalid worktree path' '
93102
test_must_fail git worktree repair /notvalid >out 2>err &&
94103
test_must_be_empty out &&
@@ -113,6 +122,18 @@ test_expect_success 'repo not found; .git not referencing repo' '
113122
test_grep ".git file does not reference a repository" err
114123
'
115124

125+
test_expect_success 'repo not found; .git not for worktree' '
126+
test_when_finished "rm -rf side other-repo && git worktree prune" &&
127+
test_create_repo other-repo &&
128+
git worktree add --detach side &&
129+
cat .git/worktrees/side/gitdir >expect &&
130+
cp -R side other-repo/side &&
131+
test_must_fail git -C other-repo worktree repair side >out 2>err &&
132+
test_cmp expect .git/worktrees/side/gitdir &&
133+
test_must_be_empty out &&
134+
test_grep ".git file is not for a linked worktree" err
135+
'
136+
116137
test_expect_success 'repo not found; .git file broken' '
117138
test_when_finished "rm -rf orig moved && git worktree prune" &&
118139
git worktree add --detach orig &&

‎worktree.c‎

Lines changed: 64 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -637,6 +637,18 @@ int other_head_refs(struct repository *repo,
637637
return ret;
638638
}
639639

640+
static const char *get_worktree_id(const char *dotgit_contents)
641+
{
642+
const char *slash = find_last_dir_sep(dotgit_contents);
643+
const char *prefix = "/worktrees";
644+
int prefixlen = strlen(prefix);
645+
if (!slash ||
646+
slash - dotgit_contents < prefixlen ||
647+
strncmp(slash - prefixlen, prefix, prefixlen))
648+
return "";
649+
return slash + 1;
650+
}
651+
640652
/*
641653
* Repair worktree's /path/to/worktree/.git file if missing, corrupt, or not
642654
* pointing at <repo>/worktrees/<id>.
@@ -684,8 +696,10 @@ static void repair_gitfile(struct worktree *wt,
684696
if (err == READ_GITFILE_ERR_NOT_A_FILE ||
685697
err == READ_GITFILE_ERR_IS_A_DIR)
686698
fn(1, wt->path, _(".git is not a file"), cb_data);
687-
else if (err || !is_git_directory(backlink.buf))
699+
else if (err)
688700
repair = _(".git file broken");
701+
else if (strcmp(get_worktree_id(dotgit_contents), wt->id))
702+
fn(1, wt->path, _("unrelated .git file"), cb_data);
689703
else if (fspathcmp(backlink.buf, repo.buf))
690704
repair = _(".git file incorrect");
691705
else if (use_relative_paths == is_absolute_path(dotgit_contents))
@@ -798,30 +812,20 @@ static int is_main_worktree_path(struct repository *repo, const char *path)
798812
* Returns -1 on failure and strbuf.len on success.
799813
*/
800814
static ssize_t infer_backlink(struct repository *repo,
801-
const char *gitfile,
815+
const char *dotgit_contents,
802816
struct strbuf *inferred)
803817
{
804-
struct strbuf actual = STRBUF_INIT;
805818
const char *id;
806819

807-
if (strbuf_read_file(&actual, gitfile, 0) < 0)
808-
goto error;
809-
if (!starts_with(actual.buf, "gitdir:"))
810-
goto error;
811-
if (!(id = find_last_dir_sep(actual.buf)))
812-
goto error;
813-
strbuf_trim(&actual);
814-
id++; /* advance past '/' to point at <id> */
820+
id = get_worktree_id(dotgit_contents);
815821
if (!*id)
816822
goto error;
817823
repo_common_path_replace(repo, inferred, "worktrees/%s", id);
818-
if (!is_directory(inferred->buf))
824+
if (!is_git_directory(inferred->buf))
819825
goto error;
820826

821-
strbuf_release(&actual);
822827
return inferred->len;
823828
error:
824-
strbuf_release(&actual);
825829
strbuf_reset(inferred); /* clear invalid path */
826830
return -1;
827831
}
@@ -840,7 +844,8 @@ void repair_worktree_at_path(struct repository *repo,
840844
struct strbuf inferred_backlink = STRBUF_INIT;
841845
struct strbuf gitdir = STRBUF_INIT;
842846
struct strbuf olddotgit = STRBUF_INIT;
843-
char *dotgit_contents = NULL;
847+
struct strbuf contents = STRBUF_INIT;
848+
const char *dotgit_contents = NULL;
844849
const char *repair = NULL;
845850
int err;
846851

@@ -856,53 +861,63 @@ void repair_worktree_at_path(struct repository *repo,
856861
goto done;
857862
}
858863

859-
infer_backlink(repo, dotgit.buf, &inferred_backlink);
864+
err = read_gitfile_raw(&contents, dotgit.buf);
865+
if (err == READ_GITFILE_ERR_NOT_A_FILE ||
866+
err == READ_GITFILE_ERR_IS_A_DIR) {
867+
fn(1, dotgit.buf,
868+
_("unable to locate repository; .git is not a file"),
869+
cb_data);
870+
goto done;
871+
} else if (err) {
872+
fn(1, dotgit.buf,
873+
_("unable to locate repository; .git file broken"),
874+
cb_data);
875+
goto done;
876+
}
877+
878+
dotgit_contents = contents.buf;
879+
infer_backlink(repo, dotgit_contents, &inferred_backlink);
860880
strbuf_realpath_forgiving(&inferred_backlink, inferred_backlink.buf, 0);
861-
dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
862-
if (dotgit_contents) {
881+
882+
if (is_absolute_path(dotgit_contents)) {
863883
strbuf_addstr(&backlink, dotgit_contents);
864-
} else if (err == READ_GITFILE_ERR_NOT_A_FILE ||
865-
err == READ_GITFILE_ERR_IS_A_DIR) {
866-
fn(1, dotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
867-
goto done;
868-
} else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
869-
if (inferred_backlink.len) {
870-
/*
871-
* Worktree's .git file does not point at a repository
872-
* but we found a .git/worktrees/<id> in this
873-
* repository with the same <id> as recorded in the
874-
* worktree's .git file so make the worktree point at
875-
* the discovered .git/worktrees/<id>.
876-
*/
877-
strbuf_swap(&backlink, &inferred_backlink);
878-
} else {
879-
fn(1, dotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
880-
goto done;
881-
}
882884
} else {
883-
fn(1, dotgit.buf, _("unable to locate repository; .git file broken"), cb_data);
885+
strbuf_addbuf(&backlink, &dotgit);
886+
strbuf_strip_suffix(&backlink, ".git");
887+
strbuf_addstr(&backlink, dotgit_contents);
888+
strbuf_realpath_forgiving(&backlink, backlink.buf, 0);
889+
}
890+
891+
if (!is_git_directory(backlink.buf) && !inferred_backlink.len) {
892+
fn(1, dotgit.buf,
893+
_("unable to locate repository; .git file does not reference a repository"),
894+
cb_data);
895+
goto done;
896+
}
897+
if (!inferred_backlink.len) {
898+
fn(1, dotgit.buf,
899+
_("unable to locate repository; .git file is not for a linked worktree"),
900+
cb_data);
884901
goto done;
885902
}
886903

887904
/*
888905
* If we got this far, either the worktree's .git file pointed at a
889-
* valid repository (i.e. read_gitfile_gently() returned success) or
906+
* valid repository (i.e. is_git_directory() returned true) or
890907
* the .git file did not point at a repository but we were able to
891908
* infer a suitable new value for the .git file by locating a
892909
* .git/worktrees/<id> in *this* repository corresponding to the <id>
893910
* recorded in the worktree's .git file.
894911
*
895-
* However, if, at this point, inferred_backlink is non-NULL (i.e. we
896-
* found a suitable .git/worktrees/<id> in *this* repository) *and* the
897-
* worktree's .git file points at a valid repository *and* those two
898-
* paths differ, then that indicates that the user probably *copied*
899-
* the main and linked worktrees to a new location as a unit rather
900-
* than *moving* them. Thus, the copied worktree's .git file actually
901-
* points at the .git/worktrees/<id> in the *original* repository, not
902-
* in the "copy" repository. In this case, point the "copy" worktree's
903-
* .git file at the "copy" repository.
912+
* Even if the worktree's .git file pointed at a valid repository,
913+
* it doesn't always mean that the backlink is correct. For example,
914+
* the user might have *copied* the main and linked worktrees to a
915+
* new location as a unit rather than *moving* them (the copied
916+
* worktree's .git file actually points at the .git/worktrees/<id>
917+
* in the *original* repository, not in the "copy" repository).
918+
* Therefore, we prioritize inferred_backlink over backlink.
904919
*/
905-
if (inferred_backlink.len && fspathcmp(backlink.buf, inferred_backlink.buf))
920+
if (fspathcmp(backlink.buf, inferred_backlink.buf))
906921
strbuf_swap(&backlink, &inferred_backlink);
907922

908923
strbuf_addf(&gitdir, "%s/gitdir", backlink.buf);
@@ -926,12 +941,12 @@ void repair_worktree_at_path(struct repository *repo,
926941
gitdir.buf, use_relative_paths);
927942
}
928943
done:
929-
free(dotgit_contents);
930944
strbuf_release(&olddotgit);
931945
strbuf_release(&backlink);
932946
strbuf_release(&inferred_backlink);
933947
strbuf_release(&gitdir);
934948
strbuf_release(&dotgit);
949+
strbuf_release(&contents);
935950
}
936951

937952
int should_prune_worktree(struct repository *repo,

0 commit comments

Comments
 (0)