Merge branch 'rs/worktree-add-basename-fixes' into jch

The string extraction logic for the branch name and worktree name
from the given path in 'git worktree add' has been corrected and
simplified to avoid out-of-bounds reads and improper handling of
trailing slashes.

* rs/worktree-add-basename-fixes:
  worktree add: let worktree_basename() return string copy
  worktree add: trim slashes when deriving branch name from path
  worktree add: reject separator-only path
  worktree add: don't read out of bounds in worktree_basename()

builtin/worktree.c

@@ -294,7 +294,7 @@ static void remove_junk_on_signal(int signo)
 	raise(signo);
 }
 
-static const char *worktree_basename(const char *path, int *olen)
+static char *worktree_basename_dup(const char *path)
 {
 	const char *name;
 	int len;
@@ -303,14 +303,11 @@ static const char *worktree_basename(const char *path, int *olen)
 	while (len && is_dir_sep(path[len - 1]))
 		len--;
 
-	for (name = path + len - 1; name > path; name--)
+	name = path + len;
-		if (is_dir_sep(*name)) {
+	while (name > path && !is_dir_sep(name[-1]))
-			name++;
+		name--;
-			break;
-		}
 
-	*olen = len;
+	return xmemdupz(name, path + len - name);
-	return name;
 }
 
 /* check that path is viable location for worktree */
@@ -464,6 +461,7 @@ static int add_worktree(const char *path, const char *refname,
 	struct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT;
 	struct strbuf sb = STRBUF_INIT;
 	const char *name;
+	char *name_to_free = NULL;
 	struct strvec child_env = STRVEC_INIT;
 	unsigned int counter = 0;
 	int len, ret;
@@ -491,12 +489,12 @@ static int add_worktree(const char *path, const char *refname,
 	if (!commit && !opts->orphan)
 		die(_("invalid reference: %s"), refname);
 
-	name = worktree_basename(path, &len);
+	name = name_to_free = worktree_basename_dup(path);
-	strbuf_add(&sb, name, path + len - name);
+	if (!*name)
-	sanitize_refname_component(sb.buf, &sb_name);
+		die(_("invalid path '%s'"), path);
+	sanitize_refname_component(name, &sb_name);
 	if (!sb_name.len)
-		BUG("How come '%s' becomes empty after sanitization?", sb.buf);
+		BUG("How come '%s' becomes empty after sanitization?", name);
-	strbuf_reset(&sb);
 	name = sb_name.buf;
 	repo_git_path_replace(the_repository, &sb_repo, "worktrees/%s", name);
 	len = sb_repo.len;
@@ -630,6 +628,7 @@ done:
 	strbuf_release(&sb_git);
 	strbuf_release(&sb_name);
 	free_worktree(wt);
+	free(name_to_free);
 	return ret;
 }
 
@@ -785,10 +784,8 @@ static void advise_disambiguating_remotes(const char *path, const char *branch,
 
 static char *dwim_branch(const struct add_opts *opts, const char *path, char **new_branch)
 {
-	int n;
 	int branch_exists;
-	const char *s = worktree_basename(path, &n);
+	char *branchname = worktree_basename_dup(path);
-	char *branchname = xstrndup(s, n);
 	struct strbuf ref = STRBUF_INIT;
 
 	branch_exists = !check_branch_ref(the_repository, &ref, branchname) &&
@@ -909,9 +906,7 @@ static int add(int ac, const char **av, const char *prefix,
 	}
 
 	if (opts.orphan && !new_branch) {
-		int n;
+		new_branch = new_branch_to_free = worktree_basename_dup(path);
-		const char *s = worktree_basename(path, &n);
-		new_branch = new_branch_to_free = xstrndup(s, n);
 	} else if (opts.orphan) {
 		; /* no-op */
 	} else if (opts.detach) {

t/t2400-worktree-add.sh

@@ -46,6 +46,10 @@ test_expect_success '"add" refuses to checkout locked branch' '
 	test_path_is_missing .git/worktrees/zere
 '
 
+test_expect_success '"add" rejects an empty path' '
+	test_must_fail git worktree add "" HEAD
+'
+
 test_expect_success 'checking out paths not complaining about linked checkouts' '
 	(
 	cd existing_empty &&
@@ -294,6 +298,11 @@ test_expect_success '"add" with <branch> omitted' '
 	test_cmp_rev HEAD bat
 '
 
+test_expect_success '"add" with trailing slash and <branch> omitted' '
+	git worktree add waffle/bit/ &&
+	test_cmp_rev HEAD bit
+'
+
 test_expect_success '"add" checks out existing branch of dwimd name' '
 	git branch dwim HEAD~1 &&
 	git worktree add dwim &&
@@ -384,6 +393,14 @@ test_expect_success '"add --orphan (no -b)"' '
 	test_cmp expected actual
 '
 
+test_expect_success '"add --orphan with trailing slash (no -b)"' '
+	test_when_finished "git worktree remove -f -f neworphan" &&
+	git worktree add --orphan ./neworphan/ &&
+	echo refs/heads/neworphan >expected &&
+	git -C neworphan symbolic-ref HEAD >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success '"add --orphan --quiet"' '
 	test_when_finished "git worktree remove -f -f orphandir" &&
 	git worktree add --quiet --orphan -b neworphan orphandir 2>log.actual &&