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

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Jan 19, 2021 at 3:20 AM Rafael Silva
<rafaeloliveira.cs@gmail.com> wrote:
> Eric Sunshine writes:
> > On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva
> > <rafaeloliveira.cs@gmail.com> wrote:
> >> +               quote_c_style(reason, &sb, NULL, CQUOTE_NODQ);
> >
> > 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:
> >
> Alright, I believe I've got the whole picture now and sorry for the
> confusion. You and Phillip clearly stated in the review cycle that the
> reason should be quoted because of the aforementioned reasons and I
> dropped when I was working on this version.

It was my fault by confusing you with the misleading mention of CQUOTE_NODQ.

> >> +       git worktree add --detach locked1 &&
> >> +       git worktree add --detach locked2 &&
> >> +       git worktree add --detach unlocked &&
> >
> > 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?
>
> Yes, this is what I intended to check when adding the `unlocked`
> worktree. I'm considering how to make this more explicit so it's clear
> for readers why the `unlocked` worktree exists in this test.

A simple in-code comment should suffice, I would think:

    git worktree add --detach locked1 &&
    git worktree add --detach locked2 &&
    # unlocked worktree should not be annotated "locked"
    git worktree add --detach unlocked &&