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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 24, 2021, 08:24 UTC
Message-ID: <CAPig+cS_7P3dCMDiWutuk+ota+NCXyz11XDv_Sgh3BCmVgeTLw@mail.gmail.com>
In-Reply-To: <be24c164-3d03-145f-abc0-4f41ed225b4e@gmail.com>

On Wed, Jan 20, 2021 at 6:00 AM Phillip Wood <phillip.wood123@gmail.com> wrote:

Show 11 quoted lines

> 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.

Show 9 quoted lines

> 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.)