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

[Kaartic Sivaraam <kaartic.sivaraam@gmail.com>]

On Tuesday 28 November 2017 09:34 AM, Junio C Hamano wrote:
> 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?
>

Probably but I'm not interested in going for a v3 that does that as I 
just wanted to show that worktree remove could be enhanced in this 
aspect and show how it could be done. So, I'll leave this in the 
#leftoverbits for the person who would be re-rolling nd/worktree-move.

---
Kaartic