| Message ID | 20200608062356.40264-3-sunshine@sunshineco.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | worktree: tighten duplicate path detection | expand |
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 &&
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.
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 &&
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. 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. 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). [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(-)