git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 3/6] worktree move: new command
Date: Thu, 20 Apr 2017 19:39:32 -0700	[thread overview]
Message-ID: <xmqq4lxi7acb.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20170420101024.7593-4-pclouds@gmail.com> ("Nguyễn Thái Ngọc Duy"'s message of "Thu, 20 Apr 2017 17:10:21 +0700")

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> 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 <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.

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


  reply	other threads:[~2017-04-21  2:39 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-20 10:10 [PATCH 0/6] nd/worktree-move update Nguyễn Thái Ngọc Duy
2017-04-20 10:10 ` [PATCH 1/6] worktree.c: add validate_worktree() Nguyễn Thái Ngọc Duy
2017-04-21  2:16   ` Junio C Hamano
2017-04-24 11:13     ` Duy Nguyen
2017-04-20 10:10 ` [PATCH 2/6] worktree.c: add update_worktree_location() Nguyễn Thái Ngọc Duy
2017-04-21  2:22   ` Junio C Hamano
2017-04-20 10:10 ` [PATCH 3/6] worktree move: new command Nguyễn Thái Ngọc Duy
2017-04-21  2:39   ` Junio C Hamano [this message]
2017-04-20 10:10 ` [PATCH 4/6] worktree move: accept destination as directory Nguyễn Thái Ngọc Duy
2017-04-21  2:44   ` Junio C Hamano
2017-04-20 10:10 ` [PATCH 5/6] worktree move: refuse to move worktrees with submodules Nguyễn Thái Ngọc Duy
2017-04-21  2:47   ` Junio C Hamano
2017-04-20 10:10 ` [PATCH 6/6] worktree remove: new command Nguyễn Thái Ngọc Duy
2017-04-21  3:33   ` Junio C Hamano
2017-04-21 14:59 ` [PATCH 0/6] nd/worktree-move update Jeff King
  -- strict thread matches above, loose matches on Subject: below --
2017-01-08  9:39 [PATCH 0/6] git worktree move/remove Nguyễn Thái Ngọc Duy
2017-01-08  9:40 ` [PATCH 3/6] worktree move: new command Nguyễn Thái Ngọc Duy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq4lxi7acb.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=pclouds@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).