On Fri, Jan 8, 2021 at 5:33 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote: > On 07/01/2021 03:34, Eric Sunshine wrote: > > On Tue, Jan 5, 2021 at 6:02 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote: > >> 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.
