Re: [PATCH v3 3/3] worktree: teach `add` to accept --reason <string> with --lock
flat view
From: Eric Sunshine <hidden>
Date: 2021-07-13 03:37:39
On Sat, Jul 10, 2021 at 8:27 PM Stephen Manz via GitGitGadget [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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 <redacted> ---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.
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
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.