[PATCH] worktree: add -z option for list subcommand

[PATCH] worktree: add -z option for list subcommand

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Add a -z option to be used in conjunction with --porcelain that gives
NUL-terminated output. This enables 'worktree list --porcelain' to
handle worktree paths that contain newlines.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
    worktree: add -z option for list subcommand
    
    Add a -z option to be used in conjunction with --porcelain that gives
    NUL-terminated output. This enables 'worktree list --porcelain' to
    handle worktree paths that contain newlines.
    
    For a previous discussion of the merits of adding a -z option vs quoting
    the worktree path see
    https://lore.kernel.org/git/CAPig+cT-9sjmkdWFEcFS=rg9ziV9b6uWNMpQ8BTYP-a258La6Q@mail.gmail.com/

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1164%2Fphillipwood%2Fwip%2Fworktree-list-nul-termination-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1164/phillipwood/wip/worktree-list-nul-termination-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1164

 Documentation/git-worktree.txt | 15 +++++++++++----
 builtin/worktree.c             | 33 +++++++++++++++++++++------------
 t/t2402-worktree-list.sh       | 21 +++++++++++++++++++++
 3 files changed, 53 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 9e862fbcf79..a3fcd498df7 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] [-b <new-branch>] <path> [<commit-ish>]
-'git worktree list' [-v | --porcelain]
+'git worktree list' [-v | --porcelain [-z]]
 'git worktree lock' [--reason <string>] <worktree>
 'git worktree move' <worktree> <new-path>
 'git worktree prune' [-n] [-v] [--expire <expire>]
@@ -223,7 +223,13 @@ This can also be set up as the default behaviour by using the
 --porcelain::
 	With `list`, output in an easy-to-parse format for scripts.
 	This format will remain stable across Git versions and regardless of user
-	configuration.  See below for details.
+	configuration.  It is recommended to combine this with `-z`.
+	See below for details.
+
+-z::
+	When `--porcelain` is specified with `list` terminate each line with a
+	NUL rather than a newline. This makes it possible to parse the output
+	when a worktree path contains a newline character.
 
 -q::
 --quiet::
@@ -411,7 +417,8 @@ working tree itself.
 
 Porcelain Format
 ~~~~~~~~~~~~~~~~
-The porcelain format has a line per attribute.  Attributes are listed with a
+The porcelain format has a line per attribute.  If `-z` is given then the lines
+are terminated with NUL rather than a newline.  Attributes are listed with a
 label and value separated by a single space.  Boolean attributes (like `bare`
 and `detached`) are listed as a label only, and are present only
 if the value is true.  Some attributes (like `locked`) can be listed as a label
@@ -449,7 +456,7 @@ prunable gitdir file points to non-existent location
 
 ------------
 
-If the lock reason contains "unusual" characters such as newline, they
+Unless `-z` is used any "unusual" characters in the lock reason such as newlines
 are escaped and the entire reason is quoted as explained for the
 configuration variable `core.quotePath` (see linkgit:git-config[1]).
 For Example:
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 0d0809276fe..b4cc586f5c5 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -575,35 +575,38 @@ static int add(int ac, const char **av, const char *prefix)
 	return add_worktree(path, branch, &opts);
 }
 
-static void show_worktree_porcelain(struct worktree *wt)
+static void show_worktree_porcelain(struct worktree *wt, int line_terminator)
 {
 	const char *reason;
 
-	printf("worktree %s\n", wt->path);
+	printf("worktree %s%c", wt->path, line_terminator);
 	if (wt->is_bare)
-		printf("bare\n");
+		printf("bare%c", line_terminator);
 	else {
-		printf("HEAD %s\n", oid_to_hex(&wt->head_oid));
+		printf("HEAD %s%c", oid_to_hex(&wt->head_oid), line_terminator);
 		if (wt->is_detached)
-			printf("detached\n");
+			printf("detached%c", line_terminator);
 		else if (wt->head_ref)
-			printf("branch %s\n", wt->head_ref);
+			printf("branch %s%c", wt->head_ref, line_terminator);
 	}
 
 	reason = worktree_lock_reason(wt);
 	if (reason && *reason) {
 		struct strbuf sb = STRBUF_INIT;
-		quote_c_style(reason, &sb, NULL, 0);
-		printf("locked %s\n", sb.buf);
+		if (line_terminator) {
+			quote_c_style(reason, &sb, NULL, 0);
+			reason = sb.buf;
+		}
+		printf("locked %s%c", reason, line_terminator);
 		strbuf_release(&sb);
 	} else if (reason)
-		printf("locked\n");
+		printf("locked%c", line_terminator);
 
 	reason = worktree_prune_reason(wt, expire);
 	if (reason)
-		printf("prunable %s\n", reason);
+		printf("prunable %s%c", reason, line_terminator);
 
-	printf("\n");
+	fputc(line_terminator, stdout);
 }
 
 static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
@@ -681,12 +684,15 @@ static void pathsort(struct worktree **wt)
 static int list(int ac, const char **av, const char *prefix)
 {
 	int porcelain = 0;
+	int line_terminator = '\n';
 
 	struct option options[] = {
 		OPT_BOOL(0, "porcelain", &porcelain, N_("machine-readable output")),
 		OPT__VERBOSE(&verbose, N_("show extended annotations and reasons, if available")),
 		OPT_EXPIRY_DATE(0, "expire", &expire,
 				N_("add 'prunable' annotation to worktrees older than <time>")),
+		OPT_SET_INT('z', NULL, &line_terminator,
+			    N_("fields are separated with NUL character"), '\0'),
 		OPT_END()
 	};
 
@@ -696,6 +702,8 @@ static int list(int ac, const char **av, const char *prefix)
 		usage_with_options(worktree_usage, options);
 	else if (verbose && porcelain)
 		die(_("options '%s' and '%s' cannot be used together"), "--verbose", "--porcelain");
+	else if (!line_terminator && !porcelain)
+		die(_("'-z' requires '--porcelain'"));
 	else {
 		struct worktree **worktrees = get_worktrees();
 		int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;
@@ -708,7 +716,8 @@ static int list(int ac, const char **av, const char *prefix)
 
 		for (i = 0; worktrees[i]; i++) {
 			if (porcelain)
-				show_worktree_porcelain(worktrees[i]);
+				show_worktree_porcelain(worktrees[i],
+							line_terminator);
 			else
 				show_worktree(worktrees[i], path_maxlen, abbrev);
 		}
diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
index c8a5a0aac6d..48d5d30d709 100755
--- a/t/t2402-worktree-list.sh
+++ b/t/t2402-worktree-list.sh
@@ -64,6 +64,27 @@ test_expect_success '"list" all worktrees --porcelain' '
 	test_cmp expect actual
 '
 
+test_expect_success '"list" all worktrees --porcelain -z' '
+	test_when_finished "rm -rf here _actual actual expect &&
+				git worktree prune" &&
+	printf "worktree %sQHEAD %sQbranch %sQQ" \
+		"$(git rev-parse --show-toplevel)" \
+		$(git rev-parse HEAD --symbolic-full-name HEAD) >expect &&
+	git worktree add --detach here main &&
+	printf "worktree %sQHEAD %sQdetachedQQ" \
+		"$(git -C here rev-parse --show-toplevel)" \
+		"$(git rev-parse HEAD)" >>expect &&
+	git worktree list --porcelain -z >_actual &&
+	cat _actual | tr "\0" Q >actual	&&
+	test_cmp expect actual
+'
+
+test_expect_success '"list" -z fails without --porcelain' '
+	test_when_finished "rm -rf here && git worktree prune" &&
+	git worktree add --detach here main &&
+	test_must_fail git worktree list -z
+'
+
 test_expect_success '"list" all worktrees with locked annotation' '
 	test_when_finished "rm -rf locked unlocked out && git worktree prune" &&
 	git worktree add --detach locked main &&

base-commit: dab1b7905d0b295f1acef9785bb2b9cbb0fdec84
-- 
gitgitgadget

Re: [PATCH] worktree: add -z option for list subcommand

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +-z::
> +	When `--porcelain` is specified with `list` terminate each line with a
> +	NUL rather than a newline. This makes it possible to parse the output
> +	when a worktree path contains a newline character.

This makes it sound as if it were impossible.  As the changes this
patch makes to the documentation and the code indicate, we were
already doing quote_c_style(), so it is not quite correct to say so.

Perhaps "makes it easier" is more accurate?

Unless the original wasn't using quote_c_style() correctly and
wasn't quoting its output, that is?

> @@ -411,7 +417,8 @@ working tree itself.
>  
>  Porcelain Format
>  ~~~~~~~~~~~~~~~~
> -The porcelain format has a line per attribute.  Attributes are listed with a
> +The porcelain format has a line per attribute.  If `-z` is given then the lines
> +are terminated with NUL rather than a newline.  Attributes are listed with a
>  label and value separated by a single space.  Boolean attributes (like `bare`
>  and `detached`) are listed as a label only, and are present only
>  if the value is true.  Some attributes (like `locked`) can be listed as a label
> @@ -449,7 +456,7 @@ prunable gitdir file points to non-existent location
>  
>  ------------
>  
> -If the lock reason contains "unusual" characters such as newline, they
> +Unless `-z` is used any "unusual" characters in the lock reason such as newlines
>  are escaped and the entire reason is quoted as explained for the

OK.  That is sensible.

>  	reason = worktree_lock_reason(wt);
>  	if (reason && *reason) {
>  		struct strbuf sb = STRBUF_INIT;
> -		quote_c_style(reason, &sb, NULL, 0);
> -		printf("locked %s\n", sb.buf);
> +		if (line_terminator) {
> +			quote_c_style(reason, &sb, NULL, 0);
> +			reason = sb.buf;
> +		}
> +		printf("locked %s%c", reason, line_terminator);

OK.  I suspect write_name_quoted() may be a better fit that does not
require us to have our own strbuf, but this should be OK.

>  		strbuf_release(&sb);
>  	} else if (reason)
> -		printf("locked\n");
> +		printf("locked%c", line_terminator);

It is a shame that we need a special code path for an empty string
given as the reason, only for the single SP after "locked", but we
have to live with it, I guess.

>  	reason = worktree_prune_reason(wt, expire);
>  	if (reason)
> -		printf("prunable %s\n", reason);
> +		printf("prunable %s%c", reason, line_terminator);
>  
> -	printf("\n");
> +	fputc(line_terminator, stdout);
>  }

All code changes looked sensible.

> +	else if (!line_terminator && !porcelain)
> +		die(_("'-z' requires '--porcelain'"));
>  	else {
>  		struct worktree **worktrees = get_worktrees();
>  		int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;
> @@ -708,7 +716,8 @@ static int list(int ac, const char **av, const char *prefix)
>  
>  		for (i = 0; worktrees[i]; i++) {
>  			if (porcelain)
> -				show_worktree_porcelain(worktrees[i]);
> +				show_worktree_porcelain(worktrees[i],
> +							line_terminator);
>  			else
>  				show_worktree(worktrees[i], path_maxlen, abbrev);
>  		}

Re: [PATCH] worktree: add -z option for list subcommand

[Eric Sunshine]

On Fri, Feb 25, 2022 at 12:59 PM Junio C Hamano <gitster@pobox.com> wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > +-z::
> > +     When `--porcelain` is specified with `list` terminate each line with a
> > +     NUL rather than a newline. This makes it possible to parse the output
> > +     when a worktree path contains a newline character.
>
> This makes it sound as if it were impossible.  As the changes this
> patch makes to the documentation and the code indicate, we were
> already doing quote_c_style(), so it is not quite correct to say so.
>
> Perhaps "makes it easier" is more accurate?
>
> Unless the original wasn't using quote_c_style() correctly and
> wasn't quoting its output, that is?

That's the case. It is impossible without this patch since `git
worktree list --porcelain` does not call quote_c_style() for the
worktree path; it only calls quote_c_style() for the lock reason.
Fixing this oversight for the worktree path for non-`-z` mode has
potential backward-compatibility concerns. I had argued[1] that we
might be able to sell it as a pure bug fix, but Phillip was
concerned[2] that it might break existing tooling. (I also share that
concern, but to a lesser degree, perhaps, since worktrees with oddball
names containing newline or a leading double-quote must be exceedingly
rare.)

[1]: https://lore.kernel.org/git/CAPig+cQq_RnanDQ3jHfNz_L58WyzmsUJBhtdrLxa=H0v_io+WA@mail.gmail.com/
[2]: https://lore.kernel.org/git/936f9b7c-6d54-00bc-f136-4cb4c2836eb6@gmail.com/

> > +             if (line_terminator) {
> > +                     quote_c_style(reason, &sb, NULL, 0);
> > +                     reason = sb.buf;
> > +             }
> > +             printf("locked %s%c", reason, line_terminator);
>
> OK.  I suspect write_name_quoted() may be a better fit that does not
> require us to have our own strbuf, but this should be OK.

Nice suggestion.

Re: [PATCH] worktree: add -z option for list subcommand

[Phillip Wood]

On 28/02/2022 08:35, Eric Sunshine wrote:
> On Fri, Feb 25, 2022 at 12:59 PM Junio C Hamano <gitster@pobox.com> wrote:
>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>> +-z::
>>> +     When `--porcelain` is specified with `list` terminate each line with a
>>> +     NUL rather than a newline. This makes it possible to parse the output
>>> +     when a worktree path contains a newline character.
>>
>> This makes it sound as if it were impossible.  As the changes this
>> patch makes to the documentation and the code indicate, we were
>> already doing quote_c_style(), so it is not quite correct to say so.
>>
>> Perhaps "makes it easier" is more accurate?
>>
>> Unless the original wasn't using quote_c_style() correctly and
>> wasn't quoting its output, that is?
> 
> That's the case. It is impossible without this patch since `git
> worktree list --porcelain` does not call quote_c_style() for the
> worktree path; it only calls quote_c_style() for the lock reason.
> Fixing this oversight for the worktree path for non-`-z` mode has
> potential backward-compatibility concerns. I had argued[1] that we
> might be able to sell it as a pure bug fix, but Phillip was
> concerned[2] that it might break existing tooling. (I also share that
> concern, but to a lesser degree, perhaps, since worktrees with oddball
> names containing newline or a leading double-quote must be exceedingly
> rare.)

If the user has not set core.quotePath to false then any unicode paths 
will be quoted unless we only quote paths with newlines and leading 
double quotes which would mean the quoting would not match all the other 
git commands. In general I think it is easier to use nul termination 
rather than having to unquote path names. In this case it is not obvious 
to me what command one would feed a quoted directory name into either 
(it's not like doing "git diff | sed ... |git update-index --stdin" or 
something like that).

Best Wishes

Phillip

> [1]: https://lore.kernel.org/git/CAPig+cQq_RnanDQ3jHfNz_L58WyzmsUJBhtdrLxa=H0v_io+WA@mail.gmail.com/
> [2]: https://lore.kernel.org/git/936f9b7c-6d54-00bc-f136-4cb4c2836eb6@gmail.com/
> 
>>> +             if (line_terminator) {
>>> +                     quote_c_style(reason, &sb, NULL, 0);
>>> +                     reason = sb.buf;
>>> +             }
>>> +             printf("locked %s%c", reason, line_terminator);
>>
>> OK.  I suspect write_name_quoted() may be a better fit that does not
>> require us to have our own strbuf, but this should be OK.
> 
> Nice suggestion.

Re: [PATCH] worktree: add -z option for list subcommand

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> Unless the original wasn't using quote_c_style() correctly and
>> wasn't quoting its output, that is?
>
> That's the case. It is impossible without this patch since `git
> worktree list --porcelain` does not call quote_c_style() for the
> worktree path; it only calls quote_c_style() for the lock reason.

Yuck.  That's an outright bug that we should fix.  I do not think it
makes "-z" unnecessary, but those who want to read from non-z output
cannot sensibly do so with funny letters in their paths with today's
output, and as long as quote_c_style() tweaks the output only when
funny letters need to be quoted, those who are reading from today's
non-z output will not be hurt, so let's fix that first or at the
same time as we add "-z".

Thanks.

Re: [PATCH] worktree: add -z option for list subcommand

[Phillip Wood]

On 25/02/2022 17:59, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> [...]
>>   	reason = worktree_lock_reason(wt);
>>   	if (reason && *reason) {
>>   		struct strbuf sb = STRBUF_INIT;
>> -		quote_c_style(reason, &sb, NULL, 0);
>> -		printf("locked %s\n", sb.buf);
>> +		if (line_terminator) {
>> +			quote_c_style(reason, &sb, NULL, 0);
>> +			reason = sb.buf;
>> +		}
>> +		printf("locked %s%c", reason, line_terminator);
> 
> OK.  I suspect write_name_quoted() may be a better fit that does not
> require us to have our own strbuf, but this should be OK.
> 
>>   		strbuf_release(&sb);
>>   	} else if (reason)
>> -		printf("locked\n");
>> +		printf("locked%c", line_terminator);
> 
> It is a shame that we need a special code path for an empty string
> given as the reason, only for the single SP after "locked", but we
> have to live with it, I guess.

We could have

	if (reason) {
		fputs("locked", stdout);
		if (*reason) {
			fputc(" ", stdout)
			write_name_quoted(reason, stdout, line_terminator);
		} else {
			fputc(line_terminator, stdout)
		
	}

which shares the code to print "locked" but I'm not sure it is any 
bettor overall though especially as write_name_quoted() means we only 
want to output a terminator when there is no reason text.


Best Wishes

Phillip

Re: [PATCH] worktree: add -z option for list subcommand

[Eric Sunshine]

On Fri, Feb 25, 2022 at 10:08 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Add a -z option to be used in conjunction with --porcelain that gives
> NUL-terminated output. This enables 'worktree list --porcelain' to
> handle worktree paths that contain newlines.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>     For a previous discussion of the merits of adding a -z option vs quoting
>     the worktree path see
>     https://lore.kernel.org/git/CAPig+cT-9sjmkdWFEcFS=rg9ziV9b6uWNMpQ8BTYP-a258La6Q@mail.gmail.com/

Thanks for resubmitting.

> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -223,7 +223,13 @@ This can also be set up as the default behaviour by using the
> +-z::
> +       When `--porcelain` is specified with `list` terminate each line with a

Nit: s/`list`/&,/

> +       NUL rather than a newline. This makes it possible to parse the output
> +       when a worktree path contains a newline character.

Or, perhaps:

    Terminate each line with NUL rather than a newline when
    `--porcelain` is specified with `list`. This makes it possible...

might be simpler(?).

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -575,35 +575,38 @@ static int add(int ac, const char **av, const char *prefix)
> -static void show_worktree_porcelain(struct worktree *wt)
> +static void show_worktree_porcelain(struct worktree *wt, int line_terminator)
>  {
> -       printf("worktree %s\n", wt->path);
> +       printf("worktree %s%c", wt->path, line_terminator);
>         if (wt->is_bare)
> -               printf("bare\n");
> +               printf("bare%c", line_terminator);
>         else {
> -               printf("HEAD %s\n", oid_to_hex(&wt->head_oid));
> +               printf("HEAD %s%c", oid_to_hex(&wt->head_oid), line_terminator);
>                 if (wt->is_detached)
> -                       printf("detached\n");
> +                       printf("detached%c", line_terminator);
>                 else if (wt->head_ref)
> -                       printf("branch %s\n", wt->head_ref);
> +                       printf("branch %s%c", wt->head_ref, line_terminator);
>         }

Good, this is easier to read than the previous version which manually
called `fputc(line_terminator, stdout)` repeatedly for the
termination. The diff is also more easily digested.

>         if (reason && *reason) {
>                 struct strbuf sb = STRBUF_INIT;
> -               quote_c_style(reason, &sb, NULL, 0);
> -               printf("locked %s\n", sb.buf);
> +               if (line_terminator) {
> +                       quote_c_style(reason, &sb, NULL, 0);
> +                       reason = sb.buf;
> +               }
> +               printf("locked %s%c", reason, line_terminator);

Junio's suggestion downstream that write_name_quoted() would be
simpler makes sense, but (as he says) is not necessarily worth a
reroll.

> @@ -681,12 +684,15 @@ static void pathsort(struct worktree **wt)
> +               OPT_SET_INT('z', NULL, &line_terminator,
> +                           N_("fields are separated with NUL character"), '\0'),

Same comment as previous review [1]:

    "fields" sounds a little odd. "lines" might make more sense.
    "records" might be even better and matches the wording of some
    other Git commands accepting `-z`.

> @@ -696,6 +702,8 @@ static int list(int ac, const char **av, const char *prefix)
>                 usage_with_options(worktree_usage, options);
>         else if (verbose && porcelain)
>                 die(_("options '%s' and '%s' cannot be used together"), "--verbose", "--porcelain");
> +       else if (!line_terminator && !porcelain)
> +               die(_("'-z' requires '--porcelain'"));

Same comment as my previous review[1]:

    Other error messages in this file don't quote command-line
    options, so `die(_("-z requires --porcelain"));` would be more
    consistent.

However, considering all the recent work Jean-Noël Avila has been
doing to recently, perhaps this should instead mirror his change to
the die() message just above the new one and instead be:

    die(_("'%s' requires '%s'"), "-z", "--porcelain");

> diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
> @@ -64,6 +64,27 @@ test_expect_success '"list" all worktrees --porcelain' '
> +test_expect_success '"list" all worktrees --porcelain -z' '
> +       test_when_finished "rm -rf here _actual actual expect &&
> +                               git worktree prune" &&
> +       printf "worktree %sQHEAD %sQbranch %sQQ" \
> +               "$(git rev-parse --show-toplevel)" \
> +               $(git rev-parse HEAD --symbolic-full-name HEAD) >expect &&
> +       git worktree add --detach here main &&
> +       printf "worktree %sQHEAD %sQdetachedQQ" \
> +               "$(git -C here rev-parse --show-toplevel)" \
> +               "$(git rev-parse HEAD)" >>expect &&
> +       git worktree list --porcelain -z >_actual &&
> +       cat _actual | tr "\0" Q >actual &&

Same comment as my previous review[1]:

    Or just use nul_to_q():
      nul_to_q <_actual >actual &&
    (And there's no need to `cat` the file.)

> +       test_cmp expect actual
> +'
> +
> +test_expect_success '"list" -z fails without --porcelain' '
> +       test_when_finished "rm -rf here && git worktree prune" &&
> +       git worktree add --detach here main &&
> +       test_must_fail git worktree list -z
> +'

Same comment as my previous review[1]:

    I don't think there's any need for this test to create (and
    cleanup) a worktree. So, the entire test could collapse to:

      test_expect_success '"list" -z fails without --porcelain' '
        test_must_fail git worktree list -z
      '

[1]: https://lore.kernel.org/git/CAPig+cT-9sjmkdWFEcFS=rg9ziV9b6uWNMpQ8BTYP-a258La6Q@mail.gmail.com/

Re: [PATCH] worktree: add -z option for list subcommand

[Phillip Wood]

Hi Eric

On 28/02/2022 08:16, Eric Sunshine wrote:
> On Fri, Feb 25, 2022 at 10:08 AM Phillip Wood via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> Add a -z option to be used in conjunction with --porcelain that gives
>> NUL-terminated output. This enables 'worktree list --porcelain' to
>> handle worktree paths that contain newlines.
>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>      For a previous discussion of the merits of adding a -z option vs quoting
>>      the worktree path see
>>      https://lore.kernel.org/git/CAPig+cT-9sjmkdWFEcFS=rg9ziV9b6uWNMpQ8BTYP-a258La6Q@mail.gmail.com/
> 
> Thanks for resubmitting.
> 
>> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
>> @@ -223,7 +223,13 @@ This can also be set up as the default behaviour by using the
>> +-z::
>> +       When `--porcelain` is specified with `list` terminate each line with a
> 
> Nit: s/`list`/&,/
> 
>> +       NUL rather than a newline. This makes it possible to parse the output
>> +       when a worktree path contains a newline character.
> 
> Or, perhaps:
> 
>      Terminate each line with NUL rather than a newline when
>      `--porcelain` is specified with `list`. This makes it possible...
> 
> might be simpler(?).
> 
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> @@ -575,35 +575,38 @@ static int add(int ac, const char **av, const char *prefix)
>> -static void show_worktree_porcelain(struct worktree *wt)
>> +static void show_worktree_porcelain(struct worktree *wt, int line_terminator)
>>   {
>> -       printf("worktree %s\n", wt->path);
>> +       printf("worktree %s%c", wt->path, line_terminator);
>>          if (wt->is_bare)
>> -               printf("bare\n");
>> +               printf("bare%c", line_terminator);
>>          else {
>> -               printf("HEAD %s\n", oid_to_hex(&wt->head_oid));
>> +               printf("HEAD %s%c", oid_to_hex(&wt->head_oid), line_terminator);
>>                  if (wt->is_detached)
>> -                       printf("detached\n");
>> +                       printf("detached%c", line_terminator);
>>                  else if (wt->head_ref)
>> -                       printf("branch %s\n", wt->head_ref);
>> +                       printf("branch %s%c", wt->head_ref, line_terminator);
>>          }
> 
> Good, this is easier to read than the previous version which manually
> called `fputc(line_terminator, stdout)` repeatedly for the
> termination. The diff is also more easily digested.
> 
>>          if (reason && *reason) {
>>                  struct strbuf sb = STRBUF_INIT;
>> -               quote_c_style(reason, &sb, NULL, 0);
>> -               printf("locked %s\n", sb.buf);
>> +               if (line_terminator) {
>> +                       quote_c_style(reason, &sb, NULL, 0);
>> +                       reason = sb.buf;
>> +               }
>> +               printf("locked %s%c", reason, line_terminator);
> 
> Junio's suggestion downstream that write_name_quoted() would be
> simpler makes sense, but (as he says) is not necessarily worth a
> reroll.

Yes, I wasn't aware of that function but I'll update as I'm going to 
re-roll anyway

>> @@ -681,12 +684,15 @@ static void pathsort(struct worktree **wt)
>> +               OPT_SET_INT('z', NULL, &line_terminator,
>> +                           N_("fields are separated with NUL character"), '\0'),
> 
> Same comment as previous review [1]:
> 
>      "fields" sounds a little odd. "lines" might make more sense.
>      "records" might be even better and matches the wording of some
>      other Git commands accepting `-z`.

It would be good to standardize this, check-attr and check-ignore use 
"records", ls-files, ls-tree and status use "entries", config uses 
"values" (which probably makes sense in the context of that command).

>> @@ -696,6 +702,8 @@ static int list(int ac, const char **av, const char *prefix)
>>                  usage_with_options(worktree_usage, options);
>>          else if (verbose && porcelain)
>>                  die(_("options '%s' and '%s' cannot be used together"), "--verbose", "--porcelain");
>> +       else if (!line_terminator && !porcelain)
>> +               die(_("'-z' requires '--porcelain'"));
> 
> Same comment as my previous review[1]:

Sorry I'd forgotten you'd left some comments before, I should have spent 
more time rereading that old thread

>      Other error messages in this file don't quote command-line
>      options, so `die(_("-z requires --porcelain"));` would be more
>      consistent.
> 
> However, considering all the recent work Jean-Noël Avila has been
> doing to recently, perhaps this should instead mirror his change to
> the die() message just above the new one and instead be:
> 
>      die(_("'%s' requires '%s'"), "-z", "--porcelain");

Yes, I'll update this, Jean-Noël made the same suggestion. I wish we had 
some functions/macros for these standard messages I think it would be 
easier to remember to use them than trying to remember the standard 
wording for messages like this.

>> diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
>> @@ -64,6 +64,27 @@ test_expect_success '"list" all worktrees --porcelain' '
>> +test_expect_success '"list" all worktrees --porcelain -z' '
>> +       test_when_finished "rm -rf here _actual actual expect &&
>> +                               git worktree prune" &&
>> +       printf "worktree %sQHEAD %sQbranch %sQQ" \
>> +               "$(git rev-parse --show-toplevel)" \
>> +               $(git rev-parse HEAD --symbolic-full-name HEAD) >expect &&
>> +       git worktree add --detach here main &&
>> +       printf "worktree %sQHEAD %sQdetachedQQ" \
>> +               "$(git -C here rev-parse --show-toplevel)" \
>> +               "$(git rev-parse HEAD)" >>expect &&
>> +       git worktree list --porcelain -z >_actual &&
>> +       cat _actual | tr "\0" Q >actual &&
> 
> Same comment as my previous review[1]:
> 
>      Or just use nul_to_q():
>        nul_to_q <_actual >actual &&
>      (And there's no need to `cat` the file.)

That's a good idea

>> +       test_cmp expect actual
>> +'
>> +
>> +test_expect_success '"list" -z fails without --porcelain' '
>> +       test_when_finished "rm -rf here && git worktree prune" &&
>> +       git worktree add --detach here main &&
>> +       test_must_fail git worktree list -z
>> +'
> 
> Same comment as my previous review[1]:
> 
>      I don't think there's any need for this test to create (and
>      cleanup) a worktree. So, the entire test could collapse to:
> 
>        test_expect_success '"list" -z fails without --porcelain' '
>          test_must_fail git worktree list -z
>        '

Good point

Thanks for your comments, sorry for making you repeat yourself from last 
time

Phillip

> [1]: https://lore.kernel.org/git/CAPig+cT-9sjmkdWFEcFS=rg9ziV9b6uWNMpQ8BTYP-a258La6Q@mail.gmail.com/

Re: [PATCH] worktree: add -z option for list subcommand

[Jean-Noël Avila]

Le 25/02/2022 à 16:08, Phillip Wood via GitGitGadget a écrit :
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
>
> @@ -696,6 +702,8 @@ static int list(int ac, const char **av, const char *prefix)
>   		usage_with_options(worktree_usage, options);
>   	else if (verbose && porcelain)
>   		die(_("options '%s' and '%s' cannot be used together"), "--verbose", "--porcelain");
> +	else if (!line_terminator && !porcelain)
> +		die(_("'-z' requires '--porcelain'"));
>   	else {
>   		struct worktree **worktrees = get_worktrees();
>   		int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;


Please better use

  die(_("the option '%s' requires '%s'"),  "-z", "--porcelain");


In order to make a no-op for translators.


Best regards,

JN

Re: [PATCH] worktree: add -z option for list subcommand

[Phillip Wood]

Hi Jean-Noël

On 28/02/2022 09:47, Jean-Noël Avila wrote:
> Le 25/02/2022 à 16:08, Phillip Wood via GitGitGadget a écrit :
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>>
>> @@ -696,6 +702,8 @@ static int list(int ac, const char **av, const 
>> char *prefix)
>>           usage_with_options(worktree_usage, options);
>>       else if (verbose && porcelain)
>>           die(_("options '%s' and '%s' cannot be used together"), 
>> "--verbose", "--porcelain");
>> +    else if (!line_terminator && !porcelain)
>> +        die(_("'-z' requires '--porcelain'"));
>>       else {
>>           struct worktree **worktrees = get_worktrees();
>>           int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;
> 
> 
> Please better use
> 
>   die(_("the option '%s' requires '%s'"),  "-z", "--porcelain");
> 
> 
> In order to make a no-op for translators.

Will do, thanks for the suggestion

Phillip

> 
> Best regards,
> 
> JN
> 

[PATCH v2] worktree: add -z option for list subcommand

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Add a -z option to be used in conjunction with --porcelain that gives
NUL-terminated output. As 'worktree list --porcelain' does not quote
worktree paths this enables it to handle worktree paths that contain
newlines.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
    worktree: add -z option for list subcommand
    
    Thanks to Eric, Jean-Noël and Junio for their comments on V1. I've
    reworded the docs and option help and tweaked the tests as suggested by
    Eric, fixed the error messages as suggested by Eric/Jean-Noël and
    changed the implementation to use write_name_quoted() as suggested by
    Junio. I've punted doing anything about quoting the output without -z
    for now, I'll fix that with and without --porcelain in another series.
    
    V1 Cover Letter: Add a -z option to be used in conjunction with
    --porcelain that gives NUL-terminated output. This enables 'worktree
    list --porcelain' to handle worktree paths that contain newlines.
    
    For a previous discussion of the merits of adding a -z option vs quoting
    the worktree path see
    https://lore.kernel.org/git/CAPig+cT-9sjmkdWFEcFS=rg9ziV9b6uWNMpQ8BTYP-a258La6Q@mail.gmail.com/

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1164%2Fphillipwood%2Fwip%2Fworktree-list-nul-termination-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1164/phillipwood/wip/worktree-list-nul-termination-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1164

Range-diff vs v1:

 1:  b954579189b ! 1:  5f0e0213583 worktree: add -z option for list subcommand
     @@ Commit message
          worktree: add -z option for list subcommand
      
          Add a -z option to be used in conjunction with --porcelain that gives
     -    NUL-terminated output. This enables 'worktree list --porcelain' to
     -    handle worktree paths that contain newlines.
     +    NUL-terminated output. As 'worktree list --porcelain' does not quote
     +    worktree paths this enables it to handle worktree paths that contain
     +    newlines.
      
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
     @@ Documentation/git-worktree.txt: This can also be set up as the default behaviour
      +	See below for details.
      +
      +-z::
     -+	When `--porcelain` is specified with `list` terminate each line with a
     -+	NUL rather than a newline. This makes it possible to parse the output
     -+	when a worktree path contains a newline character.
     ++	Terminate each line with a NUL rather than a newline when
     ++	`--porcelain` is specified with `list`. This makes it possible
     ++	to parse the output when a worktree path contains a newline
     ++	character.
       
       -q::
       --quiet::
     @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix)
       	}
       
       	reason = worktree_lock_reason(wt);
     - 	if (reason && *reason) {
     - 		struct strbuf sb = STRBUF_INIT;
     +-	if (reason && *reason) {
     +-		struct strbuf sb = STRBUF_INIT;
      -		quote_c_style(reason, &sb, NULL, 0);
      -		printf("locked %s\n", sb.buf);
     -+		if (line_terminator) {
     -+			quote_c_style(reason, &sb, NULL, 0);
     -+			reason = sb.buf;
     -+		}
     -+		printf("locked %s%c", reason, line_terminator);
     - 		strbuf_release(&sb);
     - 	} else if (reason)
     +-		strbuf_release(&sb);
     +-	} else if (reason)
      -		printf("locked\n");
     -+		printf("locked%c", line_terminator);
     ++	if (reason) {
     ++		fputs("locked", stdout);
     ++		if (*reason) {
     ++			fputc(' ', stdout);
     ++			write_name_quoted(reason, stdout, line_terminator);
     ++		} else {
     ++			fputc(line_terminator, stdout);
     ++		}
     ++	}
       
       	reason = worktree_prune_reason(wt, expire);
       	if (reason)
     @@ builtin/worktree.c: static void pathsort(struct worktree **wt)
       		OPT_EXPIRY_DATE(0, "expire", &expire,
       				N_("add 'prunable' annotation to worktrees older than <time>")),
      +		OPT_SET_INT('z', NULL, &line_terminator,
     -+			    N_("fields are separated with NUL character"), '\0'),
     ++			    N_("terminate records with a NUL character"), '\0'),
       		OPT_END()
       	};
       
     @@ builtin/worktree.c: static int list(int ac, const char **av, const char *prefix)
       	else if (verbose && porcelain)
       		die(_("options '%s' and '%s' cannot be used together"), "--verbose", "--porcelain");
      +	else if (!line_terminator && !porcelain)
     -+		die(_("'-z' requires '--porcelain'"));
     ++		die(_("the option '%s' requires '%s'"), "-z", "--porcelain");
       	else {
       		struct worktree **worktrees = get_worktrees();
       		int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;
     @@ t/t2402-worktree-list.sh: test_expect_success '"list" all worktrees --porcelain'
      +		"$(git -C here rev-parse --show-toplevel)" \
      +		"$(git rev-parse HEAD)" >>expect &&
      +	git worktree list --porcelain -z >_actual &&
     -+	cat _actual | tr "\0" Q >actual	&&
     ++	nul_to_q <_actual >actual &&
      +	test_cmp expect actual
      +'
      +
      +test_expect_success '"list" -z fails without --porcelain' '
     -+	test_when_finished "rm -rf here && git worktree prune" &&
     -+	git worktree add --detach here main &&
      +	test_must_fail git worktree list -z
      +'
      +


 Documentation/git-worktree.txt | 16 ++++++++++----
 builtin/worktree.c             | 40 ++++++++++++++++++++--------------
 t/t2402-worktree-list.sh       | 19 ++++++++++++++++
 3 files changed, 55 insertions(+), 20 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 9e862fbcf79..638e188c409 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] [-b <new-branch>] <path> [<commit-ish>]
-'git worktree list' [-v | --porcelain]
+'git worktree list' [-v | --porcelain [-z]]
 'git worktree lock' [--reason <string>] <worktree>
 'git worktree move' <worktree> <new-path>
 'git worktree prune' [-n] [-v] [--expire <expire>]
@@ -223,7 +223,14 @@ This can also be set up as the default behaviour by using the
 --porcelain::
 	With `list`, output in an easy-to-parse format for scripts.
 	This format will remain stable across Git versions and regardless of user
-	configuration.  See below for details.
+	configuration.  It is recommended to combine this with `-z`.
+	See below for details.
+
+-z::
+	Terminate each line with a NUL rather than a newline when
+	`--porcelain` is specified with `list`. This makes it possible
+	to parse the output when a worktree path contains a newline
+	character.
 
 -q::
 --quiet::
@@ -411,7 +418,8 @@ working tree itself.
 
 Porcelain Format
 ~~~~~~~~~~~~~~~~
-The porcelain format has a line per attribute.  Attributes are listed with a
+The porcelain format has a line per attribute.  If `-z` is given then the lines
+are terminated with NUL rather than a newline.  Attributes are listed with a
 label and value separated by a single space.  Boolean attributes (like `bare`
 and `detached`) are listed as a label only, and are present only
 if the value is true.  Some attributes (like `locked`) can be listed as a label
@@ -449,7 +457,7 @@ prunable gitdir file points to non-existent location
 
 ------------
 
-If the lock reason contains "unusual" characters such as newline, they
+Unless `-z` is used any "unusual" characters in the lock reason such as newlines
 are escaped and the entire reason is quoted as explained for the
 configuration variable `core.quotePath` (see linkgit:git-config[1]).
 For Example:
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 0d0809276fe..6fef936d5ac 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -575,35 +575,37 @@ static int add(int ac, const char **av, const char *prefix)
 	return add_worktree(path, branch, &opts);
 }
 
-static void show_worktree_porcelain(struct worktree *wt)
+static void show_worktree_porcelain(struct worktree *wt, int line_terminator)
 {
 	const char *reason;
 
-	printf("worktree %s\n", wt->path);
+	printf("worktree %s%c", wt->path, line_terminator);
 	if (wt->is_bare)
-		printf("bare\n");
+		printf("bare%c", line_terminator);
 	else {
-		printf("HEAD %s\n", oid_to_hex(&wt->head_oid));
+		printf("HEAD %s%c", oid_to_hex(&wt->head_oid), line_terminator);
 		if (wt->is_detached)
-			printf("detached\n");
+			printf("detached%c", line_terminator);
 		else if (wt->head_ref)
-			printf("branch %s\n", wt->head_ref);
+			printf("branch %s%c", wt->head_ref, line_terminator);
 	}
 
 	reason = worktree_lock_reason(wt);
-	if (reason && *reason) {
-		struct strbuf sb = STRBUF_INIT;
-		quote_c_style(reason, &sb, NULL, 0);
-		printf("locked %s\n", sb.buf);
-		strbuf_release(&sb);
-	} else if (reason)
-		printf("locked\n");
+	if (reason) {
+		fputs("locked", stdout);
+		if (*reason) {
+			fputc(' ', stdout);
+			write_name_quoted(reason, stdout, line_terminator);
+		} else {
+			fputc(line_terminator, stdout);
+		}
+	}
 
 	reason = worktree_prune_reason(wt, expire);
 	if (reason)
-		printf("prunable %s\n", reason);
+		printf("prunable %s%c", reason, line_terminator);
 
-	printf("\n");
+	fputc(line_terminator, stdout);
 }
 
 static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
@@ -681,12 +683,15 @@ static void pathsort(struct worktree **wt)
 static int list(int ac, const char **av, const char *prefix)
 {
 	int porcelain = 0;
+	int line_terminator = '\n';
 
 	struct option options[] = {
 		OPT_BOOL(0, "porcelain", &porcelain, N_("machine-readable output")),
 		OPT__VERBOSE(&verbose, N_("show extended annotations and reasons, if available")),
 		OPT_EXPIRY_DATE(0, "expire", &expire,
 				N_("add 'prunable' annotation to worktrees older than <time>")),
+		OPT_SET_INT('z', NULL, &line_terminator,
+			    N_("terminate records with a NUL character"), '\0'),
 		OPT_END()
 	};
 
@@ -696,6 +701,8 @@ static int list(int ac, const char **av, const char *prefix)
 		usage_with_options(worktree_usage, options);
 	else if (verbose && porcelain)
 		die(_("options '%s' and '%s' cannot be used together"), "--verbose", "--porcelain");
+	else if (!line_terminator && !porcelain)
+		die(_("the option '%s' requires '%s'"), "-z", "--porcelain");
 	else {
 		struct worktree **worktrees = get_worktrees();
 		int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;
@@ -708,7 +715,8 @@ static int list(int ac, const char **av, const char *prefix)
 
 		for (i = 0; worktrees[i]; i++) {
 			if (porcelain)
-				show_worktree_porcelain(worktrees[i]);
+				show_worktree_porcelain(worktrees[i],
+							line_terminator);
 			else
 				show_worktree(worktrees[i], path_maxlen, abbrev);
 		}
diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
index c8a5a0aac6d..79e0fce2d90 100755
--- a/t/t2402-worktree-list.sh
+++ b/t/t2402-worktree-list.sh
@@ -64,6 +64,25 @@ test_expect_success '"list" all worktrees --porcelain' '
 	test_cmp expect actual
 '
 
+test_expect_success '"list" all worktrees --porcelain -z' '
+	test_when_finished "rm -rf here _actual actual expect &&
+				git worktree prune" &&
+	printf "worktree %sQHEAD %sQbranch %sQQ" \
+		"$(git rev-parse --show-toplevel)" \
+		$(git rev-parse HEAD --symbolic-full-name HEAD) >expect &&
+	git worktree add --detach here main &&
+	printf "worktree %sQHEAD %sQdetachedQQ" \
+		"$(git -C here rev-parse --show-toplevel)" \
+		"$(git rev-parse HEAD)" >>expect &&
+	git worktree list --porcelain -z >_actual &&
+	nul_to_q <_actual >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '"list" -z fails without --porcelain' '
+	test_must_fail git worktree list -z
+'
+
 test_expect_success '"list" all worktrees with locked annotation' '
 	test_when_finished "rm -rf locked unlocked out && git worktree prune" &&
 	git worktree add --detach locked main &&

base-commit: dab1b7905d0b295f1acef9785bb2b9cbb0fdec84
-- 
gitgitgadget

Re: [PATCH v2] worktree: add -z option for list subcommand

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Add a -z option to be used in conjunction with --porcelain that gives
> NUL-terminated output. As 'worktree list --porcelain' does not quote
> worktree paths this enables it to handle worktree paths that contain
> newlines.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>     worktree: add -z option for list subcommand
>     
>     Thanks to Eric, Jean-Noël and Junio for their comments on V1. I've
>     reworded the docs and option help and tweaked the tests as suggested by
>     Eric, fixed the error messages as suggested by Eric/Jean-Noël and
>     changed the implementation to use write_name_quoted() as suggested by
>     Junio. I've punted doing anything about quoting the output without -z
>     for now, I'll fix that with and without --porcelain in another series.

Thanks for an update.  Will queue.  I think this iteration is good
to merge to 'next', but let's wait for a few days to see what others
think.

It also made me wonder if "-z" alone should be made to imply
"--porcelain" (in other words, is there a good reason to ask for
NUL-terminated output when you are producing a human-readable
output?), but we can start stricter like this patch does; we can
later loosen it if needed.

Re: [PATCH v2] worktree: add -z option for list subcommand

[Phillip Wood]

Hi Junio

On 31/03/2022 21:37, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
 >
> It also made me wonder if "-z" alone should be made to imply
> "--porcelain" (in other words, is there a good reason to ask for
> NUL-terminated output when you are producing a human-readable
> output?), but we can start stricter like this patch does; we can
> later loosen it if needed.

That's a good point about "-z" implying "--porcelain" we do something 
similar for "git status -z" I think.

Best Wishes

Phillip

Re: [PATCH v2] worktree: add -z option for list subcommand

[Eric Sunshine]

On Thu, Mar 31, 2022 at 4:37 PM Junio C Hamano <gitster@pobox.com> wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > Add a -z option to be used in conjunction with --porcelain that gives
> > NUL-terminated output. As 'worktree list --porcelain' does not quote
> > worktree paths this enables it to handle worktree paths that contain
> > newlines.
> >
> > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> > ---
> >     worktree: add -z option for list subcommand
> >
> >     Thanks to Eric, Jean-Noël and Junio for their comments on V1. I've
> >     reworded the docs and option help and tweaked the tests as suggested by
> >     Eric, fixed the error messages as suggested by Eric/Jean-Noël and
> >     changed the implementation to use write_name_quoted() as suggested by
> >     Junio. I've punted doing anything about quoting the output without -z
> >     for now, I'll fix that with and without --porcelain in another series.
>
> Thanks for an update.  Will queue.  I think this iteration is good
> to merge to 'next', but let's wait for a few days to see what others
> think.

Agreed. I think this version addresses my review comments on earlier
rounds and seems to be in good shape.

(Sorry for the late response.)