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

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

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