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

[Junio C Hamano <gitster@pobox.com>]

Eric Sunshine <sunshine@sunshineco.com> writes:

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