Re: [PATCH 4/5] worktree: add "lock" command
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- May 16, 2016, 00:09 UTC
- Message-ID
- <CAPig+cRMxYGx_gR9vei=6bcoXkhm2ntQSdqA3Vq9rtTzDd1RSQ@mail.gmail.com>
- In-Reply-To
- <20160510141729.23063-4-pclouds@gmail.com>
On Tue, May 10, 2016 at 10:17 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
Show 11 quoted lines
> 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 > @@ -11,6 +11,7 @@ SYNOPSIS > [verse] > 'git worktree add' [-f] [--detach] [--checkout] [-b <new-branch>] <path> [<branch>] > 'git worktree list' [--porcelain] > +'git worktree lock' [--reason <string>] <path> > 'git worktree prune' [-n] [-v] [--expire <expire>] > > DESCRIPTION
This forgets to update the paragraph in DESCRIPTION which currently talks about manually creating the 'locked' file. Perhaps it can be rewritten like this:
If a linked working tree is stored on a portable device or
network share which is not always mounted, you can prevent its
administrative files from being pruned by issuing the `git
worktree lock` command, optionally specifying `--reason` to
explain why the working tree is locked.The DETAILS section might also need a small update. It currently says:
...add a file named 'locked'...
but perhaps it should instead say:
...use the `git worktree lock` command, which adds a
file named `locked`,...Show 6 quoted lines
> @@ -61,6 +62,12 @@ each of the linked worktrees. The output details include if the worktree is > +lock:: > + > +When a worktree is locked, it cannot be pruned, moved or deleted. For > +example, if the worktree is on portable device that is not available > +when "git worktree <command>" is executed.
The really important reason for locking a worktree is to prevent its administrative files from being pruned *automatically*. This description doesn't properly emphasize that point.
Also, bc48328 (Documentation/git-worktree: consistently use term "linked working tree", 2015-07-20) comprehensively replaced the term "worktree" with "working tree" and, although some new instances slipped in with bb9c03b (worktree: add 'list' command, 2015-10-08), we should probably avoid adding more.
Perhaps the description of "lock" can be rewritten like this:
If a working tree is on a portable device or network share which
is not always mounted, lock it to prevent its administrative
files from being pruned automatically. This also prevents it from
being moved or deleted. Optionally, specify a reason for the lock
with `--reason`.I guess preventing it from being deleted when locked is a safety measure?
Show 6 quoted lines
> @@ -110,6 +117,9 @@ OPTIONS > --expire <time>:: > With `prune`, only expire unused working trees older than <time>. > > +--reason <string>: > + An explanation why the worktree is locked.
For consistency with other option descriptions, perhaps:
With `lock`, an explanation ...
though I don't feel strongly about it.
Show 34 quoted lines
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -459,6 +460,44 @@ static int list(int ac, const char **av, const char *prefix)
> +static int lock_worktree(int ac, const char **av, const char *prefix)
> +{
> + const char *reason = "", *old_reason;
> + struct option options[] = {
> + OPT_STRING(0, "reason", &reason, N_("string"),
> + N_("reason for locking")),
> + OPT_END()
> + };
> + struct worktree **worktrees, *wt;
> + struct strbuf dst = STRBUF_INIT;
> +
> + ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
> + if (ac != 1)
> + usage_with_options(worktree_usage, options);
> +
> + strbuf_addstr(&dst, prefix_filename(prefix,
> + strlen(prefix),
> + av[0]));
> +
> + worktrees = get_worktrees();
> + wt = find_worktree_by_path(worktrees, dst.buf);
> + 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]);
> +
> + old_reason = is_worktree_locked(wt);
> + if (old_reason) {
> + if (*old_reason)
> + die(_("already locked, reason: %s"), old_reason);
> + die(_("already locked, no reason"));
> + }As a convenience, would it make sense to allow a worktree to be re-locked with a different reason rather than erroring out?
Also, the "no reason" in the error message might be confusing. Perhaps it would be better to say merely "already locked" in this case.
> + write_file(git_common_path("worktrees/%s/locked", wt->id),
> + "%s", reason);
> + return 0;
> +}This is a tangent, but it would be nice for the "git worktree list" command to show whether a worktree is locked, and the reason (if available), both in pretty and porcelain formats. (That was another reason why I suggested to Mike, back when he was adding the "list" command, that 'struct worktree' should have a 'locked' field and get_worktrees() should populate it automatically.)
Show 28 quoted lines
> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh > @@ -0,0 +1,34 @@ > +#!/bin/sh > + > +test_description='test git worktree move, remove, lock and unlock' > + > +. ./test-lib.sh > + > +test_expect_success 'setup' ' > + test_commit init && > + git worktree add source && > + git worktree list --porcelain | grep "^worktree" >actual && > + cat <<-EOF >expected && > + worktree $TRASH_DIRECTORY > + worktree $TRASH_DIRECTORY/source > + EOF > + test_cmp expected actual > +' > + > +test_expect_success 'lock main worktree' ' > + test_must_fail git worktree lock . > +' > + > +test_expect_success 'lock linked worktree' ' > + git worktree lock --reason hahaha source && > + echo hahaha >expected && > + test_cmp expected .git/worktrees/source/locked > +'
Would it make sense to also add tests of locking from within the worktree being locked, and from within a worktree other than the one being locked (and other than the main one)?
Show 7 quoted lines
> +test_expect_success 'lock worktree twice' ' > + test_must_fail git worktree lock source && > + echo hahaha >expected && > + test_cmp expected .git/worktrees/source/locked > +' > + > +test_done