Re: [PATCH 2/8] worktree: prune corrupted worktree even if locked

[Junio C Hamano <gitster@pobox.com>]

Eric Sunshine <sunshine@sunshineco.com> writes:

> 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.

> 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.

> [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 &&