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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Rafael

Thanks for working on this

On 04/01/2021 16:21, Rafael Silva 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.

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

> 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");
>   		}
>   
>