Re: [PATCH v3 5/7] worktree: teach `list --porcelain` to annotate locked worktree

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Jan 20, 2021 at 6:00 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> On 19/01/2021 21:27, Rafael Silva wrote:
> >   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.  Some attributes (like `locked`) can be listed as a label
> > +only or with a value depending upon whether a reason is available.  The first
> > +attribute of a working tree is always `worktree`, an empty line indicates the
> > +end of the record.  For example:
>
> I think it would be helpful to document that the reasons are quoted
> according core.quotePath.

Good idea.

> I'm not sure if it is worth changing this but I wonder if it would be
> easier to parse the output if the names of attributes with optional
> reasons were always followed by a space even when there is no reason,
> otherwise the code that parses the output has to check for the name
> followed by a space or newline. A script that only cares if the worktree
> is locked can parse the output with
> l.starts_with("locked ")
> rather than having to do
> l.starts_with("locked ") || l == "locked\n"

I see where you're coming from with this suggestion, though my
knee-jerk reaction is that it would be undesirable. Even after mulling
it over for a few days, I still haven't managed to convince myself
that it would be a good idea. There are a couple reasons (at least)
for my negative reaction. The primary reason is that the trailing
space is "invisible", and as such could end up being as confusing as
it is helpful for the simple parsing case (taking into consideration
that people often don't consult documentation). The second reason is
that we're already expecting clients to be able to parse C-style
quoting/escaping of the reason, so asking them to also distinguish
between a single token `locked` and a `locked reason-for-lock` seems
like very, very minor extra complexity. (It also just feels a bit
sloppy to have that trailing space, but that's a quite minor concern.)