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

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> 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>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -624,8 +624,6 @@ static int move_worktree(int ac, const char **av, const char *prefix)
>         path = prefix_filename(prefix, av[1]);
>         strbuf_addstr(&dst, path);
>         free(path);
> -       if (file_exists(dst.buf))
> -               die(_("target '%s' already exists"), av[1]);

Nit: The distance between this "removed" conditional and the code
inserted below hampered review a bit since it made it slightly more
onerous to check for unwanted logic changes. Had patch 3/7 located
this conditional below the is_main_worktree() check (where the new
code is inserted below), this 'if' would have merely mutated into an
'else if' but not otherwise moved, thus review would have been
simplified. (Not itself worth a re-roll.)

>         worktrees = get_worktrees(0);
>         wt = find_worktree(worktrees, prefix, av[0]);
> @@ -633,6 +631,20 @@ static int move_worktree(int ac, const char **av, const char *prefix)
>                 die(_("'%s' is not a working tree"), av[0]);
>         if (is_main_worktree(wt))
>                 die(_("'%s' is a main working tree"), av[0]);
> +
> +       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);

Do we know at this point whether 'dst' has a terminating "/"? If it
does, then this addstr() could result in "//" in the path which might
be problematic on Windows.

> +               if (file_exists(dst.buf))
> +                       die(_("target '%s' already exists"), dst.buf);
> +       } else if (file_exists(dst.buf)) {
> +               die(_("target '%s' already exists"), av[1]);
> +       }

I wonder if it makes sense to collapse the duplicated
file_exists()/"target already exists" code?

    if (is_directory(...)) {
        ...
        strbuf_addstr(&dst, sep);
    }
    if (file_exists(dst.buf))
        die(_("target '%s' already exists"), dst.buf);

It changes the error message slightly for the non-directory case but
simplifies the code a bit.

>         reason = is_worktree_locked(wt);
>         if (reason) {
>                 if (*reason)

Perhaps add a test to t2028-worktree-move.sh to show that this new
behavior works?