On Tue, Jul 6, 2021 at 1:47 AM 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.
Thanks. I can see how the default lock reason in this case isn't very
interesting. In fact, I'd actually have expected the default reason to
be empty, just as it's empty when `git worktree lock` is invoked with
no `--reason`. That might be an additional thing to "fix" at some
point by someone (or in this series if you'd like to tackle it).
It's nice to see both documentation and test updates along with the
code change. See below for a few comments...
> Signed-off-by: Stephen Manz <smanz@xxxxxxxxxxxx>
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -9,7 +9,7 @@ git-worktree - Manage multiple working trees
> +'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] [-b <new-branch>] <path> [<commit-ish>]
>
> --reason <string>::
> + With `lock` or with `add --lock`, an explanation why the working tree is locked.
I realize that you're mimicking the interface of `git worktree lock`
which accepts an optional `--reason`, but I'm wondering if the
user-experience might be improved by instead allowing `--lock` to
accept the reason as an optional argument. For instance:
git worktree add --lock='just because' ...
Aside from the dissymmetry with `git worktree lock`, I haven't come up
with a reason that `--lock[=<reason>]` wouldn't be an improvement. But
perhaps I'm not imaginative enough...
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -31,6 +31,7 @@ struct add_opts {
> int checkout;
> int keep_locked;
> + const char *lock_reason;
> };
Whether or not we do go with the approach of allowing `--lock` to take
the reason as an optional argument, we don't really need two structure
members here. Instead, we can repurpose `keep_locked` as a `const char
*` which is NULL if `--lock` was not specified, otherwise non-NULL.
For the non-NULL case, it would be an empty (zero-length) string if
the optional `<reason>` was omitted, otherwise it would be the reason
given. So, no need for a distinct `lock_reason` member.
> @@ -302,10 +303,15 @@ static int add_worktree(const char *path, const char *refname,
> + if (!opts->keep_locked) {
> write_file(sb.buf, "initializing");
> - else
> - write_file(sb.buf, "added with --lock");
> + }
> + else {
> + if (opts->lock_reason)
> + write_file(sb.buf, "%s", opts->lock_reason);
> + else
> + write_file(sb.buf, _("added with --lock"));
> + }
Style on this project is to cuddle `else` with both braces:
} else {
However, in this case, it should probably just be a simple `else if`:
if (!opts->keep_locked)
write_file(sb.buf, "initializing");
else if (opts->lock_reason)
write_file(sb.buf, "%s", opts->lock_reason);
else
write_file(sb.buf, _("added with --lock"));
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> @@ -67,11 +67,22 @@ test_expect_success '"add" worktree' '
> test_expect_success '"add" worktree with lock' '
> - git rev-parse HEAD >expect &&
> git worktree add --detach --lock here-with-lock main &&
> test -f .git/worktrees/here-with-lock/locked
> '
Dropping this unused code makes sense, though on this project we would
normally do it as a separate (probably preparatory) patch, thus making
this a two-patch series (at minimum).
> +test_expect_success '"add" worktree with lock and reason' '
> + git worktree add --detach --lock --reason "why not" here-with-lock-reason main &&
> + test -f .git/worktrees/here-with-lock-reason/locked &&
> + echo why not >expect &&
> + test_cmp expect .git/worktrees/here-with-lock-reason/locked
> +'
To make this a bit friendlier to other tests which come later in the
script, it might be a good idea to unlock this worktree here at its
source. To do so unconditionally, whether the test succeeds or fails,
use test_when_finished() just after placing the lock. So, something
like this:
git worktree add --detach --lock --reason ... &&
test_when_finished "git worktree unlock here-with-lock-reason || :" &&
test -f .git/worktrees/here-with-lock-reason/locked &&
...
> +test_expect_failure '"add" worktree with reason but no lock' '
> + git worktree add --detach --reason "why not" here-with-reason-only main &&
> + test -f .git/worktrees/here-with-reason-only/locked
> +'
test_expect_failure() is for indicating known bugs or unimplemented
features, however, you did implement the check that `--reason`
requires `--lock`, so test_expect_failure() is not quite what you want
here. Instead, use test_must_fail() in the test body, and you want to
make sure that the `locked` file did not get created. So, something
like this:
test_expect_success '"add" worktree with reason but no lock' '
test_must_fail git worktree add --detach --reason ... &&
test_path_is_missing .git/worktrees/here-with-reason-only/locked
'
