Skip to content

Commit 9740ef8

Browse files
gitstergitster
committed
Merge branch 'es/worktree-duplicate-paths'
The same worktree directory must be registered only once, but "git worktree move" allowed this invariant to be violated, which has been corrected. * es/worktree-duplicate-paths: worktree: make "move" refuse to move atop missing registered worktree worktree: generalize candidate worktree path validation worktree: prune linked worktree referencing main worktree path worktree: prune duplicate entries referencing same worktree path worktree: make high-level pruning re-usable worktree: give "should be pruned?" function more meaningful name worktree: factor out repeated string literal
2 parents b8a5299 + 810382e commit 9740ef8

File tree

4 files changed

+141
-36
lines changed

4 files changed

+141
-36
lines changed

‎Documentation/git-worktree.txt‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,9 @@ OPTIONS
126126
locked working tree path, specify `--force` twice.
127127
+
128128
`move` refuses to move a locked working tree unless `--force` is specified
129-
twice.
129+
twice. If the destination is already assigned to some other working tree but is
130+
missing (for instance, if `<new-path>` was deleted manually), then `--force`
131+
allows the move to proceed; use --force twice if the destination is locked.
130132
+
131133
`remove` refuses to remove an unclean working tree unless `--force` is used.
132134
To remove a locked working tree, specify `--force` twice.

‎builtin/worktree.c‎

Lines changed: 93 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -67,37 +67,43 @@ static void delete_worktrees_dir_if_empty(void)
6767
rmdir(git_path("worktrees")); /* ignore failed removal */
6868
}
6969

70-
static int prune_worktree(const char *id, struct strbuf *reason)
70+
/*
71+
* Return true if worktree entry should be pruned, along with the reason for
72+
* pruning. Otherwise, return false and the worktree's path, or NULL if it
73+
* cannot be determined. Caller is responsible for freeing returned path.
74+
*/
75+
static int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath)
7176
{
7277
struct stat st;
7378
char *path;
7479
int fd;
7580
size_t len;
7681
ssize_t read_result;
7782

83+
*wtpath = NULL;
7884
if (!is_directory(git_path("worktrees/%s", id))) {
79-
strbuf_addf(reason, _("Removing worktrees/%s: not a valid directory"), id);
85+
strbuf_addstr(reason, _("not a valid directory"));
8086
return 1;
8187
}
8288
if (file_exists(git_path("worktrees/%s/locked", id)))
8389
return 0;
8490
if (stat(git_path("worktrees/%s/gitdir", id), &st)) {
85-
strbuf_addf(reason, _("Removing worktrees/%s: gitdir file does not exist"), id);
91+
strbuf_addstr(reason, _("gitdir file does not exist"));
8692
return 1;
8793
}
8894
fd = open(git_path("worktrees/%s/gitdir", id), O_RDONLY);
8995
if (fd < 0) {
90-
strbuf_addf(reason, _("Removing worktrees/%s: unable to read gitdir file (%s)"),
91-
id, strerror(errno));
96+
strbuf_addf(reason, _("unable to read gitdir file (%s)"),
97+
strerror(errno));
9298
return 1;
9399
}
94100
len = xsize_t(st.st_size);
95101
path = xmallocz(len);
96102

97103
read_result = read_in_full(fd, path, len);
98104
if (read_result < 0) {
99-
strbuf_addf(reason, _("Removing worktrees/%s: unable to read gitdir file (%s)"),
100-
id, strerror(errno));
105+
strbuf_addf(reason, _("unable to read gitdir file (%s)"),
106+
strerror(errno));
101107
close(fd);
102108
free(path);
103109
return 1;
@@ -106,53 +112,103 @@ static int prune_worktree(const char *id, struct strbuf *reason)
106112

107113
if (read_result != len) {
108114
strbuf_addf(reason,
109-
_("Removing worktrees/%s: short read (expected %"PRIuMAX" bytes, read %"PRIuMAX")"),
110-
id, (uintmax_t)len, (uintmax_t)read_result);
115+
_("short read (expected %"PRIuMAX" bytes, read %"PRIuMAX")"),
116+
(uintmax_t)len, (uintmax_t)read_result);
111117
free(path);
112118
return 1;
113119
}
114120
while (len && (path[len - 1] == '\n' || path[len - 1] == '\r'))
115121
len--;
116122
if (!len) {
117-
strbuf_addf(reason, _("Removing worktrees/%s: invalid gitdir file"), id);
123+
strbuf_addstr(reason, _("invalid gitdir file"));
118124
free(path);
119125
return 1;
120126
}
121127
path[len] = '\0';
122128
if (!file_exists(path)) {
123-
free(path);
124129
if (stat(git_path("worktrees/%s/index", id), &st) ||
125130
st.st_mtime <= expire) {
126-
strbuf_addf(reason, _("Removing worktrees/%s: gitdir file points to non-existent location"), id);
131+
strbuf_addstr(reason, _("gitdir file points to non-existent location"));
132+
free(path);
127133
return 1;
128134
} else {
135+
*wtpath = path;
129136
return 0;
130137
}
131138
}
132-
free(path);
139+
*wtpath = path;
133140
return 0;
134141
}
135142

143+
static void prune_worktree(const char *id, const char *reason)
144+
{
145+
if (show_only || verbose)
146+
printf_ln(_("Removing %s/%s: %s"), "worktrees", id, reason);
147+
if (!show_only)
148+
delete_git_dir(id);
149+
}
150+
151+
static int prune_cmp(const void *a, const void *b)
152+
{
153+
const struct string_list_item *x = a;
154+
const struct string_list_item *y = b;
155+
int c;
156+
157+
if ((c = fspathcmp(x->string, y->string)))
158+
return c;
159+
/*
160+
* paths same; prune_dupes() removes all but the first worktree entry
161+
* having the same path, so sort main worktree ('util' is NULL) above
162+
* linked worktrees ('util' not NULL) since main worktree can't be
163+
* removed
164+
*/
165+
if (!x->util)
166+
return -1;
167+
if (!y->util)
168+
return 1;
169+
/* paths same; sort by .git/worktrees/<id> */
170+
return strcmp(x->util, y->util);
171+
}
172+
173+
static void prune_dups(struct string_list *l)
174+
{
175+
int i;
176+
177+
QSORT(l->items, l->nr, prune_cmp);
178+
for (i = 1; i < l->nr; i++) {
179+
if (!fspathcmp(l->items[i].string, l->items[i - 1].string))
180+
prune_worktree(l->items[i].util, "duplicate entry");
181+
}
182+
}
183+
136184
static void prune_worktrees(void)
137185
{
138186
struct strbuf reason = STRBUF_INIT;
187+
struct strbuf main_path = STRBUF_INIT;
188+
struct string_list kept = STRING_LIST_INIT_NODUP;
139189
DIR *dir = opendir(git_path("worktrees"));
140190
struct dirent *d;
141191
if (!dir)
142192
return;
143193
while ((d = readdir(dir)) != NULL) {
194+
char *path;
144195
if (is_dot_or_dotdot(d->d_name))
145196
continue;
146197
strbuf_reset(&reason);
147-
if (!prune_worktree(d->d_name, &reason))
148-
continue;
149-
if (show_only || verbose)
150-
printf("%s\n", reason.buf);
151-
if (show_only)
152-
continue;
153-
delete_git_dir(d->d_name);
198+
if (should_prune_worktree(d->d_name, &reason, &path))
199+
prune_worktree(d->d_name, reason.buf);
200+
else if (path)
201+
string_list_append(&kept, path)->util = xstrdup(d->d_name);
154202
}
155203
closedir(dir);
204+
205+
strbuf_add_absolute_path(&main_path, get_git_common_dir());
206+
/* massage main worktree absolute path to match 'gitdir' content */
207+
strbuf_strip_suffix(&main_path, "/.");
208+
string_list_append(&kept, strbuf_detach(&main_path, NULL));
209+
prune_dups(&kept);
210+
string_list_clear(&kept, 1);
211+
156212
if (!show_only)
157213
delete_worktrees_dir_if_empty();
158214
strbuf_release(&reason);
@@ -224,34 +280,33 @@ static const char *worktree_basename(const char *path, int *olen)
224280
return name;
225281
}
226282

227-
static void validate_worktree_add(const char *path, const struct add_opts *opts)
283+
/* check that path is viable location for worktree */
284+
static void check_candidate_path(const char *path,
285+
int force,
286+
struct worktree **worktrees,
287+
const char *cmd)
228288
{
229-
struct worktree **worktrees;
230289
struct worktree *wt;
231290
int locked;
232291

233292
if (file_exists(path) && !is_empty_dir(path))
234293
die(_("'%s' already exists"), path);
235294

236-
worktrees = get_worktrees(0);
237295
wt = find_worktree_by_path(worktrees, path);
238296
if (!wt)
239-
goto done;
297+
return;
240298

241299
locked = !!worktree_lock_reason(wt);
242-
if ((!locked && opts->force) || (locked && opts->force > 1)) {
300+
if ((!locked && force) || (locked && force > 1)) {
243301
if (delete_git_dir(wt->id))
244-
die(_("unable to re-add worktree '%s'"), path);
245-
goto done;
302+
die(_("unusable worktree destination '%s'"), path);
303+
return;
246304
}
247305

248306
if (locked)
249-
die(_("'%s' is a missing but locked worktree;\nuse 'add -f -f' to override, or 'unlock' and 'prune' or 'remove' to clear"), path);
307+
die(_("'%s' is a missing but locked worktree;\nuse '%s -f -f' to override, or 'unlock' and 'prune' or 'remove' to clear"), cmd, path);
250308
else
251-
die(_("'%s' is a missing but already registered worktree;\nuse 'add -f' to override, or 'prune' or 'remove' to clear"), path);
252-
253-
done:
254-
free_worktrees(worktrees);
309+
die(_("'%s' is a missing but already registered worktree;\nuse '%s -f' to override, or 'prune' or 'remove' to clear"), cmd, path);
255310
}
256311

257312
static int add_worktree(const char *path, const char *refname,
@@ -268,8 +323,12 @@ static int add_worktree(const char *path, const char *refname,
268323
struct commit *commit = NULL;
269324
int is_branch = 0;
270325
struct strbuf sb_name = STRBUF_INIT;
326+
struct worktree **worktrees;
271327

272-
validate_worktree_add(path, opts);
328+
worktrees = get_worktrees(0);
329+
check_candidate_path(path, opts->force, worktrees, "add");
330+
free_worktrees(worktrees);
331+
worktrees = NULL;
273332

274333
/* is 'refname' a branch or commit? */
275334
if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
@@ -804,8 +863,7 @@ static int move_worktree(int ac, const char **av, const char *prefix)
804863
strbuf_trim_trailing_dir_sep(&dst);
805864
strbuf_addstr(&dst, sep);
806865
}
807-
if (file_exists(dst.buf))
808-
die(_("target '%s' already exists"), dst.buf);
866+
check_candidate_path(dst.buf, force, worktrees, "move");
809867

810868
validate_no_submodules(wt);
811869

‎t/t2401-worktree-prune.sh‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,4 +92,28 @@ test_expect_success 'not prune proper checkouts' '
9292
test -d .git/worktrees/nop
9393
'
9494

95+
test_expect_success 'prune duplicate (linked/linked)' '
96+
test_when_finished rm -fr .git/worktrees w1 w2 &&
97+
git worktree add --detach w1 &&
98+
git worktree add --detach w2 &&
99+
sed "s/w2/w1/" .git/worktrees/w2/gitdir >.git/worktrees/w2/gitdir.new &&
100+
mv .git/worktrees/w2/gitdir.new .git/worktrees/w2/gitdir &&
101+
git worktree prune --verbose >actual &&
102+
test_i18ngrep "duplicate entry" actual &&
103+
test -d .git/worktrees/w1 &&
104+
! test -d .git/worktrees/w2
105+
'
106+
107+
test_expect_success 'prune duplicate (main/linked)' '
108+
test_when_finished rm -fr repo wt &&
109+
test_create_repo repo &&
110+
test_commit -C repo x &&
111+
git -C repo worktree add --detach ../wt &&
112+
rm -fr wt &&
113+
mv repo wt &&
114+
git -C wt worktree prune --verbose >actual &&
115+
test_i18ngrep "duplicate entry" actual &&
116+
! test -d .git/worktrees/wt
117+
'
118+
95119
test_done

‎t/t2403-worktree-move.sh‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,27 @@ test_expect_success 'move locked worktree (force)' '
112112
git worktree move --force --force flump ploof
113113
'
114114

115+
test_expect_success 'refuse to move worktree atop existing path' '
116+
>bobble &&
117+
git worktree add --detach beeble &&
118+
test_must_fail git worktree move beeble bobble
119+
'
120+
121+
test_expect_success 'move atop existing but missing worktree' '
122+
git worktree add --detach gnoo &&
123+
git worktree add --detach pneu &&
124+
rm -fr pneu &&
125+
test_must_fail git worktree move gnoo pneu &&
126+
git worktree move --force gnoo pneu &&
127+
128+
git worktree add --detach nu &&
129+
git worktree lock nu &&
130+
rm -fr nu &&
131+
test_must_fail git worktree move pneu nu &&
132+
test_must_fail git worktree --force move pneu nu &&
133+
git worktree move --force --force pneu nu
134+
'
135+
115136
test_expect_success 'move a repo with uninitialized submodule' '
116137
git init withsub &&
117138
(

0 commit comments

Comments
 (0)