Re: [PATCH 08/10] worktree: add "move" commmand

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 1, 2017, 18:23 UTC
Message-ID: <CAPig+cRqgzB4tiTb=fHuFBTqo3QEGM3m378PvOse06KdreKo2Q@mail.gmail.com>
In-Reply-To: <20160625075433.4608-9-pclouds@gmail.com>

On Sat, Jun 25, 2016 at 3:54 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

Show 10 quoted lines

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> +static int move_worktree(int ac, const char **av, const char *prefix)
> +{
> + [...]
> +       if (file_exists(dst.buf))
> +               die(_("target '%s' already exists"), av[1]);
> +
> +       worktrees = get_worktrees();

'worktrees' is being leaked at each 'return'. Probably need to call free_worktrees().

Show 9 quoted lines

> +       wt = find_worktree(worktrees, prefix, av[0]);
> +       if (!wt)
> +               die(_("'%s' is not a working directory"), av[0]);
> +       if (is_main_worktree(wt))
> +               die(_("'%s' is a main working directory"), av[0]);
> +       if ((reason = is_worktree_locked(wt))) {
> +               if (*reason)
> +                       die(_("already locked, reason: %s"), reason);
> +               die(_("already locked, no reason"));

Saying "already locked" makes sense for the 'worktree locked' command, however, for 'worktree move', a simple "locked" would be less confusing.

Show 15 quoted lines

> +       }
> +       if (validate_worktree(wt, 0))
> +               return -1;
> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
> @@ -59,4 +59,34 @@ test_expect_success 'unlock worktree twice' '
> +test_expect_success 'move non-worktree' '
> +       mkdir abc &&
> +       test_must_fail git worktree move abc def
> +'
> +
> +test_expect_success 'move locked worktree' '
> +       git worktree lock source &&
> +       test_must_fail git worktree move source destination &&
> +       git worktree unlock source
> +'

Should the unlock be wrapped in a test_when_finished()?