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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 2, 2018, 09:35 UTC
Message-ID: <CAPig+cTAmzAK2vQHuPHJJMT+Q2fiD4P--qv_rSrc2gKbFFhPTg@mail.gmail.com>
In-Reply-To: <20180124095357.19645-5-pclouds@gmail.com>

On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

Show 13 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>
> ---
> 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.)

Show 14 quoted lines

>         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.

Show 5 quoted lines

> +               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?