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

Eric Sunshine [off-list ref] writes:

quoted

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

Thanks for thinking aloud, but I'd prefer the interface as posted,
simply because there is one less thing for users to remember.  The
justification to lock is given with the --reason=<why> argument no
matter how you acquire the lock on a worktree.

quoted

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.

Makes sense.

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"));

Excellent.

Thanks.

On Tue, Jul 6, 2021 at 3:42 PM Junio C Hamano [off-list ref] wrote:

Eric Sunshine [off-list ref] writes:

quoted

quoted

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

Thanks for thinking aloud, but I'd prefer the interface as posted,
simply because there is one less thing for users to remember.  The
justification to lock is given with the --reason=<why> argument no
matter how you acquire the lock on a worktree.

My one bit of pushback is that, although the meaning of `--reason` is
plenty clear in the context of `git worktree lock`, it may become
ambiguous in the context of `git worktree add` if worktrees ever grow
additional attributes/features which are also accompanied by
"reasons". That possibility suggests that this particular
reason-giving option of `git worktree add` ought to be named
`--lock-reason`, but `git worktree add --lock --lock-reason=<reason>`
feels clunky and redundant, which is why I was wondering if `git
worktree --lock[=<reason>]` would be a better (and more convenient)
UI.

I'm questioning the UI choice now so we can avoid backpedalling later
on, if it ever comes to that, but perhaps my concern is unfounded.
(Indeed, I haven't been able to come up with cases which would make
`--reason` ambiguous.)