* [PATCH v2 1/3] t2400: remove unneeded `git rev-parse` from '"add" worktree with lock' test
2021-07-08 15:50 ` [PATCH v2 0/3] worktree: teach add to accept --reason " Stephen Manz via GitGitGadget
@ 2021-07-08 15:50 ` Stephen Manz via GitGitGadget
2021-07-08 15:50 ` [PATCH v2 2/3] worktree: default lock string should be marked with `_()` for translation Stephen Manz via GitGitGadget
` (3 subsequent siblings)
4 siblings, 0 replies; 27+ messages in thread
From: Stephen Manz via GitGitGadget @ 2021-07-08 15:50 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Stephen Manz, Stephen Manz
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 2/3] worktree: default lock string should be marked with `_()` for translation
2021-07-08 15:50 ` [PATCH v2 0/3] worktree: teach add to accept --reason " Stephen Manz via GitGitGadget
2021-07-08 15:50 ` [PATCH v2 1/3] t2400: remove unneeded `git rev-parse` from '"add" worktree with lock' test Stephen Manz via GitGitGadget
@ 2021-07-08 15:50 ` Stephen Manz via GitGitGadget
2021-07-09 6:19 ` Eric Sunshine
2021-07-08 15:50 ` [PATCH v2 3/3] worktree: teach `add` to accept --reason <string> with --lock Stephen Manz via GitGitGadget
` (2 subsequent siblings)
4 siblings, 1 reply; 27+ messages in thread
From: Stephen Manz via GitGitGadget @ 2021-07-08 15:50 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Stephen Manz, Stephen Manz
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/3] worktree: default lock string should be marked with `_()` for translation
2021-07-08 15:50 ` [PATCH v2 2/3] worktree: default lock string should be marked with `_()` for translation Stephen Manz via GitGitGadget
@ 2021-07-09 6:19 ` Eric Sunshine
2021-07-09 15:58 ` Junio C Hamano
0 siblings, 1 reply; 27+ messages in thread
From: Eric Sunshine @ 2021-07-09 6:19 UTC (permalink / raw)
To: Stephen Manz via GitGitGadget; +Cc: Git List, Stephen Manz
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"));
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/3] worktree: default lock string should be marked with `_()` for translation
2021-07-09 6:19 ` Eric Sunshine
@ 2021-07-09 15:58 ` Junio C Hamano
0 siblings, 0 replies; 27+ messages in thread
From: Junio C Hamano @ 2021-07-09 15:58 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Stephen Manz via GitGitGadget, Git List, Stephen Manz
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 3/3] worktree: teach `add` to accept --reason <string> with --lock
2021-07-08 15:50 ` [PATCH v2 0/3] worktree: teach add to accept --reason " Stephen Manz via GitGitGadget
2021-07-08 15:50 ` [PATCH v2 1/3] t2400: remove unneeded `git rev-parse` from '"add" worktree with lock' test Stephen Manz via GitGitGadget
2021-07-08 15:50 ` [PATCH v2 2/3] worktree: default lock string should be marked with `_()` for translation Stephen Manz via GitGitGadget
@ 2021-07-08 15:50 ` Stephen Manz via GitGitGadget
2021-07-09 7:45 ` [PATCH v2 0/3] worktree: teach add to accept --reason " Eric Sunshine
2021-07-11 0:27 ` [PATCH v3 " Stephen Manz via GitGitGadget
4 siblings, 0 replies; 27+ messages in thread
From: Stephen Manz via GitGitGadget @ 2021-07-08 15:50 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Stephen Manz, Stephen Manz
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] worktree: teach add to accept --reason with --lock
2021-07-08 15:50 ` [PATCH v2 0/3] worktree: teach add to accept --reason " Stephen Manz via GitGitGadget
` (2 preceding siblings ...)
2021-07-08 15:50 ` [PATCH v2 3/3] worktree: teach `add` to accept --reason <string> with --lock Stephen Manz via GitGitGadget
@ 2021-07-09 7:45 ` Eric Sunshine
2021-07-09 16:03 ` Junio C Hamano
2021-07-11 0:27 ` [PATCH v3 " Stephen Manz via GitGitGadget
4 siblings, 1 reply; 27+ messages in thread
From: Eric Sunshine @ 2021-07-09 7:45 UTC (permalink / raw)
To: Stephen Manz via GitGitGadget; +Cc: git, Stephen Manz
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] worktree: teach add to accept --reason with --lock
2021-07-09 7:45 ` [PATCH v2 0/3] worktree: teach add to accept --reason " Eric Sunshine
@ 2021-07-09 16:03 ` Junio C Hamano
2021-07-10 7:15 ` Eric Sunshine
0 siblings, 1 reply; 27+ messages in thread
From: Junio C Hamano @ 2021-07-09 16:03 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Stephen Manz via GitGitGadget, git, Stephen Manz
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] worktree: teach add to accept --reason with --lock
2021-07-09 16:03 ` Junio C Hamano
@ 2021-07-10 7:15 ` Eric Sunshine
0 siblings, 0 replies; 27+ messages in thread
From: Eric Sunshine @ 2021-07-10 7:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stephen Manz via GitGitGadget, Git List, Stephen Manz
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 0/3] worktree: teach add to accept --reason with --lock
2021-07-08 15:50 ` [PATCH v2 0/3] worktree: teach add to accept --reason " Stephen Manz via GitGitGadget
` (3 preceding siblings ...)
2021-07-09 7:45 ` [PATCH v2 0/3] worktree: teach add to accept --reason " Eric Sunshine
@ 2021-07-11 0:27 ` Stephen Manz via GitGitGadget
2021-07-11 0:27 ` [PATCH v3 1/3] t2400: clean up '"add" worktree with lock' test Stephen Manz via GitGitGadget
` (4 more replies)
4 siblings, 5 replies; 27+ messages in thread
From: Stephen Manz via GitGitGadget @ 2021-07-11 0:27 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Stephen Manz
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 1/3] t2400: clean up '"add" worktree with lock' test
2021-07-11 0:27 ` [PATCH v3 " Stephen Manz via GitGitGadget
@ 2021-07-11 0:27 ` Stephen Manz via GitGitGadget
2021-07-11 0:27 ` [PATCH v3 2/3] worktree: mark lock strings with `_()` for translation Stephen Manz via GitGitGadget
` (3 subsequent siblings)
4 siblings, 0 replies; 27+ messages in thread
From: Stephen Manz via GitGitGadget @ 2021-07-11 0:27 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Stephen Manz, Stephen Manz
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 2/3] worktree: mark lock strings with `_()` for translation
2021-07-11 0:27 ` [PATCH v3 " Stephen Manz via GitGitGadget
2021-07-11 0:27 ` [PATCH v3 1/3] t2400: clean up '"add" worktree with lock' test Stephen Manz via GitGitGadget
@ 2021-07-11 0:27 ` Stephen Manz via GitGitGadget
2021-07-11 0:27 ` [PATCH v3 3/3] worktree: teach `add` to accept --reason <string> with --lock Stephen Manz via GitGitGadget
` (2 subsequent siblings)
4 siblings, 0 replies; 27+ messages in thread
From: Stephen Manz via GitGitGadget @ 2021-07-11 0:27 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Stephen Manz, Stephen Manz
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 3/3] worktree: teach `add` to accept --reason <string> with --lock
2021-07-11 0:27 ` [PATCH v3 " Stephen Manz via GitGitGadget
2021-07-11 0:27 ` [PATCH v3 1/3] t2400: clean up '"add" worktree with lock' test Stephen Manz via GitGitGadget
2021-07-11 0:27 ` [PATCH v3 2/3] worktree: mark lock strings with `_()` for translation Stephen Manz via GitGitGadget
@ 2021-07-11 0:27 ` Stephen Manz via GitGitGadget
2021-07-13 3:37 ` Eric Sunshine
2021-07-13 3:47 ` [PATCH v3 0/3] worktree: teach add to accept --reason " Eric Sunshine
2021-07-15 2:32 ` [PATCH v4 " Stephen Manz via GitGitGadget
4 siblings, 1 reply; 27+ messages in thread
From: Stephen Manz via GitGitGadget @ 2021-07-11 0:27 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Stephen Manz, Stephen Manz
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v3 3/3] worktree: teach `add` to accept --reason <string> with --lock
2021-07-11 0:27 ` [PATCH v3 3/3] worktree: teach `add` to accept --reason <string> with --lock Stephen Manz via GitGitGadget
@ 2021-07-13 3:37 ` Eric Sunshine
0 siblings, 0 replies; 27+ messages in thread
From: Eric Sunshine @ 2021-07-13 3:37 UTC (permalink / raw)
To: Stephen Manz via GitGitGadget; +Cc: Git List, Stephen Manz
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 0/3] worktree: teach add to accept --reason with --lock
2021-07-11 0:27 ` [PATCH v3 " Stephen Manz via GitGitGadget
` (2 preceding siblings ...)
2021-07-11 0:27 ` [PATCH v3 3/3] worktree: teach `add` to accept --reason <string> with --lock Stephen Manz via GitGitGadget
@ 2021-07-13 3:47 ` Eric Sunshine
2021-07-15 2:32 ` [PATCH v4 " Stephen Manz via GitGitGadget
4 siblings, 0 replies; 27+ messages in thread
From: Eric Sunshine @ 2021-07-13 3:47 UTC (permalink / raw)
To: Stephen Manz via GitGitGadget; +Cc: Git List, Stephen Manz
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v4 0/3] worktree: teach add to accept --reason with --lock
2021-07-11 0:27 ` [PATCH v3 " Stephen Manz via GitGitGadget
` (3 preceding siblings ...)
2021-07-13 3:47 ` [PATCH v3 0/3] worktree: teach add to accept --reason " Eric Sunshine
@ 2021-07-15 2:32 ` Stephen Manz via GitGitGadget
2021-07-15 2:32 ` [PATCH v4 1/3] t2400: clean up '"add" worktree with lock' test Stephen Manz via GitGitGadget
` (3 more replies)
4 siblings, 4 replies; 27+ messages in thread
From: Stephen Manz via GitGitGadget @ 2021-07-15 2:32 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Stephen Manz
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v4 1/3] t2400: clean up '"add" worktree with lock' test
2021-07-15 2:32 ` [PATCH v4 " Stephen Manz via GitGitGadget
@ 2021-07-15 2:32 ` Stephen Manz via GitGitGadget
2021-07-15 2:32 ` [PATCH v4 2/3] worktree: mark lock strings with `_()` for translation Stephen Manz via GitGitGadget
` (2 subsequent siblings)
3 siblings, 0 replies; 27+ messages in thread
From: Stephen Manz via GitGitGadget @ 2021-07-15 2:32 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Stephen Manz, Stephen Manz
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v4 2/3] worktree: mark lock strings with `_()` for translation
2021-07-15 2:32 ` [PATCH v4 " Stephen Manz via GitGitGadget
2021-07-15 2:32 ` [PATCH v4 1/3] t2400: clean up '"add" worktree with lock' test Stephen Manz via GitGitGadget
@ 2021-07-15 2:32 ` Stephen Manz via GitGitGadget
2021-07-15 2:32 ` [PATCH v4 3/3] worktree: teach `add` to accept --reason <string> with --lock Stephen Manz via GitGitGadget
2021-07-15 17:17 ` [PATCH v4 0/3] worktree: teach add to accept --reason " Eric Sunshine
3 siblings, 0 replies; 27+ messages in thread
From: Stephen Manz via GitGitGadget @ 2021-07-15 2:32 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Stephen Manz, Stephen Manz
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v4 3/3] worktree: teach `add` to accept --reason <string> with --lock
2021-07-15 2:32 ` [PATCH v4 " Stephen Manz via GitGitGadget
2021-07-15 2:32 ` [PATCH v4 1/3] t2400: clean up '"add" worktree with lock' test Stephen Manz via GitGitGadget
2021-07-15 2:32 ` [PATCH v4 2/3] worktree: mark lock strings with `_()` for translation Stephen Manz via GitGitGadget
@ 2021-07-15 2:32 ` Stephen Manz via GitGitGadget
2021-07-15 17:17 ` [PATCH v4 0/3] worktree: teach add to accept --reason " Eric Sunshine
3 siblings, 0 replies; 27+ messages in thread
From: Stephen Manz via GitGitGadget @ 2021-07-15 2:32 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Stephen Manz, Stephen Manz
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v4 0/3] worktree: teach add to accept --reason with --lock
2021-07-15 2:32 ` [PATCH v4 " Stephen Manz via GitGitGadget
` (2 preceding siblings ...)
2021-07-15 2:32 ` [PATCH v4 3/3] worktree: teach `add` to accept --reason <string> with --lock Stephen Manz via GitGitGadget
@ 2021-07-15 17:17 ` Eric Sunshine
2021-07-17 0:14 ` Eric Sunshine
3 siblings, 1 reply; 27+ messages in thread
From: Eric Sunshine @ 2021-07-15 17:17 UTC (permalink / raw)
To: Stephen Manz via GitGitGadget; +Cc: Git List, Stephen Manz
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 0/3] worktree: teach add to accept --reason with --lock
2021-07-15 17:17 ` [PATCH v4 0/3] worktree: teach add to accept --reason " Eric Sunshine
@ 2021-07-17 0:14 ` Eric Sunshine
0 siblings, 0 replies; 27+ messages in thread
From: Eric Sunshine @ 2021-07-17 0:14 UTC (permalink / raw)
To: Stephen Manz via GitGitGadget; +Cc: Git List, Stephen Manz
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.
^ permalink raw reply [flat|nested] 27+ messages in thread