{"thread":{"id":"56053","subject":"[PATCH] worktree: teach `add` to accept --reason <string> with --lock","startedAt":"2021-07-06T05:47:36Z","lastAt":"2021-07-17T00:15:12Z","messageCount":27,"participants":["Stephen Manz via GitGitGadget","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"429275","messageId":"pull.992.git.1625550451038.gitgitgadget@gmail.com","threadId":"56053","inReplyTo":null,"subject":"[PATCH] worktree: teach `add` to accept --reason <string> with --lock","fromName":"Stephen Manz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-06T05:47:30Z","receivedAt":"2021-07-06T05:47:36Z","isPatch":true,"sender":{"key":"name:Stephen Manz","avatar":null},"body":"From: Stephen Manz <smanz@alum.mit.edu>\n\nThe default reason stored in the lock file, \"added with --lock\",\nis unlikely to be what the user would have given in a separate\n`git worktree lock` command. Allowing `--reason` to be specified\nalong with `--lock` when adding a working tree gives the user control\nover the reason for locking without needing a second command.\n\nSigned-off-by: Stephen Manz <smanz@alum.mit.edu>\n---\n    worktree: teach add to accept --reason with --lock\n    \n    The default reason stored in the lock file, \"added with --lock\", is\n    unlikely to be what the user would have given in a separate git worktree\n    lock command. Allowing --reason to be specified along with --lock when\n    adding a working tree gives the user control over the reason for locking\n    without needing a second command.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-992%2FSRManz%2Flock_reason-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-992/SRManz/lock_reason-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/992\n\n Documentation/git-worktree.txt |  4 ++--\n builtin/worktree.c             | 16 +++++++++++++---\n t/t2400-worktree-add.sh        | 13 ++++++++++++-\n 3 files changed, 27 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex f1bb1fa5f5a..720663746ba 100644\n--- a/Documentation/git-worktree.txt\n+++ b/Documentation/git-worktree.txt\n@@ -9,7 +9,7 @@ git-worktree - Manage multiple working trees\n SYNOPSIS\n --------\n [verse]\n-'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b <new-branch>] <path> [<commit-ish>]\n+'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] [-b <new-branch>] <path> [<commit-ish>]\n 'git worktree list' [--porcelain]\n 'git worktree lock' [--reason <string>] <worktree>\n 'git worktree move' <worktree> <new-path>\n@@ -242,7 +242,7 @@ With `list`, annotate missing working trees as prunable if they are\n older than `<time>`.\n \n --reason <string>::\n-\tWith `lock`, an explanation why the working tree is locked.\n+\tWith `lock` or with `add --lock`, an explanation why the working tree is locked.\n \n <worktree>::\n \tWorking trees can be identified by path, either relative or\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 976bf8ed063..9f890af7243 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -31,6 +31,7 @@ struct add_opts {\n \tint quiet;\n \tint checkout;\n \tint keep_locked;\n+\tconst char *lock_reason;\n };\n \n static int show_only;\n@@ -302,10 +303,15 @@ static int add_worktree(const char *path, const char *refname,\n \t * after the preparation is over.\n \t */\n \tstrbuf_addf(&sb, \"%s/locked\", sb_repo.buf);\n-\tif (!opts->keep_locked)\n+\tif (!opts->keep_locked) {\n \t\twrite_file(sb.buf, \"initializing\");\n-\telse\n-\t\twrite_file(sb.buf, \"added with --lock\");\n+\t}\n+\telse {\n+\t\tif (opts->lock_reason)\n+\t\t\twrite_file(sb.buf, \"%s\", opts->lock_reason);\n+\t\telse\n+\t\t\twrite_file(sb.buf, _(\"added with --lock\"));\n+\t}\n \n \tstrbuf_addf(&sb_git, \"%s/.git\", path);\n \tif (safe_create_leading_directories_const(sb_git.buf))\n@@ -486,6 +492,8 @@ static int add(int ac, const char **av, const char *prefix)\n \t\tOPT_BOOL('d', \"detach\", &opts.detach, N_(\"detach HEAD at named commit\")),\n \t\tOPT_BOOL(0, \"checkout\", &opts.checkout, N_(\"populate the new working tree\")),\n \t\tOPT_BOOL(0, \"lock\", &opts.keep_locked, N_(\"keep the new working tree locked\")),\n+\t\tOPT_STRING(0, \"reason\", &opts.lock_reason, N_(\"string\"),\n+\t\t\t   N_(\"reason for locking\")),\n \t\tOPT__QUIET(&opts.quiet, N_(\"suppress progress reporting\")),\n \t\tOPT_PASSTHRU(0, \"track\", &opt_track, NULL,\n \t\t\t     N_(\"set up tracking mode (see git-branch(1))\"),\n@@ -500,6 +508,8 @@ static int add(int ac, const char **av, const char *prefix)\n \tac = parse_options(ac, av, prefix, options, worktree_usage, 0);\n \tif (!!opts.detach + !!new_branch + !!new_branch_force > 1)\n \t\tdie(_(\"-b, -B, and --detach are mutually exclusive\"));\n+\tif (opts.lock_reason && !opts.keep_locked)\n+\t\tdie(_(\"--reason requires --lock\"));\n \tif (ac < 1 || ac > 2)\n \t\tusage_with_options(worktree_usage, options);\n \ndiff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\nindex 96dfca15542..1f432d0f7d7 100755\n--- a/t/t2400-worktree-add.sh\n+++ b/t/t2400-worktree-add.sh\n@@ -67,11 +67,22 @@ test_expect_success '\"add\" worktree' '\n '\n \n test_expect_success '\"add\" worktree with lock' '\n-\tgit rev-parse HEAD >expect &&\n \tgit worktree add --detach --lock here-with-lock main &&\n \ttest -f .git/worktrees/here-with-lock/locked\n '\n \n+test_expect_success '\"add\" worktree with lock and reason' '\n+\tgit worktree add --detach --lock --reason \"why not\" here-with-lock-reason main &&\n+\ttest -f .git/worktrees/here-with-lock-reason/locked &&\n+\techo why not >expect &&\n+\ttest_cmp expect .git/worktrees/here-with-lock-reason/locked\n+'\n+\n+test_expect_failure '\"add\" worktree with reason but no lock' '\n+\tgit worktree add --detach --reason \"why not\" here-with-reason-only main &&\n+\ttest -f .git/worktrees/here-with-reason-only/locked\n+'\n+\n test_expect_success '\"add\" worktree from a subdir' '\n \t(\n \t\tmkdir sub &&\n\nbase-commit: 670b81a890388c60b7032a4f5b879f2ece8c4558\n-- \ngitgitgadget\n"},{"id":"429276","messageId":"CAPig+cSsPrQrP9xk8M8H339_NpYqKh1okeo1V-fAJ2zk3QeOjQ@mail.gmail.com","threadId":"56053","inReplyTo":"pull.992.git.1625550451038.gitgitgadget@gmail.com","subject":"Re: [PATCH] worktree: teach `add` to accept --reason <string> with --lock","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-07-06T06:19:43Z","receivedAt":"2021-07-06T06:19:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 6, 2021 at 1:47 AM Stephen Manz via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> The default reason stored in the lock file, \"added with --lock\",\n> is unlikely to be what the user would have given in a separate\n> `git worktree lock` command. Allowing `--reason` to be specified\n> along with `--lock` when adding a working tree gives the user control\n> over the reason for locking without needing a second command.\n\nThanks. I can see how the default lock reason in this case isn't very\ninteresting. In fact, I'd actually have expected the default reason to\nbe empty, just as it's empty when `git worktree lock` is invoked with\nno `--reason`. That might be an additional thing to \"fix\" at some\npoint by someone (or in this series if you'd like to tackle it).\n\nIt's nice to see both documentation and test updates along with the\ncode change. See below for a few comments...\n\n> Signed-off-by: Stephen Manz <smanz@alum.mit.edu>\n> ---\n> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\n> @@ -9,7 +9,7 @@ git-worktree - Manage multiple working trees\n> +'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] [-b <new-branch>] <path> [<commit-ish>]\n>\n>  --reason <string>::\n> +       With `lock` or with `add --lock`, an explanation why the working tree is locked.\n\nI realize that you're mimicking the interface of `git worktree lock`\nwhich accepts an optional `--reason`, but I'm wondering if the\nuser-experience might be improved by instead allowing `--lock` to\naccept the reason as an optional argument. For instance:\n\n    git worktree add --lock='just because' ...\n\nAside from the dissymmetry with `git worktree lock`, I haven't come up\nwith a reason that `--lock[=<reason>]` wouldn't be an improvement. But\nperhaps I'm not imaginative enough...\n\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> @@ -31,6 +31,7 @@ struct add_opts {\n>         int checkout;\n>         int keep_locked;\n> +       const char *lock_reason;\n>  };\n\nWhether or not we do go with the approach of allowing `--lock` to take\nthe reason as an optional argument, we don't really need two structure\nmembers here. Instead, we can repurpose `keep_locked` as a `const char\n*` which is NULL if `--lock` was not specified, otherwise non-NULL.\nFor the non-NULL case, it would be an empty (zero-length) string if\nthe optional `<reason>` was omitted, otherwise it would be the reason\ngiven. So, no need for a distinct `lock_reason` member.\n\n> @@ -302,10 +303,15 @@ static int add_worktree(const char *path, const char *refname,\n> +       if (!opts->keep_locked) {\n>                 write_file(sb.buf, \"initializing\");\n> -       else\n> -               write_file(sb.buf, \"added with --lock\");\n> +       }\n> +       else {\n> +               if (opts->lock_reason)\n> +                       write_file(sb.buf, \"%s\", opts->lock_reason);\n> +               else\n> +                       write_file(sb.buf, _(\"added with --lock\"));\n> +       }\n\nStyle on this project is to cuddle `else` with both braces:\n\n    } else  {\n\nHowever, in this case, it should probably just be a simple `else if`:\n\n    if (!opts->keep_locked)\n        write_file(sb.buf, \"initializing\");\n    else if (opts->lock_reason)\n        write_file(sb.buf, \"%s\", opts->lock_reason);\n    else\n        write_file(sb.buf, _(\"added with --lock\"));\n\n> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\n> @@ -67,11 +67,22 @@ test_expect_success '\"add\" worktree' '\n>  test_expect_success '\"add\" worktree with lock' '\n> -       git rev-parse HEAD >expect &&\n>         git worktree add --detach --lock here-with-lock main &&\n>         test -f .git/worktrees/here-with-lock/locked\n>  '\n\nDropping this unused code makes sense, though on this project we would\nnormally do it as a separate (probably preparatory) patch, thus making\nthis a two-patch series (at minimum).\n\n> +test_expect_success '\"add\" worktree with lock and reason' '\n> +       git worktree add --detach --lock --reason \"why not\" here-with-lock-reason main &&\n> +       test -f .git/worktrees/here-with-lock-reason/locked &&\n> +       echo why not >expect &&\n> +       test_cmp expect .git/worktrees/here-with-lock-reason/locked\n> +'\n\nTo make this a bit friendlier to other tests which come later in the\nscript, it might be a good idea to unlock this worktree here at its\nsource. To do so unconditionally, whether the test succeeds or fails,\nuse test_when_finished() just after placing the lock. So, something\nlike this:\n\n    git worktree add --detach --lock --reason ... &&\n    test_when_finished \"git worktree unlock here-with-lock-reason || :\" &&\n    test -f .git/worktrees/here-with-lock-reason/locked &&\n    ...\n\n> +test_expect_failure '\"add\" worktree with reason but no lock' '\n> +       git worktree add --detach --reason \"why not\" here-with-reason-only main &&\n> +       test -f .git/worktrees/here-with-reason-only/locked\n> +'\n\ntest_expect_failure() is for indicating known bugs or unimplemented\nfeatures, however, you did implement the check that `--reason`\nrequires `--lock`, so test_expect_failure() is not quite what you want\nhere. Instead, use test_must_fail() in the test body, and you want to\nmake sure that the `locked` file did not get created. So, something\nlike this:\n\n    test_expect_success '\"add\" worktree with reason but no lock' '\n        test_must_fail git worktree add --detach --reason ... &&\n        test_path_is_missing .git/worktrees/here-with-reason-only/locked\n    '\n"},{"id":"429330","messageId":"xmqq7di3jkki.fsf@gitster.g","threadId":"56053","inReplyTo":"CAPig+cSsPrQrP9xk8M8H339_NpYqKh1okeo1V-fAJ2zk3QeOjQ@mail.gmail.com","subject":"Re: [PATCH] worktree: teach `add` to accept --reason <string> with --lock","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-06T19:42:21Z","receivedAt":"2021-07-06T19:42:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n>>  --reason <string>::\n>> +       With `lock` or with `add --lock`, an explanation why the working tree is locked.\n>\n> I realize that you're mimicking the interface of `git worktree lock`\n> which accepts an optional `--reason`, but I'm wondering if the\n> user-experience might be improved by instead allowing `--lock` to\n> accept the reason as an optional argument. For instance:\n>\n>     git worktree add --lock='just because' ...\n\nThanks for thinking aloud, but I'd prefer the interface as posted,\nsimply because there is one less thing for users to remember.  The\njustification to lock is given with the --reason=<why> argument no\nmatter how you acquire the lock on a worktree.\n\n\n>> diff --git a/builtin/worktree.c b/builtin/worktree.c\n>> @@ -31,6 +31,7 @@ struct add_opts {\n>>         int checkout;\n>>         int keep_locked;\n>> +       const char *lock_reason;\n>>  };\n>\n> Whether or not we do go with the approach of allowing `--lock` to take\n> the reason as an optional argument, we don't really need two structure\n> members here. Instead, we can repurpose `keep_locked` as a `const char\n> *` which is NULL if `--lock` was not specified, otherwise non-NULL.\n\nMakes sense.\n\n> However, in this case, it should probably just be a simple `else if`:\n>\n>     if (!opts->keep_locked)\n>         write_file(sb.buf, \"initializing\");\n>     else if (opts->lock_reason)\n>         write_file(sb.buf, \"%s\", opts->lock_reason);\n>     else\n>         write_file(sb.buf, _(\"added with --lock\"));\n\nExcellent.\n\nThanks.\n"},{"id":"429494","messageId":"pull.992.v2.git.1625759443.gitgitgadget@gmail.com","threadId":"56053","inReplyTo":"pull.992.git.1625550451038.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] worktree: teach add to accept --reason with --lock","fromName":"Stephen Manz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-08T15:50:40Z","receivedAt":"2021-07-08T15:50:48Z","isPatch":true,"sender":{"key":"name:Stephen Manz","avatar":null},"body":"The default reason stored in the lock file, \"added with --lock\", is unlikely\nto be what the user would have given in a separate git worktree lock\ncommand. Allowing --reason to be specified along with --lock when adding a\nworking tree gives the user control over the reason for locking without\nneeding a second command.\n\nChanges since v1:\n\n * Split changes into 3 commits. The first commit is removal of git\n   rev-parse in the test above the ones I'm adding. The second is wrapping\n   the \"added with --lock\" string with _() to mark it for translation. The\n   third commit is the main change.\n * Reworked the if-else-if-else to if-else if-else\n * Added test_when_finished ... command to unlock the working tree\n * Changed test_expect_failure to test_expect_success and embedded\n   test_must_fail and test_path_is_missing commands\n\nNote: I don't see how to disambiguate --lock with no --reason from no --lock\nat all. I still think that the original keep_locked boolean is needed along\nwith the new lock_reason char array. If I don't add lock_reason and change\nkeep_locked to a char array, it will start as NULL. But it will remain NULL\nif --lock alone is given or if --lock isn't given at all.\n\nStephen Manz (3):\n  t2400: remove unneeded `git rev-parse` from '\"add\" worktree with lock'\n    test\n  worktree: default lock string should be marked with `_()` for\n    translation\n  worktree: teach `add` to accept --reason <string> with --lock\n\n Documentation/git-worktree.txt |  4 ++--\n builtin/worktree.c             |  9 ++++++++-\n t/t2400-worktree-add.sh        | 14 +++++++++++++-\n 3 files changed, 23 insertions(+), 4 deletions(-)\n\n\nbase-commit: 670b81a890388c60b7032a4f5b879f2ece8c4558\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-992%2FSRManz%2Flock_reason-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-992/SRManz/lock_reason-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/992\n\nRange-diff vs v1:\n\n -:  ----------- > 1:  5459e5bb421 t2400: remove unneeded `git rev-parse` from '\"add\" worktree with lock' test\n -:  ----------- > 2:  30196cc9369 worktree: default lock string should be marked with `_()` for translation\n 1:  233a580b212 ! 3:  4d17b31921a worktree: teach `add` to accept --reason <string> with --lock\n     @@ builtin/worktree.c: struct add_opts {\n       \n       static int show_only;\n      @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refname,\n     - \t * after the preparation is over.\n     - \t */\n       \tstrbuf_addf(&sb, \"%s/locked\", sb_repo.buf);\n     --\tif (!opts->keep_locked)\n     -+\tif (!opts->keep_locked) {\n     + \tif (!opts->keep_locked)\n       \t\twrite_file(sb.buf, \"initializing\");\n     --\telse\n     --\t\twrite_file(sb.buf, \"added with --lock\");\n     -+\t}\n     -+\telse {\n     -+\t\tif (opts->lock_reason)\n     -+\t\t\twrite_file(sb.buf, \"%s\", opts->lock_reason);\n     -+\t\telse\n     -+\t\t\twrite_file(sb.buf, _(\"added with --lock\"));\n     -+\t}\n     ++\telse if (opts->lock_reason)\n     ++\t\twrite_file(sb.buf, \"%s\", opts->lock_reason);\n     + \telse\n     + \t\twrite_file(sb.buf, _(\"added with --lock\"));\n       \n     - \tstrbuf_addf(&sb_git, \"%s/.git\", path);\n     - \tif (safe_create_leading_directories_const(sb_git.buf))\n      @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix)\n       \t\tOPT_BOOL('d', \"detach\", &opts.detach, N_(\"detach HEAD at named commit\")),\n       \t\tOPT_BOOL(0, \"checkout\", &opts.checkout, N_(\"populate the new working tree\")),\n     @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix)\n       \n      \n       ## t/t2400-worktree-add.sh ##\n     -@@ t/t2400-worktree-add.sh: test_expect_success '\"add\" worktree' '\n     - '\n     - \n     - test_expect_success '\"add\" worktree with lock' '\n     --\tgit rev-parse HEAD >expect &&\n     - \tgit worktree add --detach --lock here-with-lock main &&\n     +@@ t/t2400-worktree-add.sh: test_expect_success '\"add\" worktree with lock' '\n       \ttest -f .git/worktrees/here-with-lock/locked\n       '\n       \n      +test_expect_success '\"add\" worktree with lock and reason' '\n      +\tgit worktree add --detach --lock --reason \"why not\" here-with-lock-reason main &&\n     ++\ttest_when_finished \"git worktree unlock here-with-lock-reason || :\" &&\n      +\ttest -f .git/worktrees/here-with-lock-reason/locked &&\n      +\techo why not >expect &&\n      +\ttest_cmp expect .git/worktrees/here-with-lock-reason/locked\n      +'\n      +\n     -+test_expect_failure '\"add\" worktree with reason but no lock' '\n     -+\tgit worktree add --detach --reason \"why not\" here-with-reason-only main &&\n     -+\ttest -f .git/worktrees/here-with-reason-only/locked\n     ++test_expect_success '\"add\" worktree with reason but no lock' '\n     ++\ttest_must_fail git worktree add --detach --reason \"why not\" here-with-reason-only main &&\n     ++\ttest_path_is_missing .git/worktrees/here-with-reason-only/locked\n      +'\n      +\n       test_expect_success '\"add\" worktree from a subdir' '\n\n-- \ngitgitgadget\n"},{"id":"429495","messageId":"5459e5bb4212e47873d2862c4a96651c2f7d96b5.1625759443.git.gitgitgadget@gmail.com","threadId":"56053","inReplyTo":"pull.992.v2.git.1625759443.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] t2400: remove unneeded `git rev-parse` from '\"add\" worktree with lock' test","fromName":"Stephen Manz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-08T15:50:41Z","receivedAt":"2021-07-08T15:50:57Z","isPatch":true,"sender":{"key":"name:Stephen Manz","avatar":null},"body":"From: Stephen Manz <smanz@alum.mit.edu>\n\nIt must have come from a copy-paste of another test\n\nSigned-off-by: Stephen Manz <smanz@alum.mit.edu>\n---\n t/t2400-worktree-add.sh | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\nindex 96dfca15542..874a61dbfa7 100755\n--- a/t/t2400-worktree-add.sh\n+++ b/t/t2400-worktree-add.sh\n@@ -67,7 +67,6 @@ test_expect_success '\"add\" worktree' '\n '\n \n test_expect_success '\"add\" worktree with lock' '\n-\tgit rev-parse HEAD >expect &&\n \tgit worktree add --detach --lock here-with-lock main &&\n \ttest -f .git/worktrees/here-with-lock/locked\n '\n-- \ngitgitgadget\n\n"},{"id":"429496","messageId":"30196cc93697cd4a6a6881dcbb1073d19e96d8a0.1625759443.git.gitgitgadget@gmail.com","threadId":"56053","inReplyTo":"pull.992.v2.git.1625759443.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] worktree: default lock string should be marked with `_()` for translation","fromName":"Stephen Manz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-08T15:50:42Z","receivedAt":"2021-07-08T15:50:59Z","isPatch":true,"sender":{"key":"name:Stephen Manz","avatar":null},"body":"From: Stephen Manz <smanz@alum.mit.edu>\n\nSigned-off-by: Stephen Manz <smanz@alum.mit.edu>\n---\n builtin/worktree.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 976bf8ed063..448ec69e745 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -305,7 +305,7 @@ static int add_worktree(const char *path, const char *refname,\n \tif (!opts->keep_locked)\n \t\twrite_file(sb.buf, \"initializing\");\n \telse\n-\t\twrite_file(sb.buf, \"added with --lock\");\n+\t\twrite_file(sb.buf, _(\"added with --lock\"));\n \n \tstrbuf_addf(&sb_git, \"%s/.git\", path);\n \tif (safe_create_leading_directories_const(sb_git.buf))\n-- \ngitgitgadget\n\n"},{"id":"429497","messageId":"4d17b31921ac775ad512d4de094b4718f5dd2eb3.1625759443.git.gitgitgadget@gmail.com","threadId":"56053","inReplyTo":"pull.992.v2.git.1625759443.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] worktree: teach `add` to accept --reason <string> with --lock","fromName":"Stephen Manz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-08T15:50:43Z","receivedAt":"2021-07-08T15:51:01Z","isPatch":true,"sender":{"key":"name:Stephen Manz","avatar":null},"body":"From: Stephen Manz <smanz@alum.mit.edu>\n\nThe default reason stored in the lock file, \"added with --lock\",\nis unlikely to be what the user would have given in a separate\n`git worktree lock` command. Allowing `--reason` to be specified\nalong with `--lock` when adding a working tree gives the user control\nover the reason for locking without needing a second command.\n\nSigned-off-by: Stephen Manz <smanz@alum.mit.edu>\n---\n Documentation/git-worktree.txt |  4 ++--\n builtin/worktree.c             |  7 +++++++\n t/t2400-worktree-add.sh        | 13 +++++++++++++\n 3 files changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex f1bb1fa5f5a..720663746ba 100644\n--- a/Documentation/git-worktree.txt\n+++ b/Documentation/git-worktree.txt\n@@ -9,7 +9,7 @@ git-worktree - Manage multiple working trees\n SYNOPSIS\n --------\n [verse]\n-'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b <new-branch>] <path> [<commit-ish>]\n+'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] [-b <new-branch>] <path> [<commit-ish>]\n 'git worktree list' [--porcelain]\n 'git worktree lock' [--reason <string>] <worktree>\n 'git worktree move' <worktree> <new-path>\n@@ -242,7 +242,7 @@ With `list`, annotate missing working trees as prunable if they are\n older than `<time>`.\n \n --reason <string>::\n-\tWith `lock`, an explanation why the working tree is locked.\n+\tWith `lock` or with `add --lock`, an explanation why the working tree is locked.\n \n <worktree>::\n \tWorking trees can be identified by path, either relative or\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 448ec69e745..074169508d0 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -31,6 +31,7 @@ struct add_opts {\n \tint quiet;\n \tint checkout;\n \tint keep_locked;\n+\tconst char *lock_reason;\n };\n \n static int show_only;\n@@ -304,6 +305,8 @@ static int add_worktree(const char *path, const char *refname,\n \tstrbuf_addf(&sb, \"%s/locked\", sb_repo.buf);\n \tif (!opts->keep_locked)\n \t\twrite_file(sb.buf, \"initializing\");\n+\telse if (opts->lock_reason)\n+\t\twrite_file(sb.buf, \"%s\", opts->lock_reason);\n \telse\n \t\twrite_file(sb.buf, _(\"added with --lock\"));\n \n@@ -486,6 +489,8 @@ static int add(int ac, const char **av, const char *prefix)\n \t\tOPT_BOOL('d', \"detach\", &opts.detach, N_(\"detach HEAD at named commit\")),\n \t\tOPT_BOOL(0, \"checkout\", &opts.checkout, N_(\"populate the new working tree\")),\n \t\tOPT_BOOL(0, \"lock\", &opts.keep_locked, N_(\"keep the new working tree locked\")),\n+\t\tOPT_STRING(0, \"reason\", &opts.lock_reason, N_(\"string\"),\n+\t\t\t   N_(\"reason for locking\")),\n \t\tOPT__QUIET(&opts.quiet, N_(\"suppress progress reporting\")),\n \t\tOPT_PASSTHRU(0, \"track\", &opt_track, NULL,\n \t\t\t     N_(\"set up tracking mode (see git-branch(1))\"),\n@@ -500,6 +505,8 @@ static int add(int ac, const char **av, const char *prefix)\n \tac = parse_options(ac, av, prefix, options, worktree_usage, 0);\n \tif (!!opts.detach + !!new_branch + !!new_branch_force > 1)\n \t\tdie(_(\"-b, -B, and --detach are mutually exclusive\"));\n+\tif (opts.lock_reason && !opts.keep_locked)\n+\t\tdie(_(\"--reason requires --lock\"));\n \tif (ac < 1 || ac > 2)\n \t\tusage_with_options(worktree_usage, options);\n \ndiff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\nindex 874a61dbfa7..de904448e59 100755\n--- a/t/t2400-worktree-add.sh\n+++ b/t/t2400-worktree-add.sh\n@@ -71,6 +71,19 @@ test_expect_success '\"add\" worktree with lock' '\n \ttest -f .git/worktrees/here-with-lock/locked\n '\n \n+test_expect_success '\"add\" worktree with lock and reason' '\n+\tgit worktree add --detach --lock --reason \"why not\" here-with-lock-reason main &&\n+\ttest_when_finished \"git worktree unlock here-with-lock-reason || :\" &&\n+\ttest -f .git/worktrees/here-with-lock-reason/locked &&\n+\techo why not >expect &&\n+\ttest_cmp expect .git/worktrees/here-with-lock-reason/locked\n+'\n+\n+test_expect_success '\"add\" worktree with reason but no lock' '\n+\ttest_must_fail git worktree add --detach --reason \"why not\" here-with-reason-only main &&\n+\ttest_path_is_missing .git/worktrees/here-with-reason-only/locked\n+'\n+\n test_expect_success '\"add\" worktree from a subdir' '\n \t(\n \t\tmkdir sub &&\n-- \ngitgitgadget\n"},{"id":"429524","messageId":"CAPig+cQbBPGN+Dcvmy+ZAZeKLpffRrN4-2PNjRHzDHj56axcJg@mail.gmail.com","threadId":"56053","inReplyTo":"xmqq7di3jkki.fsf@gitster.g","subject":"Re: [PATCH] worktree: teach `add` to accept --reason <string> with --lock","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-07-09T06:11:34Z","receivedAt":"2021-07-09T06:11:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 6, 2021 at 3:42 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> >>  --reason <string>::\n> >> +       With `lock` or with `add --lock`, an explanation why the working tree is locked.\n> >\n> > I realize that you're mimicking the interface of `git worktree lock`\n> > which accepts an optional `--reason`, but I'm wondering if the\n> > user-experience might be improved by instead allowing `--lock` to\n> > accept the reason as an optional argument. For instance:\n> >\n> >     git worktree add --lock='just because' ...\n>\n> Thanks for thinking aloud, but I'd prefer the interface as posted,\n> simply because there is one less thing for users to remember.  The\n> justification to lock is given with the --reason=<why> argument no\n> matter how you acquire the lock on a worktree.\n\nMy one bit of pushback is that, although the meaning of `--reason` is\nplenty clear in the context of `git worktree lock`, it may become\nambiguous in the context of `git worktree add` if worktrees ever grow\nadditional attributes/features which are also accompanied by\n\"reasons\". That possibility suggests that this particular\nreason-giving option of `git worktree add` ought to be named\n`--lock-reason`, but `git worktree add --lock --lock-reason=<reason>`\nfeels clunky and redundant, which is why I was wondering if `git\nworktree --lock[=<reason>]` would be a better (and more convenient)\nUI.\n\nI'm questioning the UI choice now so we can avoid backpedalling later\non, if it ever comes to that, but perhaps my concern is unfounded.\n(Indeed, I haven't been able to come up with cases which would make\n`--reason` ambiguous.)\n"},{"id":"429525","messageId":"CAPig+cRiEYcz8-F0FzQRetF9UoN1Di099JVgCz0920kQ3a+BwQ@mail.gmail.com","threadId":"56053","inReplyTo":"30196cc93697cd4a6a6881dcbb1073d19e96d8a0.1625759443.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] worktree: default lock string should be marked with `_()` for translation","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-07-09T06:19:50Z","receivedAt":"2021-07-09T06:20:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jul 8, 2021 at 11:50 AM Stephen Manz via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> @@ -305,7 +305,7 @@ static int add_worktree(const char *path, const char *refname,\n>         if (!opts->keep_locked)\n>                 write_file(sb.buf, \"initializing\");\n>         else\n> -               write_file(sb.buf, \"added with --lock\");\n> +               write_file(sb.buf, _(\"added with --lock\"));\n\nMakes perfect sense, though the \"initializing\" string in the `then`\narm probably deserves the same treatment:\n\n    write_file(sb.buf, _(\"initializing\"));\n"},{"id":"429526","messageId":"YOf+tANuox4LRD7d@flurp.local","threadId":"56053","inReplyTo":"pull.992.v2.git.1625759443.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/3] worktree: teach add to accept --reason with --lock","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-07-09T07:45:56Z","receivedAt":"2021-07-09T07:46:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jul 08, 2021 at 03:50:40PM +0000, Stephen Manz via GitGitGadget wrote:\n> The default reason stored in the lock file, \"added with --lock\", is unlikely\n> to be what the user would have given in a separate git worktree lock\n> command. Allowing --reason to be specified along with --lock when adding a\n> working tree gives the user control over the reason for locking without\n> needing a second command.\n>\n> Changes since v1:\n>\n>  * Split changes into 3 commits. The first commit is removal of git\n>    rev-parse in the test above the ones I'm adding. The second is wrapping\n>    the \"added with --lock\" string with _() to mark it for translation. The\n>    third commit is the main change.\n>  * Reworked the if-else-if-else to if-else if-else\n>  * Added test_when_finished ... command to unlock the working tree\n>  * Changed test_expect_failure to test_expect_success and embedded\n>    test_must_fail and test_path_is_missing commands\n\nThanks, all these changes make sense. Aside from a missing `_()` in\npatch [2/3] mentioned in my review of that patch, everything looks\nfine.\n\n> Note: I don't see how to disambiguate --lock with no --reason from no --lock\n> at all. I still think that the original keep_locked boolean is needed along\n> with the new lock_reason char array. If I don't add lock_reason and change\n> keep_locked to a char array, it will start as NULL. But it will remain NULL\n> if --lock alone is given or if --lock isn't given at all.\n\nThe reason I suggested re-purposing `add_opts.keep_locked` is to avoid\npolluting that structure with members having overlapping meaning, thus\nreducing the confusion-factor for clients of that structure (assuming\nthat a tri-state `keep_locked` is indeed less confusing). That doesn't\npreclude adding a new variable or two local to the `add()` function to\nfacilitate keeping `add_opts` pure. For instance, it might be as\nsimple as the below patch.\n\nAnyhow, it's a minor suggestion and the sort of thing which can be\ndealt with later if someone deems it important. Moreover, it's\nsubjective enough that it should not be a blocker for this patch\nseries if you don't do it that way. As mentioned above -- missing\n`_()` aside -- the entire series looks reasonable as long as people\nfeel the UI choice is sound. (I, personally, still favor\n`--lock[=<reason>]`, but I'm just one reviewer...)\n\n--- >8 ---\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 976bf8ed06..22df2b60f2 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -30,7 +30,7 @@ struct add_opts {\n \tint detach;\n \tint quiet;\n \tint checkout;\n-\tint keep_locked;\n+\tconst char *keep_locked;\n };\n \n static int show_only;\n@@ -304,6 +304,8 @@ static int add_worktree(const char *path, const char *refname,\n \tstrbuf_addf(&sb, \"%s/locked\", sb_repo.buf);\n \tif (!opts->keep_locked)\n \t\twrite_file(sb.buf, \"initializing\");\n+\telse if (*opts->keep_locked)\n+\t\twrite_file(sb.buf, \"%s\", opts->keep_locked);\n \telse\n \t\twrite_file(sb.buf, \"added with --lock\");\n \n@@ -475,6 +477,8 @@ static int add(int ac, const char **av, const char *prefix)\n \tconst char *branch;\n \tconst char *new_branch = NULL;\n \tconst char *opt_track = NULL;\n+\tint keep_locked = 0;\n+\tconst char *lock_reason = NULL;\n \tstruct option options[] = {\n \t\tOPT__FORCE(&opts.force,\n \t\t\t   N_(\"checkout <branch> even if already checked out in other worktree\"),\n@@ -485,7 +489,9 @@ static int add(int ac, const char **av, const char *prefix)\n \t\t\t   N_(\"create or reset a branch\")),\n \t\tOPT_BOOL('d', \"detach\", &opts.detach, N_(\"detach HEAD at named commit\")),\n \t\tOPT_BOOL(0, \"checkout\", &opts.checkout, N_(\"populate the new working tree\")),\n-\t\tOPT_BOOL(0, \"lock\", &opts.keep_locked, N_(\"keep the new working tree locked\")),\n+\t\tOPT_BOOL(0, \"lock\", &keep_locked, N_(\"keep the new working tree locked\")),\n+\t\tOPT_STRING(0, \"reason\", &lock_reason, N_(\"string\"),\n+\t\t\t   N_(\"reason for locking\")),\n \t\tOPT__QUIET(&opts.quiet, N_(\"suppress progress reporting\")),\n \t\tOPT_PASSTHRU(0, \"track\", &opt_track, NULL,\n \t\t\t     N_(\"set up tracking mode (see git-branch(1))\"),\n@@ -500,6 +506,10 @@ static int add(int ac, const char **av, const char *prefix)\n \tac = parse_options(ac, av, prefix, options, worktree_usage, 0);\n \tif (!!opts.detach + !!new_branch + !!new_branch_force > 1)\n \t\tdie(_(\"-b, -B, and --detach are mutually exclusive\"));\n+\tif (lock_reason && !keep_locked)\n+\t\tdie(_(\"--reason requires --lock\"));\n+\tif (keep_locked)\n+\t\topts.keep_locked = lock_reason ? lock_reason : \"\";\n \tif (ac < 1 || ac > 2)\n \t\tusage_with_options(worktree_usage, options);\n \n-- \n2.32.0.372.g085311d9fa\n"},{"id":"429552","messageId":"xmqqczrr8qax.fsf@gitster.g","threadId":"56053","inReplyTo":"CAPig+cQbBPGN+Dcvmy+ZAZeKLpffRrN4-2PNjRHzDHj56axcJg@mail.gmail.com","subject":"Re: [PATCH] worktree: teach `add` to accept --reason <string> with --lock","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-09T15:23:02Z","receivedAt":"2021-07-09T15:23:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> \"reasons\". That possibility suggests that this particular\n> reason-giving option of `git worktree add` ought to be named\n> `--lock-reason`, but `git worktree add --lock --lock-reason=<reason>`\n> feels clunky and redundant, which is why I was wondering if `git\n> worktree --lock[=<reason>]` would be a better (and more convenient)\n> UI.\n\nSure, but\n\n    $ git worktree add --lock --reason=why-do-i-want-to-lock \\\n\t\t--frotz --reason=why-do-i-want-to-frotz\n\nwould work just fine, no?\n"},{"id":"429558","messageId":"xmqqzguv7a34.fsf@gitster.g","threadId":"56053","inReplyTo":"CAPig+cRiEYcz8-F0FzQRetF9UoN1Di099JVgCz0920kQ3a+BwQ@mail.gmail.com","subject":"Re: [PATCH v2 2/3] worktree: default lock string should be marked with `_()` for translation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-09T15:58:39Z","receivedAt":"2021-07-09T15:58:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Thu, Jul 8, 2021 at 11:50 AM Stephen Manz via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n>> diff --git a/builtin/worktree.c b/builtin/worktree.c\n>> @@ -305,7 +305,7 @@ static int add_worktree(const char *path, const char *refname,\n>>         if (!opts->keep_locked)\n>>                 write_file(sb.buf, \"initializing\");\n>>         else\n>> -               write_file(sb.buf, \"added with --lock\");\n>> +               write_file(sb.buf, _(\"added with --lock\"));\n>\n> Makes perfect sense, though the \"initializing\" string in the `then`\n> arm probably deserves the same treatment:\n>\n>     write_file(sb.buf, _(\"initializing\"));\n\nGood eyes.\n"},{"id":"429559","messageId":"xmqqv95j79ut.fsf@gitster.g","threadId":"56053","inReplyTo":"YOf+tANuox4LRD7d@flurp.local","subject":"Re: [PATCH v2 0/3] worktree: teach add to accept --reason with --lock","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-09T16:03:38Z","receivedAt":"2021-07-09T16:03:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> The reason I suggested re-purposing `add_opts.keep_locked` is to avoid\n> polluting that structure with members having overlapping meaning, thus\n> reducing the confusion-factor for clients of that structure (assuming\n> that a tri-state `keep_locked` is indeed less confusing). That doesn't\n> preclude adding a new variable or two local to the `add()` function to\n> facilitate keeping `add_opts` pure. For instance, it might be as\n> simple as the below patch.\n\nTrue.  It is less trivial to construct the command line option\nparser so that --reason=<why> and --lock can be given in any order\n(e.g. they no longer can be a simple OPT_BOOL and OPT_STRING that\ncan be given independently but needs some postprocessing like your\npatch did), but it is not rocket science and keeping add_opts struct\nleaner will reduce the risk of runtime confusion, I would think, but\nat the same time, the room for runtime confusion would probably be\nminor to begin with---so I am fine, if the coder cannot cleanly\nwrite the option parser to do so, with the code as posted.\n"},{"id":"429588","messageId":"CAPig+cQEn0k8YEYWPep0yxn5N6xdtTE9hc1Po-DGCHu09UFzHA@mail.gmail.com","threadId":"56053","inReplyTo":"xmqqczrr8qax.fsf@gitster.g","subject":"Re: [PATCH] worktree: teach `add` to accept --reason <string> with --lock","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-07-10T07:11:54Z","receivedAt":"2021-07-10T07:12:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jul 9, 2021 at 11:23 AM Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> > \"reasons\". That possibility suggests that this particular\n> > reason-giving option of `git worktree add` ought to be named\n> > `--lock-reason`, but `git worktree add --lock --lock-reason=<reason>`\n> > feels clunky and redundant, which is why I was wondering if `git\n> > worktree --lock[=<reason>]` would be a better (and more convenient)\n> > UI.\n>\n> Sure, but\n>\n>     $ git worktree add --lock --reason=why-do-i-want-to-lock \\\n>                 --frotz --reason=why-do-i-want-to-frotz\n>\n> would work just fine, no?\n\nYes, taking the stance that option order is significant would be workable.\n"},{"id":"429589","messageId":"CAPig+cQyOgQb1zHA49kOT4+2SmTxUjZpfQ-f1ZxV1AwigCz01A@mail.gmail.com","threadId":"56053","inReplyTo":"xmqqv95j79ut.fsf@gitster.g","subject":"Re: [PATCH v2 0/3] worktree: teach add to accept --reason with --lock","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-07-10T07:15:44Z","receivedAt":"2021-07-10T07:15:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jul 9, 2021 at 12:03 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> > The reason I suggested re-purposing `add_opts.keep_locked` is to avoid\n> > polluting that structure with members having overlapping meaning, thus\n> > reducing the confusion-factor for clients of that structure (assuming\n> > that a tri-state `keep_locked` is indeed less confusing). That doesn't\n> > preclude adding a new variable or two local to the `add()` function to\n> > facilitate keeping `add_opts` pure. For instance, it might be as\n> > simple as the below patch.\n>\n> True.  It is less trivial to construct the command line option\n> parser so that --reason=<why> and --lock can be given in any order\n> (e.g. they no longer can be a simple OPT_BOOL and OPT_STRING that\n> can be given independently but needs some postprocessing like your\n> patch did), but it is not rocket science and keeping add_opts struct\n> leaner will reduce the risk of runtime confusion, I would think, but\n> at the same time, the room for runtime confusion would probably be\n> minor to begin with---so I am fine, if the coder cannot cleanly\n> write the option parser to do so, with the code as posted.\n\nAlthough the leaner approach seems \"simpler\" and more obvious to me,\nit is subjective, and I can see how other people might find a\ntri-state add_opts::keep_locked confusing. So, I'm fine with it either\nway and don't consider it a blocker at all.\n"},{"id":"429655","messageId":"ceb7a58b0049a8afa8c5f9f706b9a0b0abfb843e.1625963240.git.gitgitgadget@gmail.com","threadId":"56053","inReplyTo":"pull.992.v3.git.1625963240.gitgitgadget@gmail.com","subject":"[PATCH v3 2/3] worktree: mark lock strings with `_()` for translation","fromName":"Stephen Manz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-11T00:27:19Z","receivedAt":"2021-07-11T00:27:29Z","isPatch":true,"sender":{"key":"name:Stephen Manz","avatar":null},"body":"From: Stephen Manz <smanz@alum.mit.edu>\n\n- default lock string, \"added with --lock\"\n- temporary lock string, \"initializing\"\n\nSigned-off-by: Stephen Manz <smanz@alum.mit.edu>\n---\n builtin/worktree.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 976bf8ed063..4829b9507ff 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -303,9 +303,9 @@ static int add_worktree(const char *path, const char *refname,\n \t */\n \tstrbuf_addf(&sb, \"%s/locked\", sb_repo.buf);\n \tif (!opts->keep_locked)\n-\t\twrite_file(sb.buf, \"initializing\");\n+\t\twrite_file(sb.buf, _(\"initializing\"));\n \telse\n-\t\twrite_file(sb.buf, \"added with --lock\");\n+\t\twrite_file(sb.buf, _(\"added with --lock\"));\n \n \tstrbuf_addf(&sb_git, \"%s/.git\", path);\n \tif (safe_create_leading_directories_const(sb_git.buf))\n-- \ngitgitgadget\n\n"},{"id":"429656","messageId":"5618933279dadf48ba51b79fc11beec8c163820e.1625963240.git.gitgitgadget@gmail.com","threadId":"56053","inReplyTo":"pull.992.v3.git.1625963240.gitgitgadget@gmail.com","subject":"[PATCH v3 1/3] t2400: clean up '\"add\" worktree with lock' test","fromName":"Stephen Manz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-11T00:27:18Z","receivedAt":"2021-07-11T00:27:29Z","isPatch":true,"sender":{"key":"name:Stephen Manz","avatar":null},"body":"From: Stephen Manz <smanz@alum.mit.edu>\n\n- remove unneeded `git rev-parse` which must have come from a copy-paste\n  of another test\n- unlock the worktree with test_when_finished\n\nSigned-off-by: Stephen Manz <smanz@alum.mit.edu>\n---\n t/t2400-worktree-add.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\nindex 96dfca15542..93d3795cab9 100755\n--- a/t/t2400-worktree-add.sh\n+++ b/t/t2400-worktree-add.sh\n@@ -67,8 +67,8 @@ test_expect_success '\"add\" worktree' '\n '\n \n test_expect_success '\"add\" worktree with lock' '\n-\tgit rev-parse HEAD >expect &&\n \tgit worktree add --detach --lock here-with-lock main &&\n+\ttest_when_finished \"git worktree unlock here-with-lock || :\" &&\n \ttest -f .git/worktrees/here-with-lock/locked\n '\n \n-- \ngitgitgadget\n\n"},{"id":"429657","messageId":"pull.992.v3.git.1625963240.gitgitgadget@gmail.com","threadId":"56053","inReplyTo":"pull.992.v2.git.1625759443.gitgitgadget@gmail.com","subject":"[PATCH v3 0/3] worktree: teach add to accept --reason with --lock","fromName":"Stephen Manz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-11T00:27:17Z","receivedAt":"2021-07-11T00:27:29Z","isPatch":true,"sender":{"key":"name:Stephen Manz","avatar":null},"body":"The default reason stored in the lock file, \"added with --lock\", is unlikely\nto be what the user would have given in a separate git worktree lock\ncommand. Allowing --reason to be specified along with --lock when adding a\nworking tree gives the user control over the reason for locking without\nneeding a second command.\n\nChanges since v2:\n\n * Updated first of the 3 commits to include a step in the test to unlock\n   the worktree when test completes and modified commit message accordingly.\n * Updated second commit to wrap \"initializing\" string with _() to mark it\n   for translation, as requested. I had originally opted not to mark it,\n   since, when --lock is not given, file locked gets removed after the\n   working tree is populated. Thus, it's not really user-facing. Modified\n   the commit message accordingly.\n * Updated the third commit to have no new lock_reason member of struct\n   add_opts and changed type of member keep_locked from int to const char *,\n   as suggested.\n\nThe file, .git/worktrees/name-of-worktree/locked, is created even if --lock\nisn't given, although only temporarily. In this instance, \"initializing\" is\nwritten to the file, but the file is removed at the end of the add-worktree\noperation. My goal was to maintain this behavior,\n\nChanges since v1:\n\n * Split changes into 3 commits. The first commit is removal of git\n   rev-parse in the test above the ones I'm adding. The second is wrapping\n   the \"added with --lock\" string with _() to mark it for translation. The\n   third commit is the main change.\n * Reworked the if-else-if-else to if-else if-else\n * Added test_when_finished ... command to unlock the working tree\n * Changed test_expect_failure to test_expect_success and embedded\n   test_must_fail and test_path_is_missing commands\n\nNote: I don't see how to disambiguate --lock with no --reason from no --lock\nat all. I still think that the original keep_locked boolean is needed along\nwith the new lock_reason char array. If I don't add lock_reason and change\nkeep_locked to a char array, it will start as NULL. But it will remain NULL\nif --lock alone is given or if --lock isn't given at all.\n\ncc: Eric Sunshine sunshine@sunshineco.com\n\nStephen Manz (3):\n  t2400: clean up '\"add\" worktree with lock' test\n  worktree: mark lock strings with `_()` for translation\n  worktree: teach `add` to accept --reason <string> with --lock\n\n Documentation/git-worktree.txt |  4 ++--\n builtin/worktree.c             | 21 ++++++++++++++++-----\n t/t2400-worktree-add.sh        | 15 ++++++++++++++-\n 3 files changed, 32 insertions(+), 8 deletions(-)\n\n\nbase-commit: d486ca60a51c9cb1fe068803c3f540724e95e83a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-992%2FSRManz%2Flock_reason-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-992/SRManz/lock_reason-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/992\n\nRange-diff vs v2:\n\n 1:  5459e5bb421 ! 1:  5618933279d t2400: remove unneeded `git rev-parse` from '\"add\" worktree with lock' test\n     @@ Metadata\n      Author: Stephen Manz <smanz@alum.mit.edu>\n      \n       ## Commit message ##\n     -    t2400: remove unneeded `git rev-parse` from '\"add\" worktree with lock' test\n     +    t2400: clean up '\"add\" worktree with lock' test\n      \n     -    It must have come from a copy-paste of another test\n     +    - remove unneeded `git rev-parse` which must have come from a copy-paste\n     +      of another test\n     +    - unlock the worktree with test_when_finished\n      \n          Signed-off-by: Stephen Manz <smanz@alum.mit.edu>\n      \n     @@ t/t2400-worktree-add.sh: test_expect_success '\"add\" worktree' '\n       test_expect_success '\"add\" worktree with lock' '\n      -\tgit rev-parse HEAD >expect &&\n       \tgit worktree add --detach --lock here-with-lock main &&\n     ++\ttest_when_finished \"git worktree unlock here-with-lock || :\" &&\n       \ttest -f .git/worktrees/here-with-lock/locked\n       '\n     + \n 2:  30196cc9369 ! 2:  ceb7a58b004 worktree: default lock string should be marked with `_()` for translation\n     @@ Metadata\n      Author: Stephen Manz <smanz@alum.mit.edu>\n      \n       ## Commit message ##\n     -    worktree: default lock string should be marked with `_()` for translation\n     +    worktree: mark lock strings with `_()` for translation\n     +\n     +    - default lock string, \"added with --lock\"\n     +    - temporary lock string, \"initializing\"\n      \n          Signed-off-by: Stephen Manz <smanz@alum.mit.edu>\n      \n       ## builtin/worktree.c ##\n      @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refname,\n     + \t */\n     + \tstrbuf_addf(&sb, \"%s/locked\", sb_repo.buf);\n       \tif (!opts->keep_locked)\n     - \t\twrite_file(sb.buf, \"initializing\");\n     +-\t\twrite_file(sb.buf, \"initializing\");\n     ++\t\twrite_file(sb.buf, _(\"initializing\"));\n       \telse\n      -\t\twrite_file(sb.buf, \"added with --lock\");\n      +\t\twrite_file(sb.buf, _(\"added with --lock\"));\n 3:  4d17b31921a ! 3:  9a414a3078b worktree: teach `add` to accept --reason <string> with --lock\n     @@ Documentation/git-worktree.txt: With `list`, annotate missing working trees as p\n      \n       ## builtin/worktree.c ##\n      @@ builtin/worktree.c: struct add_opts {\n     + \tint detach;\n       \tint quiet;\n       \tint checkout;\n     - \tint keep_locked;\n     -+\tconst char *lock_reason;\n     +-\tint keep_locked;\n     ++\tconst char *keep_locked;\n       };\n       \n       static int show_only;\n      @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refname,\n     + \t * after the preparation is over.\n     + \t */\n       \tstrbuf_addf(&sb, \"%s/locked\", sb_repo.buf);\n     - \tif (!opts->keep_locked)\n     - \t\twrite_file(sb.buf, \"initializing\");\n     -+\telse if (opts->lock_reason)\n     -+\t\twrite_file(sb.buf, \"%s\", opts->lock_reason);\n     +-\tif (!opts->keep_locked)\n     +-\t\twrite_file(sb.buf, _(\"initializing\"));\n     ++\tif (opts->keep_locked)\n     ++\t\twrite_file(sb.buf, \"%s\", opts->keep_locked);\n       \telse\n     - \t\twrite_file(sb.buf, _(\"added with --lock\"));\n     +-\t\twrite_file(sb.buf, _(\"added with --lock\"));\n     ++\t\twrite_file(sb.buf, _(\"initializing\"));\n       \n     + \tstrbuf_addf(&sb_git, \"%s/.git\", path);\n     + \tif (safe_create_leading_directories_const(sb_git.buf))\n      @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix)\n     + \tconst char *branch;\n     + \tconst char *new_branch = NULL;\n     + \tconst char *opt_track = NULL;\n     ++\tconst char *lock_reason = NULL;\n     ++\tint keep_locked = 0;\n     + \tstruct option options[] = {\n     + \t\tOPT__FORCE(&opts.force,\n     + \t\t\t   N_(\"checkout <branch> even if already checked out in other worktree\"),\n     +@@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix)\n     + \t\t\t   N_(\"create or reset a branch\")),\n       \t\tOPT_BOOL('d', \"detach\", &opts.detach, N_(\"detach HEAD at named commit\")),\n       \t\tOPT_BOOL(0, \"checkout\", &opts.checkout, N_(\"populate the new working tree\")),\n     - \t\tOPT_BOOL(0, \"lock\", &opts.keep_locked, N_(\"keep the new working tree locked\")),\n     -+\t\tOPT_STRING(0, \"reason\", &opts.lock_reason, N_(\"string\"),\n     +-\t\tOPT_BOOL(0, \"lock\", &opts.keep_locked, N_(\"keep the new working tree locked\")),\n     ++\t\tOPT_BOOL(0, \"lock\", &keep_locked, N_(\"keep the new working tree locked\")),\n     ++\t\tOPT_STRING(0, \"reason\", &lock_reason, N_(\"string\"),\n      +\t\t\t   N_(\"reason for locking\")),\n       \t\tOPT__QUIET(&opts.quiet, N_(\"suppress progress reporting\")),\n       \t\tOPT_PASSTHRU(0, \"track\", &opt_track, NULL,\n     @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix)\n       \tac = parse_options(ac, av, prefix, options, worktree_usage, 0);\n       \tif (!!opts.detach + !!new_branch + !!new_branch_force > 1)\n       \t\tdie(_(\"-b, -B, and --detach are mutually exclusive\"));\n     -+\tif (opts.lock_reason && !opts.keep_locked)\n     ++\tif (lock_reason && !keep_locked)\n      +\t\tdie(_(\"--reason requires --lock\"));\n     ++\tif (lock_reason)\n     ++\t\topts.keep_locked = lock_reason;\n     ++\telse if (keep_locked)\n     ++\t\topts.keep_locked = _(\"added with --lock\");\n     ++\n       \tif (ac < 1 || ac > 2)\n       \t\tusage_with_options(worktree_usage, options);\n       \n\n-- \ngitgitgadget\n"},{"id":"429658","messageId":"9a414a3078bb481ba19449065d47e6e27322cef2.1625963240.git.gitgitgadget@gmail.com","threadId":"56053","inReplyTo":"pull.992.v3.git.1625963240.gitgitgadget@gmail.com","subject":"[PATCH v3 3/3] worktree: teach `add` to accept --reason <string> with --lock","fromName":"Stephen Manz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-11T00:27:20Z","receivedAt":"2021-07-11T00:27:32Z","isPatch":true,"sender":{"key":"name:Stephen Manz","avatar":null},"body":"From: Stephen Manz <smanz@alum.mit.edu>\n\nThe default reason stored in the lock file, \"added with --lock\",\nis unlikely to be what the user would have given in a separate\n`git worktree lock` command. Allowing `--reason` to be specified\nalong with `--lock` when adding a working tree gives the user control\nover the reason for locking without needing a second command.\n\nSigned-off-by: Stephen Manz <smanz@alum.mit.edu>\n---\n Documentation/git-worktree.txt |  4 ++--\n builtin/worktree.c             | 21 ++++++++++++++++-----\n t/t2400-worktree-add.sh        | 13 +++++++++++++\n 3 files changed, 31 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex 66e67e6cbfa..8a7cbdd19c1 100644\n--- a/Documentation/git-worktree.txt\n+++ b/Documentation/git-worktree.txt\n@@ -9,7 +9,7 @@ git-worktree - Manage multiple working trees\n SYNOPSIS\n --------\n [verse]\n-'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b <new-branch>] <path> [<commit-ish>]\n+'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] [-b <new-branch>] <path> [<commit-ish>]\n 'git worktree list' [--porcelain]\n 'git worktree lock' [--reason <string>] <worktree>\n 'git worktree move' <worktree> <new-path>\n@@ -242,7 +242,7 @@ With `list`, annotate missing working trees as prunable if they are\n older than `<time>`.\n \n --reason <string>::\n-\tWith `lock`, an explanation why the working tree is locked.\n+\tWith `lock` or with `add --lock`, an explanation why the working tree is locked.\n \n <worktree>::\n \tWorking trees can be identified by path, either relative or\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 4829b9507ff..0d0a80da61f 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -30,7 +30,7 @@ struct add_opts {\n \tint detach;\n \tint quiet;\n \tint checkout;\n-\tint keep_locked;\n+\tconst char *keep_locked;\n };\n \n static int show_only;\n@@ -302,10 +302,10 @@ static int add_worktree(const char *path, const char *refname,\n \t * after the preparation is over.\n \t */\n \tstrbuf_addf(&sb, \"%s/locked\", sb_repo.buf);\n-\tif (!opts->keep_locked)\n-\t\twrite_file(sb.buf, _(\"initializing\"));\n+\tif (opts->keep_locked)\n+\t\twrite_file(sb.buf, \"%s\", opts->keep_locked);\n \telse\n-\t\twrite_file(sb.buf, _(\"added with --lock\"));\n+\t\twrite_file(sb.buf, _(\"initializing\"));\n \n \tstrbuf_addf(&sb_git, \"%s/.git\", path);\n \tif (safe_create_leading_directories_const(sb_git.buf))\n@@ -475,6 +475,8 @@ static int add(int ac, const char **av, const char *prefix)\n \tconst char *branch;\n \tconst char *new_branch = NULL;\n \tconst char *opt_track = NULL;\n+\tconst char *lock_reason = NULL;\n+\tint keep_locked = 0;\n \tstruct option options[] = {\n \t\tOPT__FORCE(&opts.force,\n \t\t\t   N_(\"checkout <branch> even if already checked out in other worktree\"),\n@@ -485,7 +487,9 @@ static int add(int ac, const char **av, const char *prefix)\n \t\t\t   N_(\"create or reset a branch\")),\n \t\tOPT_BOOL('d', \"detach\", &opts.detach, N_(\"detach HEAD at named commit\")),\n \t\tOPT_BOOL(0, \"checkout\", &opts.checkout, N_(\"populate the new working tree\")),\n-\t\tOPT_BOOL(0, \"lock\", &opts.keep_locked, N_(\"keep the new working tree locked\")),\n+\t\tOPT_BOOL(0, \"lock\", &keep_locked, N_(\"keep the new working tree locked\")),\n+\t\tOPT_STRING(0, \"reason\", &lock_reason, N_(\"string\"),\n+\t\t\t   N_(\"reason for locking\")),\n \t\tOPT__QUIET(&opts.quiet, N_(\"suppress progress reporting\")),\n \t\tOPT_PASSTHRU(0, \"track\", &opt_track, NULL,\n \t\t\t     N_(\"set up tracking mode (see git-branch(1))\"),\n@@ -500,6 +504,13 @@ static int add(int ac, const char **av, const char *prefix)\n \tac = parse_options(ac, av, prefix, options, worktree_usage, 0);\n \tif (!!opts.detach + !!new_branch + !!new_branch_force > 1)\n \t\tdie(_(\"-b, -B, and --detach are mutually exclusive\"));\n+\tif (lock_reason && !keep_locked)\n+\t\tdie(_(\"--reason requires --lock\"));\n+\tif (lock_reason)\n+\t\topts.keep_locked = lock_reason;\n+\telse if (keep_locked)\n+\t\topts.keep_locked = _(\"added with --lock\");\n+\n \tif (ac < 1 || ac > 2)\n \t\tusage_with_options(worktree_usage, options);\n \ndiff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\nindex 93d3795cab9..7590064076a 100755\n--- a/t/t2400-worktree-add.sh\n+++ b/t/t2400-worktree-add.sh\n@@ -72,6 +72,19 @@ test_expect_success '\"add\" worktree with lock' '\n \ttest -f .git/worktrees/here-with-lock/locked\n '\n \n+test_expect_success '\"add\" worktree with lock and reason' '\n+\tgit worktree add --detach --lock --reason \"why not\" here-with-lock-reason main &&\n+\ttest_when_finished \"git worktree unlock here-with-lock-reason || :\" &&\n+\ttest -f .git/worktrees/here-with-lock-reason/locked &&\n+\techo why not >expect &&\n+\ttest_cmp expect .git/worktrees/here-with-lock-reason/locked\n+'\n+\n+test_expect_success '\"add\" worktree with reason but no lock' '\n+\ttest_must_fail git worktree add --detach --reason \"why not\" here-with-reason-only main &&\n+\ttest_path_is_missing .git/worktrees/here-with-reason-only/locked\n+'\n+\n test_expect_success '\"add\" worktree from a subdir' '\n \t(\n \t\tmkdir sub &&\n-- \ngitgitgadget\n"},{"id":"429862","messageId":"CAPig+cQVSUg1aqry_hMydJ=Uo=-VhOog6TUTpG=0on0LUcw8Dg@mail.gmail.com","threadId":"56053","inReplyTo":"9a414a3078bb481ba19449065d47e6e27322cef2.1625963240.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 3/3] worktree: teach `add` to accept --reason <string> with --lock","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-07-13T03:37:25Z","receivedAt":"2021-07-13T03:37:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Jul 10, 2021 at 8:27 PM Stephen Manz via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> The default reason stored in the lock file, \"added with --lock\",\n> is unlikely to be what the user would have given in a separate\n> `git worktree lock` command. Allowing `--reason` to be specified\n> along with `--lock` when adding a working tree gives the user control\n> over the reason for locking without needing a second command.\n>\n> Signed-off-by: Stephen Manz <smanz@alum.mit.edu>\n> ---\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> @@ -302,10 +302,10 @@ static int add_worktree(const char *path, const char *refname,\n>         strbuf_addf(&sb, \"%s/locked\", sb_repo.buf);\n> -       if (!opts->keep_locked)\n> -               write_file(sb.buf, _(\"initializing\"));\n> +       if (opts->keep_locked)\n> +               write_file(sb.buf, \"%s\", opts->keep_locked);\n>         else\n> -               write_file(sb.buf, _(\"added with --lock\"));\n> +               write_file(sb.buf, _(\"initializing\"));\n\nChanging the condition around to handle the positive case first makes\nthe diff noisier, but the resulting code is a bit easier to reason\nabout. Okay.\n\n> @@ -500,6 +504,13 @@ static int add(int ac, const char **av, const char *prefix)\n> +       if (lock_reason && !keep_locked)\n> +               die(_(\"--reason requires --lock\"));\n> +       if (lock_reason)\n> +               opts.keep_locked = lock_reason;\n> +       else if (keep_locked)\n> +               opts.keep_locked = _(\"added with --lock\");\n\nThe benefit of relocating the \"added with --lock\" literal from\nadd_worktree() to add() wasn't immediately obvious, aside from making\nthe `if` statement in add_worktree() a bit less complex. But I managed\nto convince myself that the relocation makes sense since add() knows\nabout the `--lock` option, whereas add_worktree() is merely a consumer\nof `add_opts` without specific knowledge of how the fields in that\nstructure get set. Okay.\n\n> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\n> @@ -72,6 +72,19 @@ test_expect_success '\"add\" worktree with lock' '\n> +test_expect_success '\"add\" worktree with lock and reason' '\n> +       git worktree add --detach --lock --reason \"why not\" here-with-lock-reason main &&\n> +       test_when_finished \"git worktree unlock here-with-lock-reason || :\" &&\n> +       test -f .git/worktrees/here-with-lock-reason/locked &&\n> +       echo why not >expect &&\n> +       test_cmp expect .git/worktrees/here-with-lock-reason/locked\n> +'\n\nTwo very minor comments:\n\nFirst, considering that test_cmp() will fail anyhow if the `locked`\nfile is missing, the `test -f` is redundant.\n\nSecond, the lack of quotes around \"why not\" in the `echo ... >expect`\nstatement gives me a moment's pause since it relies upon the fact that\n`echo` will insert exactly one space between the \"why\" and \"not\"\narguments (which happens to match the single space in the\ndouble-quoted argument to `--reason`). For clarity and that extra bit\nof robustness, I'd probably have used a single double-quoted string\nargument with `echo`.\n\nAnyhow, those are extremely minor comments, probably not worth a\nre-roll but perhaps something to keep in mind if you do re-roll for\nsome other more important reason.\n"},{"id":"429863","messageId":"CAPig+cSVsJ9AtAMqtRQpyuosCDCGi+mu2C1PJUK49WTb5KvcWQ@mail.gmail.com","threadId":"56053","inReplyTo":"pull.992.v3.git.1625963240.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/3] worktree: teach add to accept --reason with --lock","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-07-13T03:47:08Z","receivedAt":"2021-07-13T03:47:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Jul 10, 2021 at 8:27 PM Stephen Manz via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> Changes since v2:\n>  * Updated second commit to wrap \"initializing\" string with _() to mark it\n>    for translation, as requested. I had originally opted not to mark it,\n>    since, when --lock is not given, file locked gets removed after the\n>    working tree is populated. Thus, it's not really user-facing. Modified\n>    the commit message accordingly.\n\nTrue. Makes sense. A side-benefit of wrapping it in `_(...)`, though,\nis that it saves the next person who touches that code from wondering\nwhy only one of the messages has been localized, so it's nice to see\nthe revised patch making both strings localizable.\n\nI read over the series and played around with the implementation. It\nall looks good. With or without addressing the couple extremely minor\ncomments I left on patch [3/3] regarding one of the tests, consider\nthis series:\n\n    Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>\n\nThanks.\n"},{"id":"430168","messageId":"pull.992.v4.git.1626316350.gitgitgadget@gmail.com","threadId":"56053","inReplyTo":"pull.992.v3.git.1625963240.gitgitgadget@gmail.com","subject":"[PATCH v4 0/3] worktree: teach add to accept --reason with --lock","fromName":"Stephen Manz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-15T02:32:27Z","receivedAt":"2021-07-15T02:32:35Z","isPatch":true,"sender":{"key":"name:Stephen Manz","avatar":null},"body":"The default reason stored in the lock file, \"added with --lock\", is unlikely\nto be what the user would have given in a separate git worktree lock\ncommand. Allowing --reason to be specified along with --lock when adding a\nworking tree gives the user control over the reason for locking without\nneeding a second command.\n\nChanges since v3:\n\n * Updated test to define a shell variable, lock_reason, set to \"why not\".\n   Expand the variable to use with --reason and to echo to file, expected.\n   This avoids the spacing comment made by Eric Sunshine.\n\nChanges since v2:\n\n * Updated first of the 3 commits to include a step in the test to unlock\n   the worktree when test completes and modified commit message accordingly.\n * Updated second commit to wrap \"initializing\" string with _() to mark it\n   for translation, as requested. I had originally opted not to mark it,\n   since, when --lock is not given, file locked gets removed after the\n   working tree is populated. Thus, it's not really user-facing. Modified\n   the commit message accordingly.\n * Updated the third commit to have no new lock_reason member of struct\n   add_opts and changed type of member keep_locked from int to const char *,\n   as suggested.\n\nThe file, .git/worktrees/name-of-worktree/locked, is created even if --lock\nisn't given, although only temporarily. In this instance, \"initializing\" is\nwritten to the file, but the file is removed at the end of the add-worktree\noperation. My goal was to maintain this behavior and is the reason my\npost-parsing code doesn't quite match the suggested patch.\n\nChanges since v1:\n\n * Split changes into 3 commits. The first commit is removal of git\n   rev-parse in the test above the ones I'm adding. The second is wrapping\n   the \"added with --lock\" string with _() to mark it for translation. The\n   third commit is the main change.\n * Reworked the if-else-if-else to if-else if-else\n * Added test_when_finished ... command to unlock the working tree\n * Changed test_expect_failure to test_expect_success and embedded\n   test_must_fail and test_path_is_missing commands\n\nNote: I don't see how to disambiguate --lock with no --reason from no --lock\nat all. I still think that the original keep_locked boolean is needed along\nwith the new lock_reason char array. If I don't add lock_reason and change\nkeep_locked to a char array, it will start as NULL. But it will remain NULL\nif --lock alone is given or if --lock isn't given at all.\n\ncc: Eric Sunshine sunshine@sunshineco.com\n\nStephen Manz (3):\n  t2400: clean up '\"add\" worktree with lock' test\n  worktree: mark lock strings with `_()` for translation\n  worktree: teach `add` to accept --reason <string> with --lock\n\n Documentation/git-worktree.txt |  4 ++--\n builtin/worktree.c             | 21 ++++++++++++++++-----\n t/t2400-worktree-add.sh        | 16 +++++++++++++++-\n 3 files changed, 33 insertions(+), 8 deletions(-)\n\n\nbase-commit: d486ca60a51c9cb1fe068803c3f540724e95e83a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-992%2FSRManz%2Flock_reason-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-992/SRManz/lock_reason-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/992\n\nRange-diff vs v3:\n\n 1:  5618933279d = 1:  5618933279d t2400: clean up '\"add\" worktree with lock' test\n 2:  ceb7a58b004 = 2:  ceb7a58b004 worktree: mark lock strings with `_()` for translation\n 3:  9a414a3078b ! 3:  4b6bb50d3d6 worktree: teach `add` to accept --reason <string> with --lock\n     @@ t/t2400-worktree-add.sh: test_expect_success '\"add\" worktree with lock' '\n       '\n       \n      +test_expect_success '\"add\" worktree with lock and reason' '\n     -+\tgit worktree add --detach --lock --reason \"why not\" here-with-lock-reason main &&\n     ++\tlock_reason=\"why not\" &&\n     ++\tgit worktree add --detach --lock --reason \"$lock_reason\" here-with-lock-reason main &&\n      +\ttest_when_finished \"git worktree unlock here-with-lock-reason || :\" &&\n      +\ttest -f .git/worktrees/here-with-lock-reason/locked &&\n     -+\techo why not >expect &&\n     ++\techo \"$lock_reason\" >expect &&\n      +\ttest_cmp expect .git/worktrees/here-with-lock-reason/locked\n      +'\n      +\n\n-- \ngitgitgadget\n"},{"id":"430169","messageId":"5618933279dadf48ba51b79fc11beec8c163820e.1626316350.git.gitgitgadget@gmail.com","threadId":"56053","inReplyTo":"pull.992.v4.git.1626316350.gitgitgadget@gmail.com","subject":"[PATCH v4 1/3] t2400: clean up '\"add\" worktree with lock' test","fromName":"Stephen Manz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-15T02:32:28Z","receivedAt":"2021-07-15T02:32:37Z","isPatch":true,"sender":{"key":"name:Stephen Manz","avatar":null},"body":"From: Stephen Manz <smanz@alum.mit.edu>\n\n- remove unneeded `git rev-parse` which must have come from a copy-paste\n  of another test\n- unlock the worktree with test_when_finished\n\nSigned-off-by: Stephen Manz <smanz@alum.mit.edu>\n---\n t/t2400-worktree-add.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\nindex 96dfca15542..93d3795cab9 100755\n--- a/t/t2400-worktree-add.sh\n+++ b/t/t2400-worktree-add.sh\n@@ -67,8 +67,8 @@ test_expect_success '\"add\" worktree' '\n '\n \n test_expect_success '\"add\" worktree with lock' '\n-\tgit rev-parse HEAD >expect &&\n \tgit worktree add --detach --lock here-with-lock main &&\n+\ttest_when_finished \"git worktree unlock here-with-lock || :\" &&\n \ttest -f .git/worktrees/here-with-lock/locked\n '\n \n-- \ngitgitgadget\n\n"},{"id":"430170","messageId":"ceb7a58b0049a8afa8c5f9f706b9a0b0abfb843e.1626316350.git.gitgitgadget@gmail.com","threadId":"56053","inReplyTo":"pull.992.v4.git.1626316350.gitgitgadget@gmail.com","subject":"[PATCH v4 2/3] worktree: mark lock strings with `_()` for translation","fromName":"Stephen Manz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-15T02:32:29Z","receivedAt":"2021-07-15T02:32:37Z","isPatch":true,"sender":{"key":"name:Stephen Manz","avatar":null},"body":"From: Stephen Manz <smanz@alum.mit.edu>\n\n- default lock string, \"added with --lock\"\n- temporary lock string, \"initializing\"\n\nSigned-off-by: Stephen Manz <smanz@alum.mit.edu>\n---\n builtin/worktree.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 976bf8ed063..4829b9507ff 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -303,9 +303,9 @@ static int add_worktree(const char *path, const char *refname,\n \t */\n \tstrbuf_addf(&sb, \"%s/locked\", sb_repo.buf);\n \tif (!opts->keep_locked)\n-\t\twrite_file(sb.buf, \"initializing\");\n+\t\twrite_file(sb.buf, _(\"initializing\"));\n \telse\n-\t\twrite_file(sb.buf, \"added with --lock\");\n+\t\twrite_file(sb.buf, _(\"added with --lock\"));\n \n \tstrbuf_addf(&sb_git, \"%s/.git\", path);\n \tif (safe_create_leading_directories_const(sb_git.buf))\n-- \ngitgitgadget\n\n"},{"id":"430171","messageId":"4b6bb50d3d66ce8b033c475fbb9d3bda672e2612.1626316350.git.gitgitgadget@gmail.com","threadId":"56053","inReplyTo":"pull.992.v4.git.1626316350.gitgitgadget@gmail.com","subject":"[PATCH v4 3/3] worktree: teach `add` to accept --reason <string> with --lock","fromName":"Stephen Manz via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-15T02:32:30Z","receivedAt":"2021-07-15T02:32:38Z","isPatch":true,"sender":{"key":"name:Stephen Manz","avatar":null},"body":"From: Stephen Manz <smanz@alum.mit.edu>\n\nThe default reason stored in the lock file, \"added with --lock\",\nis unlikely to be what the user would have given in a separate\n`git worktree lock` command. Allowing `--reason` to be specified\nalong with `--lock` when adding a working tree gives the user control\nover the reason for locking without needing a second command.\n\nSigned-off-by: Stephen Manz <smanz@alum.mit.edu>\n---\n Documentation/git-worktree.txt |  4 ++--\n builtin/worktree.c             | 21 ++++++++++++++++-----\n t/t2400-worktree-add.sh        | 14 ++++++++++++++\n 3 files changed, 32 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex 66e67e6cbfa..8a7cbdd19c1 100644\n--- a/Documentation/git-worktree.txt\n+++ b/Documentation/git-worktree.txt\n@@ -9,7 +9,7 @@ git-worktree - Manage multiple working trees\n SYNOPSIS\n --------\n [verse]\n-'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b <new-branch>] <path> [<commit-ish>]\n+'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] [-b <new-branch>] <path> [<commit-ish>]\n 'git worktree list' [--porcelain]\n 'git worktree lock' [--reason <string>] <worktree>\n 'git worktree move' <worktree> <new-path>\n@@ -242,7 +242,7 @@ With `list`, annotate missing working trees as prunable if they are\n older than `<time>`.\n \n --reason <string>::\n-\tWith `lock`, an explanation why the working tree is locked.\n+\tWith `lock` or with `add --lock`, an explanation why the working tree is locked.\n \n <worktree>::\n \tWorking trees can be identified by path, either relative or\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 4829b9507ff..0d0a80da61f 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -30,7 +30,7 @@ struct add_opts {\n \tint detach;\n \tint quiet;\n \tint checkout;\n-\tint keep_locked;\n+\tconst char *keep_locked;\n };\n \n static int show_only;\n@@ -302,10 +302,10 @@ static int add_worktree(const char *path, const char *refname,\n \t * after the preparation is over.\n \t */\n \tstrbuf_addf(&sb, \"%s/locked\", sb_repo.buf);\n-\tif (!opts->keep_locked)\n-\t\twrite_file(sb.buf, _(\"initializing\"));\n+\tif (opts->keep_locked)\n+\t\twrite_file(sb.buf, \"%s\", opts->keep_locked);\n \telse\n-\t\twrite_file(sb.buf, _(\"added with --lock\"));\n+\t\twrite_file(sb.buf, _(\"initializing\"));\n \n \tstrbuf_addf(&sb_git, \"%s/.git\", path);\n \tif (safe_create_leading_directories_const(sb_git.buf))\n@@ -475,6 +475,8 @@ static int add(int ac, const char **av, const char *prefix)\n \tconst char *branch;\n \tconst char *new_branch = NULL;\n \tconst char *opt_track = NULL;\n+\tconst char *lock_reason = NULL;\n+\tint keep_locked = 0;\n \tstruct option options[] = {\n \t\tOPT__FORCE(&opts.force,\n \t\t\t   N_(\"checkout <branch> even if already checked out in other worktree\"),\n@@ -485,7 +487,9 @@ static int add(int ac, const char **av, const char *prefix)\n \t\t\t   N_(\"create or reset a branch\")),\n \t\tOPT_BOOL('d', \"detach\", &opts.detach, N_(\"detach HEAD at named commit\")),\n \t\tOPT_BOOL(0, \"checkout\", &opts.checkout, N_(\"populate the new working tree\")),\n-\t\tOPT_BOOL(0, \"lock\", &opts.keep_locked, N_(\"keep the new working tree locked\")),\n+\t\tOPT_BOOL(0, \"lock\", &keep_locked, N_(\"keep the new working tree locked\")),\n+\t\tOPT_STRING(0, \"reason\", &lock_reason, N_(\"string\"),\n+\t\t\t   N_(\"reason for locking\")),\n \t\tOPT__QUIET(&opts.quiet, N_(\"suppress progress reporting\")),\n \t\tOPT_PASSTHRU(0, \"track\", &opt_track, NULL,\n \t\t\t     N_(\"set up tracking mode (see git-branch(1))\"),\n@@ -500,6 +504,13 @@ static int add(int ac, const char **av, const char *prefix)\n \tac = parse_options(ac, av, prefix, options, worktree_usage, 0);\n \tif (!!opts.detach + !!new_branch + !!new_branch_force > 1)\n \t\tdie(_(\"-b, -B, and --detach are mutually exclusive\"));\n+\tif (lock_reason && !keep_locked)\n+\t\tdie(_(\"--reason requires --lock\"));\n+\tif (lock_reason)\n+\t\topts.keep_locked = lock_reason;\n+\telse if (keep_locked)\n+\t\topts.keep_locked = _(\"added with --lock\");\n+\n \tif (ac < 1 || ac > 2)\n \t\tusage_with_options(worktree_usage, options);\n \ndiff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\nindex 93d3795cab9..37ad79470fb 100755\n--- a/t/t2400-worktree-add.sh\n+++ b/t/t2400-worktree-add.sh\n@@ -72,6 +72,20 @@ test_expect_success '\"add\" worktree with lock' '\n \ttest -f .git/worktrees/here-with-lock/locked\n '\n \n+test_expect_success '\"add\" worktree with lock and reason' '\n+\tlock_reason=\"why not\" &&\n+\tgit worktree add --detach --lock --reason \"$lock_reason\" here-with-lock-reason main &&\n+\ttest_when_finished \"git worktree unlock here-with-lock-reason || :\" &&\n+\ttest -f .git/worktrees/here-with-lock-reason/locked &&\n+\techo \"$lock_reason\" >expect &&\n+\ttest_cmp expect .git/worktrees/here-with-lock-reason/locked\n+'\n+\n+test_expect_success '\"add\" worktree with reason but no lock' '\n+\ttest_must_fail git worktree add --detach --reason \"why not\" here-with-reason-only main &&\n+\ttest_path_is_missing .git/worktrees/here-with-reason-only/locked\n+'\n+\n test_expect_success '\"add\" worktree from a subdir' '\n \t(\n \t\tmkdir sub &&\n-- \ngitgitgadget\n"},{"id":"430244","messageId":"CAPig+cT6KJ9=9pOS9jiXd0XQYdD0j1aZSyy29WyJ4w3x1dvqhg@mail.gmail.com","threadId":"56053","inReplyTo":"pull.992.v4.git.1626316350.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 0/3] worktree: teach add to accept --reason with --lock","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-07-15T17:17:37Z","receivedAt":"2021-07-15T17:17:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jul 14, 2021 at 10:32 PM Stephen Manz via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> The default reason stored in the lock file, \"added with --lock\", is unlikely\n> to be what the user would have given in a separate git worktree lock\n> command. Allowing --reason to be specified along with --lock when adding a\n> working tree gives the user control over the reason for locking without\n> needing a second command.\n>\n> Changes since v3:\n>\n>  * Updated test to define a shell variable, lock_reason, set to \"why not\".\n>    Expand the variable to use with --reason and to echo to file, expected.\n>    This avoids the spacing comment made by Eric Sunshine.\n\nThanks, these changes look fine to me. I don't know whether or not\nJunio has merged this series down to his \"next\" branch yet locally,\nthough. If he has, then he may not pick up your v4, and this minor\ncleanup change could instead be done as a standalone patch atop v3 (or\ncould be dropped since v3 was probably \"good enough\").\n\nIn any case, this series is still Reviewed-by: me.\n"},{"id":"430401","messageId":"CAPig+cSmkrQDDWgU-RTbAaswaGsG1xxgvOjxHjLc-8Bwgqk8wg@mail.gmail.com","threadId":"56053","inReplyTo":"CAPig+cT6KJ9=9pOS9jiXd0XQYdD0j1aZSyy29WyJ4w3x1dvqhg@mail.gmail.com","subject":"Re: [PATCH v4 0/3] worktree: teach add to accept --reason with --lock","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-07-17T00:14:45Z","receivedAt":"2021-07-17T00:15:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jul 15, 2021 at 1:17 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Wed, Jul 14, 2021 at 10:32 PM Stephen Manz via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n> > Changes since v3:\n> >\n> >  * Updated test to define a shell variable, lock_reason, set to \"why not\".\n> >    Expand the variable to use with --reason and to echo to file, expected.\n> >    This avoids the spacing comment made by Eric Sunshine.\n>\n> Thanks, these changes look fine to me. I don't know whether or not\n> Junio has merged this series down to his \"next\" branch yet locally,\n> though. If he has, then he may not pick up your v4, and this minor\n> cleanup change could instead be done as a standalone patch atop v3 (or\n> could be dropped since v3 was probably \"good enough\").\n\nIt looks like Junio did pick up v4, so no need for a standalone\ncleanup patch atop v3.\n"}]}