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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 28, 2017, 04:04 UTC
Message-ID: <xmqqy3mr6ocp.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAPig+cT3jbZ=VRWpggw_tvegAGnjGQ0Mxp8m2DdKKPhfKTVEWA@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 9 quoted lines

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

I do not think the user minds to see "there is no such directory there"; actually that would be beneficial, even.

But you are right; validate_worktree() would need to become aware of the possibility that it can be passed such a "corrupt" worktree to handle if that approach is followed.

The case we are discussing, i.e. the user removed the directory without telling Git to give it a chance to remove control information, may be common enough that it becomes a worthwhile addition to make the "quiet" boolean that occupies a whole int to an unsigned that is a collection of bits, and pass "missing_ok" bit in that flag word to the validate_worktree() to tell that the caller can tolerate the "user removed it while we were looking the other way" case and can handle it gracefully. But that would be a lot larger change, and "the caller checks before calling validate" as done with this [RFC v2] may be a good way to add the feature with the least impact to the codebase.

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

Yes, use of goto is also a good way to avoid having to worry about the future evolution of the codeflow in this function.

So perhaps

	if (the directory is no longer there)
		goto cleanup;
	if (the worktree does not validate)
		return -1;
	... the original code to (carefully) remove the 
	... worktree itself

	cleanup:

	... remove control info
	... free resources held in variables
	... return from the function

is what we want?

In any case, I think this needs thawing nd/worktree-move topic before it can move forward, as all the functions we discussed in this thread are the ones that were introduced in that topic and do not exist in the mainline.