Bug: git worktree remove and overwritten directory

Bug: git worktree remove and overwritten directory

[Jonathan Müller]

Apologies, if this has already been reported.

It seems to be impossible to remove a git worktree whose location has 
been replaced by the main working tree (don't ask how I found out). 
Steps to reproduce:

```
git init test
cd test
git commit --allow-empty -m"Initial commit"
git branch some-branch
git worktree add ../test2 some-branch
cd ../
rm -rf test2  # Remove the worktree folder
mv test test2 # Main git repository now located where the worktree used 
to be
```

Afterwards, `git worktree list` reports:

/home/foonathan/test2  e7bb748 [master]
/home/foonathan/test2  e7bb748 [some-branch]

And both `git worktree remove .` and `git worktree remove ../test2` 
report an error `fatal: '../test2' is a main working tree`. I had to 
manually remove the corresponding folder from `.git/worktree` to get rid 
of it. The issue is especially annoying, because you can't check out 
`some-branch` anymore (as it's already checked out in the worktree).

I've tested it with git version 2.26.2 and also the version currently on 
the next branch.

Thank you,
Jonathan

Re: Bug: git worktree remove and overwritten directory

[Shourya Shukla]

Hello Jonathan!

> And both `git worktree remove .` and `git worktree remove ../test2`
> report an error `fatal: '../test2' is a main working tree`. I had to
> manually remove the corresponding folder from `.git/worktree` to get rid
> of it. The issue is especially annoying, because you can't check out
> `some-branch` anymore (as it's already checked out in the worktree).

If you refer to the documentation of `git worktree` you may find this:

    Multiple checkout in general is still experimental, and the support
    for submodules is incomplete. It is NOT recommended to make multiple
    checkouts of a superproject.

Alright, so if you were to do a checkout to `some-branch` even if you
never deleted anything, you won't be able to perform this action because
it's not possible, therefore *this* thing is not a bug. Why can't you
perform this you ask? Because this is the point of the whole command. If
you were allowed to checkout, then you would be able to have two
different copies of the same thing at one instant of time, which is
something we don't want.

Moving on, one may say, "I am technically in the same repo only, its
just that the folder names are changed, so I can technically do a
checkout now because my worktree is gone right?". No, we cannot,
because: technicalities. You see, that when you create a worktree,
you add an entry in `$GIT_DIR/worktrees/` that `test2` is a registered
worktree by you. Now if one does the mischief you did by renaming `test`
to `test2`, you confuse the system as to what exactly is going on. It
still thinks that we are on `test2`, but in fact we are on `test`.
Hence, the dual nature we have here. Please refer to this for more detail:
https://git-scm.com/docs/git-worktree#_details

Now, if you delete the `$GIT_DIR/worktrees/` folder (proceeding
further), you will be able to checkout to `some-branch` because the
confusion is gone now as Git is not aware of any worktrees you ever
created.

Therefore the final set of commands to overcome this will be:
    git init test
    cd test
    git commit --allow-empty -m"Initial commit"
    git branch some-branch
    git worktree add ../test2 some-branch
    cd ../
    rm -rf test2
    mv test test2
    rm -rf .git/worktrees
    git checkout some-branch
    ... we can checkout successfully now :)

Now, I can be wrong in some places in my explanation, but this is what
I could infer from the problem.

So, coming to the ultimate question:
    Is this a bug?
I don't really think so. Why you ask? Because this is the way things
should have functioned. It is more of a vulnerability.
You may want to refer to this:
https://stackoverflow.com/a/402944/10751129

But again, this doesn't mean that it can't be fixed.
Thanks to you I got introduced to a new git command (git worktree) :)

Regards,
Shourya Shukla

Re: Bug: git worktree remove and overwritten directory

[Eric Sunshine]

On Wed, May 20, 2020 at 6:36 AM Jonathan Müller
<jonathanmueller.dev@gmail.com> wrote:
> It seems to be impossible to remove a git worktree whose location has
> been replaced by the main working tree [...]
>
> cd test
> git worktree add ../test2 some-branch
> cd ../
> rm -rf test2  # Remove the worktree folder
> mv test test2 # Main git repository now located where the worktree used
> to be
>
> Afterwards, `git worktree list` reports:
>
> /home/foonathan/test2  e7bb748 [master]
> /home/foonathan/test2  e7bb748 [some-branch]
>
> And both `git worktree remove .` and `git worktree remove ../test2`
> report an error `fatal: '../test2' is a main working tree`. I had to
> manually remove the corresponding folder from `.git/worktree` to get rid
> of it. The issue is especially annoying, because you can't check out
> `some-branch` anymore (as it's already checked out in the worktree).

Git tries hard to prevent the same directory from being registered to
multiple worktrees, however, there is not much it can do to prevent a
person from shooting himself in the foot by making changes like this
outside of Git's control. Thus, this seems like a case of "if it
hurts, don't do it".

However, "git worktree" could possibly do a better job of helping you
recover from such a situation. In particular, I think it should be
reasonably easy to enhance "git worktree prune" to detect this
situation and automatically prune the non-main now-bogus worktree
entry.

It may be possible to special-case "git worktree remove" to detect
this situation and automatically prune the bogus entry too, but I'm
quite hesitant to suggest that sort of special case both because the
implementation would likely be ugly, and it could lead to a plethora
of additional ugly special-cases as people discover even more ways to
shoot themselves in the feet.

If you have ideas about how else the situation can be improved, please
feel free to share them, as such discussion may shed light on
possibilities which are not presently obvious.

Re: Bug: git worktree remove and overwritten directory

[Jonathan Müller]

On 20.05.20 16:22, Eric Sunshine wrote:
> Git tries hard to prevent the same directory from being registered to
> multiple worktrees, however, there is not much it can do to prevent a
> person from shooting himself in the foot by making changes like this
> outside of Git's control. Thus, this seems like a case of "if it
> hurts, don't do it".
> 

I agree and didn't expect git to "work".

> However, "git worktree" could possibly do a better job of helping you
> recover from such a situation. In particular, I think it should be
> reasonably easy to enhance "git worktree prune" to detect this
> situation and automatically prune the non-main now-bogus worktree
> entry.

At the very least, the somewhat confusing error message could be 
replaced by a "you messed up the worktrees, please delete the 
corresponding entry in .git/worktree" or something like that. But 
enhancing `git worktree prune` would be better. It was, in fact, the 
first command I ran to try and fix the problem.

> It may be possible to special-case "git worktree remove" to detect
> this situation and automatically prune the bogus entry too, but I'm
> quite hesitant to suggest that sort of special case both because the
> implementation would likely be ugly, and it could lead to a plethora
> of additional ugly special-cases as people discover even more ways to
> shoot themselves in the feet.

As said above, I think git worktree remove could issue a better error if 
it detects multiple worktrees with an identical path.

Re: Bug: git worktree remove and overwritten directory

[Eric Sunshine]

On Wed, May 20, 2020 at 10:28 AM Jonathan Müller
<jonathanmueller.dev@gmail.com> wrote:
> On 20.05.20 16:22, Eric Sunshine wrote:
> > Git tries hard to prevent the same directory from being registered to
> > multiple worktrees, however, there is not much it can do to prevent a
> > person from shooting himself in the foot by making changes like this
> > outside of Git's control. Thus, this seems like a case of "if it
> > hurts, don't do it".
>
> I agree and didn't expect git to "work".

I just posted a patch series[1] which enhances "git worktree prune" to
detect and deal with multiple worktree entries referencing the same
path.

> > However, "git worktree" could possibly do a better job of helping you
> > recover from such a situation. In particular, I think it should be
> > reasonably easy to enhance "git worktree prune" to detect this
> > situation and automatically prune the non-main now-bogus worktree
> > entry.
>
> At the very least, the somewhat confusing error message could be
> replaced by a "you messed up the worktrees, please delete the
> corresponding entry in .git/worktree" or something like that. But
> enhancing `git worktree prune` would be better. It was, in fact, the
> first command I ran to try and fix the problem.
>
> As said above, I think git worktree remove could issue a better error if
> it detects multiple worktrees with an identical path.

Hmm, the message it printed complaining that you tried removing the
main worktree seems pretty accurate since that extra worktree entry
was indeed pointing at the main worktree. I suppose it would be
possible to have "git worktree remove" also perform "corruption
detection" so as to present additional information which might clarify
the complaint. Of course, if that is done, then it would make sense to
make all "git worktree" commands likewise report corruption. However,
I haven't convinced myself that we need to go that far. Anyhow, the
posted patch series[1] addresses the immediate problem.

[1]: https://lore.kernel.org/git/20200608062356.40264-1-sunshine@sunshineco.com/T/

Re: Bug: git worktree remove and overwritten directory

[Jonathan Müller]

On 08.06.20 08:42, Eric Sunshine wrote:
> On Wed, May 20, 2020 at 10:28 AM Jonathan Müller
> <jonathanmueller.dev@gmail.com> wrote:
>> On 20.05.20 16:22, Eric Sunshine wrote:
>>> Git tries hard to prevent the same directory from being registered to
>>> multiple worktrees, however, there is not much it can do to prevent a
>>> person from shooting himself in the foot by making changes like this
>>> outside of Git's control. Thus, this seems like a case of "if it
>>> hurts, don't do it".
>>
>> I agree and didn't expect git to "work".
> 
> I just posted a patch series[1] which enhances "git worktree prune" to
> detect and deal with multiple worktree entries referencing the same
> path.

Excellent, thank you!

> 
>>> However, "git worktree" could possibly do a better job of helping you
>>> recover from such a situation. In particular, I think it should be
>>> reasonably easy to enhance "git worktree prune" to detect this
>>> situation and automatically prune the non-main now-bogus worktree
>>> entry.
>>
>> At the very least, the somewhat confusing error message could be
>> replaced by a "you messed up the worktrees, please delete the
>> corresponding entry in .git/worktree" or something like that. But
>> enhancing `git worktree prune` would be better. It was, in fact, the
>> first command I ran to try and fix the problem.
>>
>> As said above, I think git worktree remove could issue a better error if
>> it detects multiple worktrees with an identical path.
> 
> Hmm, the message it printed complaining that you tried removing the
> main worktree seems pretty accurate since that extra worktree entry
> was indeed pointing at the main worktree. I suppose it would be
> possible to have "git worktree remove" also perform "corruption
> detection" so as to present additional information which might clarify
> the complaint. Of course, if that is done, then it would make sense to
> make all "git worktree" commands likewise report corruption. However,
> I haven't convinced myself that we need to go that far. Anyhow, the
> posted patch series[1] addresses the immediate problem.
> 
> [1]: https://lore.kernel.org/git/20200608062356.40264-1-sunshine@sunshineco.com/T/
> 

You're probably right. Fixing git worktree prune to handle the situation 
is sufficient for me, as that was what I tried.

Thanks a lot,
Jonathan