Re: [PATCH 3/7] worktree move: new command

[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:
> This command allows to relocate linked worktrees. Main worktree cannot
> (yet) be moved.
> [...]
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -79,6 +80,11 @@ files from being pruned automatically. This also prevents it from
> +move::
> +
> +Move a working tree to a new location. Note that the main working tree
> +cannot be moved.
> +
> @@ -281,7 +287,6 @@ performed manually, such as:
>  - `remove` to remove a linked working tree and its administrative files (and
>    warn if the working tree is dirty)
> -- `mv` to move or rename a working tree and update its administrative files

A couple other places in this document ought to be updated.
Specifically, in DESCRIPTION:

    If you move a linked working tree, you need to manually update the
    administrative files so that they do not get pruned automatically.
    See section "DETAILS" for more information.

which can probably be dropped in its entirety. And, in DETAILS:

    If you move a linked working tree, you need to update the 'gitdir'
    file...

should probably be reworded to

    If you _manually_ move a linked working tree, you need to update
    the 'gitdir' file...

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -605,6 +606,53 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)
> +static int move_worktree(int ac, const char **av, const char *prefix)
> +{
> +       [...]
> +       worktrees = get_worktrees(0);
> +       wt = find_worktree(worktrees, prefix, av[0]);
> +       if (!wt)
> +               die(_("'%s' is not a working tree"), av[0]);

This is still leaking 'worktrees'[1]. You probably want
free_worktrees() immediately after the find_worktree() invocation.

> +       if (is_main_worktree(wt))
> +               die(_("'%s' is a main working tree"), av[0]);
> +       reason = is_worktree_locked(wt);
> +       if (reason) {
> +               if (*reason)
> +                       die(_("cannot move a locked working tree, lock reason: %s"),
> +                           reason);
> +               die(_("cannot move a locked working tree"));
> +       }
> +       if (validate_worktree(wt, &errmsg))
> +               die(_("validation failed, cannot move working tree:\n%s"),
> +                   errmsg.buf);

Minor: All the other error messages are presented on a single line.
Despite this error message potentially being quite long, it worries me
a bit that presenting it on two lines could complicate scripted
clients if they need to collect the error message for special
handling.

> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
> @@ -59,4 +59,35 @@ test_expect_success 'unlock worktree twice' '
> +test_expect_success 'move locked worktree' '
> +       git worktree lock source &&
> +       test_must_fail git worktree move source destination &&
> +       git worktree unlock source
> +'

Also from [1], wrapping 'unlock' inside a test_when_finished()
invocation seems potentially desirable.

> +test_expect_success 'move worktree' '
> +       toplevel="$(pwd)" &&
> +       git worktree move source destination &&
> +       test_path_is_missing source &&
> +       git worktree list --porcelain | grep "^worktree" >actual &&
> +       cat <<-EOF >expected &&
> +       worktree $toplevel
> +       worktree $toplevel/destination
> +       worktree $toplevel/elsewhere
> +       EOF
> +       test_cmp expected actual &&

This seems somewhat fragile. If someone inserts a test before this one
which adds another worktree, then this test would break. Perhaps the
filtering of the --porcelain output could be more strict? (Not
necessarily worth a re-roll, though.)

> +       git -C destination log --format=%s >actual2 &&
> +       echo init >expected2 &&
> +       test_cmp expected2 actual2
> +'

[1]: https://public-inbox.org/git/CAPig+cRqgzB4tiTb=fHuFBTqo3QEGM3m378PvOse06KdreKo2Q@mail.gmail.com/