Re: [PATCH 3/4] worktree add: trim slashes when deriving branch name from path

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

René Scharfe <l.s.r@web.de> writes:

> worktree_basename() sets `n` to the length of `path` without trailing
> path separators, not to the length of the basename.  This matters when
> deriving a branch name from a path with more than one component.  E.g.:
>
>    path: /new/worktree/
>    s:         ^
>    n:    |-----------|
>
> So here xstrndup(s, n) copies up to 13 characters from "worktree/",
> effectively to the end of the string, including the trailing dash.
>
> Path separators are not allowed at the end of branch names, so strip
> them off by calculating the basename length and extracting just that
> part.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  builtin/worktree.c      |  4 ++--
>  t/t2400-worktree-add.sh | 13 +++++++++++++
>  2 files changed, 15 insertions(+), 2 deletions(-)

Hmph, so am I correct to understand that the symptom observable by
end-users of this is that we used to attempt creating "bit/" branch
when they request

    $ git worktree add waffle/bit/

and it wouldn't have worked until they said

    $ git worktree add waffle/bit

instead?  Not allowing a trailing slash when naming a directory is
nasty (it is a good practice to explicitly give a trailing slash
when naming a directory to avoid confusion), and it is a good fix.

It also should work fine with waffle/bit/// even though there is no
strong reason to allow it ;-).


> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index a53e815cc9..01c245778e 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -769,7 +769,7 @@ static char *dwim_branch(const char *path, char **new_branch)
>  	int n;
>  	int branch_exists;
>  	const char *s = worktree_basename(path, &n);
> -	char *branchname = xstrndup(s, n);
> +	char *branchname = xmemdupz(s, path + n - s);
>  	struct strbuf ref = STRBUF_INIT;
>  
>  	branch_exists = !check_branch_ref(the_repository, &ref, branchname) &&
> @@ -878,7 +878,7 @@ static int add(int ac, const char **av, const char *prefix,
>  	if (opts.orphan && !new_branch) {
>  		int n;
>  		const char *s = worktree_basename(path, &n);
> -		new_branch = new_branch_to_free = xstrndup(s, n);
> +		new_branch = new_branch_to_free = xmemdupz(s, path + n - s);
>  	} else if (opts.orphan) {
>  		; /* no-op */
>  	} else if (opts.detach) {
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> index 280d2e2c07..7e2811fa77 100755
> --- a/t/t2400-worktree-add.sh
> +++ b/t/t2400-worktree-add.sh
> @@ -298,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 &&
> @@ -388,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 &&