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

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
<rafaeloliveira.cs@gmail.com> 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/

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

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

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