Re: [PATCH 4/6] worktree move: accept destination as directory

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 21, 2017, 02:44 UTC
Message-ID: <xmqqzifa5viu.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170420101024.7593-5-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 24 quoted lines

> Similar to "mv a b/", which is actually "mv a b/a", we extract basename
> of source worktree and create a directory of the same name at
> destination if dst path is a directory.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/worktree.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index e3fbfe2a71..116507e47e 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -540,7 +540,13 @@ static int move_worktree(int ac, const char **av, const char *prefix)
>  		usage_with_options(worktree_usage, options);
>  
>  	strbuf_addstr(&dst, prefix_filename(prefix, av[1]));
> -	if (file_exists(dst.buf))
> +	if (is_directory(dst.buf))
> +		/*
> +		 * keep going, dst will be appended after we get the
> +		 * source's absolute path
> +		 */
> +		;

Ugly. Why not do that "infer the real destination b/a when existing directory b/ was given" here, without "else if" so that we can catch "hey, b/a exists already" with the existing code? That way you do not have to do is_directory() twice.

For that matter, perhaps we should go back to 3/6 and move the "if dst.buf exists error out" to after wt is validated. That would make it stand out why having these is_directory() on the same thing twice is ugly, I would think.

Show 21 quoted lines

> +	else if (file_exists(dst.buf))
>  		die(_("target '%s' already exists"), av[1]);
>  
>  	worktrees = get_worktrees(0);
> @@ -558,6 +564,17 @@ static int move_worktree(int ac, const char **av, const char *prefix)
>  	if (validate_worktree(wt, 0))
>  		return -1;
>  
> +	if (is_directory(dst.buf)) {
> +		const char *sep = find_last_dir_sep(wt->path);
> +
> +		if (!sep)
> +			die(_("could not figure out destination name from '%s'"),
> +			    wt->path);
> +		strbuf_addstr(&dst, sep);
> +		if (file_exists(dst.buf))
> +			die(_("target '%s' already exists"), dst.buf);
> +	}
> +
>  	if (rename(wt->path, dst.buf) == -1)
>  		die_errno(_("failed to move '%s' to '%s'"), wt->path, dst.buf);