Re: [RFC PATCH v2] builtin/worktree: enhance worktree removal
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 28, 2017, 03:02 UTC
- Message-ID
- <xmqq7eub85rz.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20171127173621.6707-1-kaartic.sivaraam@gmail.com>
Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
Show 13 quoted lines
> 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).
Show 12 quoted lines
> +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.
Show 19 quoted lines
> 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?
Show 15 quoted lines
> @@ -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;
> }