On Tue, Nov 21, 2017 at 10:55 PM, Junio C Hamano <[email protected]> wrote:
> 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.
> "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.
> 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.
