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

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

[Phillip Wood]

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>
---
I found an old patch that added NUL termination. I've rebased it
but the new test fails as there seems to be another worktree thats
been added since I wrote this, anyway I thought it might be a useful
start for adding a `-z` option.

 Documentation/git-worktree.txt | 13 +++++++++---
 builtin/worktree.c             | 36 ++++++++++++++++++++++------------
 t/t2402-worktree-list.sh       | 22 +++++++++++++++++++++
 3 files changed, 56 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index af06128cc9..a07b593cc3 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b <new-branch>] <path> [<commit-ish>]
-'git worktree list' [--porcelain]
+'git worktree list' [--porcelain [-z]]
 'git worktree lock' [--reason <string>] <worktree>
 'git worktree move' <worktree> <new-path>
 'git worktree prune' [-n] [-v] [--expire <expire>]
@@ -217,7 +217,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::
@@ -369,7 +375,8 @@ $ git worktree list
 
 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.  The first attribute of a working tree is always
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 197fd24a55..0cd331873c 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -640,19 +640,25 @@ 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)
 {
-	printf("worktree %s\n", wt->path);
-	if (wt->is_bare)
-		printf("bare\n");
-	else {
-		printf("HEAD %s\n", oid_to_hex(&wt->head_oid));
-		if (wt->is_detached)
-			printf("detached\n");
-		else if (wt->head_ref)
-			printf("branch %s\n", wt->head_ref);
+	printf("worktree %s", wt->path);
+	fputc(line_terminator, stdout);
+	if (wt->is_bare) {
+		printf("bare");
+		fputc(line_terminator, stdout);
+	} else {
+		printf("HEAD %s", oid_to_hex(&wt->head_oid));
+		fputc(line_terminator, stdout);
+		if (wt->is_detached) {
+			printf("detached");
+			fputc(line_terminator, stdout);
+		} else if (wt->head_ref) {
+			printf("branch %s", wt->head_ref);
+			fputc(line_terminator, stdout);
+		}
 	}
-	printf("\n");
+	fputc(line_terminator, stdout);
 }
 
 static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
@@ -720,15 +726,20 @@ 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_SET_INT('z', NULL, &line_terminator,
+			    N_("fields are separated with NUL character"), '\0'),
 		OPT_END()
 	};
 
 	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
 	if (ac)
 		usage_with_options(worktree_usage, options);
+	else if (!line_terminator && !porcelain)
+		die(_("'-z' requires '--porcelain'"));
 	else {
 		struct worktree **worktrees = get_worktrees();
 		int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;
@@ -741,7 +752,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 795ddca2e4..acd9f8ab84 100755
--- a/t/t2402-worktree-list.sh
+++ b/t/t2402-worktree-list.sh
@@ -71,6 +71,28 @@ test_expect_success '"list" all worktrees with locked annotation' '
 	! grep "/unlocked  *[0-9a-f].* locked$" out
 '
 
+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)" \
+		"$(git symbolic-ref HEAD)" >expect &&
+	git worktree add --detach here master &&
+	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 master &&
+	test_must_fail git worktree list -z
+'
+
 test_expect_success 'bare repo setup' '
 	git init --bare bare1 &&
 	echo "data" >file1 &&
-- 
2.30.0

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

[Eric Sunshine]

On Tue, Jan 5, 2021 at 6:02 AM Phillip Wood <phillip.wood123@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.

Adding a -z mode makes a lot of sense. This, along with a fix to call
quote_c_style() on paths in normal (not `-z`) porcelain mode, can
easily be done after Rafael's series lands. Or they could be done
before his series, though that might make a lot of extra work for him.

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> I found an old patch that added NUL termination. I've rebased it
> but the new test fails as there seems to be another worktree thats
> been added since I wrote this, anyway I thought it might be a useful
> start for adding a `-z` option.

The test failure is fallout from a "bug" in a test added by Rafael's
earlier series[1] which was not caught by the reviewer[2]. In fact,
his current series has a drive-by fix[3] for this bug in patch [6/7].
Applying that fix (or the refinement[4] I suggested in my review)
makes your test work.

[1]: https://lore.kernel.org/git/20201011101152.5291-2-rafaeloliveira.cs@gmail.com/
[2]: https://lore.kernel.org/git/CAPig+cS8hvX4GCsnfLBnQ4Q_AkUad=bw7rjVcaOqSEqcLZvx8w@mail.gmail.com/
[3]: https://lore.kernel.org/git/20210104162128.95281-7-rafaeloliveira.cs@gmail.com/
[4]: https://lore.kernel.org/git/CAPig+cRysXpK0e1xXOuVd+EtkeyTk8FR6MUrL=Bg3W4ve8jdNA@mail.gmail.com/

> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -217,7 +217,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
> +       NUL rather than a newline. This makes it possible to parse the output
> +       when a worktree path contains a newline character.

If we fix normal-mode porcelain to call quote_c_style(), then we can
drop the last sentence or refine it to say something along the lines
of it being easier to deal with paths with embedded newlines than in
normal porcelain mode.

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

Perhaps this could all be done a bit more concisely with printf()
alone rather than combining it with fputc(). For instance:

    printf("worktree %s%c", wt->path, line_terminator);

and so on.

> -       printf("\n");
> +       fputc(line_terminator, stdout);

This use of fputc() makes sense.

> @@ -720,15 +726,20 @@ static void pathsort(struct worktree **wt)
>  static int list(int ac, const char **av, const char *prefix)
>  {
> +               OPT_SET_INT('z', NULL, &line_terminator,
> +                           N_("fields are separated with NUL character"), '\0'),

"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`.

> +       else if (!line_terminator && !porcelain)
> +               die(_("'-z' requires '--porcelain'"));

Other error messages in this file don't quote command-line options, so:

    die(_("-z requires --porcelain"));

would be more consistent.

> diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
> @@ -71,6 +71,28 @@ test_expect_success '"list" all worktrees with locked annotation' '
> +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)" \
> +               "$(git symbolic-ref HEAD)" >expect &&
> +       git worktree add --detach here master &&
> +       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 &&

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 master &&
> +       test_must_fail git worktree list -z
> +'

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
    '

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

[Phillip Wood]

Hi Eric & Rafael

On 07/01/2021 03:34, Eric Sunshine wrote:
> On Tue, Jan 5, 2021 at 6:02 AM Phillip Wood <phillip.wood123@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.
> 
> Adding a -z mode makes a lot of sense. This, along with a fix to call
> quote_c_style() on paths in normal (not `-z`) porcelain mode, 

I'm concerned that starting to quote paths will break backwards 
compatibility. Even if we restricted the quoting to just those paths 
that contain '\n' there is no way to distinguish between a quoted path 
and one that begins and ends with ". This is the reason I prefer to add 
`-z` instead of taking Rafael's patch to quote the lock reason as that 
patch still leaves the output of `git worktree list --porcelain` 
ambiguous and it cannot be fixed without breaking existing users. A 
counter argument to all this is that there are thousands of users on 
file systems that cannot have newlines in paths and Rafael's patch is 
definitely a net win for them.

> can
> easily be done after Rafael's series lands. Or they could be done
> before his series, though that might make a lot of extra work for him.

I would definitely be easiest to wait for Rafael's series to land if we 
want to add `-z` separately

>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>> I found an old patch that added NUL termination. I've rebased it
>> but the new test fails as there seems to be another worktree thats
>> been added since I wrote this, anyway I thought it might be a useful
>> start for adding a `-z` option.
> 
> The test failure is fallout from a "bug" in a test added by Rafael's
> earlier series[1] which was not caught by the reviewer[2]. In fact,
> his current series has a drive-by fix[3] for this bug in patch [6/7].
> Applying that fix (or the refinement[4] I suggested in my review)
> makes your test work.

Thanks ever so much for taking the time to track down the cause of the 
test failure.

Best Wishes

Phillip

> [1]: https://lore.kernel.org/git/20201011101152.5291-2-rafaeloliveira.cs@gmail.com/
> [2]: https://lore.kernel.org/git/CAPig+cS8hvX4GCsnfLBnQ4Q_AkUad=bw7rjVcaOqSEqcLZvx8w@mail.gmail.com/
> [3]: https://lore.kernel.org/git/20210104162128.95281-7-rafaeloliveira.cs@gmail.com/
> [4]: https://lore.kernel.org/git/CAPig+cRysXpK0e1xXOuVd+EtkeyTk8FR6MUrL=Bg3W4ve8jdNA@mail.gmail.com/
> 
>> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
>> @@ -217,7 +217,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
>> +       NUL rather than a newline. This makes it possible to parse the output
>> +       when a worktree path contains a newline character.
> 
> If we fix normal-mode porcelain to call quote_c_style(), then we can
> drop the last sentence or refine it to say something along the lines
> of it being easier to deal with paths with embedded newlines than in
> normal porcelain mode.
> 
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> @@ -640,19 +640,25 @@ static int add(int ac, const char **av, const char *prefix)
>> +static void show_worktree_porcelain(struct worktree *wt, int line_terminator)
>>   {
>> +       printf("worktree %s", wt->path);
>> +       fputc(line_terminator, stdout);
>> +       if (wt->is_bare) {
>> +               printf("bare");
>> +               fputc(line_terminator, stdout);
>> +       } else {
>> +               printf("HEAD %s", oid_to_hex(&wt->head_oid));
>> +               fputc(line_terminator, stdout);
>> +               if (wt->is_detached) {
>> +                       printf("detached");
>> +                       fputc(line_terminator, stdout);
>> +               } else if (wt->head_ref) {
>> +                       printf("branch %s", wt->head_ref);
>> +                       fputc(line_terminator, stdout);
>> +               }
> 
> Perhaps this could all be done a bit more concisely with printf()
> alone rather than combining it with fputc(). For instance:
> 
>      printf("worktree %s%c", wt->path, line_terminator);
> 
> and so on.
> 
>> -       printf("\n");
>> +       fputc(line_terminator, stdout);
> 
> This use of fputc() makes sense.
> 
>> @@ -720,15 +726,20 @@ static void pathsort(struct worktree **wt)
>>   static int list(int ac, const char **av, const char *prefix)
>>   {
>> +               OPT_SET_INT('z', NULL, &line_terminator,
>> +                           N_("fields are separated with NUL character"), '\0'),
> 
> "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`.
> 
>> +       else if (!line_terminator && !porcelain)
>> +               die(_("'-z' requires '--porcelain'"));
> 
> Other error messages in this file don't quote command-line options, so:
> 
>      die(_("-z requires --porcelain"));
> 
> would be more consistent.
> 
>> diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
>> @@ -71,6 +71,28 @@ test_expect_success '"list" all worktrees with locked annotation' '
>> +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)" \
>> +               "$(git symbolic-ref HEAD)" >expect &&
>> +       git worktree add --detach here master &&
>> +       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 &&
> 
> 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 master &&
>> +       test_must_fail git worktree list -z
>> +'
> 
> 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
>      '
> 

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

[Eric Sunshine]

On Fri, Jan 8, 2021 at 5:33 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> On 07/01/2021 03:34, Eric Sunshine wrote:
> > On Tue, Jan 5, 2021 at 6:02 AM Phillip Wood <phillip.wood123@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.
> >
> > Adding a -z mode makes a lot of sense. This, along with a fix to call
> > quote_c_style() on paths in normal (not `-z`) porcelain mode,
>
> I'm concerned that starting to quote paths will break backwards
> compatibility. Even if we restricted the quoting to just those paths
> that contain '\n' there is no way to distinguish between a quoted path
> and one that begins and ends with ".

Backward compatibility is a valid concern, though I haven't managed to
convince myself that it would matter much in this case. In one sense,
the failure of the porcelain format to properly quote/escape paths
when needed can be viewed as an outright bug and, although we value
backward compatibility, we also value correctness, and such bug fixes
have been accepted in the past. Especially in a case such as this, it
seems exceedingly unlikely that fixing the bug would be harmful or
break existing tooling (though, of course that possibility always
exists, even if remotely so).

I can imagine ways in which tooling might be engineered to work around
the shortcoming that `git worktree list --porcelain` doesn't properly
handle newlines embedded in paths, but such tooling would almost
certainly be so fragile anyhow that it would break as we add more keys
to the extensible porcelain format. Coupled with the fact that
newlines embedded in paths are so exceedingly unlikely, it's hard to
imagine that fixing this bug would have a big impact on existing
tooling.

The case you mention about a path which happens to have a double-quote
as its first character does concern me a bit more since existing
tooling wouldn't have had to jump through hoops, or indeed do anything
special, with such paths, unlike the embedded newline case. But then,
too, it's pretty hard to imagine this coming up much, if at all, in
practice. That's not to say that I can't imagine a path, in general,
beginning with a quote, but keeping in mind that we're talking only
about worktree paths, it seems exceedingly unlikely.

My gut feeling (for what it's worth) is that worktree paths containing
embedded newlines (or other special characters) or beginning with a
double-quote is so unlikely to come in in actual practice that viewing
this as a bug fix is probably a reasonable approach, whereas some
other approach -- such as introducing porcelain-v2 or creating a new
porcelain key, say "worktreepath" which supersedes "worktree" (without
retiring "worktree") -- may be overkill.

None of the above is an argument against a complementary `-z` mode,
which I think is a very good idea.

> This is the reason I prefer to add
> `-z` instead of taking Rafael's patch to quote the lock reason as that
> patch still leaves the output of `git worktree list --porcelain`
> ambiguous and it cannot be fixed without breaking existing users. A
> counter argument to all this is that there are thousands of users on
> file systems that cannot have newlines in paths and Rafael's patch is
> definitely a net win for them.

Rafael's patch is quoting only the lock-reason, not the worktree path,
so I think it's orthogonal to this discussion. Also, his patch is
introducing `lock` as a new attribute in porcelain output, not
modifying behavior of an existing `lock` attribute.

[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 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

[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

[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
> 

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

[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

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

[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.)