From c9b77f2cea2f0b4ee34d1d7a25cd1ebed767caf4 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Mon, 8 Jun 2020 02:23:49 -0400 Subject: [PATCH 1/7] worktree: factor out repeated string literal For each worktree removed by "git worktree prune", it reports the reason for the removal. All reasons share the common prefix "Removing worktrees/%s:". As new removal reasons are added, this prefix needs to be duplicated, which is error-prone and potentially cumbersome. Therefore, factor out the common prefix. Although this change seems to increase the "sentence lego quotient", it should be reasonably safe, as the reason for removal is a distinct clause, not strictly related to the prefix. Moreover, the "worktrees" in "Removing worktrees/%s:" is a path literal which ought not be localized, so by factoring it out, we can more easily avoid exposing that path fragment to translators. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- builtin/worktree.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/builtin/worktree.c b/builtin/worktree.c index 24f22800f3..27325f42c5 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -76,19 +76,19 @@ static int prune_worktree(const char *id, struct strbuf *reason) ssize_t read_result; if (!is_directory(git_path("worktrees/%s", id))) { - strbuf_addf(reason, _("Removing worktrees/%s: not a valid directory"), id); + strbuf_addstr(reason, _("not a valid directory")); return 1; } if (file_exists(git_path("worktrees/%s/locked", id))) return 0; if (stat(git_path("worktrees/%s/gitdir", id), &st)) { - strbuf_addf(reason, _("Removing worktrees/%s: gitdir file does not exist"), id); + strbuf_addstr(reason, _("gitdir file does not exist")); return 1; } fd = open(git_path("worktrees/%s/gitdir", id), O_RDONLY); if (fd < 0) { - strbuf_addf(reason, _("Removing worktrees/%s: unable to read gitdir file (%s)"), - id, strerror(errno)); + strbuf_addf(reason, _("unable to read gitdir file (%s)"), + strerror(errno)); return 1; } len = xsize_t(st.st_size); @@ -96,8 +96,8 @@ static int prune_worktree(const char *id, struct strbuf *reason) read_result = read_in_full(fd, path, len); if (read_result < 0) { - strbuf_addf(reason, _("Removing worktrees/%s: unable to read gitdir file (%s)"), - id, strerror(errno)); + strbuf_addf(reason, _("unable to read gitdir file (%s)"), + strerror(errno)); close(fd); free(path); return 1; @@ -106,15 +106,15 @@ static int prune_worktree(const char *id, struct strbuf *reason) if (read_result != len) { strbuf_addf(reason, - _("Removing worktrees/%s: short read (expected %"PRIuMAX" bytes, read %"PRIuMAX")"), - id, (uintmax_t)len, (uintmax_t)read_result); + _("short read (expected %"PRIuMAX" bytes, read %"PRIuMAX")"), + (uintmax_t)len, (uintmax_t)read_result); free(path); return 1; } while (len && (path[len - 1] == '\n' || path[len - 1] == '\r')) len--; if (!len) { - strbuf_addf(reason, _("Removing worktrees/%s: invalid gitdir file"), id); + strbuf_addstr(reason, _("invalid gitdir file")); free(path); return 1; } @@ -123,7 +123,7 @@ static int prune_worktree(const char *id, struct strbuf *reason) free(path); if (stat(git_path("worktrees/%s/index", id), &st) || st.st_mtime <= expire) { - strbuf_addf(reason, _("Removing worktrees/%s: gitdir file points to non-existent location"), id); + strbuf_addstr(reason, _("gitdir file points to non-existent location")); return 1; } else { return 0; @@ -147,7 +147,8 @@ static void prune_worktrees(void) if (!prune_worktree(d->d_name, &reason)) continue; if (show_only || verbose) - printf("%s\n", reason.buf); + printf_ln(_("Removing %s/%s: %s"), + "worktrees", d->d_name, reason.buf); if (show_only) continue; delete_git_dir(d->d_name); From 1b14d40b385580a366c92f8c2a9373e6fd7cb26f Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Wed, 10 Jun 2020 02:30:44 -0400 Subject: [PATCH 2/7] worktree: give "should be pruned?" function more meaningful name Readers of the name prune_worktree() are likely to expect the function to actually prune a worktree, however, it only answers the question "should this worktree be pruned?". Give it a name more reflective of its true purpose to avoid such confusion. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- builtin/worktree.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/worktree.c b/builtin/worktree.c index 27325f42c5..0402f244ea 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -67,7 +67,7 @@ static void delete_worktrees_dir_if_empty(void) rmdir(git_path("worktrees")); /* ignore failed removal */ } -static int prune_worktree(const char *id, struct strbuf *reason) +static int should_prune_worktree(const char *id, struct strbuf *reason) { struct stat st; char *path; @@ -144,7 +144,7 @@ static void prune_worktrees(void) if (is_dot_or_dotdot(d->d_name)) continue; strbuf_reset(&reason); - if (!prune_worktree(d->d_name, &reason)) + if (!should_prune_worktree(d->d_name, &reason)) continue; if (show_only || verbose) printf_ln(_("Removing %s/%s: %s"), From dd9609a12e83969be6536853b2846866dafdfc98 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Wed, 10 Jun 2020 02:30:45 -0400 Subject: [PATCH 3/7] worktree: make high-level pruning re-usable The low-level logic for removing a worktree is well encapsulated in delete_git_dir(). However, high-level details related to pruning a worktree -- such as dealing with verbosity and dry-run mode -- are not encapsulated. Factor out this high-level logic into its own function so it can be re-used as new worktree corruption detectors are added. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- builtin/worktree.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/builtin/worktree.c b/builtin/worktree.c index 0402f244ea..9284e490b9 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -133,6 +133,14 @@ static int should_prune_worktree(const char *id, struct strbuf *reason) return 0; } +static void prune_worktree(const char *id, const char *reason) +{ + if (show_only || verbose) + printf_ln(_("Removing %s/%s: %s"), "worktrees", id, reason); + if (!show_only) + delete_git_dir(id); +} + static void prune_worktrees(void) { struct strbuf reason = STRBUF_INIT; @@ -146,12 +154,7 @@ static void prune_worktrees(void) strbuf_reset(&reason); if (!should_prune_worktree(d->d_name, &reason)) continue; - if (show_only || verbose) - printf_ln(_("Removing %s/%s: %s"), - "worktrees", d->d_name, reason.buf); - if (show_only) - continue; - delete_git_dir(d->d_name); + prune_worktree(d->d_name, reason.buf); } closedir(dir); if (!show_only) From 4a3ce479ce7c8f268f42b725e471e35967da9b4f Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Wed, 10 Jun 2020 02:30:46 -0400 Subject: [PATCH 4/7] worktree: prune duplicate entries referencing same worktree path A fundamental restriction of linked working trees is that there must only ever be a single worktree associated with a particular path, thus "git worktree add" explicitly disallows creation of a new worktree at the same location as an existing registered worktree. Nevertheless, users can still "shoot themselves in the foot" by mucking with administrative files in .git/worktree//. Worse, "git worktree move" is careless[1] and allows a worktree to be moved atop a registered but missing worktree (which can happen, for instance, if the worktree is on removable media). For instance: $ git clone foo.git $ cd foo $ git worktree add ../bar $ git worktree add ../baz $ rm -rf ../bar $ git worktree move ../baz ../bar $ git worktree list .../foo beefd00f [master] .../bar beefd00f [bar] .../bar beefd00f [baz] Help users recover from this form of corruption by teaching "git worktree prune" to detect when multiple worktrees are associated with the same path. [1]: A subsequent commit will fix "git worktree move" validation to be more strict. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- builtin/worktree.c | 49 ++++++++++++++++++++++++++++++++++----- t/t2401-worktree-prune.sh | 12 ++++++++++ 2 files changed, 55 insertions(+), 6 deletions(-) diff --git a/builtin/worktree.c b/builtin/worktree.c index 9284e490b9..02a5844705 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -67,7 +67,12 @@ static void delete_worktrees_dir_if_empty(void) rmdir(git_path("worktrees")); /* ignore failed removal */ } -static int should_prune_worktree(const char *id, struct strbuf *reason) +/* + * Return true if worktree entry should be pruned, along with the reason for + * pruning. Otherwise, return false and the worktree's path, or NULL if it + * cannot be determined. Caller is responsible for freeing returned path. + */ +static int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath) { struct stat st; char *path; @@ -75,6 +80,7 @@ static int should_prune_worktree(const char *id, struct strbuf *reason) size_t len; ssize_t read_result; + *wtpath = NULL; if (!is_directory(git_path("worktrees/%s", id))) { strbuf_addstr(reason, _("not a valid directory")); return 1; @@ -120,16 +126,17 @@ static int should_prune_worktree(const char *id, struct strbuf *reason) } path[len] = '\0'; if (!file_exists(path)) { - free(path); if (stat(git_path("worktrees/%s/index", id), &st) || st.st_mtime <= expire) { strbuf_addstr(reason, _("gitdir file points to non-existent location")); + free(path); return 1; } else { + *wtpath = path; return 0; } } - free(path); + *wtpath = path; return 0; } @@ -141,22 +148,52 @@ static void prune_worktree(const char *id, const char *reason) delete_git_dir(id); } +static int prune_cmp(const void *a, const void *b) +{ + const struct string_list_item *x = a; + const struct string_list_item *y = b; + int c; + + if ((c = fspathcmp(x->string, y->string))) + return c; + /* paths same; sort by .git/worktrees/ */ + return strcmp(x->util, y->util); +} + +static void prune_dups(struct string_list *l) +{ + int i; + + QSORT(l->items, l->nr, prune_cmp); + for (i = 1; i < l->nr; i++) { + if (!fspathcmp(l->items[i].string, l->items[i - 1].string)) + prune_worktree(l->items[i].util, "duplicate entry"); + } +} + static void prune_worktrees(void) { struct strbuf reason = STRBUF_INIT; + struct string_list kept = STRING_LIST_INIT_NODUP; DIR *dir = opendir(git_path("worktrees")); struct dirent *d; if (!dir) return; while ((d = readdir(dir)) != NULL) { + char *path; if (is_dot_or_dotdot(d->d_name)) continue; strbuf_reset(&reason); - if (!should_prune_worktree(d->d_name, &reason)) - continue; - prune_worktree(d->d_name, reason.buf); + if (should_prune_worktree(d->d_name, &reason, &path)) + prune_worktree(d->d_name, reason.buf); + else if (path) + string_list_append(&kept, path)->util = xstrdup(d->d_name); } closedir(dir); + + prune_dups(&kept); + string_list_clear(&kept, 1); + if (!show_only) delete_worktrees_dir_if_empty(); strbuf_release(&reason); diff --git a/t/t2401-worktree-prune.sh b/t/t2401-worktree-prune.sh index b7d6d5d45a..fd3916fee0 100755 --- a/t/t2401-worktree-prune.sh +++ b/t/t2401-worktree-prune.sh @@ -92,4 +92,16 @@ test_expect_success 'not prune proper checkouts' ' test -d .git/worktrees/nop ' +test_expect_success 'prune duplicate (linked/linked)' ' + test_when_finished rm -fr .git/worktrees w1 w2 && + git worktree add --detach w1 && + git worktree add --detach w2 && + sed "s/w2/w1/" .git/worktrees/w2/gitdir >.git/worktrees/w2/gitdir.new && + mv .git/worktrees/w2/gitdir.new .git/worktrees/w2/gitdir && + git worktree prune --verbose >actual && + test_i18ngrep "duplicate entry" actual && + test -d .git/worktrees/w1 && + ! test -d .git/worktrees/w2 +' + test_done From 916133ef8edcda54b0bff03a949c0858cd8d3280 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Wed, 10 Jun 2020 02:30:47 -0400 Subject: [PATCH 5/7] worktree: prune linked worktree referencing main worktree path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "git worktree prune" detects when multiple entries are associated with the same path and prunes the duplicates, however, it does not detect when a linked worktree points at the path of the main worktree. Although "git worktree add" disallows creating a new worktree with the same path as the main worktree, such a case can arise outside the control of Git even without the user mucking with .git/worktree// administrative files. For instance: $ git clone foo.git $ git -C foo worktree add ../bar $ rm -rf bar $ mv foo bar $ git -C bar worktree list .../bar deadfeeb [master] .../bar deadfeeb [bar] Help the user recover from such corruption by extending "git worktree prune" to also detect when a linked worktree is associated with the path of the main worktree. Reported-by: Jonathan Müller Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- builtin/worktree.c | 15 +++++++++++++++ t/t2401-worktree-prune.sh | 12 ++++++++++++ 2 files changed, 27 insertions(+) diff --git a/builtin/worktree.c b/builtin/worktree.c index 02a5844705..9fe9e442c7 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -156,6 +156,16 @@ static int prune_cmp(const void *a, const void *b) if ((c = fspathcmp(x->string, y->string))) return c; + /* + * paths same; prune_dupes() removes all but the first worktree entry + * having the same path, so sort main worktree ('util' is NULL) above + * linked worktrees ('util' not NULL) since main worktree can't be + * removed + */ + if (!x->util) + return -1; + if (!y->util) + return 1; /* paths same; sort by .git/worktrees/ */ return strcmp(x->util, y->util); } @@ -174,6 +184,7 @@ static void prune_dups(struct string_list *l) static void prune_worktrees(void) { struct strbuf reason = STRBUF_INIT; + struct strbuf main_path = STRBUF_INIT; struct string_list kept = STRING_LIST_INIT_NODUP; DIR *dir = opendir(git_path("worktrees")); struct dirent *d; @@ -191,6 +202,10 @@ static void prune_worktrees(void) } closedir(dir); + strbuf_add_absolute_path(&main_path, get_git_common_dir()); + /* massage main worktree absolute path to match 'gitdir' content */ + strbuf_strip_suffix(&main_path, "/."); + string_list_append(&kept, strbuf_detach(&main_path, NULL)); prune_dups(&kept); string_list_clear(&kept, 1); diff --git a/t/t2401-worktree-prune.sh b/t/t2401-worktree-prune.sh index fd3916fee0..a6ce7f590b 100755 --- a/t/t2401-worktree-prune.sh +++ b/t/t2401-worktree-prune.sh @@ -104,4 +104,16 @@ test_expect_success 'prune duplicate (linked/linked)' ' ! test -d .git/worktrees/w2 ' +test_expect_success 'prune duplicate (main/linked)' ' + test_when_finished rm -fr repo wt && + test_create_repo repo && + test_commit -C repo x && + git -C repo worktree add --detach ../wt && + rm -fr wt && + mv repo wt && + git -C wt worktree prune --verbose >actual && + test_i18ngrep "duplicate entry" actual && + ! test -d .git/worktrees/wt +' + test_done From d179af679bd5f70506732e1c9242d367efc7ea42 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Wed, 10 Jun 2020 02:30:48 -0400 Subject: [PATCH 6/7] worktree: generalize candidate worktree path validation "git worktree add" checks that the specified path is a valid location for a new worktree by ensuring that the path does not already exist and is not already registered to another worktree (a path can be registered but missing, for instance, if it resides on removable media). Since "git worktree add" is not the only command which should perform such validation ("git worktree move" ought to also), generalize the the validation function for use by other callers, as well. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- builtin/worktree.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/builtin/worktree.c b/builtin/worktree.c index 9fe9e442c7..89a61891ab 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -280,34 +280,33 @@ static const char *worktree_basename(const char *path, int *olen) return name; } -static void validate_worktree_add(const char *path, const struct add_opts *opts) +/* check that path is viable location for worktree */ +static void check_candidate_path(const char *path, + int force, + struct worktree **worktrees, + const char *cmd) { - struct worktree **worktrees; struct worktree *wt; int locked; if (file_exists(path) && !is_empty_dir(path)) die(_("'%s' already exists"), path); - worktrees = get_worktrees(0); wt = find_worktree_by_path(worktrees, path); if (!wt) - goto done; + return; locked = !!worktree_lock_reason(wt); - if ((!locked && opts->force) || (locked && opts->force > 1)) { + if ((!locked && force) || (locked && force > 1)) { if (delete_git_dir(wt->id)) - die(_("unable to re-add worktree '%s'"), path); - goto done; + die(_("unusable worktree destination '%s'"), path); + return; } if (locked) - die(_("'%s' is a missing but locked worktree;\nuse 'add -f -f' to override, or 'unlock' and 'prune' or 'remove' to clear"), path); + die(_("'%s' is a missing but locked worktree;\nuse '%s -f -f' to override, or 'unlock' and 'prune' or 'remove' to clear"), cmd, path); else - die(_("'%s' is a missing but already registered worktree;\nuse 'add -f' to override, or 'prune' or 'remove' to clear"), path); - -done: - free_worktrees(worktrees); + die(_("'%s' is a missing but already registered worktree;\nuse '%s -f' to override, or 'prune' or 'remove' to clear"), cmd, path); } static int add_worktree(const char *path, const char *refname, @@ -324,8 +323,12 @@ static int add_worktree(const char *path, const char *refname, struct commit *commit = NULL; int is_branch = 0; struct strbuf sb_name = STRBUF_INIT; + struct worktree **worktrees; - validate_worktree_add(path, opts); + worktrees = get_worktrees(0); + check_candidate_path(path, opts->force, worktrees, "add"); + free_worktrees(worktrees); + worktrees = NULL; /* is 'refname' a branch or commit? */ if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) && From 810382ed378731314c03627a8a2d8962b814ad17 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Wed, 10 Jun 2020 02:30:49 -0400 Subject: [PATCH 7/7] worktree: make "move" refuse to move atop missing registered worktree "git worktree add" takes special care to avoid creating a new worktree at a location already registered to an existing worktree even if that worktree is missing (which can happen, for instance, if the worktree resides on removable media). "git worktree move", however, is not so careful when validating the destination location and will happily move the source worktree atop the location of a missing worktree. This leads to the anomalous situation of multiple worktrees being associated with the same path, which is expressly forbidden by design. For example: $ git clone foo.git $ cd foo $ git worktree add ../bar $ git worktree add ../baz $ rm -rf ../bar $ git worktree move ../baz ../bar $ git worktree list .../foo beefd00f [master] .../bar beefd00f [bar] .../bar beefd00f [baz] $ git worktree remove ../bar fatal: validation failed, cannot remove working tree: '.../bar' does not point back to '.git/worktrees/bar' Fix this shortcoming by enhancing "git worktree move" to perform the same additional validation of the destination directory as done by "git worktree add". While at it, add a test to verify that "git worktree move" won't move a worktree atop an existing (non-worktree) path -- a restriction which has always been in place but was never tested. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- Documentation/git-worktree.txt | 4 +++- builtin/worktree.c | 3 +-- t/t2403-worktree-move.sh | 21 +++++++++++++++++++++ 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt index 85d92c9761..4796c3c05e 100644 --- a/Documentation/git-worktree.txt +++ b/Documentation/git-worktree.txt @@ -126,7 +126,9 @@ OPTIONS locked working tree path, specify `--force` twice. + `move` refuses to move a locked working tree unless `--force` is specified -twice. +twice. If the destination is already assigned to some other working tree but is +missing (for instance, if `` was deleted manually), then `--force` +allows the move to proceed; use --force twice if the destination is locked. + `remove` refuses to remove an unclean working tree unless `--force` is used. To remove a locked working tree, specify `--force` twice. diff --git a/builtin/worktree.c b/builtin/worktree.c index 89a61891ab..4dec3951dc 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -860,8 +860,7 @@ static int move_worktree(int ac, const char **av, const char *prefix) strbuf_trim_trailing_dir_sep(&dst); strbuf_addstr(&dst, sep); } - if (file_exists(dst.buf)) - die(_("target '%s' already exists"), dst.buf); + check_candidate_path(dst.buf, force, worktrees, "move"); validate_no_submodules(wt); diff --git a/t/t2403-worktree-move.sh b/t/t2403-worktree-move.sh index 939d18d728..a4e1a178e0 100755 --- a/t/t2403-worktree-move.sh +++ b/t/t2403-worktree-move.sh @@ -112,6 +112,27 @@ test_expect_success 'move locked worktree (force)' ' git worktree move --force --force flump ploof ' +test_expect_success 'refuse to move worktree atop existing path' ' + >bobble && + git worktree add --detach beeble && + test_must_fail git worktree move beeble bobble +' + +test_expect_success 'move atop existing but missing worktree' ' + git worktree add --detach gnoo && + git worktree add --detach pneu && + rm -fr pneu && + test_must_fail git worktree move gnoo pneu && + git worktree move --force gnoo pneu && + + git worktree add --detach nu && + git worktree lock nu && + rm -fr nu && + test_must_fail git worktree move pneu nu && + test_must_fail git worktree --force move pneu nu && + git worktree move --force --force pneu nu +' + test_expect_success 'move a repo with uninitialized submodule' ' git init withsub && (