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

On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva
[off-list ref] wrote:

Commit c57b3367be (worktree: teach `list` to annotate locked worktree,
2020-10-11) taught "git worktree list" to annotate locked worktrees by
appending "locked" text to its output, however, this is not listed in
the --porcelain format.

Teach "list --porcelain" to do the same and add a "locked" attribute
followed by its reason, thus making both default and porcelain format
consistent. If the locked reason is not available then only "locked"
is shown.

The output of the "git worktree list --porcelain" becomes like so:

    $ git worktree list --porcelain
    ...
    worktree /path/to/locked
    HEAD 123abcdea123abcd123acbd123acbda123abcd12
    detached
    locked

    worktree /path/to/locked-with-reason
    HEAD abc123abc123abc123abc123abc123abc123abc1
    detached
    locked reason why it is locked
    ...

All good.

The locked reason is quoted to prevent newline characters (i.e: LF or
CRLF) breaking the --porcelain format as each attribute is shown per
line. For example:

   $ git worktree list --porcelain
   ...
   locked worktree's path mounted in\nremovable device
   ...

I have a bit to say about this below.

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.

Perhaps:
s/depending whether/depending upon whether/

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.

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.

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?

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.