Re: [PATCH 3/7] worktree move: new command
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Feb 2, 2018, 09:15 UTC
- Message-ID
- <CAPig+cQeQRppb2y4YyQnqWWnCO0TXE-PjfTAhxakCJNk45ec1w@mail.gmail.com>
- In-Reply-To
- <20180124095357.19645-4-pclouds@gmail.com>
On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
Show 16 quoted lines
> 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...Show 9 quoted lines
> 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.
Show 12 quoted lines
> + 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.
Show 7 quoted lines
> 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.
Show 11 quoted lines
> +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/