Re: [PATCH v3 3/3] worktree: teach `add` to accept --reason <string> with --lock

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 13, 2021, 03:37 UTC
Message-ID: <CAPig+cQVSUg1aqry_hMydJ=Uo=-VhOog6TUTpG=0on0LUcw8Dg@mail.gmail.com>
In-Reply-To: <9a414a3078bb481ba19449065d47e6e27322cef2.1625963240.git.gitgitgadget@gmail.com>

On Sat, Jul 10, 2021 at 8:27 PM Stephen Manz via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 18 quoted lines

> The default reason stored in the lock file, "added with --lock",
> is unlikely to be what the user would have given in a separate
> `git worktree lock` command. Allowing `--reason` to be specified
> along with `--lock` when adding a working tree gives the user control
> over the reason for locking without needing a second command.
>
> Signed-off-by: Stephen Manz <smanz@alum.mit.edu>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -302,10 +302,10 @@ static int add_worktree(const char *path, const char *refname,
>         strbuf_addf(&sb, "%s/locked", sb_repo.buf);
> -       if (!opts->keep_locked)
> -               write_file(sb.buf, _("initializing"));
> +       if (opts->keep_locked)
> +               write_file(sb.buf, "%s", opts->keep_locked);
>         else
> -               write_file(sb.buf, _("added with --lock"));
> +               write_file(sb.buf, _("initializing"));

Changing the condition around to handle the positive case first makes the diff noisier, but the resulting code is a bit easier to reason about. Okay.

Show 7 quoted lines

> @@ -500,6 +504,13 @@ static int add(int ac, const char **av, const char *prefix)
> +       if (lock_reason && !keep_locked)
> +               die(_("--reason requires --lock"));
> +       if (lock_reason)
> +               opts.keep_locked = lock_reason;
> +       else if (keep_locked)
> +               opts.keep_locked = _("added with --lock");

The benefit of relocating the "added with --lock" literal from add_worktree() to add() wasn't immediately obvious, aside from making the `if` statement in add_worktree() a bit less complex. But I managed to convince myself that the relocation makes sense since add() knows about the `--lock` option, whereas add_worktree() is merely a consumer of `add_opts` without specific knowledge of how the fields in that structure get set. Okay.

Show 9 quoted lines

> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> @@ -72,6 +72,19 @@ test_expect_success '"add" worktree with lock' '
> +test_expect_success '"add" worktree with lock and reason' '
> +       git worktree add --detach --lock --reason "why not" here-with-lock-reason main &&
> +       test_when_finished "git worktree unlock here-with-lock-reason || :" &&
> +       test -f .git/worktrees/here-with-lock-reason/locked &&
> +       echo why not >expect &&
> +       test_cmp expect .git/worktrees/here-with-lock-reason/locked
> +'

Two very minor comments:

First, considering that test_cmp() will fail anyhow if the `locked` file is missing, the `test -f` is redundant.

Second, the lack of quotes around "why not" in the `echo ... >expect` statement gives me a moment's pause since it relies upon the fact that `echo` will insert exactly one space between the "why" and "not" arguments (which happens to match the single space in the double-quoted argument to `--reason`). For clarity and that extra bit of robustness, I'd probably have used a single double-quoted string argument with `echo`.

Anyhow, those are extremely minor comments, probably not worth a re-roll but perhaps something to keep in mind if you do re-roll for some other more important reason.