Re: [PATCH 3/6] worktree move: new command
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 21, 2017, 02:39 UTC
- Message-ID
- <xmqq4lxi7acb.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170420101024.7593-4-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 40 quoted lines
> There are two options to move the main worktree, but both have > complications, so it's not implemented yet. Anyway the options are: > > - convert the main worktree to a linked one and move it away, leave the > git repository where it is. The repo essentially becomes bare after > this move. > > - move the repository with the main worktree. The tricky part is make > sure all file descriptors to the repository are closed, or it may > fail on Windows. > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > Documentation/git-worktree.txt | 7 +++++- > builtin/worktree.c | 41 ++++++++++++++++++++++++++++++++++ > contrib/completion/git-completion.bash | 2 +- > t/t2028-worktree-move.sh | 31 +++++++++++++++++++++++++ > 4 files changed, 79 insertions(+), 2 deletions(-) > > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > index 553cf8413f..b47a3247bb 100644 > --- a/Documentation/git-worktree.txt > +++ b/Documentation/git-worktree.txt > @@ -12,6 +12,7 @@ SYNOPSIS > 'git worktree add' [-f] [--detach] [--checkout] [-b <new-branch>] <path> [<branch>] > 'git worktree list' [--porcelain] > 'git worktree lock' [--reason <string>] <worktree> > +'git worktree move' <worktree> <new-path> > 'git worktree prune' [-n] [-v] [--expire <expire>] > 'git worktree unlock' <worktree> > > @@ -71,6 +72,11 @@ files from being pruned automatically. This also prevents it from > being moved or deleted. Optionally, specify a reason for the lock > with `--reason`. > > +move:: > + > +Move a working tree to a new location. Note that the main working tree > +cannot be moved yet. > +
You do not need to say "yet" here. It may come, or it may never come, and it does not matter to the readers an iota when they read this. The only thing that matters to them that they need to know is that they cannot move the primary one.
Show 23 quoted lines
> +static int move_worktree(int ac, const char **av, const char *prefix)
> +{
> + struct option options[] = {
> + OPT_END()
> + };
> + struct worktree **worktrees, *wt;
> + struct strbuf dst = STRBUF_INIT;
> + const char *reason;
> +
> + ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
> + if (ac != 2)
> + usage_with_options(worktree_usage, options);
> +
> + strbuf_addstr(&dst, prefix_filename(prefix, av[1]));
> + if (file_exists(dst.buf))
> + die(_("target '%s' already exists"), av[1]);
> +
> + worktrees = get_worktrees(0);
> + 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]);s/directory/tree/ perhaps, as Documentation/git-worktree.txt advertises these as "working trees"?
The user _may_ be well aware that av[0] is the primary one, and this message would solicit a "Huh--so what?" from such a user, unless it says that moving the primary one is not supported.
> + reason = is_worktree_locked(wt);
> + if (reason) {
> + if (*reason)
> + die(_("already locked, reason: %s"), reason);Good.
> + die(_("already locked, no reason"));I would suggest s/, no reason// here. To somebody who reads these two lines of calls to die(), it is clear what you wanted to mean by that (i.e. we would have given the reason string if it were avalable, but there isn't, so we are stressing the fact that we got nothing), but to an end user who only sees the latter, without necessarily knowing that some other times the former message may have been given, it is confusing.
For that matter, I am not sure "already" is a good phrase to use here. It's not like the end-user is asking to lock the worktree. If we refuse to move a locked worktree, perhaps we should say so. i.e. "cannot move a locked working tree (reason for locking: %s)" or something.