Re: [PATCH v2 4/6] worktree: teach `list --porcelain` to annotate locked worktree

>> Additionally, c57b3367be (worktree: teach `list` to annotate locked
>> worktree, 2020-10-11) introduced a new test to ensure locked worktrees
>> are listed with "locked" annotation. However, the test does not clean up
>> after itself as "git worktree prune" is not going to remove the locked
>> worktree in the first place. This not only leaves the test in an unclean
>> state it also potentially breaks following tests that relies on the
>> "git worktree list" output. Let's fix that by unlocking the worktree
>> before the "prune" command.
>
> The actual code change to fix this bug is about as minimal as it gets,
> but the explanation you've written here is lengthy enough and nicely
> self-contained that it suggests splitting it off to its own patch. And
> since you can re-use this paragraph almost verbatim as the commit
> message, it shouldn't require much work to do so. On the other hand,
> it is itself not necessarily worth a re-roll, but if you do re-roll,
> perhaps it's worth considering.
>

>> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
>> @@ -377,8 +377,10 @@ Porcelain Format
>>  The porcelain format has a line per attribute.  Attributes are listed with a
>>  label and value separated by a single space.  Boolean attributes (like `bare`
>>  and `detached`) are listed as a label only, and are present only
>> -if the value is true.  The first attribute of a working tree is always
>> -`worktree`, an empty line indicates the end of the record.  For example:
>> +if the value is true.  Some attributes (like `locked`) can be listed as a label
>> +only or with a value depending whether a reason is available.  The first
>
> Perhaps:
> s/depending whether/depending upon whether/
>

>> +attribute of a working tree is always `worktree`, an empty line indicates the
>> +end of the record.  For example:
>> @@ -393,6 +395,16 @@ worktree /path/to/other-linked-worktree
>> +worktree /path/to/linked-worktree-locked
>> +HEAD 5678abc5678abc5678abc5678abc5678abc5678c
>> +branch refs/heads/locked
>> +locked
>> +
>> +worktree /path/to/linked-worktree-locked-with-reason
>> +HEAD 3456def3456def3456def3456def3456def3456b
>> +branch refs/heads/locked-with-reason
>> +locked reason why is locked
>
> I was momentarily confused by the branch named `locked` with the
> `locked` attribute in the first new stanza. Perhaps take a hint from
> the second new stanza and call the first one `locked-no-reason`:
>
>     worktree /path/to/linked-worktree-locked-no-reason
>     HEAD 5678abc5678abc5678abc5678abc5678abc5678c
>     branch refs/heads/locked-no-reason
>     locked
>
> Again, though, not worth a re-roll.
>

>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> @@ -579,6 +582,16 @@ static void show_worktree_porcelain(struct worktree *wt)
>> +       reason = worktree_lock_reason(wt);
>> +       if (reason && *reason) {
>> +               struct strbuf sb = STRBUF_INIT;
>> +               quote_c_style(reason, &sb, NULL, CQUOTE_NODQ);
>> +               printf("locked %s\n", sb.buf);
>> +               strbuf_release(&sb);
>> +       } else if (reason)
>> +               printf("locked\n");
>
> This needs a change, and it's totally my fault that it does. In my
> previous review, I mentioned that if the lock reason contains special
> characters, we want those special characters escaped and the reason
> quoted, but _only_ if it contains special characters. However, I then
> incorrectly said to call quote_c_style() with CQUOTE_NODQ to achieve
> that behavior. In fact, CQUOTE_NODQ gives us the wrong behavior since
> it avoids quoting the string which, as Phillip pointed out, makes it
> impossible to distinguish between a string which just happens to
> contain the two-character sequence '\' and 'n', and an escaped newline
> "\n". So, the above should really be:
>
>     quote_c_style(reason, &sb, NULL, 0);
>
> The example in the commit message should be adjusted to account for
> this change, as well:
>
>     In porcelain mode, if the lock reason contains special characters
>     such as newlines, they are escaped with backslashes and the entire
>     reason is enclosed in double quotes. For example:
>
>     $ git worktree list --porcelain
>     ...
>     locked "worktree's path mounted in\nremovable device"
>     ...
>
> And, of course, the new test will need a slight adjustment.
>

>> diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
>> @@ -66,11 +66,43 @@ test_expect_success '"list" all worktrees with locked annotation' '
>> +test_expect_success '"list" all worktrees --porcelain with locked' '
>> +       test_when_finished "rm -rf locked1 locked2 unlocked out actual expect && git worktree prune" &&
>> +       echo "locked" >expect &&
>> +       echo "locked with reason" >>expect &&
>> +       git worktree add --detach locked1 &&
>> +       git worktree add --detach locked2 &&
>> +       git worktree add --detach unlocked &&
>> +       git worktree lock locked1 &&
>> +       git worktree lock locked2 --reason "with reason" &&
>> +       test_when_finished "git worktree unlock locked1 && git worktree unlock locked2" &&
>> +       git worktree list --porcelain >out &&
>> +       grep "^locked" out >actual &&
>> +       test_cmp expect actual
>> +'
>
> So, the purpose of the `unlocked` worktree in this test is to prove
> that it didn't accidentally get annotated with `locked`? (Since, if it
> did get annotated, then `actual` would contain too many lines and not
> match `expect`.) Is that correct?
>

>> +test_expect_success '"list" all worktrees --porcelain with locked reason newline escaped' '
>> +       test_when_finished "rm -rf locked_lf locked_crlf out actual expect && git worktree prune" &&
>> +       printf "locked locked\\\\r\\\\nreason\n" >expect &&
>> +       printf "locked locked\\\\nreason\n" >>expect &&
>> +       git worktree add --detach locked_lf &&
>> +       git worktree add --detach locked_crlf &&
>> +       printf "locked\nreason\n\n" >reason_lf &&
>> +       printf "locked\r\nreason\n\n" >reason_crlf &&
>
> The trailing "\n\n" is unneeded. Due to the way `$(...)` expansion
> works (dropping trailing whitespace), you'll get the same successful
> test result with:
>
>     printf "locked\nreason\n" >reason_lf &&
>     printf "locked\r\nreason\n" >reason_crlf &&
>
> and even with:
>
>     printf "locked\nreason" >reason_lf &&
>     printf "locked\r\nreason" >reason_crlf &&
>
>> +       git worktree lock locked_lf --reason "$(cat reason_lf)" &&
>> +       git worktree lock locked_crlf --reason "$(cat reason_crlf)" &&
>
> You could also just embed the `printf`'s here rather than using these
> temporary files.
>
>     git worktree lock --reason $(printf "...") <path> &&
>

> Or, if we care only about testing LF, and not about CRLF, even this would work:
>
>     git worktree lock --reason "reason with
>     newline" <path> &&
>
> but that gets a bit ugly.
>
> Anyhow, all the line terminator commentary about this test is a matter
> of personal taste, probably not worth a re-roll or even changing.