Re: [PATCH v2 4/6] worktree: teach `list --porcelain` to annotate locked worktree
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Jan 19, 2021, 17:16 UTC
- Message-ID
- <CAPig+cRZCfuhdX7O4RFLbzxoOeQF1BEopjJM=3SfuuEd+cCEMw@mail.gmail.com>
- In-Reply-To
- <gohp6kbldlfkig.fsf@gmail.com>
On Tue, Jan 19, 2021 at 3:20 AM Rafael Silva <rafaeloliveira.cs@gmail.com> wrote:
Show 20 quoted lines
> 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.
Show 12 quoted lines
> >> + 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 &&