Subject: Re: [PATCH 3/6] worktree move: new command

From: Junio C Hamano <[email-masked]>
Date: Thu, 20 Apr 2017 19:39:32 -0700

Nguyễn Thái Ngọc Duy  <[email-masked]> writes:

> 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 <[email-masked]>
> ---
>  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.

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