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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 28, 2017, 03:48 UTC
Message-ID: <CAPig+cT3jbZ=VRWpggw_tvegAGnjGQ0Mxp8m2DdKKPhfKTVEWA@mail.gmail.com>
In-Reply-To: <xmqq7eub85rz.fsf@gitster.mtv.corp.google.com>

On Mon, Nov 27, 2017 at 10:02 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 12 quoted lines

> 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.