worktree: add -z option for list subcommand

Documentation/git-worktree.txt +2 -1

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

builtin/worktree.c +4 -1

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

Documentation/git-worktree.txt This can also be set up as the default behaviour by using the · line 232

Quoted 4 lines · Phillip Wood via GitGitGadget

+-z::

Documentation/git-worktree.txt +3 -0

+     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?

builtin/worktree.c static int add(int ac, const char **av, const char *prefix) · line 600

Quoted 5 lines

builtin/worktree.c +5 -0

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

Quoted 6 lines · Junio C Hamano

Documentation/git-worktree.txt This can also be set up as the default behaviour by using the · line 232

Quoted 4 lines · Phillip Wood via GitGitGadget

+-z::

Documentation/git-worktree.txt +3 -0

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

Quoted 8 lines

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/[email protected]/

Quoted 8 lines

builtin/worktree.c static int add(int ac, const char **av, const char *prefix) · line 600

Quoted 5 lines

builtin/worktree.c +5 -0

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

Quoted 2 lines

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.

"Phillip Wood via GitGitGadget" <[email protected]> writes:

[...]

builtin/worktree.c static int add(int ac, const char **av, const char *prefix) · line 600

Quoted 10 lines

  	reason = worktree_lock_reason(wt);
  	if (reason && *reason) {
  		struct strbuf sb = STRBUF_INIT;

builtin/worktree.c +5 -2

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

builtin/worktree.c static int add(int ac, const char **av, const char *prefix) · line 603

Quoted 4 lines

  		strbuf_release(&sb);
  	} else if (reason)

builtin/worktree.c +1 -1

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

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 <[email protected]>

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

builtin/worktree.c +3 -3

@@ -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);

t/t2402-worktree-list.sh +2 -0

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

Quoted 9 lines · Phillip Wood via GitGitGadget

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 <[email protected]>

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

Documentation/git-worktree.txt This can also be set up as the default behaviour by using the · line 224

Quoted 4 lines

Documentation/git-worktree.txt +2 -0

@@ -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/&,/

Documentation/git-worktree.txt This can also be set up as the default behaviour by using the · line 232

Quoted 2 lines

Documentation/git-worktree.txt +2 -0

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

Quoted 10 lines

builtin/worktree.c +3 -3

@@ -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);

builtin/worktree.c static int add(int ac, const char **av, const char *prefix) · line 586

Quoted 10 lines

builtin/worktree.c +3 -3

         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.

builtin/worktree.c static int add(int ac, const char **av, const char *prefix) · line 600

Quoted 9 lines

builtin/worktree.c +5 -2

         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.

builtin/worktree.c static void pathsort(struct worktree **wt) · line 685

Quoted 3 lines

builtin/worktree.c +2 -0

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

builtin/worktree.c static int list(int ac, const char **av, const char *prefix) · line 706

Quoted 6 lines

builtin/worktree.c +2 -0

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

Quoted 4 lines

t/t2402-worktree-list.sh +2 -0

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

t/t2402-worktree-list.sh test_expect_success '"list" all worktrees --porcelain' ' · line 75

Quoted 10 lines

t/t2402-worktree-list.sh +10 -0

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

t/t2402-worktree-list.sh test_expect_success '"list" all worktrees --porcelain' ' · line 86

Quoted 8 lines

t/t2402-worktree-list.sh +8 -0

+       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/

Le 25/02/2022 à 16:08, Phillip Wood via GitGitGadget a écrit :

Quoted 12 lines

From: Phillip Wood <[email protected]>

builtin/worktree.c +2 -0

@@ -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();

Quoted 1 line

          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

From: Phillip Wood <[email protected]>

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 <[email protected]>

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

"Phillip Wood via GitGitGadget" <[email protected]> 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.

Quoted 9 lines · Phillip Wood via GitGitGadget

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 <[email protected]>

---
worktree: add -z option for list subcommand

Quoted 6 lines

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.