[RFC PATCH] builtin/worktree: enhance worktree removal

[RFC PATCH] builtin/worktree: enhance worktree removal

[Kaartic Sivaraam]

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.

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---

Hello Duy,

I noticed that your remove command could be enhanced for a particular
case. So, I made up an ad-hoc patch "illustrating" how it could be done.
I may have broken something by quieting out 'validate_worktree()'
function, so take note of it.

You might add this as a separate commit or just incorporate it into
one of your commits if you re-roll your 'nd/worktree-move' branch.

Thanks,
Kaartic

 builtin/worktree.c | 38 ++++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index b5afba164..f70bc0bd8 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -605,6 +605,22 @@ 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,9 +650,17 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
 			die(_("already locked, reason: %s"), reason);
 		die(_("already locked, no reason"));
 	}
-	if (validate_worktree(wt, 0))
-		return -1;
-
+	if (validate_worktree(wt, 1)) {
+		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;
+		} else {
+			return -1;
+		}
+	}
 	if (!force) {
 		struct argv_array child_env = ARGV_ARRAY_INIT;
 		struct child_process cp;
@@ -670,13 +694,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;
 }
-- 
2.15.0.345.gf926f18f3

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

[Eric Sunshine]

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

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

[Junio C Hamano]

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?

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

[Eric Sunshine]

On Tue, Nov 21, 2017 at 9:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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).

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

[Eric Sunshine]

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

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

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

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

[Eric Sunshine]

On Tue, Nov 21, 2017 at 10:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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).

Correct. The log message is confusing, enough so that my knee-jerk
reaction was that the patch was trying to re-invent 'git worktree
prune'. That seemed odd, so I spent extra time trying to figure out
what it really meant, which led to my rather lengthy response asking
if I was understanding the situation correctly.

>         "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 "refusal" is by accident, so perhaps phrase it like this:

    However, due to an oversight, it aborts with an error if the
    worktree directory is _already_ removed.

or something.

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

Yes, I quite like it; it conveys the information necessary to
understand the issue.

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.

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

[Kaartic Sivaraam]

On Wednesday 22 November 2017 03:07 AM, Eric Sunshine wrote:
> 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?
> 

Yes. My primary motive for sending this is that, I thought it didn't 
make much sense for 'remove' to fail just because the directory didn't 
exist. Further, I thought it would be more flexible to allow for 
'selective' pruning of worktrees. The use case in which, I thought, this 
flexibility might prove helpful is that of 'scripts'.

Consider a hypothetical script that plays around with a git repository. 
Further, consider that spawns a new worktree for temporary use. It's 
quite natural for the script to consider cleaning up it's own mess and 
thus it might consider removing the worktree it created. With the 
'remove' command it is as easy is,

git worktree remove <worktree_id>

But what if it was the case that the directory might/might not exist 
when that part of the script is reached. In case the directory didn't 
exist the 'remove' command errors out that it didn't exist instead of 
just removing the worktree "entry" for that <worktree_id>. I thought it 
wasn't a good thing and thus this script.

After writing this, I feel I haven't justified it well (impostor 
syndrome, possibly). Anyways, at the end of the day this "ad-hoc" patch 
was just a result of my gut feeling that, "It didn't make sense for the 
'remove' command to fail when the directory didn't exist". The 
implementation is just a "sloppy" illustration of how it could be 
achieved and should "better" not be used as such.


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

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

[Kaartic Sivaraam]

On Wednesday 22 November 2017 09:25 AM, Junio C Hamano wrote:
> 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).

I shouldn't have made the log message as 'ad hoc' as I made the patch, 
sorry :-(

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

Much better!

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

[Kaartic Sivaraam]

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

Nice suggestion. It seems to be a much better thing to do than silencing 
'validate_worktree' (it was an "ad-hoc" patch, you see). I'll send an 
update incorporating this suggestion (of course, with the suggested 
commit message), when I find time.

Thanks,
Kaartic

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

[Eric Sunshine]

On Wed, Nov 22, 2017 at 12:09 PM, Kaartic Sivaraam
<kaartic.sivaraam@gmail.com> wrote:
> 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?

I'm perfectly fine with having 'git worktree list' provide this extra
information; that was the intention from the start. The "quick"
summary of all worktrees provided by 'git worktree list' is exactly
the sort of place where you're most likely to notice something
unexpected, such as the missing worktree directory. And, this extra
information doesn't have to be noisy:

--- 8< ---
% git worktree list
giggle     89ea799ffc [master]
../bobble  f172cb543d [feature1]  locked
../fumple  6453c84b7d (detached HEAD)  prunable
--- 8< ---

And, the verbose case:

--- 8< ---
% git worktree list -v
giggle     89ea799ffc [master]
../bobble  f172cb543d [feature1]
    locked: worktree on removable media
../fumple  6453c84b7d (detached HEAD)
    prunable: directory does not exist
--- 8< ---

An "info" command you speak of might also have value, but that's
orthogonal to this extra information that 'git worktree list' could
provide.

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

[Kaartic Sivaraam]

"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). However, due to a
possible oversight, it aborts with an error 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.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 Changes in v2:

  - incorporated the suggestion to avoid quieting `validate_worktree()`
    to detect inexistent directory (thanks, Eric!)

  - used the suggested (much better) commit message

 builtin/worktree.c | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

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;
 }
-- 
2.15.0.345.gf926f18f3

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

[Junio C Hamano]

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:

> 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
> + */

	/*
	 * our multi-line comment should be formatted
	 * more like this, giving slash-asterisk at the
	 * beginning and asterisk-slash at the end their
	 * own line.
	 */

There are other instances of the same in this patch, I suspect, but
because this seemed to depend on other things in 'pu' that are not
ready (if it depends on something that is stalled or abandoned, we
need to first get it unabandoned before this can even come close to
'pu'), I didn't create a topic branch for this RFC patch to view the
resulting file as a whole (this review is based only on the patch
e-mail).

> +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;
> +}
> +

This lifts a small section of remove_worktree() to make it usable
from other places.  But see below.

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

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 */

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.  With this patch, we have two 3-line blocks that call
remove_worktree_entry(wt->id), free_worktrees(worktrees) and returns
ret, and these happen to be identical, but the next person who would
be mucking with the code (perhaps adding more variables that need to
be reset in this codeflow) can easily miss one of these two places.

The resulting code would make the body of "if (!force)" block too
deeply nested, I suspect, but that is an indication that that part
is overlong and overly complex in the context of this function, and
can and should migrate to its own helper function.

Hmm?

> @@ -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;
>  }

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

[Eric Sunshine]

On Mon, Nov 27, 2017 at 10:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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

[Junio C Hamano]

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.

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

[Eric Sunshine]

On Mon, Nov 27, 2017 at 11:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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?

Yes, that's what I had in mind.

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

[Kaartic Sivaraam]

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