Re: [RFC PATCH] builtin/worktree: enhance worktree removal

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 22, 2017, 04:37 UTC
Message-ID: <CAPig+cQ1D+MFZkEX5yQ-DXu_CPf4oa507ChNjYqA=vG1CdZ4SQ@mail.gmail.com>
In-Reply-To: <xmqqvai3ufu9.fsf@gitster.mtv.corp.google.com>

On Tue, Nov 21, 2017 at 10:55 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

> OK, so the proposed log message was a bit confusing for those who
> are *not* the person who wrote it (who knew why existing behaviour
> was inadequate and did not describe how "worktree remove" would fail
> under such a scenario to illustrate it, incorrectly assuming that
> everybody who reads the proposed log message already *knows* how it
> would fail).

Correct. The log message is confusing, enough so that my knee-jerk reaction was that the patch was trying to re-invent 'git worktree prune'. That seemed odd, so I spent extra time trying to figure out what it really meant, which led to my rather lengthy response asking if I was understanding the situation correctly.

Show 5 quoted lines

>         "git worktree remove" removes both the named worktree
>         directory and the administrative information for it after
>         checking that there is no local modifications that would be
>         lost (which is a handy safety measure).  It however refuses
>         to work if the worktree directory is _already_ removed.

The "refusal" is by accident, so perhaps phrase it like this:

    However, due to an oversight, it aborts with an error if the
    worktree directory is _already_ removed.

or something.

Show 7 quoted lines

>         The user could use "git worktree prune" after seeing the
>         error and realizing the situation, but at that point, there
>         is nothing gained by leaving only the administrative data
>         behind.  Teach "git worktree remove" to go ahead and remove
>         the trace of the worktree in such a case.
>
> or soemthing like that?

Yes, I quite like it; it conveys the information necessary to understand the issue.

Kaartic: Regarding the actual patch, rather than silencing
validate_worktree() (which seems an unfortunate thing to do), isn't it
possible simply to do a quick test to see if the worktree directory
exists before calling validate_worktree()? If it doesn't exist, then
just skip down to the part of the code which does the 'prune'
operation.