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

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

[Stephen Manz via GitGitGadget]

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(-)

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

base-commit: 670b81a890388c60b7032a4f5b879f2ece8c4558
-- 
gitgitgadget

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

[Eric Sunshine]

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
    '

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

[Junio C Hamano]

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.

[PATCH v2 0/3] worktree: teach add to accept --reason with --lock

[Stephen Manz via GitGitGadget]

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.

Changes since v1:

 * Split changes into 3 commits. The first commit is removal of git
   rev-parse in the test above the ones I'm adding. The second is wrapping
   the "added with --lock" string with _() to mark it for translation. The
   third commit is the main change.
 * Reworked the if-else-if-else to if-else if-else
 * Added test_when_finished ... command to unlock the working tree
 * Changed test_expect_failure to test_expect_success and embedded
   test_must_fail and test_path_is_missing commands

Note: I don't see how to disambiguate --lock with no --reason from no --lock
at all. I still think that the original keep_locked boolean is needed along
with the new lock_reason char array. If I don't add lock_reason and change
keep_locked to a char array, it will start as NULL. But it will remain NULL
if --lock alone is given or if --lock isn't given at all.

Stephen Manz (3):
  t2400: remove unneeded `git rev-parse` from '"add" worktree with lock'
    test
  worktree: default lock string should be marked with `_()` for
    translation
  worktree: teach `add` to accept --reason <string> with --lock

 Documentation/git-worktree.txt |  4 ++--
 builtin/worktree.c             |  9 ++++++++-
 t/t2400-worktree-add.sh        | 14 +++++++++++++-
 3 files changed, 23 insertions(+), 4 deletions(-)


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

Range-diff vs v1:

 -:  ----------- > 1:  5459e5bb421 t2400: remove unneeded `git rev-parse` from '"add" worktree with lock' test
 -:  ----------- > 2:  30196cc9369 worktree: default lock string should be marked with `_()` for translation
 1:  233a580b212 ! 3:  4d17b31921a worktree: teach `add` to accept --reason <string> with --lock
     @@ builtin/worktree.c: struct add_opts {
       
       static int show_only;
      @@ builtin/worktree.c: 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) {
     + 	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"));
     -+	}
     ++	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))
      @@ builtin/worktree.c: 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")),
     @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix)
       
      
       ## t/t2400-worktree-add.sh ##
     -@@ t/t2400-worktree-add.sh: 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 &&
     +@@ t/t2400-worktree-add.sh: test_expect_success '"add" worktree with lock' '
       	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_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
      +'
      +
     -+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 with reason but no lock' '
     ++	test_must_fail git worktree add --detach --reason "why not" here-with-reason-only main &&
     ++	test_path_is_missing .git/worktrees/here-with-reason-only/locked
      +'
      +
       test_expect_success '"add" worktree from a subdir' '

-- 
gitgitgadget

[PATCH v2 1/3] t2400: remove unneeded `git rev-parse` from '"add" worktree with lock' test

[Stephen Manz via GitGitGadget]

From: Stephen Manz <smanz@alum.mit.edu>

It must have come from a copy-paste of another test

Signed-off-by: Stephen Manz <smanz@alum.mit.edu>
---
 t/t2400-worktree-add.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index 96dfca15542..874a61dbfa7 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -67,7 +67,6 @@ 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
 '
-- 
gitgitgadget

[PATCH v2 2/3] worktree: default lock string should be marked with `_()` for translation

[Stephen Manz via GitGitGadget]

From: Stephen Manz <smanz@alum.mit.edu>

Signed-off-by: Stephen Manz <smanz@alum.mit.edu>
---
 builtin/worktree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 976bf8ed063..448ec69e745 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -305,7 +305,7 @@ 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");
+		write_file(sb.buf, _("added with --lock"));
 
 	strbuf_addf(&sb_git, "%s/.git", path);
 	if (safe_create_leading_directories_const(sb_git.buf))
-- 
gitgitgadget

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

[Stephen Manz via GitGitGadget]

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>
---
 Documentation/git-worktree.txt |  4 ++--
 builtin/worktree.c             |  7 +++++++
 t/t2400-worktree-add.sh        | 13 +++++++++++++
 3 files changed, 22 insertions(+), 2 deletions(-)

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 448ec69e745..074169508d0 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;
@@ -304,6 +305,8 @@ 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");
+	else if (opts->lock_reason)
+		write_file(sb.buf, "%s", opts->lock_reason);
 	else
 		write_file(sb.buf, _("added with --lock"));
 
@@ -486,6 +489,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 +505,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 874a61dbfa7..de904448e59 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -71,6 +71,19 @@ test_expect_success '"add" worktree with lock' '
 	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_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
+'
+
+test_expect_success '"add" worktree with reason but no lock' '
+	test_must_fail git worktree add --detach --reason "why not" here-with-reason-only main &&
+	test_path_is_missing .git/worktrees/here-with-reason-only/locked
+'
+
 test_expect_success '"add" worktree from a subdir' '
 	(
 		mkdir sub &&
-- 
gitgitgadget

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

[Eric Sunshine]

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

Re: [PATCH v2 2/3] worktree: default lock string should be marked with `_()` for translation

[Eric Sunshine]

On Thu, Jul 8, 2021 at 11:50 AM Stephen Manz via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -305,7 +305,7 @@ 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");
> +               write_file(sb.buf, _("added with --lock"));

Makes perfect sense, though the "initializing" string in the `then`
arm probably deserves the same treatment:

    write_file(sb.buf, _("initializing"));

Re: [PATCH v2 0/3] worktree: teach add to accept --reason with --lock

[Eric Sunshine]

On Thu, Jul 08, 2021 at 03:50:40PM +0000, Stephen Manz via GitGitGadget 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.
>
> Changes since v1:
>
>  * Split changes into 3 commits. The first commit is removal of git
>    rev-parse in the test above the ones I'm adding. The second is wrapping
>    the "added with --lock" string with _() to mark it for translation. The
>    third commit is the main change.
>  * Reworked the if-else-if-else to if-else if-else
>  * Added test_when_finished ... command to unlock the working tree
>  * Changed test_expect_failure to test_expect_success and embedded
>    test_must_fail and test_path_is_missing commands

Thanks, all these changes make sense. Aside from a missing `_()` in
patch [2/3] mentioned in my review of that patch, everything looks
fine.

> Note: I don't see how to disambiguate --lock with no --reason from no --lock
> at all. I still think that the original keep_locked boolean is needed along
> with the new lock_reason char array. If I don't add lock_reason and change
> keep_locked to a char array, it will start as NULL. But it will remain NULL
> if --lock alone is given or if --lock isn't given at all.

The reason I suggested re-purposing `add_opts.keep_locked` is to avoid
polluting that structure with members having overlapping meaning, thus
reducing the confusion-factor for clients of that structure (assuming
that a tri-state `keep_locked` is indeed less confusing). That doesn't
preclude adding a new variable or two local to the `add()` function to
facilitate keeping `add_opts` pure. For instance, it might be as
simple as the below patch.

Anyhow, it's a minor suggestion and the sort of thing which can be
dealt with later if someone deems it important. Moreover, it's
subjective enough that it should not be a blocker for this patch
series if you don't do it that way. As mentioned above -- missing
`_()` aside -- the entire series looks reasonable as long as people
feel the UI choice is sound. (I, personally, still favor
`--lock[=<reason>]`, but I'm just one reviewer...)

--- >8 ---

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 976bf8ed06..22df2b60f2 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -30,7 +30,7 @@ struct add_opts {
 	int detach;
 	int quiet;
 	int checkout;
-	int keep_locked;
+	const char *keep_locked;
 };
 
 static int show_only;
@@ -304,6 +304,8 @@ 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");
+	else if (*opts->keep_locked)
+		write_file(sb.buf, "%s", opts->keep_locked);
 	else
 		write_file(sb.buf, "added with --lock");
 
@@ -475,6 +477,8 @@ static int add(int ac, const char **av, const char *prefix)
 	const char *branch;
 	const char *new_branch = NULL;
 	const char *opt_track = NULL;
+	int keep_locked = 0;
+	const char *lock_reason = NULL;
 	struct option options[] = {
 		OPT__FORCE(&opts.force,
 			   N_("checkout <branch> even if already checked out in other worktree"),
@@ -485,7 +489,9 @@ static int add(int ac, const char **av, const char *prefix)
 			   N_("create or reset a branch")),
 		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_BOOL(0, "lock", &keep_locked, N_("keep the new working tree locked")),
+		OPT_STRING(0, "reason", &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 +506,10 @@ 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 (lock_reason && !keep_locked)
+		die(_("--reason requires --lock"));
+	if (keep_locked)
+		opts.keep_locked = lock_reason ? lock_reason : "";
 	if (ac < 1 || ac > 2)
 		usage_with_options(worktree_usage, options);
 
-- 
2.32.0.372.g085311d9fa

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

[Junio C Hamano]

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?

Re: [PATCH v2 2/3] worktree: default lock string should be marked with `_()` for translation

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Thu, Jul 8, 2021 at 11:50 AM Stephen Manz via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> @@ -305,7 +305,7 @@ 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");
>> +               write_file(sb.buf, _("added with --lock"));
>
> Makes perfect sense, though the "initializing" string in the `then`
> arm probably deserves the same treatment:
>
>     write_file(sb.buf, _("initializing"));

Good eyes.

Re: [PATCH v2 0/3] worktree: teach add to accept --reason with --lock

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> The reason I suggested re-purposing `add_opts.keep_locked` is to avoid
> polluting that structure with members having overlapping meaning, thus
> reducing the confusion-factor for clients of that structure (assuming
> that a tri-state `keep_locked` is indeed less confusing). That doesn't
> preclude adding a new variable or two local to the `add()` function to
> facilitate keeping `add_opts` pure. For instance, it might be as
> simple as the below patch.

True.  It is less trivial to construct the command line option
parser so that --reason=<why> and --lock can be given in any order
(e.g. they no longer can be a simple OPT_BOOL and OPT_STRING that
can be given independently but needs some postprocessing like your
patch did), but it is not rocket science and keeping add_opts struct
leaner will reduce the risk of runtime confusion, I would think, but
at the same time, the room for runtime confusion would probably be
minor to begin with---so I am fine, if the coder cannot cleanly
write the option parser to do so, with the code as posted.

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

[Eric Sunshine]

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.

Re: [PATCH v2 0/3] worktree: teach add to accept --reason with --lock

[Eric Sunshine]

On Fri, Jul 9, 2021 at 12:03 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > The reason I suggested re-purposing `add_opts.keep_locked` is to avoid
> > polluting that structure with members having overlapping meaning, thus
> > reducing the confusion-factor for clients of that structure (assuming
> > that a tri-state `keep_locked` is indeed less confusing). That doesn't
> > preclude adding a new variable or two local to the `add()` function to
> > facilitate keeping `add_opts` pure. For instance, it might be as
> > simple as the below patch.
>
> True.  It is less trivial to construct the command line option
> parser so that --reason=<why> and --lock can be given in any order
> (e.g. they no longer can be a simple OPT_BOOL and OPT_STRING that
> can be given independently but needs some postprocessing like your
> patch did), but it is not rocket science and keeping add_opts struct
> leaner will reduce the risk of runtime confusion, I would think, but
> at the same time, the room for runtime confusion would probably be
> minor to begin with---so I am fine, if the coder cannot cleanly
> write the option parser to do so, with the code as posted.

Although the leaner approach seems "simpler" and more obvious to me,
it is subjective, and I can see how other people might find a
tri-state add_opts::keep_locked confusing. So, I'm fine with it either
way and don't consider it a blocker at all.

[PATCH v3 0/3] worktree: teach add to accept --reason with --lock

[Stephen Manz via GitGitGadget]

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.

Changes since v2:

 * Updated first of the 3 commits to include a step in the test to unlock
   the worktree when test completes and modified commit message accordingly.
 * Updated second commit to wrap "initializing" string with _() to mark it
   for translation, as requested. I had originally opted not to mark it,
   since, when --lock is not given, file locked gets removed after the
   working tree is populated. Thus, it's not really user-facing. Modified
   the commit message accordingly.
 * Updated the third commit to have no new lock_reason member of struct
   add_opts and changed type of member keep_locked from int to const char *,
   as suggested.

The file, .git/worktrees/name-of-worktree/locked, is created even if --lock
isn't given, although only temporarily. In this instance, "initializing" is
written to the file, but the file is removed at the end of the add-worktree
operation. My goal was to maintain this behavior,

Changes since v1:

 * Split changes into 3 commits. The first commit is removal of git
   rev-parse in the test above the ones I'm adding. The second is wrapping
   the "added with --lock" string with _() to mark it for translation. The
   third commit is the main change.
 * Reworked the if-else-if-else to if-else if-else
 * Added test_when_finished ... command to unlock the working tree
 * Changed test_expect_failure to test_expect_success and embedded
   test_must_fail and test_path_is_missing commands

Note: I don't see how to disambiguate --lock with no --reason from no --lock
at all. I still think that the original keep_locked boolean is needed along
with the new lock_reason char array. If I don't add lock_reason and change
keep_locked to a char array, it will start as NULL. But it will remain NULL
if --lock alone is given or if --lock isn't given at all.

cc: Eric Sunshine sunshine@sunshineco.com

Stephen Manz (3):
  t2400: clean up '"add" worktree with lock' test
  worktree: mark lock strings with `_()` for translation
  worktree: teach `add` to accept --reason <string> with --lock

 Documentation/git-worktree.txt |  4 ++--
 builtin/worktree.c             | 21 ++++++++++++++++-----
 t/t2400-worktree-add.sh        | 15 ++++++++++++++-
 3 files changed, 32 insertions(+), 8 deletions(-)


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

Range-diff vs v2:

 1:  5459e5bb421 ! 1:  5618933279d t2400: remove unneeded `git rev-parse` from '"add" worktree with lock' test
     @@ Metadata
      Author: Stephen Manz <smanz@alum.mit.edu>
      
       ## Commit message ##
     -    t2400: remove unneeded `git rev-parse` from '"add" worktree with lock' test
     +    t2400: clean up '"add" worktree with lock' test
      
     -    It must have come from a copy-paste of another test
     +    - remove unneeded `git rev-parse` which must have come from a copy-paste
     +      of another test
     +    - unlock the worktree with test_when_finished
      
          Signed-off-by: Stephen Manz <smanz@alum.mit.edu>
      
     @@ t/t2400-worktree-add.sh: 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_when_finished "git worktree unlock here-with-lock || :" &&
       	test -f .git/worktrees/here-with-lock/locked
       '
     + 
 2:  30196cc9369 ! 2:  ceb7a58b004 worktree: default lock string should be marked with `_()` for translation
     @@ Metadata
      Author: Stephen Manz <smanz@alum.mit.edu>
      
       ## Commit message ##
     -    worktree: default lock string should be marked with `_()` for translation
     +    worktree: mark lock strings with `_()` for translation
     +
     +    - default lock string, "added with --lock"
     +    - temporary lock string, "initializing"
      
          Signed-off-by: Stephen Manz <smanz@alum.mit.edu>
      
       ## builtin/worktree.c ##
      @@ builtin/worktree.c: 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");
     +-		write_file(sb.buf, "initializing");
     ++		write_file(sb.buf, _("initializing"));
       	else
      -		write_file(sb.buf, "added with --lock");
      +		write_file(sb.buf, _("added with --lock"));
 3:  4d17b31921a ! 3:  9a414a3078b worktree: teach `add` to accept --reason <string> with --lock
     @@ Documentation/git-worktree.txt: With `list`, annotate missing working trees as p
      
       ## builtin/worktree.c ##
      @@ builtin/worktree.c: struct add_opts {
     + 	int detach;
       	int quiet;
       	int checkout;
     - 	int keep_locked;
     -+	const char *lock_reason;
     +-	int keep_locked;
     ++	const char *keep_locked;
       };
       
       static int show_only;
      @@ builtin/worktree.c: 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)
     - 		write_file(sb.buf, "initializing");
     -+	else if (opts->lock_reason)
     -+		write_file(sb.buf, "%s", opts->lock_reason);
     +-	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, _("added with --lock"));
     ++		write_file(sb.buf, _("initializing"));
       
     + 	strbuf_addf(&sb_git, "%s/.git", path);
     + 	if (safe_create_leading_directories_const(sb_git.buf))
      @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix)
     + 	const char *branch;
     + 	const char *new_branch = NULL;
     + 	const char *opt_track = NULL;
     ++	const char *lock_reason = NULL;
     ++	int keep_locked = 0;
     + 	struct option options[] = {
     + 		OPT__FORCE(&opts.force,
     + 			   N_("checkout <branch> even if already checked out in other worktree"),
     +@@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix)
     + 			   N_("create or reset a branch")),
       		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"),
     +-		OPT_BOOL(0, "lock", &opts.keep_locked, N_("keep the new working tree locked")),
     ++		OPT_BOOL(0, "lock", &keep_locked, N_("keep the new working tree locked")),
     ++		OPT_STRING(0, "reason", &lock_reason, N_("string"),
      +			   N_("reason for locking")),
       		OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
       		OPT_PASSTHRU(0, "track", &opt_track, NULL,
     @@ builtin/worktree.c: 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)
     ++	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");
     ++
       	if (ac < 1 || ac > 2)
       		usage_with_options(worktree_usage, options);
       

-- 
gitgitgadget

[PATCH v3 1/3] t2400: clean up '"add" worktree with lock' test

[Stephen Manz via GitGitGadget]

From: Stephen Manz <smanz@alum.mit.edu>

- remove unneeded `git rev-parse` which must have come from a copy-paste
  of another test
- unlock the worktree with test_when_finished

Signed-off-by: Stephen Manz <smanz@alum.mit.edu>
---
 t/t2400-worktree-add.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index 96dfca15542..93d3795cab9 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -67,8 +67,8 @@ 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_when_finished "git worktree unlock here-with-lock || :" &&
 	test -f .git/worktrees/here-with-lock/locked
 '
 
-- 
gitgitgadget

[PATCH v3 2/3] worktree: mark lock strings with `_()` for translation

[Stephen Manz via GitGitGadget]

From: Stephen Manz <smanz@alum.mit.edu>

- default lock string, "added with --lock"
- temporary lock string, "initializing"

Signed-off-by: Stephen Manz <smanz@alum.mit.edu>
---
 builtin/worktree.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 976bf8ed063..4829b9507ff 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -303,9 +303,9 @@ 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");
+		write_file(sb.buf, _("initializing"));
 	else
-		write_file(sb.buf, "added with --lock");
+		write_file(sb.buf, _("added with --lock"));
 
 	strbuf_addf(&sb_git, "%s/.git", path);
 	if (safe_create_leading_directories_const(sb_git.buf))
-- 
gitgitgadget

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

[Stephen Manz via GitGitGadget]

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>
---
 Documentation/git-worktree.txt |  4 ++--
 builtin/worktree.c             | 21 ++++++++++++++++-----
 t/t2400-worktree-add.sh        | 13 +++++++++++++
 3 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 66e67e6cbfa..8a7cbdd19c1 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 4829b9507ff..0d0a80da61f 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -30,7 +30,7 @@ struct add_opts {
 	int detach;
 	int quiet;
 	int checkout;
-	int keep_locked;
+	const char *keep_locked;
 };
 
 static int show_only;
@@ -302,10 +302,10 @@ 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)
-		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"));
 
 	strbuf_addf(&sb_git, "%s/.git", path);
 	if (safe_create_leading_directories_const(sb_git.buf))
@@ -475,6 +475,8 @@ static int add(int ac, const char **av, const char *prefix)
 	const char *branch;
 	const char *new_branch = NULL;
 	const char *opt_track = NULL;
+	const char *lock_reason = NULL;
+	int keep_locked = 0;
 	struct option options[] = {
 		OPT__FORCE(&opts.force,
 			   N_("checkout <branch> even if already checked out in other worktree"),
@@ -485,7 +487,9 @@ static int add(int ac, const char **av, const char *prefix)
 			   N_("create or reset a branch")),
 		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_BOOL(0, "lock", &keep_locked, N_("keep the new working tree locked")),
+		OPT_STRING(0, "reason", &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 +504,13 @@ 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 (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");
+
 	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 93d3795cab9..7590064076a 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -72,6 +72,19 @@ test_expect_success '"add" worktree with lock' '
 	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_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
+'
+
+test_expect_success '"add" worktree with reason but no lock' '
+	test_must_fail git worktree add --detach --reason "why not" here-with-reason-only main &&
+	test_path_is_missing .git/worktrees/here-with-reason-only/locked
+'
+
 test_expect_success '"add" worktree from a subdir' '
 	(
 		mkdir sub &&
-- 
gitgitgadget

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

[Eric Sunshine]

On Sat, Jul 10, 2021 at 8:27 PM 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.
>
> Signed-off-by: Stephen Manz <smanz@alum.mit.edu>
> ---
> 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.

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

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

Re: [PATCH v3 0/3] worktree: teach add to accept --reason with --lock

[Eric Sunshine]

On Sat, Jul 10, 2021 at 8:27 PM Stephen Manz via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Changes since v2:
>  * Updated second commit to wrap "initializing" string with _() to mark it
>    for translation, as requested. I had originally opted not to mark it,
>    since, when --lock is not given, file locked gets removed after the
>    working tree is populated. Thus, it's not really user-facing. Modified
>    the commit message accordingly.

True. Makes sense. A side-benefit of wrapping it in `_(...)`, though,
is that it saves the next person who touches that code from wondering
why only one of the messages has been localized, so it's nice to see
the revised patch making both strings localizable.

I read over the series and played around with the implementation. It
all looks good. With or without addressing the couple extremely minor
comments I left on patch [3/3] regarding one of the tests, consider
this series:

    Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>

Thanks.

[PATCH v4 0/3] worktree: teach add to accept --reason with --lock

[Stephen Manz via GitGitGadget]

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.

Changes since v3:

 * Updated test to define a shell variable, lock_reason, set to "why not".
   Expand the variable to use with --reason and to echo to file, expected.
   This avoids the spacing comment made by Eric Sunshine.

Changes since v2:

 * Updated first of the 3 commits to include a step in the test to unlock
   the worktree when test completes and modified commit message accordingly.
 * Updated second commit to wrap "initializing" string with _() to mark it
   for translation, as requested. I had originally opted not to mark it,
   since, when --lock is not given, file locked gets removed after the
   working tree is populated. Thus, it's not really user-facing. Modified
   the commit message accordingly.
 * Updated the third commit to have no new lock_reason member of struct
   add_opts and changed type of member keep_locked from int to const char *,
   as suggested.

The file, .git/worktrees/name-of-worktree/locked, is created even if --lock
isn't given, although only temporarily. In this instance, "initializing" is
written to the file, but the file is removed at the end of the add-worktree
operation. My goal was to maintain this behavior and is the reason my
post-parsing code doesn't quite match the suggested patch.

Changes since v1:

 * Split changes into 3 commits. The first commit is removal of git
   rev-parse in the test above the ones I'm adding. The second is wrapping
   the "added with --lock" string with _() to mark it for translation. The
   third commit is the main change.
 * Reworked the if-else-if-else to if-else if-else
 * Added test_when_finished ... command to unlock the working tree
 * Changed test_expect_failure to test_expect_success and embedded
   test_must_fail and test_path_is_missing commands

Note: I don't see how to disambiguate --lock with no --reason from no --lock
at all. I still think that the original keep_locked boolean is needed along
with the new lock_reason char array. If I don't add lock_reason and change
keep_locked to a char array, it will start as NULL. But it will remain NULL
if --lock alone is given or if --lock isn't given at all.

cc: Eric Sunshine sunshine@sunshineco.com

Stephen Manz (3):
  t2400: clean up '"add" worktree with lock' test
  worktree: mark lock strings with `_()` for translation
  worktree: teach `add` to accept --reason <string> with --lock

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


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

Range-diff vs v3:

 1:  5618933279d = 1:  5618933279d t2400: clean up '"add" worktree with lock' test
 2:  ceb7a58b004 = 2:  ceb7a58b004 worktree: mark lock strings with `_()` for translation
 3:  9a414a3078b ! 3:  4b6bb50d3d6 worktree: teach `add` to accept --reason <string> with --lock
     @@ t/t2400-worktree-add.sh: 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 &&
     ++	lock_reason="why not" &&
     ++	git worktree add --detach --lock --reason "$lock_reason" 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 &&
     ++	echo "$lock_reason" >expect &&
      +	test_cmp expect .git/worktrees/here-with-lock-reason/locked
      +'
      +

-- 
gitgitgadget

[PATCH v4 1/3] t2400: clean up '"add" worktree with lock' test

[Stephen Manz via GitGitGadget]

From: Stephen Manz <smanz@alum.mit.edu>

- remove unneeded `git rev-parse` which must have come from a copy-paste
  of another test
- unlock the worktree with test_when_finished

Signed-off-by: Stephen Manz <smanz@alum.mit.edu>
---
 t/t2400-worktree-add.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index 96dfca15542..93d3795cab9 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -67,8 +67,8 @@ 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_when_finished "git worktree unlock here-with-lock || :" &&
 	test -f .git/worktrees/here-with-lock/locked
 '
 
-- 
gitgitgadget

[PATCH v4 2/3] worktree: mark lock strings with `_()` for translation

[Stephen Manz via GitGitGadget]

From: Stephen Manz <smanz@alum.mit.edu>

- default lock string, "added with --lock"
- temporary lock string, "initializing"

Signed-off-by: Stephen Manz <smanz@alum.mit.edu>
---
 builtin/worktree.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 976bf8ed063..4829b9507ff 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -303,9 +303,9 @@ 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");
+		write_file(sb.buf, _("initializing"));
 	else
-		write_file(sb.buf, "added with --lock");
+		write_file(sb.buf, _("added with --lock"));
 
 	strbuf_addf(&sb_git, "%s/.git", path);
 	if (safe_create_leading_directories_const(sb_git.buf))
-- 
gitgitgadget

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

[Stephen Manz via GitGitGadget]

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>
---
 Documentation/git-worktree.txt |  4 ++--
 builtin/worktree.c             | 21 ++++++++++++++++-----
 t/t2400-worktree-add.sh        | 14 ++++++++++++++
 3 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 66e67e6cbfa..8a7cbdd19c1 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 4829b9507ff..0d0a80da61f 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -30,7 +30,7 @@ struct add_opts {
 	int detach;
 	int quiet;
 	int checkout;
-	int keep_locked;
+	const char *keep_locked;
 };
 
 static int show_only;
@@ -302,10 +302,10 @@ 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)
-		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"));
 
 	strbuf_addf(&sb_git, "%s/.git", path);
 	if (safe_create_leading_directories_const(sb_git.buf))
@@ -475,6 +475,8 @@ static int add(int ac, const char **av, const char *prefix)
 	const char *branch;
 	const char *new_branch = NULL;
 	const char *opt_track = NULL;
+	const char *lock_reason = NULL;
+	int keep_locked = 0;
 	struct option options[] = {
 		OPT__FORCE(&opts.force,
 			   N_("checkout <branch> even if already checked out in other worktree"),
@@ -485,7 +487,9 @@ static int add(int ac, const char **av, const char *prefix)
 			   N_("create or reset a branch")),
 		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_BOOL(0, "lock", &keep_locked, N_("keep the new working tree locked")),
+		OPT_STRING(0, "reason", &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 +504,13 @@ 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 (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");
+
 	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 93d3795cab9..37ad79470fb 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -72,6 +72,20 @@ test_expect_success '"add" worktree with lock' '
 	test -f .git/worktrees/here-with-lock/locked
 '
 
+test_expect_success '"add" worktree with lock and reason' '
+	lock_reason="why not" &&
+	git worktree add --detach --lock --reason "$lock_reason" 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 "$lock_reason" >expect &&
+	test_cmp expect .git/worktrees/here-with-lock-reason/locked
+'
+
+test_expect_success '"add" worktree with reason but no lock' '
+	test_must_fail git worktree add --detach --reason "why not" here-with-reason-only main &&
+	test_path_is_missing .git/worktrees/here-with-reason-only/locked
+'
+
 test_expect_success '"add" worktree from a subdir' '
 	(
 		mkdir sub &&
-- 
gitgitgadget

Re: [PATCH v4 0/3] worktree: teach add to accept --reason with --lock

[Eric Sunshine]

On Wed, Jul 14, 2021 at 10:32 PM 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.
>
> Changes since v3:
>
>  * Updated test to define a shell variable, lock_reason, set to "why not".
>    Expand the variable to use with --reason and to echo to file, expected.
>    This avoids the spacing comment made by Eric Sunshine.

Thanks, these changes look fine to me. I don't know whether or not
Junio has merged this series down to his "next" branch yet locally,
though. If he has, then he may not pick up your v4, and this minor
cleanup change could instead be done as a standalone patch atop v3 (or
could be dropped since v3 was probably "good enough").

In any case, this series is still Reviewed-by: me.

Re: [PATCH v4 0/3] worktree: teach add to accept --reason with --lock

[Eric Sunshine]

On Thu, Jul 15, 2021 at 1:17 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Jul 14, 2021 at 10:32 PM Stephen Manz via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > Changes since v3:
> >
> >  * Updated test to define a shell variable, lock_reason, set to "why not".
> >    Expand the variable to use with --reason and to echo to file, expected.
> >    This avoids the spacing comment made by Eric Sunshine.
>
> Thanks, these changes look fine to me. I don't know whether or not
> Junio has merged this series down to his "next" branch yet locally,
> though. If he has, then he may not pick up your v4, and this minor
> cleanup change could instead be done as a standalone patch atop v3 (or
> could be dropped since v3 was probably "good enough").

It looks like Junio did pick up v4, so no need for a standalone
cleanup patch atop v3.