RFC patchbuiltin/worktree: enhance worktree removal

> The new feature to 'remove' worktree was handy to remove specific
> worktrees. It didn't cover one particular case of removal. Specifically,
> if there is an "entry" (a directory in <main_worktree>/.git/worktrees)
> for a worktree but the worktree repository itself does not exist then
> it means that the "entry" is stale and it could just be removed.
>
> So, in case there's a "worktree entry" but not "worktree direectory"
> then just remove the 'stale' entry.

> Let me see if I understand. Sometimes you know that you've deleted the
> worktree directory, in which case 'git worktree prune' is the obvious
> choice. However, there may be cases when you've forgotten that you
> deleted the worktree directory (or it got deleted some other way), yet
> it still shows up in "git worktree list" output with no indication
> that it has been deleted (though, ideally, it should tell you so[1]).
> In this case, you see a worktree that you know you no longer want, so
> you invoke "git worktree remove" but that errors out because the
> actual directory is already gone. This patch make the operation
> succeed, despite the missing directory. Is that correct?

> Eric Sunshine <sunshine@sunshineco.com> writes:
>> Let me see if I understand. Sometimes you know that you've deleted the
>> worktree directory, in which case 'git worktree prune' is the obvious
>> choice. However, there may be cases when you've forgotten that you
>> deleted the worktree directory (or it got deleted some other way), yet
>> it still shows up in "git worktree list" output with no indication
>> that it has been deleted (though, ideally, it should tell you so[1]).
>> In this case, you see a worktree that you know you no longer want, so
>> you invoke "git worktree remove" but that errors out because the
>> actual directory is already gone. This patch make the operation
>> succeed, despite the missing directory. Is that correct?
>
> Hmph, so the user could be using "git worktree prune" after seeing
> the error?  Was there a reason behind "git worktree remove" to be
> extra careful to make sure both existed before going forward, or was
> this a simple oversight?

> The erroring out in this case looks like simple oversight. Most
> likely, this particular case did not occur to Duy. The code does
> intentionally check the directory to see if it is dirty so that it can
> warn the user (in which case the user can re-run with --force or take
> other corrective action), but erroring out if the directory is merely

> an indirect (and unintended) result of trying to check for dirtiness.
>
> So, Kaatic's patch is intended to address that oversight (though I
> haven't examined the implementation closely; I was just trying to
> understand the reason for the patch).

> On Tue, Nov 21, 2017 at 10:14 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> The erroring out in this case looks like simple oversight. Most
>> likely, this particular case did not occur to Duy. The code does
>> intentionally check the directory to see if it is dirty so that it can
>> warn the user (in which case the user can re-run with --force or take
>> other corrective action), but erroring out if the directory is merely
>
> "...erroring out if the directory _is missing_ is merely..."
>
>> an indirect (and unintended) result of trying to check for dirtiness.
>>
>> So, Kaatic's patch is intended to address that oversight (though I
>> haven't examined the implementation closely; I was just trying to
>> understand the reason for the patch).

> OK, so the proposed log message was a bit confusing for those who
> are *not* the person who wrote it (who knew why existing behaviour
> was inadequate and did not describe how "worktree remove" would fail
> under such a scenario to illustrate it, incorrectly assuming that
> everybody who reads the proposed log message already *knows* how it
> would fail).

>         "git worktree remove" removes both the named worktree
>         directory and the administrative information for it after
>         checking that there is no local modifications that would be
>         lost (which is a handy safety measure).  It however refuses
>         to work if the worktree directory is _already_ removed.

>         The user could use "git worktree prune" after seeing the
>         error and realizing the situation, but at that point, there
>         is nothing gained by leaving only the administrative data
>         behind.  Teach "git worktree remove" to go ahead and remove
>         the trace of the worktree in such a case.
>
> or soemthing like that?

> 
> Kaartic: Regarding the actual patch, rather than silencing
> validate_worktree() (which seems an unfortunate thing to do), isn't it
> possible simply to do a quick test to see if the worktree directory
> exists before calling validate_worktree()? If it doesn't exist, then
> just skip down to the part of the code which does the 'prune'
> operation.
> 

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index b5afba164..6eab91889 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -605,6 +605,23 @@ static int move_worktree(int ac, const char **av, const char *prefix)
>  	return update_worktree_location(wt, dst.buf);
>  }
>  
> +/* Removes the .git/worktrees/worktree_id directory for
> + * the given worktree_id
> + *
> + * Returns 0 on success and non-zero value in case of failure
> + */

> +static int remove_worktree_entry(char *worktree_id) {
> +	int ret = 0;
> +	struct strbuf we_path = STRBUF_INIT;
> +	strbuf_addstr(&we_path, git_common_path("worktrees/%s", worktree_id));
> +	if (remove_dir_recursively(&we_path, 0)) {
> +		error_errno(_("failed to delete '%s'"), we_path.buf);
> +		ret = -1;
> +	}
> +	strbuf_release(&we_path);
> +	return ret;
> +}
> +

>  static int remove_worktree(int ac, const char **av, const char *prefix)
>  {
>  	int force = 0;
> @@ -634,6 +651,16 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
>  			die(_("already locked, reason: %s"), reason);
>  		die(_("already locked, no reason"));
>  	}
> +
> +	if (!file_exists(wt->path)) {
> +	/* There's a worktree entry but the worktree directory
> +	 * doesn't exist. So, just remove the worktree entry.
> +	 */
> +		ret = remove_worktree_entry(wt->id);
> +		free_worktrees(worktrees);
> +		return ret;
> +	}
> +
>  	if (validate_worktree(wt, 0))
>  		return -1;

> @@ -670,13 +697,7 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
>  		error_errno(_("failed to delete '%s'"), sb.buf);
>  		ret = -1;
>  	}
> -	strbuf_reset(&sb);
> -	strbuf_addstr(&sb, git_common_path("worktrees/%s", wt->id));
> -	if (remove_dir_recursively(&sb, 0)) {
> -		error_errno(_("failed to delete '%s'"), sb.buf);
> -		ret = -1;
> -	}
> -	strbuf_release(&sb);
> +	ret = remove_worktree_entry(wt->id);
>  	free_worktrees(worktrees);
>  	return ret;
>  }

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

> Eric Sunshine <sunshine@sunshineco.com> writes:
>> 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?

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

> Eric Sunshine <sunshine@sunshineco.com> writes:
>>> So, Kaatic's patch is intended to address that oversight (though I
>>> haven't examined the implementation closely; I was just trying to
>>> understand the reason for the patch).
> 
> OK, so the proposed log message was a bit confusing for those who
> are *not* the person who wrote it (who knew why existing behaviour
> was inadequate and did not describe how "worktree remove" would fail
> under such a scenario to illustrate it, incorrectly assuming that
> everybody who reads the proposed log message already *knows* how it
> would fail).

> 
> 	"git worktree remove" removes both the named worktree
> 	directory and the administrative information for it after
> 	checking that there is no local modifications that would be
> 	lost (which is a handy safety measure).  It however refuses
> 	to work if the worktree directory is _already_ removed.
> 
> 	The user could use "git worktree prune" after seeing the
> 	error and realizing the situation, but at that point, there
> 	is nothing gained by leaving only the administrative data
> 	behind.  Teach "git worktree remove" to go ahead and remove
> 	the trace of the worktree in such a case.
> 
> or soemthing like that?
> 

> On Tue, Nov 21, 2017 at 10:09 AM, Kaartic Sivaraam
> <kaartic.sivaraam@gmail.com> wrote:
>> The new feature to 'remove' worktree was handy to remove specific
>> worktrees. It didn't cover one particular case of removal. Specifically,
>> if there is an "entry" (a directory in <main_worktree>/.git/worktrees)
>> for a worktree but the worktree repository itself does not exist then
>> it means that the "entry" is stale and it could just be removed.
>>
>> So, in case there's a "worktree entry" but not "worktree direectory"
>> then just remove the 'stale' entry.
> 
> Let me see if I understand. Sometimes you know that you've deleted the
> worktree directory, in which case 'git worktree prune' is the obvious
> choice. However, there may be cases when you've forgotten that you
> deleted the worktree directory (or it got deleted some other way), yet
> it still shows up in "git worktree list" output with no indication
> that it has been deleted (though, ideally, it should tell you so[1]).
> In this case, you see a worktree that you know you no longer want, so
> you invoke "git worktree remove" but that errors out because the
> actual directory is already gone. This patch make the operation
> succeed, despite the missing directory. Is that correct?
> 

> [1]: Excerpt from:
> https://public-inbox.org/git/CAPig+cTTrv2C7JLu1dr4+N8xo+7YQ+deiwLDA835wBGD6fhS1g@mail.gmail.com/
> 
> Other information which would be nice to display for each worktree
> [by the 'list' command] (possibly controlled by a --verbose flag):
> 
>     * the checked out branch or detached head
>     * whether it is locked
>          - the lock reason (if available)
>          - and whether the worktree is currently accessible
>      * whether it can be pruned
>          - and the prune reason if so
> 

> On Wednesday 22 November 2017 03:07 AM, Eric Sunshine wrote:
>> [1]: Excerpt from:
>> https://public-inbox.org/git/CAPig+cTTrv2C7JLu1dr4+N8xo+7YQ+deiwLDA835wBGD6fhS1g@mail.gmail.com/
>>
>> Other information which would be nice to display for each worktree
>> [by the 'list' command] (possibly controlled by a --verbose flag):
>>
>>     * the checked out branch or detached head
>>     * whether it is locked
>>          - the lock reason (if available)
>>          - and whether the worktree is currently accessible
>>      * whether it can be pruned
>>          - and the prune reason if so
>
> It would nice to see this information. It would be very useful but 'list'
> doesn't seem to be the right sub-sub-command to give such information. Maybe
> there should be a new sub-sub-command named 'info' or something to give such
> information?