| Message ID | pull.992.v4.git.1626316350.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | worktree: teach add to accept --reason with --lock | expand |
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.
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.
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 +' +