Re: [PATCH 5/7] worktree: `list` escape lock reason in --porcelain
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Jan 5, 2021, 10:29 UTC
- Message-ID
- <428d16e8-fdb8-a587-6a0b-39c6c50eba99@gmail.com>
- In-Reply-To
- <20210104162128.95281-6-rafaeloliveira.cs@gmail.com>
Hi Rafael
Thanks for working on this
On 04/01/2021 16:21, Rafael Silva 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.
As the porcelain output format cannot handle worktree paths that contain newlines either I think it would be better to add a `-z` flag which uses NUL termination as we have for many other commands (diff, ls-files etc). This would fix the problem forever without having to worry about quoting each time a new field is added.
If you really need to quote the lock reason then please use one of the path quoting routines (probably quote_c_style() or write_name_quoted()) in quote.c rather than rolling your own - the code below gives the same output for a string that contains the two characters `\` followed by `n` as it does for the single character `\n`. People are (hopefully) used to dequoting our path quoting and there are routines out there to do it (we have one git Git.pm) using a function in quote.c will allow them to use those routines here.
Best Wishes
Phillip
Show 50 quoted lines
> Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
> ---
> builtin/worktree.c | 26 +++++++++++++++++++++++---
> 1 file changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index dedd4089e5..9156ccd67e 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -567,6 +567,24 @@ static int add(int ac, const char **av, const char *prefix)
> return add_worktree(path, branch, &opts);
> }
>
> +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') {
> + strbuf_addstr(&escaped, "\\r\\n");
> + r++;
> + } else if (*r == '\n')
> + strbuf_addstr(&escaped, "\\n");
> + else
> + strbuf_addch(&escaped, *r);
> + }
> +
> + return strbuf_detach(&escaped, NULL);
> +}
> +
> static void show_worktree_porcelain(struct worktree *wt)
> {
> printf("worktree %s\n", wt->path);
> @@ -580,9 +598,11 @@ static void show_worktree_porcelain(struct worktree *wt)
> printf("branch %s\n", wt->head_ref);
>
> 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
> printf("locked\n");
> }
>
>