Re: [PATCH] worktree: add -z option for list subcommand

On Fri, Jan 8, 2021 at 5:33 AM Phillip Wood [off-list ref] wrote:

On 07/01/2021 03:34, Eric Sunshine wrote:

quoted

On Tue, Jan 5, 2021 at 6:02 AM Phillip Wood [off-list ref] wrote:

quoted

Add a -z option to be used in conjunction with --porcelain that gives
NUL-terminated output. This enables 'worktree list --porcelain' to
handle worktree paths that contain newlines.

Adding a -z mode makes a lot of sense. This, along with a fix to call
quote_c_style() on paths in normal (not `-z`) porcelain mode,

I'm concerned that starting to quote paths will break backwards
compatibility. Even if we restricted the quoting to just those paths
that contain '\n' there is no way to distinguish between a quoted path
and one that begins and ends with ".

Backward compatibility is a valid concern, though I haven't managed to
convince myself that it would matter much in this case. In one sense,
the failure of the porcelain format to properly quote/escape paths
when needed can be viewed as an outright bug and, although we value
backward compatibility, we also value correctness, and such bug fixes
have been accepted in the past. Especially in a case such as this, it
seems exceedingly unlikely that fixing the bug would be harmful or
break existing tooling (though, of course that possibility always
exists, even if remotely so).

I can imagine ways in which tooling might be engineered to work around
the shortcoming that `git worktree list --porcelain` doesn't properly
handle newlines embedded in paths, but such tooling would almost
certainly be so fragile anyhow that it would break as we add more keys
to the extensible porcelain format. Coupled with the fact that
newlines embedded in paths are so exceedingly unlikely, it's hard to
imagine that fixing this bug would have a big impact on existing
tooling.

The case you mention about a path which happens to have a double-quote
as its first character does concern me a bit more since existing
tooling wouldn't have had to jump through hoops, or indeed do anything
special, with such paths, unlike the embedded newline case. But then,
too, it's pretty hard to imagine this coming up much, if at all, in
practice. That's not to say that I can't imagine a path, in general,
beginning with a quote, but keeping in mind that we're talking only
about worktree paths, it seems exceedingly unlikely.

My gut feeling (for what it's worth) is that worktree paths containing
embedded newlines (or other special characters) or beginning with a
double-quote is so unlikely to come in in actual practice that viewing
this as a bug fix is probably a reasonable approach, whereas some
other approach -- such as introducing porcelain-v2 or creating a new
porcelain key, say "worktreepath" which supersedes "worktree" (without
retiring "worktree") -- may be overkill.

None of the above is an argument against a complementary `-z` mode,
which I think is a very good idea.

This is the reason I prefer to add
`-z` instead of taking Rafael's patch to quote the lock reason as that
patch still leaves the output of `git worktree list --porcelain`
ambiguous and it cannot be fixed without breaking existing users. A
counter argument to all this is that there are thousands of users on
file systems that cannot have newlines in paths and Rafael's patch is
definitely a net win for them.

Rafael's patch is quoting only the lock-reason, not the worktree path,
so I think it's orthogonal to this discussion. Also, his patch is
introducing `lock` as a new attribute in porcelain output, not
modifying behavior of an existing `lock` attribute.