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

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Nov 27, 2017 at 10:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I actually wonder if this "early check and return" is making the
> code unmaintainable.  What if we instead did it with just the
> codeflow restructuring, perhaps like so?
>
>         if (!validate_worktree(wt, 0)) {
>                 /* OK, work tree is sound */
>                 if (!force) {
>                         /* protect from information lossage */
>                 }
>                 /* do the actual worktree removal */
>         }
>         /* remove the control info */

With this approach, validate_worktree() will print an error message
saying that the worktree directory is missing before the control info
is actually removed. Kaartic's original patch silenced the message
(and _all_ error messages from validate_worktree()) by passing 1 as
the second argument. That seemed heavy-handed, so I suggested keeping
the validate_worktree() call as-is but doing the directory-missing
check first as a special case.

But perhaps that special case isn't necessary. Perhaps I was
overcautious about having those error messages silenced. After all,
the user has explicitly asked to delete the worktree, so (presumably)
nothing is lost by honoring that request, even if there are validation
errors. (We could even add a --verbose option to 'remove' to re-enable
those messages, but that's probably overkill.)

> There is no need for a new helper function when done that way, which
> allows us not to worry about two clean-up places drifting apart over
> time.

I had envisioned a simple 'goto remove_control_info' rather than extra
nesting or refactoring, but that's a minor point.