On Sat, Jul 10, 2021 at 8:27 PM Stephen Manz via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
> 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@xxxxxxxxxxxx>
> ---
> 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.
> @@ -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.
> 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.
