Re: [PATCH 5/7] worktree: `list` escape lock reason in --porcelain
flat view
From: Eric Sunshine <hidden>
Date: 2021-01-06 09:00:25
On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva [off-list ref] wrote:
"git worktree list" porcelain format shows one attribute per line, each attribute is listed with a label and value separated by a single space. If any worktree is locked, a "locked" attribute is listed followed by the reason, if available, otherwise only "locked" is shown. In case the lock reason contains newline characters (i.e: LF or CRLF) this will cause the format to break as each line should correspond to one attribute. This leads to unwanted behavior, specially as the porcelain is intended to be machine-readable. To address this shortcoming let's teach "git worktree list" to escape any newline character before outputting the locked reason for porcelain format.
s/specially/especially/
quoted hunk ↗ jump to hunk
Signed-off-by: Rafael Silva <redacted> ---diff --git a/builtin/worktree.c b/builtin/worktree.c@@ -567,6 +567,24 @@ static int add(int ac, const char **av, const char *prefix) +static char *worktree_escape_reason(char *reason) +{ + struct strbuf escaped = STRBUF_INIT; + char *r; + + for (r = reason; *r; r++) { + if (*r == '\r' && *(r + 1) && *(r + 1) == '\n') {
The `*(r +1)` in the middle of the condition is redundant. The same
case is already handled by the `*(r + 1) == '\n'` which follows it
since EOL ('\0') won't match '\n'.
quoted hunk ↗ jump to hunk
+ strbuf_addstr(&escaped, "\\r\\n"); + r++; + } else if (*r == '\n') + strbuf_addstr(&escaped, "\\n"); + else + strbuf_addch(&escaped, *r); + } + + return strbuf_detach(&escaped, NULL); +}
As Phillip already mentioned upstream, we can use one of the functions from quote.c rather than rolling out own here. quote_c_style(), as Phillip suggested, is almost certainly the best choice for a couple reasons. First, when called with CQUOTE_NODQ, it adds quotes around the string if there are any characters which need to be escaped, but doesn't quote the string if it contains no special characters. This makes the output more nicely readable in the typical case when there are no special characters, and makes it possible to distinguish the case Phillip pointed out between literal two characters "\n" in a string and '\n' representing a newline. Second, consumers of our porcelain are already used to consuming strings produced by quote_c_style().
quoted hunk ↗ jump to hunk
@@ -580,9 +598,11 @@ static void show_worktree_porcelain(struct worktree *wt) if (worktree_lock_reason(wt)) { - if (*wt->lock_reason) - printf("locked %s\n", wt->lock_reason); - else + if (*wt->lock_reason) { + char *reason = worktree_escape_reason(wt->lock_reason); + printf("locked %s\n", reason); + free(reason); + } else
It would be preferable to fold this change directly into the preceding patch, thus eliminating this patch altogether. That way this patch series doesn't introduce a state in which the behavior is knowingly "broken", but instead is correct from the start. Also, since you can use the existing quote_c_style(), the change made by this patch becomes tiny, thus is easily folded into the earlier patch.