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

From: Junio C Hamano <gitster@pobox.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Stephen Manz via GitGitGadget <gitgitgadget@gmail.com>,
	Git List <git@vger.kernel.org>, Stephen Manz <smanz@alum.mit.edu>
Subject: Re: [PATCH] worktree: teach `add` to accept --reason <string> with --lock
Date: Tue, 06 Jul 2021 12:42:21 -0700	[thread overview]
Message-ID: <xmqq7di3jkki.fsf@gitster.g> (raw)
In-Reply-To: <CAPig+cSsPrQrP9xk8M8H339_NpYqKh1okeo1V-fAJ2zk3QeOjQ@mail.gmail.com> (Eric Sunshine's message of "Tue, 6 Jul 2021 02:19:43 -0400")

Eric Sunshine <sunshine@sunshineco.com> writes:

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

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