Re: [PATCH 2/8] worktree: prune corrupted worktree even if locked
Eric Sunshine <sunshine@sunshineco.com> writes:
Show 5 quoted lines
> The .git/worktrees/<id>/locked file created by "git worktree lock" is
> intended to prevent a missing worktree -- which might reside on a
> removable device or network share -- from being pruned. It is not meant
> to prevent a corrupt worktree from being pruned, yet it short-circuits
> almost all "git worktree prune" corruption checks.
The '.git/worktrees/<id>/locked' file is what 'It' in "It is not meant to" refers to, but the 'it' in "yet it short-circuits" cannot refer to the same thing---my reading hiccuped there.
"Its presence causes most of the corruption checks skipped by 'git worktree prune'", perhaps.
Show 5 quoted lines
> This can make it
> impossible[1] to prune a worktree which becomes corrupt after the lock
> is placed since "git worktree prune" won't prune it, and it may not even
> be possible to unlock it with "git worktree unlock", depending upon the
> nature of the corruption.
The latter is because... "worktree unlock" does not skip corruption check and refuses to unlock a corrupted worktree, or something?
> Therefore, delay the check for .git/worktrees/<id>/locked until after
> all forms of corruption have been checked so that it behaves as
> originally intended (to wit: preventing pruning of a missing worktree
> only).
An alternative could be to allow unlocking a worktree even if it is corrupt, and with that, such an unprunable corrupt worktree can first be unlocked and then pruned? A naive first thought is that might make it slightly safer, but the reason why this approach was taken is because the end user already said 'prune' so that should trump whatever ".git/worktrees/<id>/" has?
But the intent of locking a worktree is "make sure that the end user is aware of the fact that it is locked before allowing the worktree to be pruned", isn't it? Unless there is a way for a corruption to add the "locked" file the end-user did not intend to have, if we sense the "locked" file given to a worktree, shouldn't we honor that existing "locked" file's intent?
I am growing skeptical about the approach taken by this step. There must be something missing that I may become aware of after reading the remainder of the series.
... goes back and digs a bit ...
This came from 23af91d1 (prune: strategies for linked checkouts, 2014-11-30) which explains:
To prevent `git prune --worktrees` from deleting a
$GIT_DIR/worktrees entry (which can be useful in some
situations, such as when the entry's working tree is stored on a
portable device), add a file named 'locked' to the entry's
directory.Notice that "in some situations, such as" gives just one example, and it is clear that it is the only envisioned use case.
It therefore feels more sensible to honor the "locked" file whether the actual worktree (or just a part of it) is accessible or not when "prune" gets exercised. After all, if some parts of the actual worktree gets moved to removal media when not in use, such a partial removal may make the worktree appear as if it is "corrupt". We do not want to declare that it is corrupt and we ignore the locked state, or do we?
Thanks.
Show 61 quoted lines
> [1]: Impossible, that is, without manually mucking around with
> .git/worktrees/<id>/ administrative files.
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
> builtin/worktree.c | 4 ++--
> t/t2401-worktree-prune.sh | 14 ++++++++++++--
> 2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 9b15f19fc5..f7351413af 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -79,8 +79,6 @@ static int prune_worktree(const char *id, struct strbuf *reason)
> 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_addstr(reason, _("gitdir file does not exist"));
> return 1;
> @@ -121,6 +119,8 @@ static int prune_worktree(const char *id, struct strbuf *reason)
> path[len] = '\0';
> if (!file_exists(path)) {
> free(path);
> + if (file_exists(git_path("worktrees/%s/locked", id)))
> + return 0;
> if (stat(git_path("worktrees/%s/index", id), &st) ||
> st.st_mtime <= expire) {
> strbuf_addstr(reason, _("gitdir file points to non-existent location"));
> diff --git a/t/t2401-worktree-prune.sh b/t/t2401-worktree-prune.sh
> index b7d6d5d45a..9be8e97d66 100755
> --- a/t/t2401-worktree-prune.sh
> +++ b/t/t2401-worktree-prune.sh
> @@ -69,13 +69,23 @@ test_expect_success 'prune directories with gitdir pointing to nowhere' '
> '
>
> test_expect_success 'not prune locked checkout' '
> - test_when_finished rm -r .git/worktrees &&
> - mkdir -p .git/worktrees/ghi &&
> + test_when_finished rm -fr .git/worktrees ghi &&
> + git worktree add ghi &&
> : >.git/worktrees/ghi/locked &&
> + rm -r ghi &&
> git worktree prune &&
> test -d .git/worktrees/ghi
> '
>
> +test_expect_success 'prune corrupt despite lock' '
> + test_when_finished rm -fr .git/worktrees ghi &&
> + mkdir -p .git/worktrees/ghi &&
> + : >.git/worktrees/ghi/gitdir &&
> + : >.git/worktrees/ghi/locked &&
> + git worktree prune &&
> + ! test -d .git/worktrees/ghi
> +'
> +
> test_expect_success 'not prune recent checkouts' '
> test_when_finished rm -r .git/worktrees &&
> git worktree add jlm HEAD &&
Eric Sunshine
