From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mon, 16 May 2016 00:09:00 GMT
Subject: Re: [PATCH 4/5] worktree: add "lock" command
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:
> 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`,...

> @@ -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?

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

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

> 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)?

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