[7/8] worktree: generalize candidate worktree path validation

Commit Message

Eric Sunshine June 8, 2020, 6:23 a.m. UTC

"git worktree add" checks that the specified path is a valid location
for a new worktree by ensuring that the path does not already exist and
is not already registered to another worktree (a path can be registered
but missing, for instance, if it resides on removable media). Since "git
worktree add" is not the only command which should perform such
validation ("git worktree move" ought to also), generalize the the
validation function for use by other callers, as well.

Signed-off-by: Eric Sunshine <[e-mail]>
---
 builtin/worktree.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

Comments

Junio C Hamano June 8, 2020, 10:02 p.m. UTC | #1

Eric Sunshine <[e-mail]> writes:

> "git worktree add" checks that the specified path is a valid location
> for a new worktree by ensuring that the path does not already exist and
> is not already registered to another worktree (a path can be registered
> but missing, for instance, if it resides on removable media). Since "git
> worktree add" is not the only command which should perform such
> validation ("git worktree move" ought to also), generalize the the
> validation function for use by other callers, as well.

Makes sense.

> -	validate_worktree_add(path, opts);
> +	worktrees = get_worktrees(0);
> +	check_candidate_path(path, opts->force, worktrees, "add");
> +	free_worktrees(worktrees);
> +	worktrees = NULL;

It is somewhat unsatisfying that the libified helper still signals
its displeasure by dying, but a faithful conversion that can be
cleaned up later (if we wanted to) like this step is easier to
reason about.

Patch

diff --git a/builtin/worktree.c b/builtin/worktree.c
index eebd77c46d..7c0637234e 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -274,34 +274,33 @@  static const char *worktree_basename(const char *path, int *olen)
 	return name;
 }
 
-static void validate_worktree_add(const char *path, const struct add_opts *opts)
+/* check that path is viable location for worktree */
+static void check_candidate_path(const char *path,
+				 int force,
+				 struct worktree **worktrees,
+				 const char *cmd)
 {
-	struct worktree **worktrees;
 	struct worktree *wt;
 	int locked;
 
 	if (file_exists(path) && !is_empty_dir(path))
 		die(_("'%s' already exists"), path);
 
-	worktrees = get_worktrees(0);
 	wt = find_worktree_by_path(worktrees, path);
 	if (!wt)
-		goto done;
+		return;
 
 	locked = !!worktree_lock_reason(wt);
-	if ((!locked && opts->force) || (locked && opts->force > 1)) {
+	if ((!locked && force) || (locked && force > 1)) {
 		if (delete_git_dir(wt->id))
-		    die(_("unable to re-add worktree '%s'"), path);
-		goto done;
+		    die(_("unusable worktree destination '%s'"), path);
+		return;
 	}
 
 	if (locked)
-		die(_("'%s' is a missing but locked worktree;\nuse 'add -f -f' to override, or 'unlock' and 'prune' or 'remove' to clear"), path);
+		die(_("'%s' is a missing but locked worktree;\nuse '%s -f -f' to override, or 'unlock' and 'prune' or 'remove' to clear"), cmd, path);
 	else
-		die(_("'%s' is a missing but already registered worktree;\nuse 'add -f' to override, or 'prune' or 'remove' to clear"), path);
-
-done:
-	free_worktrees(worktrees);
+		die(_("'%s' is a missing but already registered worktree;\nuse '%s -f' to override, or 'prune' or 'remove' to clear"), cmd, path);
 }
 
 static int add_worktree(const char *path, const char *refname,
@@ -318,8 +317,12 @@  static int add_worktree(const char *path, const char *refname,
 	struct commit *commit = NULL;
 	int is_branch = 0;
 	struct strbuf sb_name = STRBUF_INIT;
+	struct worktree **worktrees;
 
-	validate_worktree_add(path, opts);
+	worktrees = get_worktrees(0);
+	check_candidate_path(path, opts->force, worktrees, "add");
+	free_worktrees(worktrees);
+	worktrees = NULL;
 
 	/* is 'refname' a branch or commit? */
 	if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&