| Message ID | pull.992.v2.git.1625759443.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | worktree: teach add to accept --reason with --lock | expand |
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);
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.
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.
The default reason stored in the lock file, "added with --lock", is unlikely to be what the user would have given in a separate git worktree lock command. Allowing --reason to be specified along with --lock when adding a working tree gives the user control over the reason for locking without needing a second command. Changes since v1: * Split changes into 3 commits. The first commit is removal of git rev-parse in the test above the ones I'm adding. The second is wrapping the "added with --lock" string with _() to mark it for translation. The third commit is the main change. * Reworked the if-else-if-else to if-else if-else * Added test_when_finished ... command to unlock the working tree * Changed test_expect_failure to test_expect_success and embedded test_must_fail and test_path_is_missing commands Note: I don't see how to disambiguate --lock with no --reason from no --lock at all. I still think that the original keep_locked boolean is needed along with the new lock_reason char array. If I don't add lock_reason and change keep_locked to a char array, it will start as NULL. But it will remain NULL if --lock alone is given or if --lock isn't given at all. Stephen Manz (3): t2400: remove unneeded `git rev-parse` from '"add" worktree with lock' test worktree: default lock string should be marked with `_()` for translation worktree: teach `add` to accept --reason <string> with --lock Documentation/git-worktree.txt | 4 ++-- builtin/worktree.c | 9 ++++++++- t/t2400-worktree-add.sh | 14 +++++++++++++- 3 files changed, 23 insertions(+), 4 deletions(-) base-commit: 670b81a890388c60b7032a4f5b879f2ece8c4558 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-992%2FSRManz%2Flock_reason-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-992/SRManz/lock_reason-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/992 Range-diff vs v1: -: ----------- > 1: 5459e5bb421 t2400: remove unneeded `git rev-parse` from '"add" worktree with lock' test -: ----------- > 2: 30196cc9369 worktree: default lock string should be marked with `_()` for translation 1: 233a580b212 ! 3: 4d17b31921a worktree: teach `add` to accept --reason <string> with --lock @@ builtin/worktree.c: struct add_opts { static int show_only; @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refname, - * after the preparation is over. - */ strbuf_addf(&sb, "%s/locked", sb_repo.buf); -- if (!opts->keep_locked) -+ if (!opts->keep_locked) { + if (!opts->keep_locked) write_file(sb.buf, "initializing"); -- else -- write_file(sb.buf, "added with --lock"); -+ } -+ else { -+ if (opts->lock_reason) -+ write_file(sb.buf, "%s", opts->lock_reason); -+ else -+ write_file(sb.buf, _("added with --lock")); -+ } ++ else if (opts->lock_reason) ++ write_file(sb.buf, "%s", opts->lock_reason); + else + write_file(sb.buf, _("added with --lock")); - strbuf_addf(&sb_git, "%s/.git", path); - if (safe_create_leading_directories_const(sb_git.buf)) @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix) OPT_BOOL('d', "detach", &opts.detach, N_("detach HEAD at named commit")), OPT_BOOL(0, "checkout", &opts.checkout, N_("populate the new working tree")), @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix) ## t/t2400-worktree-add.sh ## -@@ t/t2400-worktree-add.sh: test_expect_success '"add" worktree' ' - ' - - test_expect_success '"add" worktree with lock' ' -- git rev-parse HEAD >expect && - git worktree add --detach --lock here-with-lock main && +@@ t/t2400-worktree-add.sh: test_expect_success '"add" worktree with lock' ' test -f .git/worktrees/here-with-lock/locked ' +test_expect_success '"add" worktree with lock and reason' ' + git worktree add --detach --lock --reason "why not" here-with-lock-reason main && ++ test_when_finished "git worktree unlock here-with-lock-reason || :" && + test -f .git/worktrees/here-with-lock-reason/locked && + echo why not >expect && + test_cmp expect .git/worktrees/here-with-lock-reason/locked +' + -+test_expect_failure '"add" worktree with reason but no lock' ' -+ git worktree add --detach --reason "why not" here-with-reason-only main && -+ test -f .git/worktrees/here-with-reason-only/locked ++test_expect_success '"add" worktree with reason but no lock' ' ++ test_must_fail git worktree add --detach --reason "why not" here-with-reason-only main && ++ test_path_is_missing .git/worktrees/here-with-reason-only/locked +' + test_expect_success '"add" worktree from a subdir' '