worktree: teach add to accept --reason <string> with --lock

[SRManz]

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.

Thanks for taking the time to contribute to Git! Please be advised that the
Git community does not use github.com for their contributions. Instead, we use
a mailing list (git@vger.kernel.org) for code submissions, code reviews, and
bug reports. Nevertheless, you can use GitGitGadget (https://gitgitgadget.github.io/)
to conveniently send your Pull Requests commits to our mailing list.

Please read the "guidelines for contributing" linked above!

cc: Eric Sunshine sunshine@sunshineco.com

cc: Eric Sunshine sunshine@sunshineco.com

[gitgitgadget]

Welcome to GitGitGadget

Hi @SRManz, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that your Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

• the lines should not exceed 76 columns,
• the first line should be like a header and typically start with a prefix like "tests:" or "revisions:" to state which subsystem the change is about, and
• the commit messages' body should be describing the "why?" of the change.
• Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

• Search: is:pr is:open "/allow"

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the Libera Chat IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Libera Chat. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

[gitgitgadget]

There is an issue in commit 6c08695:
Commit not signed off

[SRManz]

@forivall Could you /allow me, please? Thanks.

[SRManz]

/preview

[gitgitgadget]

Error: User SRManz is not permitted to use GitGitGadget

[dscho]

/allow

[gitgitgadget]

User SRManz is now allowed to use GitGitGadget.

WARNING: SRManz has no public email address set on GitHub

[SRManz]

/preview

[gitgitgadget]

Error: Could not determine full name of SRManz

[SRManz]

/preview

[gitgitgadget]

Error: Could not determine public email of SRManz

[SRManz]

/submit

[gitgitgadget]

Submitted as pull.992.git.1625550451038.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-992/SRManz/lock_reason-v1

To fetch this version to local tag pr-992/SRManz/lock_reason-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-992/SRManz/lock_reason-v1

[gitgitgadget]

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Tue, Jul 6, 2021 at 1:47 AM Stephen Manz via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> The default reason stored in the lock file, "added with --lock",
> is unlikely to be what the user would have given in a separate
> `git worktree lock` command. Allowing `--reason` to be specified
> along with `--lock` when adding a working tree gives the user control
> over the reason for locking without needing a second command.

Thanks. I can see how the default lock reason in this case isn't very
interesting. In fact, I'd actually have expected the default reason to
be empty, just as it's empty when `git worktree lock` is invoked with
no `--reason`. That might be an additional thing to "fix" at some
point by someone (or in this series if you'd like to tackle it).

It's nice to see both documentation and test updates along with the
code change. See below for a few comments...

> Signed-off-by: Stephen Manz <smanz@alum.mit.edu>
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -9,7 +9,7 @@ git-worktree - Manage multiple working trees
> +'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] [-b <new-branch>] <path> [<commit-ish>]
>
>  --reason <string>::
> +       With `lock` or with `add --lock`, an explanation why the working tree is locked.

I realize that you're mimicking the interface of `git worktree lock`
which accepts an optional `--reason`, but I'm wondering if the
user-experience might be improved by instead allowing `--lock` to
accept the reason as an optional argument. For instance:

    git worktree add --lock='just because' ...

Aside from the dissymmetry with `git worktree lock`, I haven't come up
with a reason that `--lock[=<reason>]` wouldn't be an improvement. But
perhaps I'm not imaginative enough...

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -31,6 +31,7 @@ struct add_opts {
>         int checkout;
>         int keep_locked;
> +       const char *lock_reason;
>  };

Whether or not we do go with the approach of allowing `--lock` to take
the reason as an optional argument, we don't really need two structure
members here. Instead, we can repurpose `keep_locked` as a `const char
*` which is NULL if `--lock` was not specified, otherwise non-NULL.
For the non-NULL case, it would be an empty (zero-length) string if
the optional `<reason>` was omitted, otherwise it would be the reason
given. So, no need for a distinct `lock_reason` member.

> @@ -302,10 +303,15 @@ static int add_worktree(const char *path, const char *refname,
> +       if (!opts->keep_locked) {
>                 write_file(sb.buf, "initializing");
> -       else
> -               write_file(sb.buf, "added with --lock");
> +       }
> +       else {
> +               if (opts->lock_reason)
> +                       write_file(sb.buf, "%s", opts->lock_reason);
> +               else
> +                       write_file(sb.buf, _("added with --lock"));
> +       }

Style on this project is to cuddle `else` with both braces:

    } else  {

However, in this case, it should probably just be a simple `else if`:

    if (!opts->keep_locked)
        write_file(sb.buf, "initializing");
    else if (opts->lock_reason)
        write_file(sb.buf, "%s", opts->lock_reason);
    else
        write_file(sb.buf, _("added with --lock"));

> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> @@ -67,11 +67,22 @@ test_expect_success '"add" worktree' '
>  test_expect_success '"add" worktree with lock' '
> -       git rev-parse HEAD >expect &&
>         git worktree add --detach --lock here-with-lock main &&
>         test -f .git/worktrees/here-with-lock/locked
>  '

Dropping this unused code makes sense, though on this project we would
normally do it as a separate (probably preparatory) patch, thus making
this a two-patch series (at minimum).

> +test_expect_success '"add" worktree with lock and reason' '
> +       git worktree add --detach --lock --reason "why not" here-with-lock-reason main &&
> +       test -f .git/worktrees/here-with-lock-reason/locked &&
> +       echo why not >expect &&
> +       test_cmp expect .git/worktrees/here-with-lock-reason/locked
> +'

To make this a bit friendlier to other tests which come later in the
script, it might be a good idea to unlock this worktree here at its
source. To do so unconditionally, whether the test succeeds or fails,
use test_when_finished() just after placing the lock. So, something
like this:

    git worktree add --detach --lock --reason ... &&
    test_when_finished "git worktree unlock here-with-lock-reason || :" &&
    test -f .git/worktrees/here-with-lock-reason/locked &&
    ...

> +test_expect_failure '"add" worktree with reason but no lock' '
> +       git worktree add --detach --reason "why not" here-with-reason-only main &&
> +       test -f .git/worktrees/here-with-reason-only/locked
> +'

test_expect_failure() is for indicating known bugs or unimplemented
features, however, you did implement the check that `--reason`
requires `--lock`, so test_expect_failure() is not quite what you want
here. Instead, use test_must_fail() in the test body, and you want to
make sure that the `locked` file did not get created. So, something
like this:

    test_expect_success '"add" worktree with reason but no lock' '
        test_must_fail git worktree add --detach --reason ... &&
        test_path_is_missing .git/worktrees/here-with-reason-only/locked
    '

[gitgitgadget]

User Eric Sunshine <sunshine@sunshineco.com> has been added to the cc: list.

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Eric Sunshine <sunshine@sunshineco.com> writes:

>>  --reason <string>::
>> +       With `lock` or with `add --lock`, an explanation why the working tree is locked.
>
> I realize that you're mimicking the interface of `git worktree lock`
> which accepts an optional `--reason`, but I'm wondering if the
> user-experience might be improved by instead allowing `--lock` to
> accept the reason as an optional argument. For instance:
>
>     git worktree add --lock='just because' ...

Thanks for thinking aloud, but I'd prefer the interface as posted,
simply because there is one less thing for users to remember.  The
justification to lock is given with the --reason=<why> argument no
matter how you acquire the lock on a worktree.


>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> @@ -31,6 +31,7 @@ struct add_opts {
>>         int checkout;
>>         int keep_locked;
>> +       const char *lock_reason;
>>  };
>
> Whether or not we do go with the approach of allowing `--lock` to take
> the reason as an optional argument, we don't really need two structure
> members here. Instead, we can repurpose `keep_locked` as a `const char
> *` which is NULL if `--lock` was not specified, otherwise non-NULL.

Makes sense.

> However, in this case, it should probably just be a simple `else if`:
>
>     if (!opts->keep_locked)
>         write_file(sb.buf, "initializing");
>     else if (opts->lock_reason)
>         write_file(sb.buf, "%s", opts->lock_reason);
>     else
>         write_file(sb.buf, _("added with --lock"));

Excellent.

Thanks.

[SRManz]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Eric Sunshine <sunshine@sunshineco.com> writes:

>>  --reason <string>::
>> +       With `lock` or with `add --lock`, an explanation why the working tree is locked.
>
> I realize that you're mimicking the interface of `git worktree lock`
> which accepts an optional `--reason`, but I'm wondering if the
> user-experience might be improved by instead allowing `--lock` to
> accept the reason as an optional argument. For instance:
>
>     git worktree add --lock='just because' ...

Thanks for thinking aloud, but I'd prefer the interface as posted,
simply because there is one less thing for users to remember.  The
justification to lock is given with the --reason=<why> argument no
matter how you acquire the lock on a worktree.

I had considered something like what Eric suggested, but opted to keep --reason "why" for the reason Junio gave.

diff --git a/builtin/worktree.c b/builtin/worktree.c
@@ -31,6 +31,7 @@ struct add_opts {
int checkout;
int keep_locked;

•   const char *lock_reason;
  

};

Whether or not we do go with the approach of allowing --lock to take
the reason as an optional argument, we don't really need two structure
members here. Instead, we can repurpose keep_locked as a const char * which is NULL if --lock was not specified, otherwise non-NULL.

Makes sense.

To me, that would create ambiguity. --lock with no --reason would have a NULL string, which would be the same as if --lock weren't given at all. Am I missing something?

However, in this case, it should probably just be a simple else if:

if (!opts->keep_locked)
    write_file(sb.buf, "initializing");
else if (opts->lock_reason)
    write_file(sb.buf, "%s", opts->lock_reason);
else
    write_file(sb.buf, _("added with --lock"));

Excellent.

Wow! What was I thinking! Note the review item I created regarding my placing _() around "added with --lock". Should I do a separate commit?

Thanks.

My apologies if I'm not following the conventions for comments on pull requests. Lots to learn...

[SRManz]

Accidentally clicked Close with comment button.

[gitgitgadget]

This branch is now known as sm/worktree-add-lock.

[gitgitgadget]

This patch series was integrated into seen via git@5eb1070.

[gitgitgadget]

There was a status update in the "New Topics" section about the branch sm/worktree-add-lock on the Git mailing list:

"git worktree add --lock" learned to record why the worktree is
locked with a custom message.

Expecting a reroll.

[gitgitgadget]

This patch series was integrated into seen via git@069e085.

[gitgitgadget]

This patch series was integrated into seen via git@731f3c7.

[dscho]

@SRManz if you want to send a v2, please /submit again. (I would also recommend editing the initial comment, which is sent as "cover letter", to include a summary of the changes, making any re-review easier.)

[SRManz]

/submit

[gitgitgadget]

Submitted as pull.992.v3.git.1625963240.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-992/SRManz/lock_reason-v3

To fetch this version to local tag pr-992/SRManz/lock_reason-v3:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-992/SRManz/lock_reason-v3

[gitgitgadget]

This patch series was integrated into seen via git@dcb1c1e.

[gitgitgadget]

On the Git mailing list, Eric Sunshine wrote (reply to this):

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.

[gitgitgadget]

On the Git mailing list, Eric Sunshine wrote (reply to this):

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.

[gitgitgadget]

This patch series was integrated into seen via git@2d4dbbc.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch sm/worktree-add-lock on the Git mailing list:

"git worktree add --lock" learned to record why the worktree is
locked with a custom message.

Ready?

[gitgitgadget]

This patch series was integrated into seen via git@a74a0c7.

[SRManz]

/submit

[gitgitgadget]

Submitted as pull.992.v4.git.1626316350.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-992/SRManz/lock_reason-v4

To fetch this version to local tag pr-992/SRManz/lock_reason-v4:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-992/SRManz/lock_reason-v4

[gitgitgadget]

On the Git mailing list, Eric Sunshine wrote (reply to this):

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.

[gitgitgadget]

This patch series was integrated into seen via git@a288ac1.

[gitgitgadget]

On the Git mailing list, Eric Sunshine wrote (reply to this):

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.

[gitgitgadget]

This patch series was integrated into seen via git@77e72f7.

[gitgitgadget]

This patch series was integrated into next via git@609c0a4.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch sm/worktree-add-lock on the Git mailing list:

"git worktree add --lock" learned to record why the worktree is
locked with a custom message.

Will merge to 'master'.

[gitgitgadget]

This patch series was integrated into seen via git@7dcd64c.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch sm/worktree-add-lock on the Git mailing list:

"git worktree add --lock" learned to record why the worktree is
locked with a custom message.

Will merge to 'master'.

[gitgitgadget]

This patch series was integrated into seen via git@70f0cf7.

[gitgitgadget]

This patch series was integrated into seen via git@33911ed.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch sm/worktree-add-lock on the Git mailing list:

"git worktree add --lock" learned to record why the worktree is
locked with a custom message.

Will merge to 'master'.

[gitgitgadget]

This patch series was integrated into seen via git@01369fd.

[gitgitgadget]

This patch series was integrated into next via git@01369fd.

[gitgitgadget]

This patch series was integrated into master via git@01369fd.

[gitgitgadget]

Closed via 01369fd.