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

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Jun 8, 2020 at 5:24 PM Junio C Hamano <gitster@pobox.com> wrote:
> 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.

I can adopt that wording, but see below...

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

That bit of the commit message was a late addition and somewhat
intentionally hand-wavy. I don't think "git worktree unlock" will
currently die or misbehave due to corruption, but was thinking that it
someday might if additional checks are ever added. But, it's not
worth pursuing since...

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

You're not the only person skeptical about this patch. I flip-flopped
on it multiple times, first convincing myself it was the right thing
to do, then convincing myself that the original code was correct, and
so forth. That's a good indication that such a change overall is
questionable.

Aside from that, this patch is unrelated to the intent of this series.
So, I'll drop it when I re-roll.