diff mbox series

worktree: teach `add` to accept --reason <string> with --lock

Message ID pull.992.git.1625550451038.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series worktree: teach `add` to accept --reason <string> with --lock | expand

Commit Message

Stephen Manz July 6, 2021, 5:47 a.m. UTC 
From: Stephen Manz <smanz@alum.mit.edu>

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@alum.mit.edu>
---
    worktree: teach add to accept --reason with --lock
    
    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.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-992%2FSRManz%2Flock_reason-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-992/SRManz/lock_reason-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/992

 Documentation/git-worktree.txt |  4 ++--
 builtin/worktree.c             | 16 +++++++++++++---
 t/t2400-worktree-add.sh        | 13 ++++++++++++-
 3 files changed, 27 insertions(+), 6 deletions(-)


base-commit: 670b81a890388c60b7032a4f5b879f2ece8c4558

Comments

Eric Sunshine July 6, 2021, 6:19 a.m. UTC | #1 
On Tue, Jul 6, 2021 at 1:47 AM Stephen Manz via GitGitGadget
<gitgitgadget@gmail.com> 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@alum.mit.edu>
> ---
> 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
    '
Junio C Hamano July 6, 2021, 7:42 p.m. UTC | #2 
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.
Eric Sunshine July 9, 2021, 6:11 a.m. UTC | #3 
On Tue, Jul 6, 2021 at 3:42 PM Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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.)
Junio C Hamano July 9, 2021, 3:23 p.m. UTC | #4 
Eric Sunshine <sunshine@sunshineco.com> writes:

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

Sure, but

    $ git worktree add --lock --reason=why-do-i-want-to-lock \
		--frotz --reason=why-do-i-want-to-frotz

would work just fine, no?
Eric Sunshine July 10, 2021, 7:11 a.m. UTC | #5 
On Fri, Jul 9, 2021 at 11:23 AM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > "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.
>
> Sure, but
>
>     $ git worktree add --lock --reason=why-do-i-want-to-lock \
>                 --frotz --reason=why-do-i-want-to-frotz
>
> would work just fine, no?

Yes, taking the stance that option order is significant would be workable.
diff mbox series

Patch

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index f1bb1fa5f5a..720663746ba 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -9,7 +9,7 @@  git-worktree - Manage multiple working trees
 SYNOPSIS
 --------
 [verse]
-'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b <new-branch>] <path> [<commit-ish>]
+'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] [-b <new-branch>] <path> [<commit-ish>]
 'git worktree list' [--porcelain]
 'git worktree lock' [--reason <string>] <worktree>
 'git worktree move' <worktree> <new-path>
@@ -242,7 +242,7 @@  With `list`, annotate missing working trees as prunable if they are
 older than `<time>`.
 
 --reason <string>::
-	With `lock`, an explanation why the working tree is locked.
+	With `lock` or with `add --lock`, an explanation why the working tree is locked.
 
 <worktree>::
 	Working trees can be identified by path, either relative or
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 976bf8ed063..9f890af7243 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -31,6 +31,7 @@  struct add_opts {
 	int quiet;
 	int checkout;
 	int keep_locked;
+	const char *lock_reason;
 };
 
 static int show_only;
@@ -302,10 +303,15 @@  static int add_worktree(const char *path, const char *refname,
 	 * after the preparation is over.
 	 */
 	strbuf_addf(&sb, "%s/locked", sb_repo.buf);
-	if (!opts->keep_locked)
+	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"));
+	}
 
 	strbuf_addf(&sb_git, "%s/.git", path);
 	if (safe_create_leading_directories_const(sb_git.buf))
@@ -486,6 +492,8 @@  static int add(int ac, const char **av, const char *prefix)
 		OPT_BOOL('d', "detach", &opts.detach, N_("detach HEAD at named commit")),
 		OPT_BOOL(0, "checkout", &opts.checkout, N_("populate the new working tree")),
 		OPT_BOOL(0, "lock", &opts.keep_locked, N_("keep the new working tree locked")),
+		OPT_STRING(0, "reason", &opts.lock_reason, N_("string"),
+			   N_("reason for locking")),
 		OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
 		OPT_PASSTHRU(0, "track", &opt_track, NULL,
 			     N_("set up tracking mode (see git-branch(1))"),
@@ -500,6 +508,8 @@  static int add(int ac, const char **av, const char *prefix)
 	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
 	if (!!opts.detach + !!new_branch + !!new_branch_force > 1)
 		die(_("-b, -B, and --detach are mutually exclusive"));
+	if (opts.lock_reason && !opts.keep_locked)
+		die(_("--reason requires --lock"));
 	if (ac < 1 || ac > 2)
 		usage_with_options(worktree_usage, options);
 
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index 96dfca15542..1f432d0f7d7 100755
--- 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
 '
 
+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
+'
+
+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_success '"add" worktree from a subdir' '
 	(
 		mkdir sub &&