Re: [PATCH 2/2] Make "git branch -d" prune missing worktrees automatically.

> [cc:+duy]
> 
> On Fri, Oct 18, 2019 at 3:43 PM Peter Jones <pjones@redhat.com> wrote:
>> On Thu, Oct 17, 2019 at 01:28:09PM -0400, Eric Sunshine wrote:
>>> Echoing SEZDER's comment on patch 1/2, this behavior is an intentional
>>> design choice and safety feature of the worktree implementation since
>>> worktrees may exist on removable media or remote filesystems which
>>> might not always be mounted; hence, the presence of commands "git
>>> worktree prune" and "git worktree remove".
>>
>> Okay, I see that use case now - I hadn't realized there was an
>> intentional design decision here, and honestly that's anything but clear
>> from the *code*.
> 
> It can indeed sometimes be difficult to get a high-level functional
> overview by examining code in isolation. In this case, at least,
> git-worktree documentation tries to be clear about the "why" and "how"
> of the pruning behavior (which is not to say that the documentation --
> or the code -- can't be improved to communicate this better).
> 
>> It's surprising, for example, that my patches didn't break a single
>> test case.
> 
> Tests suites are never perfect, and an attempt to prune a dangling
> worktree by deleting a branch likely never occurred to the
> git-worktree implementer(s).
> 
>>> These minor implementation comments aside, before considering this
>>> patch series, it would be nice to see a compelling argument as to why
>>> this change of behavior, which undercuts a deliberate design decision,
>>> is really desirable.
>>
>> Okay, so just for clarity, when you say there's a deliberate design
>> decision, which behavior here are you talking about? If you mean making
>> "lock" work, I don't have any issue with that. If you mean not cleaning
>> up when we do other commands, then I don't see why that's a concern -
>> after all, that's exactly what "lock" is for.
> 
> To clarify, I'm talking about Duy's deliberate design decision to
> model git-worktree auto-pruning after Git's own garbage-collection
> behavior. That model includes, not only explicit locking, but a grace
> period before dangling worktree administrative files can be pruned
> automatically (see the gc.worktreePruneExpire configuration).
> 
> The point of git-worktree's grace period (just like git-gc's grace
> period) is to avoid deleting potentially precious information
> permanently. For instance, the worktree-local "index" file might have
> some changes staged but not yet committed. Under the existing model,
> those staged changes are immune from being accidentally deleted
> permanently until after the grace period expires or until they are
> thrown away deliberately (say, via "git worktree prune --expire=now").
> 
>> Assuming it is the "lock" behavior we're talking about, I don't think I
>> actually have any intention of breaking this design decision, just
>> making my workflow (without "lock") nag at me less for what seem like
>> pretty trivial issues.
> 
> The ability to lock a worktree is an extra safety measure built atop
> the grace period mechanism to provide a way to completely override
> auto-pruning; it is not meant as an alternate or replacement safety
> mechanism to the grace period, but instead augments it. So, a behavior
> change which respects only one of those safety mechanisms but not the
> other is likely flawed.
> 
> And, importantly, people may already be relying upon this behavior of
> having an automatic grace period -- without having to place a worktree
> lock manually -- so changing behavior arbitrarily could break existing
> workflows and result in data loss.
> 
>> I can easily accommodate "git worktree lock". What bugs me though, is
>> that using worktrees basically means I have to replace fairly regular
>> filesystem activities with worktree commands, and it doesn't seem to be
>> *necessary* in any way. And I'm going to forget. A lot.
>>
>> To me, there doesn't seem to be any reason these need to behave any different:
>>
>> $ git worktree add foo foo
>> $ rm -rf foo
>> vs
>> $ git worktree add foo foo
>> $ git worktree remove foo
>>
>> And in fact the only difference right now, aside from some very
>> minuscule storage requirements that haven't gotten cleaned up, is the
>> first one leaves an artifact that tells it to give me errors later until
>> I run "git worktree prune" myself.
> 
> I understand the pain point, but I also understand Duy's motivation
> for being very careful about pruning worktree administrative files
> automatically (so as to avoid data loss, such as changes already
> staged to a worktree-local "index" file). While the proposed change
> may address the pain point, it nevertheless creates the possibility of
> accidental loss which Duy was careful to avoid when designing worktree
> mechanics. Although annoying, the current behavior gives you the
> opportunity to avoid that accidental loss by forcing you to take
> deliberate action to remove the worktree administrative files.
> 
> Perhaps there is some way to address the pain point without breaking
> the fundamental promise made by git-worktree about being careful with
> worktree metadata[*], but the changes proposed by this patch series
> seem insufficient (even if the patch is reworked to respect worktree
> locking). I've cc:'d Duy in case he wants to chime in.

> [*] For instance, perhaps before auto-pruning, it could check whether
> the index is recording staged changes or conflict information, and
> only allow auto-pruning if the index is clean. *But* there may be
> other ways for information to be lost permanently (beyond a dirty
> "index") which don't occur to me at present, so this has to be
> considered carefully.
>