Re: [PATCH 5/7] worktree: `list` escape lock reason in --porcelain

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 6, 2021, 08:59 UTC
Message-ID: <CAPig+cSH6PKt8YvDJhMBvzvePYLqbf70uVV8TERoMj+kfxJRHQ@mail.gmail.com>
In-Reply-To: <20210104162128.95281-6-rafaeloliveira.cs@gmail.com>

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva <rafaeloliveira.cs@gmail.com> wrote:

Show 11 quoted lines

> "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/

Show 11 quoted lines

> Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
> ---
> 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'.

Show 10 quoted lines

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

Show 10 quoted lines

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