{"thread":{"id":"57475","subject":"[PATCH] worktree: add -z option for list subcommand","startedAt":"2022-02-25T15:08:54Z","lastAt":"2023-05-11T04:12:33Z","messageCount":14,"participants":["Phillip Wood via GitGitGadget","Junio C Hamano","Eric Sunshine","Jean-Noël Avila","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"449585","messageId":"pull.1164.git.1645801727732.gitgitgadget@gmail.com","threadId":"57475","inReplyTo":null,"subject":"[PATCH] worktree: add -z option for list subcommand","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-25T15:08:47Z","receivedAt":"2022-02-25T15:08:54Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nAdd a -z option to be used in conjunction with --porcelain that gives\nNUL-terminated output. This enables 'worktree list --porcelain' to\nhandle worktree paths that contain newlines.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n    worktree: add -z option for list subcommand\n    \n    Add a -z option to be used in conjunction with --porcelain that gives\n    NUL-terminated output. This enables 'worktree list --porcelain' to\n    handle worktree paths that contain newlines.\n    \n    For a previous discussion of the merits of adding a -z option vs quoting\n    the worktree path see\n    https://lore.kernel.org/git/CAPig+cT-9sjmkdWFEcFS=rg9ziV9b6uWNMpQ8BTYP-a258La6Q@mail.gmail.com/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1164%2Fphillipwood%2Fwip%2Fworktree-list-nul-termination-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1164/phillipwood/wip/worktree-list-nul-termination-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1164\n\n Documentation/git-worktree.txt | 15 +++++++++++----\n builtin/worktree.c             | 33 +++++++++++++++++++++------------\n t/t2402-worktree-list.sh       | 21 +++++++++++++++++++++\n 3 files changed, 53 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex 9e862fbcf79..a3fcd498df7 100644\n--- a/Documentation/git-worktree.txt\n+++ b/Documentation/git-worktree.txt\n@@ -10,7 +10,7 @@ SYNOPSIS\n --------\n [verse]\n 'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] [-b <new-branch>] <path> [<commit-ish>]\n-'git worktree list' [-v | --porcelain]\n+'git worktree list' [-v | --porcelain [-z]]\n 'git worktree lock' [--reason <string>] <worktree>\n 'git worktree move' <worktree> <new-path>\n 'git worktree prune' [-n] [-v] [--expire <expire>]\n@@ -223,7 +223,13 @@ This can also be set up as the default behaviour by using the\n --porcelain::\n \tWith `list`, output in an easy-to-parse format for scripts.\n \tThis format will remain stable across Git versions and regardless of user\n-\tconfiguration.  See below for details.\n+\tconfiguration.  It is recommended to combine this with `-z`.\n+\tSee below for details.\n+\n+-z::\n+\tWhen `--porcelain` is specified with `list` terminate each line with a\n+\tNUL rather than a newline. This makes it possible to parse the output\n+\twhen a worktree path contains a newline character.\n \n -q::\n --quiet::\n@@ -411,7 +417,8 @@ working tree itself.\n \n Porcelain Format\n ~~~~~~~~~~~~~~~~\n-The porcelain format has a line per attribute.  Attributes are listed with a\n+The porcelain format has a line per attribute.  If `-z` is given then the lines\n+are terminated with NUL rather than a newline.  Attributes are listed with a\n label and value separated by a single space.  Boolean attributes (like `bare`\n and `detached`) are listed as a label only, and are present only\n if the value is true.  Some attributes (like `locked`) can be listed as a label\n@@ -449,7 +456,7 @@ prunable gitdir file points to non-existent location\n \n ------------\n \n-If the lock reason contains \"unusual\" characters such as newline, they\n+Unless `-z` is used any \"unusual\" characters in the lock reason such as newlines\n are escaped and the entire reason is quoted as explained for the\n configuration variable `core.quotePath` (see linkgit:git-config[1]).\n For Example:\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 0d0809276fe..b4cc586f5c5 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -575,35 +575,38 @@ static int add(int ac, const char **av, const char *prefix)\n \treturn add_worktree(path, branch, &opts);\n }\n \n-static void show_worktree_porcelain(struct worktree *wt)\n+static void show_worktree_porcelain(struct worktree *wt, int line_terminator)\n {\n \tconst char *reason;\n \n-\tprintf(\"worktree %s\\n\", wt->path);\n+\tprintf(\"worktree %s%c\", wt->path, line_terminator);\n \tif (wt->is_bare)\n-\t\tprintf(\"bare\\n\");\n+\t\tprintf(\"bare%c\", line_terminator);\n \telse {\n-\t\tprintf(\"HEAD %s\\n\", oid_to_hex(&wt->head_oid));\n+\t\tprintf(\"HEAD %s%c\", oid_to_hex(&wt->head_oid), line_terminator);\n \t\tif (wt->is_detached)\n-\t\t\tprintf(\"detached\\n\");\n+\t\t\tprintf(\"detached%c\", line_terminator);\n \t\telse if (wt->head_ref)\n-\t\t\tprintf(\"branch %s\\n\", wt->head_ref);\n+\t\t\tprintf(\"branch %s%c\", wt->head_ref, line_terminator);\n \t}\n \n \treason = worktree_lock_reason(wt);\n \tif (reason && *reason) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n-\t\tquote_c_style(reason, &sb, NULL, 0);\n-\t\tprintf(\"locked %s\\n\", sb.buf);\n+\t\tif (line_terminator) {\n+\t\t\tquote_c_style(reason, &sb, NULL, 0);\n+\t\t\treason = sb.buf;\n+\t\t}\n+\t\tprintf(\"locked %s%c\", reason, line_terminator);\n \t\tstrbuf_release(&sb);\n \t} else if (reason)\n-\t\tprintf(\"locked\\n\");\n+\t\tprintf(\"locked%c\", line_terminator);\n \n \treason = worktree_prune_reason(wt, expire);\n \tif (reason)\n-\t\tprintf(\"prunable %s\\n\", reason);\n+\t\tprintf(\"prunable %s%c\", reason, line_terminator);\n \n-\tprintf(\"\\n\");\n+\tfputc(line_terminator, stdout);\n }\n \n static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)\n@@ -681,12 +684,15 @@ static void pathsort(struct worktree **wt)\n static int list(int ac, const char **av, const char *prefix)\n {\n \tint porcelain = 0;\n+\tint line_terminator = '\\n';\n \n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"porcelain\", &porcelain, N_(\"machine-readable output\")),\n \t\tOPT__VERBOSE(&verbose, N_(\"show extended annotations and reasons, if available\")),\n \t\tOPT_EXPIRY_DATE(0, \"expire\", &expire,\n \t\t\t\tN_(\"add 'prunable' annotation to worktrees older than <time>\")),\n+\t\tOPT_SET_INT('z', NULL, &line_terminator,\n+\t\t\t    N_(\"fields are separated with NUL character\"), '\\0'),\n \t\tOPT_END()\n \t};\n \n@@ -696,6 +702,8 @@ static int list(int ac, const char **av, const char *prefix)\n \t\tusage_with_options(worktree_usage, options);\n \telse if (verbose && porcelain)\n \t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--verbose\", \"--porcelain\");\n+\telse if (!line_terminator && !porcelain)\n+\t\tdie(_(\"'-z' requires '--porcelain'\"));\n \telse {\n \t\tstruct worktree **worktrees = get_worktrees();\n \t\tint path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;\n@@ -708,7 +716,8 @@ static int list(int ac, const char **av, const char *prefix)\n \n \t\tfor (i = 0; worktrees[i]; i++) {\n \t\t\tif (porcelain)\n-\t\t\t\tshow_worktree_porcelain(worktrees[i]);\n+\t\t\t\tshow_worktree_porcelain(worktrees[i],\n+\t\t\t\t\t\t\tline_terminator);\n \t\t\telse\n \t\t\t\tshow_worktree(worktrees[i], path_maxlen, abbrev);\n \t\t}\ndiff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh\nindex c8a5a0aac6d..48d5d30d709 100755\n--- a/t/t2402-worktree-list.sh\n+++ b/t/t2402-worktree-list.sh\n@@ -64,6 +64,27 @@ test_expect_success '\"list\" all worktrees --porcelain' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '\"list\" all worktrees --porcelain -z' '\n+\ttest_when_finished \"rm -rf here _actual actual expect &&\n+\t\t\t\tgit worktree prune\" &&\n+\tprintf \"worktree %sQHEAD %sQbranch %sQQ\" \\\n+\t\t\"$(git rev-parse --show-toplevel)\" \\\n+\t\t$(git rev-parse HEAD --symbolic-full-name HEAD) >expect &&\n+\tgit worktree add --detach here main &&\n+\tprintf \"worktree %sQHEAD %sQdetachedQQ\" \\\n+\t\t\"$(git -C here rev-parse --show-toplevel)\" \\\n+\t\t\"$(git rev-parse HEAD)\" >>expect &&\n+\tgit worktree list --porcelain -z >_actual &&\n+\tcat _actual | tr \"\\0\" Q >actual\t&&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '\"list\" -z fails without --porcelain' '\n+\ttest_when_finished \"rm -rf here && git worktree prune\" &&\n+\tgit worktree add --detach here main &&\n+\ttest_must_fail git worktree list -z\n+'\n+\n test_expect_success '\"list\" all worktrees with locked annotation' '\n \ttest_when_finished \"rm -rf locked unlocked out && git worktree prune\" &&\n \tgit worktree add --detach locked main &&\n\nbase-commit: dab1b7905d0b295f1acef9785bb2b9cbb0fdec84\n-- \ngitgitgadget\n"},{"id":"449615","messageId":"xmqqh78mesj6.fsf@gitster.g","threadId":"57475","inReplyTo":"pull.1164.git.1645801727732.gitgitgadget@gmail.com","subject":"Re: [PATCH] worktree: add -z option for list subcommand","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-25T17:59:41Z","receivedAt":"2022-02-25T17:59:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> +-z::\n> +\tWhen `--porcelain` is specified with `list` terminate each line with a\n> +\tNUL rather than a newline. This makes it possible to parse the output\n> +\twhen a worktree path contains a newline character.\n\nThis makes it sound as if it were impossible.  As the changes this\npatch makes to the documentation and the code indicate, we were\nalready doing quote_c_style(), so it is not quite correct to say so.\n\nPerhaps \"makes it easier\" is more accurate?\n\nUnless the original wasn't using quote_c_style() correctly and\nwasn't quoting its output, that is?\n\n> @@ -411,7 +417,8 @@ working tree itself.\n>  \n>  Porcelain Format\n>  ~~~~~~~~~~~~~~~~\n> -The porcelain format has a line per attribute.  Attributes are listed with a\n> +The porcelain format has a line per attribute.  If `-z` is given then the lines\n> +are terminated with NUL rather than a newline.  Attributes are listed with a\n>  label and value separated by a single space.  Boolean attributes (like `bare`\n>  and `detached`) are listed as a label only, and are present only\n>  if the value is true.  Some attributes (like `locked`) can be listed as a label\n> @@ -449,7 +456,7 @@ prunable gitdir file points to non-existent location\n>  \n>  ------------\n>  \n> -If the lock reason contains \"unusual\" characters such as newline, they\n> +Unless `-z` is used any \"unusual\" characters in the lock reason such as newlines\n>  are escaped and the entire reason is quoted as explained for the\n\nOK.  That is sensible.\n\n>  \treason = worktree_lock_reason(wt);\n>  \tif (reason && *reason) {\n>  \t\tstruct strbuf sb = STRBUF_INIT;\n> -\t\tquote_c_style(reason, &sb, NULL, 0);\n> -\t\tprintf(\"locked %s\\n\", sb.buf);\n> +\t\tif (line_terminator) {\n> +\t\t\tquote_c_style(reason, &sb, NULL, 0);\n> +\t\t\treason = sb.buf;\n> +\t\t}\n> +\t\tprintf(\"locked %s%c\", reason, line_terminator);\n\nOK.  I suspect write_name_quoted() may be a better fit that does not\nrequire us to have our own strbuf, but this should be OK.\n\n>  \t\tstrbuf_release(&sb);\n>  \t} else if (reason)\n> -\t\tprintf(\"locked\\n\");\n> +\t\tprintf(\"locked%c\", line_terminator);\n\nIt is a shame that we need a special code path for an empty string\ngiven as the reason, only for the single SP after \"locked\", but we\nhave to live with it, I guess.\n\n>  \treason = worktree_prune_reason(wt, expire);\n>  \tif (reason)\n> -\t\tprintf(\"prunable %s\\n\", reason);\n> +\t\tprintf(\"prunable %s%c\", reason, line_terminator);\n>  \n> -\tprintf(\"\\n\");\n> +\tfputc(line_terminator, stdout);\n>  }\n\nAll code changes looked sensible.\n\n> +\telse if (!line_terminator && !porcelain)\n> +\t\tdie(_(\"'-z' requires '--porcelain'\"));\n>  \telse {\n>  \t\tstruct worktree **worktrees = get_worktrees();\n>  \t\tint path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;\n> @@ -708,7 +716,8 @@ static int list(int ac, const char **av, const char *prefix)\n>  \n>  \t\tfor (i = 0; worktrees[i]; i++) {\n>  \t\t\tif (porcelain)\n> -\t\t\t\tshow_worktree_porcelain(worktrees[i]);\n> +\t\t\t\tshow_worktree_porcelain(worktrees[i],\n> +\t\t\t\t\t\t\tline_terminator);\n>  \t\t\telse\n>  \t\t\t\tshow_worktree(worktrees[i], path_maxlen, abbrev);\n>  \t\t}\n"},{"id":"449728","messageId":"CAPig+cTx-n_WQqvVYyegtvW8gRbXEPsm5C7fmHq=9PZeANZ-wQ@mail.gmail.com","threadId":"57475","inReplyTo":"pull.1164.git.1645801727732.gitgitgadget@gmail.com","subject":"Re: [PATCH] worktree: add -z option for list subcommand","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-02-28T08:16:47Z","receivedAt":"2022-02-28T08:17:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 25, 2022 at 10:08 AM Phillip Wood via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> Add a -z option to be used in conjunction with --porcelain that gives\n> NUL-terminated output. This enables 'worktree list --porcelain' to\n> handle worktree paths that contain newlines.\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>     For a previous discussion of the merits of adding a -z option vs quoting\n>     the worktree path see\n>     https://lore.kernel.org/git/CAPig+cT-9sjmkdWFEcFS=rg9ziV9b6uWNMpQ8BTYP-a258La6Q@mail.gmail.com/\n\nThanks for resubmitting.\n\n> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\n> @@ -223,7 +223,13 @@ This can also be set up as the default behaviour by using the\n> +-z::\n> +       When `--porcelain` is specified with `list` terminate each line with a\n\nNit: s/`list`/&,/\n\n> +       NUL rather than a newline. This makes it possible to parse the output\n> +       when a worktree path contains a newline character.\n\nOr, perhaps:\n\n    Terminate each line with NUL rather than a newline when\n    `--porcelain` is specified with `list`. This makes it possible...\n\nmight be simpler(?).\n\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> @@ -575,35 +575,38 @@ static int add(int ac, const char **av, const char *prefix)\n> -static void show_worktree_porcelain(struct worktree *wt)\n> +static void show_worktree_porcelain(struct worktree *wt, int line_terminator)\n>  {\n> -       printf(\"worktree %s\\n\", wt->path);\n> +       printf(\"worktree %s%c\", wt->path, line_terminator);\n>         if (wt->is_bare)\n> -               printf(\"bare\\n\");\n> +               printf(\"bare%c\", line_terminator);\n>         else {\n> -               printf(\"HEAD %s\\n\", oid_to_hex(&wt->head_oid));\n> +               printf(\"HEAD %s%c\", oid_to_hex(&wt->head_oid), line_terminator);\n>                 if (wt->is_detached)\n> -                       printf(\"detached\\n\");\n> +                       printf(\"detached%c\", line_terminator);\n>                 else if (wt->head_ref)\n> -                       printf(\"branch %s\\n\", wt->head_ref);\n> +                       printf(\"branch %s%c\", wt->head_ref, line_terminator);\n>         }\n\nGood, this is easier to read than the previous version which manually\ncalled `fputc(line_terminator, stdout)` repeatedly for the\ntermination. The diff is also more easily digested.\n\n>         if (reason && *reason) {\n>                 struct strbuf sb = STRBUF_INIT;\n> -               quote_c_style(reason, &sb, NULL, 0);\n> -               printf(\"locked %s\\n\", sb.buf);\n> +               if (line_terminator) {\n> +                       quote_c_style(reason, &sb, NULL, 0);\n> +                       reason = sb.buf;\n> +               }\n> +               printf(\"locked %s%c\", reason, line_terminator);\n\nJunio's suggestion downstream that write_name_quoted() would be\nsimpler makes sense, but (as he says) is not necessarily worth a\nreroll.\n\n> @@ -681,12 +684,15 @@ static void pathsort(struct worktree **wt)\n> +               OPT_SET_INT('z', NULL, &line_terminator,\n> +                           N_(\"fields are separated with NUL character\"), '\\0'),\n\nSame comment as previous review [1]:\n\n    \"fields\" sounds a little odd. \"lines\" might make more sense.\n    \"records\" might be even better and matches the wording of some\n    other Git commands accepting `-z`.\n\n> @@ -696,6 +702,8 @@ static int list(int ac, const char **av, const char *prefix)\n>                 usage_with_options(worktree_usage, options);\n>         else if (verbose && porcelain)\n>                 die(_(\"options '%s' and '%s' cannot be used together\"), \"--verbose\", \"--porcelain\");\n> +       else if (!line_terminator && !porcelain)\n> +               die(_(\"'-z' requires '--porcelain'\"));\n\nSame comment as my previous review[1]:\n\n    Other error messages in this file don't quote command-line\n    options, so `die(_(\"-z requires --porcelain\"));` would be more\n    consistent.\n\nHowever, considering all the recent work Jean-Noël Avila has been\ndoing to recently, perhaps this should instead mirror his change to\nthe die() message just above the new one and instead be:\n\n    die(_(\"'%s' requires '%s'\"), \"-z\", \"--porcelain\");\n\n> diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh\n> @@ -64,6 +64,27 @@ test_expect_success '\"list\" all worktrees --porcelain' '\n> +test_expect_success '\"list\" all worktrees --porcelain -z' '\n> +       test_when_finished \"rm -rf here _actual actual expect &&\n> +                               git worktree prune\" &&\n> +       printf \"worktree %sQHEAD %sQbranch %sQQ\" \\\n> +               \"$(git rev-parse --show-toplevel)\" \\\n> +               $(git rev-parse HEAD --symbolic-full-name HEAD) >expect &&\n> +       git worktree add --detach here main &&\n> +       printf \"worktree %sQHEAD %sQdetachedQQ\" \\\n> +               \"$(git -C here rev-parse --show-toplevel)\" \\\n> +               \"$(git rev-parse HEAD)\" >>expect &&\n> +       git worktree list --porcelain -z >_actual &&\n> +       cat _actual | tr \"\\0\" Q >actual &&\n\nSame comment as my previous review[1]:\n\n    Or just use nul_to_q():\n      nul_to_q <_actual >actual &&\n    (And there's no need to `cat` the file.)\n\n> +       test_cmp expect actual\n> +'\n> +\n> +test_expect_success '\"list\" -z fails without --porcelain' '\n> +       test_when_finished \"rm -rf here && git worktree prune\" &&\n> +       git worktree add --detach here main &&\n> +       test_must_fail git worktree list -z\n> +'\n\nSame comment as my previous review[1]:\n\n    I don't think there's any need for this test to create (and\n    cleanup) a worktree. So, the entire test could collapse to:\n\n      test_expect_success '\"list\" -z fails without --porcelain' '\n        test_must_fail git worktree list -z\n      '\n\n[1]: https://lore.kernel.org/git/CAPig+cT-9sjmkdWFEcFS=rg9ziV9b6uWNMpQ8BTYP-a258La6Q@mail.gmail.com/\n"},{"id":"449729","messageId":"CAPig+cQF3y64bPN8h4XvAYGX-g0LZedh-cAM+JLKdW_DPAWJaw@mail.gmail.com","threadId":"57475","inReplyTo":"xmqqh78mesj6.fsf@gitster.g","subject":"Re: [PATCH] worktree: add -z option for list subcommand","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-02-28T08:35:09Z","receivedAt":"2022-02-28T08:35:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 25, 2022 at 12:59 PM Junio C Hamano <gitster@pobox.com> wrote:\n> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> > +-z::\n> > +     When `--porcelain` is specified with `list` terminate each line with a\n> > +     NUL rather than a newline. This makes it possible to parse the output\n> > +     when a worktree path contains a newline character.\n>\n> This makes it sound as if it were impossible.  As the changes this\n> patch makes to the documentation and the code indicate, we were\n> already doing quote_c_style(), so it is not quite correct to say so.\n>\n> Perhaps \"makes it easier\" is more accurate?\n>\n> Unless the original wasn't using quote_c_style() correctly and\n> wasn't quoting its output, that is?\n\nThat's the case. It is impossible without this patch since `git\nworktree list --porcelain` does not call quote_c_style() for the\nworktree path; it only calls quote_c_style() for the lock reason.\nFixing this oversight for the worktree path for non-`-z` mode has\npotential backward-compatibility concerns. I had argued[1] that we\nmight be able to sell it as a pure bug fix, but Phillip was\nconcerned[2] that it might break existing tooling. (I also share that\nconcern, but to a lesser degree, perhaps, since worktrees with oddball\nnames containing newline or a leading double-quote must be exceedingly\nrare.)\n\n[1]: https://lore.kernel.org/git/CAPig+cQq_RnanDQ3jHfNz_L58WyzmsUJBhtdrLxa=H0v_io+WA@mail.gmail.com/\n[2]: https://lore.kernel.org/git/936f9b7c-6d54-00bc-f136-4cb4c2836eb6@gmail.com/\n\n> > +             if (line_terminator) {\n> > +                     quote_c_style(reason, &sb, NULL, 0);\n> > +                     reason = sb.buf;\n> > +             }\n> > +             printf(\"locked %s%c\", reason, line_terminator);\n>\n> OK.  I suspect write_name_quoted() may be a better fit that does not\n> require us to have our own strbuf, but this should be OK.\n\nNice suggestion.\n"},{"id":"449737","messageId":"90fcd727-414f-fc1f-fab3-2cbce5790aa0@free.fr","threadId":"57475","inReplyTo":"pull.1164.git.1645801727732.gitgitgadget@gmail.com","subject":"Re: [PATCH] worktree: add -z option for list subcommand","fromName":"Jean-Noël Avila","fromEmail":"jn.avila@free.fr","sentAt":"2022-02-28T09:47:00Z","receivedAt":"2022-02-28T09:47:10Z","isPatch":true,"sender":{"key":"jn.avila@free.fr","avatar":"https://avatars.githubusercontent.com/u/156172?v=4"},"body":"Le 25/02/2022 à 16:08, Phillip Wood via GitGitGadget a écrit :\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n>\n> @@ -696,6 +702,8 @@ static int list(int ac, const char **av, const char *prefix)\n>   \t\tusage_with_options(worktree_usage, options);\n>   \telse if (verbose && porcelain)\n>   \t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--verbose\", \"--porcelain\");\n> +\telse if (!line_terminator && !porcelain)\n> +\t\tdie(_(\"'-z' requires '--porcelain'\"));\n>   \telse {\n>   \t\tstruct worktree **worktrees = get_worktrees();\n>   \t\tint path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;\n\n\nPlease better use\n\n  die(_(\"the option '%s' requires '%s'\"),  \"-z\", \"--porcelain\");\n\n\nIn order to make a no-op for translators.\n\n\nBest regards,\n\nJN\n\n"},{"id":"449740","messageId":"074263e5-7974-6272-2b9a-a55f1b6598d3@gmail.com","threadId":"57475","inReplyTo":"90fcd727-414f-fc1f-fab3-2cbce5790aa0@free.fr","subject":"Re: [PATCH] worktree: add -z option for list subcommand","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-02-28T10:57:29Z","receivedAt":"2022-02-28T10:58:54Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Jean-Noël\n\nOn 28/02/2022 09:47, Jean-Noël Avila wrote:\n> Le 25/02/2022 à 16:08, Phillip Wood via GitGitGadget a écrit :\n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>\n>>\n>> @@ -696,6 +702,8 @@ static int list(int ac, const char **av, const \n>> char *prefix)\n>>           usage_with_options(worktree_usage, options);\n>>       else if (verbose && porcelain)\n>>           die(_(\"options '%s' and '%s' cannot be used together\"), \n>> \"--verbose\", \"--porcelain\");\n>> +    else if (!line_terminator && !porcelain)\n>> +        die(_(\"'-z' requires '--porcelain'\"));\n>>       else {\n>>           struct worktree **worktrees = get_worktrees();\n>>           int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;\n> \n> \n> Please better use\n> \n>   die(_(\"the option '%s' requires '%s'\"),  \"-z\", \"--porcelain\");\n> \n> \n> In order to make a no-op for translators.\n\nWill do, thanks for the suggestion\n\nPhillip\n\n> \n> Best regards,\n> \n> JN\n> \n\n"},{"id":"449745","messageId":"dd16416d-15c4-7f80-aaf6-a36d101e8928@gmail.com","threadId":"57475","inReplyTo":"CAPig+cTx-n_WQqvVYyegtvW8gRbXEPsm5C7fmHq=9PZeANZ-wQ@mail.gmail.com","subject":"Re: [PATCH] worktree: add -z option for list subcommand","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-02-28T11:08:58Z","receivedAt":"2022-02-28T11:09:44Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Eric\n\nOn 28/02/2022 08:16, Eric Sunshine wrote:\n> On Fri, Feb 25, 2022 at 10:08 AM Phillip Wood via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n>> Add a -z option to be used in conjunction with --porcelain that gives\n>> NUL-terminated output. This enables 'worktree list --porcelain' to\n>> handle worktree paths that contain newlines.\n>>\n>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>> ---\n>>      For a previous discussion of the merits of adding a -z option vs quoting\n>>      the worktree path see\n>>      https://lore.kernel.org/git/CAPig+cT-9sjmkdWFEcFS=rg9ziV9b6uWNMpQ8BTYP-a258La6Q@mail.gmail.com/\n> \n> Thanks for resubmitting.\n> \n>> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\n>> @@ -223,7 +223,13 @@ This can also be set up as the default behaviour by using the\n>> +-z::\n>> +       When `--porcelain` is specified with `list` terminate each line with a\n> \n> Nit: s/`list`/&,/\n> \n>> +       NUL rather than a newline. This makes it possible to parse the output\n>> +       when a worktree path contains a newline character.\n> \n> Or, perhaps:\n> \n>      Terminate each line with NUL rather than a newline when\n>      `--porcelain` is specified with `list`. This makes it possible...\n> \n> might be simpler(?).\n> \n>> diff --git a/builtin/worktree.c b/builtin/worktree.c\n>> @@ -575,35 +575,38 @@ static int add(int ac, const char **av, const char *prefix)\n>> -static void show_worktree_porcelain(struct worktree *wt)\n>> +static void show_worktree_porcelain(struct worktree *wt, int line_terminator)\n>>   {\n>> -       printf(\"worktree %s\\n\", wt->path);\n>> +       printf(\"worktree %s%c\", wt->path, line_terminator);\n>>          if (wt->is_bare)\n>> -               printf(\"bare\\n\");\n>> +               printf(\"bare%c\", line_terminator);\n>>          else {\n>> -               printf(\"HEAD %s\\n\", oid_to_hex(&wt->head_oid));\n>> +               printf(\"HEAD %s%c\", oid_to_hex(&wt->head_oid), line_terminator);\n>>                  if (wt->is_detached)\n>> -                       printf(\"detached\\n\");\n>> +                       printf(\"detached%c\", line_terminator);\n>>                  else if (wt->head_ref)\n>> -                       printf(\"branch %s\\n\", wt->head_ref);\n>> +                       printf(\"branch %s%c\", wt->head_ref, line_terminator);\n>>          }\n> \n> Good, this is easier to read than the previous version which manually\n> called `fputc(line_terminator, stdout)` repeatedly for the\n> termination. The diff is also more easily digested.\n> \n>>          if (reason && *reason) {\n>>                  struct strbuf sb = STRBUF_INIT;\n>> -               quote_c_style(reason, &sb, NULL, 0);\n>> -               printf(\"locked %s\\n\", sb.buf);\n>> +               if (line_terminator) {\n>> +                       quote_c_style(reason, &sb, NULL, 0);\n>> +                       reason = sb.buf;\n>> +               }\n>> +               printf(\"locked %s%c\", reason, line_terminator);\n> \n> Junio's suggestion downstream that write_name_quoted() would be\n> simpler makes sense, but (as he says) is not necessarily worth a\n> reroll.\n\nYes, I wasn't aware of that function but I'll update as I'm going to \nre-roll anyway\n\n>> @@ -681,12 +684,15 @@ static void pathsort(struct worktree **wt)\n>> +               OPT_SET_INT('z', NULL, &line_terminator,\n>> +                           N_(\"fields are separated with NUL character\"), '\\0'),\n> \n> Same comment as previous review [1]:\n> \n>      \"fields\" sounds a little odd. \"lines\" might make more sense.\n>      \"records\" might be even better and matches the wording of some\n>      other Git commands accepting `-z`.\n\nIt would be good to standardize this, check-attr and check-ignore use \n\"records\", ls-files, ls-tree and status use \"entries\", config uses \n\"values\" (which probably makes sense in the context of that command).\n\n>> @@ -696,6 +702,8 @@ static int list(int ac, const char **av, const char *prefix)\n>>                  usage_with_options(worktree_usage, options);\n>>          else if (verbose && porcelain)\n>>                  die(_(\"options '%s' and '%s' cannot be used together\"), \"--verbose\", \"--porcelain\");\n>> +       else if (!line_terminator && !porcelain)\n>> +               die(_(\"'-z' requires '--porcelain'\"));\n> \n> Same comment as my previous review[1]:\n\nSorry I'd forgotten you'd left some comments before, I should have spent \nmore time rereading that old thread\n\n>      Other error messages in this file don't quote command-line\n>      options, so `die(_(\"-z requires --porcelain\"));` would be more\n>      consistent.\n> \n> However, considering all the recent work Jean-Noël Avila has been\n> doing to recently, perhaps this should instead mirror his change to\n> the die() message just above the new one and instead be:\n> \n>      die(_(\"'%s' requires '%s'\"), \"-z\", \"--porcelain\");\n\nYes, I'll update this, Jean-Noël made the same suggestion. I wish we had \nsome functions/macros for these standard messages I think it would be \neasier to remember to use them than trying to remember the standard \nwording for messages like this.\n\n>> diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh\n>> @@ -64,6 +64,27 @@ test_expect_success '\"list\" all worktrees --porcelain' '\n>> +test_expect_success '\"list\" all worktrees --porcelain -z' '\n>> +       test_when_finished \"rm -rf here _actual actual expect &&\n>> +                               git worktree prune\" &&\n>> +       printf \"worktree %sQHEAD %sQbranch %sQQ\" \\\n>> +               \"$(git rev-parse --show-toplevel)\" \\\n>> +               $(git rev-parse HEAD --symbolic-full-name HEAD) >expect &&\n>> +       git worktree add --detach here main &&\n>> +       printf \"worktree %sQHEAD %sQdetachedQQ\" \\\n>> +               \"$(git -C here rev-parse --show-toplevel)\" \\\n>> +               \"$(git rev-parse HEAD)\" >>expect &&\n>> +       git worktree list --porcelain -z >_actual &&\n>> +       cat _actual | tr \"\\0\" Q >actual &&\n> \n> Same comment as my previous review[1]:\n> \n>      Or just use nul_to_q():\n>        nul_to_q <_actual >actual &&\n>      (And there's no need to `cat` the file.)\n\nThat's a good idea\n\n>> +       test_cmp expect actual\n>> +'\n>> +\n>> +test_expect_success '\"list\" -z fails without --porcelain' '\n>> +       test_when_finished \"rm -rf here && git worktree prune\" &&\n>> +       git worktree add --detach here main &&\n>> +       test_must_fail git worktree list -z\n>> +'\n> \n> Same comment as my previous review[1]:\n> \n>      I don't think there's any need for this test to create (and\n>      cleanup) a worktree. So, the entire test could collapse to:\n> \n>        test_expect_success '\"list\" -z fails without --porcelain' '\n>          test_must_fail git worktree list -z\n>        '\n\nGood point\n\nThanks for your comments, sorry for making you repeat yourself from last \ntime\n\nPhillip\n\n> [1]: https://lore.kernel.org/git/CAPig+cT-9sjmkdWFEcFS=rg9ziV9b6uWNMpQ8BTYP-a258La6Q@mail.gmail.com/\n\n"},{"id":"449773","messageId":"e90340b0-09da-bf3a-f8eb-47fa408c51f1@gmail.com","threadId":"57475","inReplyTo":"xmqqh78mesj6.fsf@gitster.g","subject":"Re: [PATCH] worktree: add -z option for list subcommand","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-02-28T16:00:21Z","receivedAt":"2022-02-28T16:00:27Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 25/02/2022 17:59, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> [...]\n>>   \treason = worktree_lock_reason(wt);\n>>   \tif (reason && *reason) {\n>>   \t\tstruct strbuf sb = STRBUF_INIT;\n>> -\t\tquote_c_style(reason, &sb, NULL, 0);\n>> -\t\tprintf(\"locked %s\\n\", sb.buf);\n>> +\t\tif (line_terminator) {\n>> +\t\t\tquote_c_style(reason, &sb, NULL, 0);\n>> +\t\t\treason = sb.buf;\n>> +\t\t}\n>> +\t\tprintf(\"locked %s%c\", reason, line_terminator);\n> \n> OK.  I suspect write_name_quoted() may be a better fit that does not\n> require us to have our own strbuf, but this should be OK.\n> \n>>   \t\tstrbuf_release(&sb);\n>>   \t} else if (reason)\n>> -\t\tprintf(\"locked\\n\");\n>> +\t\tprintf(\"locked%c\", line_terminator);\n> \n> It is a shame that we need a special code path for an empty string\n> given as the reason, only for the single SP after \"locked\", but we\n> have to live with it, I guess.\n\nWe could have\n\n\tif (reason) {\n\t\tfputs(\"locked\", stdout);\n\t\tif (*reason) {\n\t\t\tfputc(\" \", stdout)\n\t\t\twrite_name_quoted(reason, stdout, line_terminator);\n\t\t} else {\n\t\t\tfputc(line_terminator, stdout)\n\t\t\n\t}\n\nwhich shares the code to print \"locked\" but I'm not sure it is any \nbettor overall though especially as write_name_quoted() means we only \nwant to output a terminator when there is no reason text.\n\n\nBest Wishes\n\nPhillip\n"},{"id":"449775","messageId":"bb17e9ee-d92f-40df-3069-bd86fc79c3cf@gmail.com","threadId":"57475","inReplyTo":"CAPig+cQF3y64bPN8h4XvAYGX-g0LZedh-cAM+JLKdW_DPAWJaw@mail.gmail.com","subject":"Re: [PATCH] worktree: add -z option for list subcommand","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-02-28T16:10:06Z","receivedAt":"2022-02-28T16:10:13Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 28/02/2022 08:35, Eric Sunshine wrote:\n> On Fri, Feb 25, 2022 at 12:59 PM Junio C Hamano <gitster@pobox.com> wrote:\n>> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>>> +-z::\n>>> +     When `--porcelain` is specified with `list` terminate each line with a\n>>> +     NUL rather than a newline. This makes it possible to parse the output\n>>> +     when a worktree path contains a newline character.\n>>\n>> This makes it sound as if it were impossible.  As the changes this\n>> patch makes to the documentation and the code indicate, we were\n>> already doing quote_c_style(), so it is not quite correct to say so.\n>>\n>> Perhaps \"makes it easier\" is more accurate?\n>>\n>> Unless the original wasn't using quote_c_style() correctly and\n>> wasn't quoting its output, that is?\n> \n> That's the case. It is impossible without this patch since `git\n> worktree list --porcelain` does not call quote_c_style() for the\n> worktree path; it only calls quote_c_style() for the lock reason.\n> Fixing this oversight for the worktree path for non-`-z` mode has\n> potential backward-compatibility concerns. I had argued[1] that we\n> might be able to sell it as a pure bug fix, but Phillip was\n> concerned[2] that it might break existing tooling. (I also share that\n> concern, but to a lesser degree, perhaps, since worktrees with oddball\n> names containing newline or a leading double-quote must be exceedingly\n> rare.)\n\nIf the user has not set core.quotePath to false then any unicode paths \nwill be quoted unless we only quote paths with newlines and leading \ndouble quotes which would mean the quoting would not match all the other \ngit commands. In general I think it is easier to use nul termination \nrather than having to unquote path names. In this case it is not obvious \nto me what command one would feed a quoted directory name into either \n(it's not like doing \"git diff | sed ... |git update-index --stdin\" or \nsomething like that).\n\nBest Wishes\n\nPhillip\n\n> [1]: https://lore.kernel.org/git/CAPig+cQq_RnanDQ3jHfNz_L58WyzmsUJBhtdrLxa=H0v_io+WA@mail.gmail.com/\n> [2]: https://lore.kernel.org/git/936f9b7c-6d54-00bc-f136-4cb4c2836eb6@gmail.com/\n> \n>>> +             if (line_terminator) {\n>>> +                     quote_c_style(reason, &sb, NULL, 0);\n>>> +                     reason = sb.buf;\n>>> +             }\n>>> +             printf(\"locked %s%c\", reason, line_terminator);\n>>\n>> OK.  I suspect write_name_quoted() may be a better fit that does not\n>> require us to have our own strbuf, but this should be OK.\n> \n> Nice suggestion.\n\n"},{"id":"449786","messageId":"xmqqwnhe3o96.fsf@gitster.g","threadId":"57475","inReplyTo":"CAPig+cQF3y64bPN8h4XvAYGX-g0LZedh-cAM+JLKdW_DPAWJaw@mail.gmail.com","subject":"Re: [PATCH] worktree: add -z option for list subcommand","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-28T17:16:37Z","receivedAt":"2022-02-28T17:16:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n>> Unless the original wasn't using quote_c_style() correctly and\n>> wasn't quoting its output, that is?\n>\n> That's the case. It is impossible without this patch since `git\n> worktree list --porcelain` does not call quote_c_style() for the\n> worktree path; it only calls quote_c_style() for the lock reason.\n\nYuck.  That's an outright bug that we should fix.  I do not think it\nmakes \"-z\" unnecessary, but those who want to read from non-z output\ncannot sensibly do so with funny letters in their paths with today's\noutput, and as long as quote_c_style() tweaks the output only when\nfunny letters need to be quoted, those who are reading from today's\nnon-z output will not be hurt, so let's fix that first or at the\nsame time as we add \"-z\".\n\nThanks.\n"},{"id":"452803","messageId":"pull.1164.v2.git.1648743688825.gitgitgadget@gmail.com","threadId":"57475","inReplyTo":"pull.1164.git.1645801727732.gitgitgadget@gmail.com","subject":"[PATCH v2] worktree: add -z option for list subcommand","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-31T16:21:28Z","receivedAt":"2022-03-31T16:21:35Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nAdd a -z option to be used in conjunction with --porcelain that gives\nNUL-terminated output. As 'worktree list --porcelain' does not quote\nworktree paths this enables it to handle worktree paths that contain\nnewlines.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n    worktree: add -z option for list subcommand\n    \n    Thanks to Eric, Jean-Noël and Junio for their comments on V1. I've\n    reworded the docs and option help and tweaked the tests as suggested by\n    Eric, fixed the error messages as suggested by Eric/Jean-Noël and\n    changed the implementation to use write_name_quoted() as suggested by\n    Junio. I've punted doing anything about quoting the output without -z\n    for now, I'll fix that with and without --porcelain in another series.\n    \n    V1 Cover Letter: Add a -z option to be used in conjunction with\n    --porcelain that gives NUL-terminated output. This enables 'worktree\n    list --porcelain' to handle worktree paths that contain newlines.\n    \n    For a previous discussion of the merits of adding a -z option vs quoting\n    the worktree path see\n    https://lore.kernel.org/git/CAPig+cT-9sjmkdWFEcFS=rg9ziV9b6uWNMpQ8BTYP-a258La6Q@mail.gmail.com/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1164%2Fphillipwood%2Fwip%2Fworktree-list-nul-termination-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1164/phillipwood/wip/worktree-list-nul-termination-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1164\n\nRange-diff vs v1:\n\n 1:  b954579189b ! 1:  5f0e0213583 worktree: add -z option for list subcommand\n     @@ Commit message\n          worktree: add -z option for list subcommand\n      \n          Add a -z option to be used in conjunction with --porcelain that gives\n     -    NUL-terminated output. This enables 'worktree list --porcelain' to\n     -    handle worktree paths that contain newlines.\n     +    NUL-terminated output. As 'worktree list --porcelain' does not quote\n     +    worktree paths this enables it to handle worktree paths that contain\n     +    newlines.\n      \n          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n      \n     @@ Documentation/git-worktree.txt: This can also be set up as the default behaviour\n      +\tSee below for details.\n      +\n      +-z::\n     -+\tWhen `--porcelain` is specified with `list` terminate each line with a\n     -+\tNUL rather than a newline. This makes it possible to parse the output\n     -+\twhen a worktree path contains a newline character.\n     ++\tTerminate each line with a NUL rather than a newline when\n     ++\t`--porcelain` is specified with `list`. This makes it possible\n     ++\tto parse the output when a worktree path contains a newline\n     ++\tcharacter.\n       \n       -q::\n       --quiet::\n     @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix)\n       \t}\n       \n       \treason = worktree_lock_reason(wt);\n     - \tif (reason && *reason) {\n     - \t\tstruct strbuf sb = STRBUF_INIT;\n     +-\tif (reason && *reason) {\n     +-\t\tstruct strbuf sb = STRBUF_INIT;\n      -\t\tquote_c_style(reason, &sb, NULL, 0);\n      -\t\tprintf(\"locked %s\\n\", sb.buf);\n     -+\t\tif (line_terminator) {\n     -+\t\t\tquote_c_style(reason, &sb, NULL, 0);\n     -+\t\t\treason = sb.buf;\n     -+\t\t}\n     -+\t\tprintf(\"locked %s%c\", reason, line_terminator);\n     - \t\tstrbuf_release(&sb);\n     - \t} else if (reason)\n     +-\t\tstrbuf_release(&sb);\n     +-\t} else if (reason)\n      -\t\tprintf(\"locked\\n\");\n     -+\t\tprintf(\"locked%c\", line_terminator);\n     ++\tif (reason) {\n     ++\t\tfputs(\"locked\", stdout);\n     ++\t\tif (*reason) {\n     ++\t\t\tfputc(' ', stdout);\n     ++\t\t\twrite_name_quoted(reason, stdout, line_terminator);\n     ++\t\t} else {\n     ++\t\t\tfputc(line_terminator, stdout);\n     ++\t\t}\n     ++\t}\n       \n       \treason = worktree_prune_reason(wt, expire);\n       \tif (reason)\n     @@ builtin/worktree.c: static void pathsort(struct worktree **wt)\n       \t\tOPT_EXPIRY_DATE(0, \"expire\", &expire,\n       \t\t\t\tN_(\"add 'prunable' annotation to worktrees older than <time>\")),\n      +\t\tOPT_SET_INT('z', NULL, &line_terminator,\n     -+\t\t\t    N_(\"fields are separated with NUL character\"), '\\0'),\n     ++\t\t\t    N_(\"terminate records with a NUL character\"), '\\0'),\n       \t\tOPT_END()\n       \t};\n       \n     @@ builtin/worktree.c: static int list(int ac, const char **av, const char *prefix)\n       \telse if (verbose && porcelain)\n       \t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--verbose\", \"--porcelain\");\n      +\telse if (!line_terminator && !porcelain)\n     -+\t\tdie(_(\"'-z' requires '--porcelain'\"));\n     ++\t\tdie(_(\"the option '%s' requires '%s'\"), \"-z\", \"--porcelain\");\n       \telse {\n       \t\tstruct worktree **worktrees = get_worktrees();\n       \t\tint path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;\n     @@ t/t2402-worktree-list.sh: test_expect_success '\"list\" all worktrees --porcelain'\n      +\t\t\"$(git -C here rev-parse --show-toplevel)\" \\\n      +\t\t\"$(git rev-parse HEAD)\" >>expect &&\n      +\tgit worktree list --porcelain -z >_actual &&\n     -+\tcat _actual | tr \"\\0\" Q >actual\t&&\n     ++\tnul_to_q <_actual >actual &&\n      +\ttest_cmp expect actual\n      +'\n      +\n      +test_expect_success '\"list\" -z fails without --porcelain' '\n     -+\ttest_when_finished \"rm -rf here && git worktree prune\" &&\n     -+\tgit worktree add --detach here main &&\n      +\ttest_must_fail git worktree list -z\n      +'\n      +\n\n\n Documentation/git-worktree.txt | 16 ++++++++++----\n builtin/worktree.c             | 40 ++++++++++++++++++++--------------\n t/t2402-worktree-list.sh       | 19 ++++++++++++++++\n 3 files changed, 55 insertions(+), 20 deletions(-)\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex 9e862fbcf79..638e188c409 100644\n--- a/Documentation/git-worktree.txt\n+++ b/Documentation/git-worktree.txt\n@@ -10,7 +10,7 @@ SYNOPSIS\n --------\n [verse]\n 'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] [-b <new-branch>] <path> [<commit-ish>]\n-'git worktree list' [-v | --porcelain]\n+'git worktree list' [-v | --porcelain [-z]]\n 'git worktree lock' [--reason <string>] <worktree>\n 'git worktree move' <worktree> <new-path>\n 'git worktree prune' [-n] [-v] [--expire <expire>]\n@@ -223,7 +223,14 @@ This can also be set up as the default behaviour by using the\n --porcelain::\n \tWith `list`, output in an easy-to-parse format for scripts.\n \tThis format will remain stable across Git versions and regardless of user\n-\tconfiguration.  See below for details.\n+\tconfiguration.  It is recommended to combine this with `-z`.\n+\tSee below for details.\n+\n+-z::\n+\tTerminate each line with a NUL rather than a newline when\n+\t`--porcelain` is specified with `list`. This makes it possible\n+\tto parse the output when a worktree path contains a newline\n+\tcharacter.\n \n -q::\n --quiet::\n@@ -411,7 +418,8 @@ working tree itself.\n \n Porcelain Format\n ~~~~~~~~~~~~~~~~\n-The porcelain format has a line per attribute.  Attributes are listed with a\n+The porcelain format has a line per attribute.  If `-z` is given then the lines\n+are terminated with NUL rather than a newline.  Attributes are listed with a\n label and value separated by a single space.  Boolean attributes (like `bare`\n and `detached`) are listed as a label only, and are present only\n if the value is true.  Some attributes (like `locked`) can be listed as a label\n@@ -449,7 +457,7 @@ prunable gitdir file points to non-existent location\n \n ------------\n \n-If the lock reason contains \"unusual\" characters such as newline, they\n+Unless `-z` is used any \"unusual\" characters in the lock reason such as newlines\n are escaped and the entire reason is quoted as explained for the\n configuration variable `core.quotePath` (see linkgit:git-config[1]).\n For Example:\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 0d0809276fe..6fef936d5ac 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -575,35 +575,37 @@ static int add(int ac, const char **av, const char *prefix)\n \treturn add_worktree(path, branch, &opts);\n }\n \n-static void show_worktree_porcelain(struct worktree *wt)\n+static void show_worktree_porcelain(struct worktree *wt, int line_terminator)\n {\n \tconst char *reason;\n \n-\tprintf(\"worktree %s\\n\", wt->path);\n+\tprintf(\"worktree %s%c\", wt->path, line_terminator);\n \tif (wt->is_bare)\n-\t\tprintf(\"bare\\n\");\n+\t\tprintf(\"bare%c\", line_terminator);\n \telse {\n-\t\tprintf(\"HEAD %s\\n\", oid_to_hex(&wt->head_oid));\n+\t\tprintf(\"HEAD %s%c\", oid_to_hex(&wt->head_oid), line_terminator);\n \t\tif (wt->is_detached)\n-\t\t\tprintf(\"detached\\n\");\n+\t\t\tprintf(\"detached%c\", line_terminator);\n \t\telse if (wt->head_ref)\n-\t\t\tprintf(\"branch %s\\n\", wt->head_ref);\n+\t\t\tprintf(\"branch %s%c\", wt->head_ref, line_terminator);\n \t}\n \n \treason = worktree_lock_reason(wt);\n-\tif (reason && *reason) {\n-\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\tquote_c_style(reason, &sb, NULL, 0);\n-\t\tprintf(\"locked %s\\n\", sb.buf);\n-\t\tstrbuf_release(&sb);\n-\t} else if (reason)\n-\t\tprintf(\"locked\\n\");\n+\tif (reason) {\n+\t\tfputs(\"locked\", stdout);\n+\t\tif (*reason) {\n+\t\t\tfputc(' ', stdout);\n+\t\t\twrite_name_quoted(reason, stdout, line_terminator);\n+\t\t} else {\n+\t\t\tfputc(line_terminator, stdout);\n+\t\t}\n+\t}\n \n \treason = worktree_prune_reason(wt, expire);\n \tif (reason)\n-\t\tprintf(\"prunable %s\\n\", reason);\n+\t\tprintf(\"prunable %s%c\", reason, line_terminator);\n \n-\tprintf(\"\\n\");\n+\tfputc(line_terminator, stdout);\n }\n \n static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)\n@@ -681,12 +683,15 @@ static void pathsort(struct worktree **wt)\n static int list(int ac, const char **av, const char *prefix)\n {\n \tint porcelain = 0;\n+\tint line_terminator = '\\n';\n \n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"porcelain\", &porcelain, N_(\"machine-readable output\")),\n \t\tOPT__VERBOSE(&verbose, N_(\"show extended annotations and reasons, if available\")),\n \t\tOPT_EXPIRY_DATE(0, \"expire\", &expire,\n \t\t\t\tN_(\"add 'prunable' annotation to worktrees older than <time>\")),\n+\t\tOPT_SET_INT('z', NULL, &line_terminator,\n+\t\t\t    N_(\"terminate records with a NUL character\"), '\\0'),\n \t\tOPT_END()\n \t};\n \n@@ -696,6 +701,8 @@ static int list(int ac, const char **av, const char *prefix)\n \t\tusage_with_options(worktree_usage, options);\n \telse if (verbose && porcelain)\n \t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--verbose\", \"--porcelain\");\n+\telse if (!line_terminator && !porcelain)\n+\t\tdie(_(\"the option '%s' requires '%s'\"), \"-z\", \"--porcelain\");\n \telse {\n \t\tstruct worktree **worktrees = get_worktrees();\n \t\tint path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;\n@@ -708,7 +715,8 @@ static int list(int ac, const char **av, const char *prefix)\n \n \t\tfor (i = 0; worktrees[i]; i++) {\n \t\t\tif (porcelain)\n-\t\t\t\tshow_worktree_porcelain(worktrees[i]);\n+\t\t\t\tshow_worktree_porcelain(worktrees[i],\n+\t\t\t\t\t\t\tline_terminator);\n \t\t\telse\n \t\t\t\tshow_worktree(worktrees[i], path_maxlen, abbrev);\n \t\t}\ndiff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh\nindex c8a5a0aac6d..79e0fce2d90 100755\n--- a/t/t2402-worktree-list.sh\n+++ b/t/t2402-worktree-list.sh\n@@ -64,6 +64,25 @@ test_expect_success '\"list\" all worktrees --porcelain' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '\"list\" all worktrees --porcelain -z' '\n+\ttest_when_finished \"rm -rf here _actual actual expect &&\n+\t\t\t\tgit worktree prune\" &&\n+\tprintf \"worktree %sQHEAD %sQbranch %sQQ\" \\\n+\t\t\"$(git rev-parse --show-toplevel)\" \\\n+\t\t$(git rev-parse HEAD --symbolic-full-name HEAD) >expect &&\n+\tgit worktree add --detach here main &&\n+\tprintf \"worktree %sQHEAD %sQdetachedQQ\" \\\n+\t\t\"$(git -C here rev-parse --show-toplevel)\" \\\n+\t\t\"$(git rev-parse HEAD)\" >>expect &&\n+\tgit worktree list --porcelain -z >_actual &&\n+\tnul_to_q <_actual >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '\"list\" -z fails without --porcelain' '\n+\ttest_must_fail git worktree list -z\n+'\n+\n test_expect_success '\"list\" all worktrees with locked annotation' '\n \ttest_when_finished \"rm -rf locked unlocked out && git worktree prune\" &&\n \tgit worktree add --detach locked main &&\n\nbase-commit: dab1b7905d0b295f1acef9785bb2b9cbb0fdec84\n-- \ngitgitgadget\n"},{"id":"452834","messageId":"xmqqk0c926zf.fsf@gitster.g","threadId":"57475","inReplyTo":"pull.1164.v2.git.1648743688825.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] worktree: add -z option for list subcommand","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-31T20:37:56Z","receivedAt":"2022-03-31T20:38:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> Add a -z option to be used in conjunction with --porcelain that gives\n> NUL-terminated output. As 'worktree list --porcelain' does not quote\n> worktree paths this enables it to handle worktree paths that contain\n> newlines.\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>     worktree: add -z option for list subcommand\n>     \n>     Thanks to Eric, Jean-Noël and Junio for their comments on V1. I've\n>     reworded the docs and option help and tweaked the tests as suggested by\n>     Eric, fixed the error messages as suggested by Eric/Jean-Noël and\n>     changed the implementation to use write_name_quoted() as suggested by\n>     Junio. I've punted doing anything about quoting the output without -z\n>     for now, I'll fix that with and without --porcelain in another series.\n\nThanks for an update.  Will queue.  I think this iteration is good\nto merge to 'next', but let's wait for a few days to see what others\nthink.\n\nIt also made me wonder if \"-z\" alone should be made to imply\n\"--porcelain\" (in other words, is there a good reason to ask for\nNUL-terminated output when you are producing a human-readable\noutput?), but we can start stricter like this patch does; we can\nlater loosen it if needed.\n\n\n"},{"id":"453039","messageId":"d0622cca-245a-ac98-cc96-201a9c28c3cb@gmail.com","threadId":"57475","inReplyTo":"xmqqk0c926zf.fsf@gitster.g","subject":"Re: [PATCH v2] worktree: add -z option for list subcommand","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-04-04T15:47:17Z","receivedAt":"2022-04-04T15:47:25Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 31/03/2022 21:37, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n >\n> It also made me wonder if \"-z\" alone should be made to imply\n> \"--porcelain\" (in other words, is there a good reason to ask for\n> NUL-terminated output when you are producing a human-readable\n> output?), but we can start stricter like this patch does; we can\n> later loosen it if needed.\n\nThat's a good point about \"-z\" implying \"--porcelain\" we do something \nsimilar for \"git status -z\" I think.\n\nBest Wishes\n\nPhillip\n\n\n"},{"id":"477019","messageId":"CAPig+cSmwQGSmm=drVodNzL0OmfG0EFawev=PjzJ4TbKgJhHsg@mail.gmail.com","threadId":"57475","inReplyTo":"xmqqk0c926zf.fsf@gitster.g","subject":"Re: [PATCH v2] worktree: add -z option for list subcommand","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-05-11T04:11:58Z","receivedAt":"2023-05-11T04:12:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 31, 2022 at 4:37 PM Junio C Hamano <gitster@pobox.com> wrote:\n> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> > Add a -z option to be used in conjunction with --porcelain that gives\n> > NUL-terminated output. As 'worktree list --porcelain' does not quote\n> > worktree paths this enables it to handle worktree paths that contain\n> > newlines.\n> >\n> > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> > ---\n> >     worktree: add -z option for list subcommand\n> >\n> >     Thanks to Eric, Jean-Noël and Junio for their comments on V1. I've\n> >     reworded the docs and option help and tweaked the tests as suggested by\n> >     Eric, fixed the error messages as suggested by Eric/Jean-Noël and\n> >     changed the implementation to use write_name_quoted() as suggested by\n> >     Junio. I've punted doing anything about quoting the output without -z\n> >     for now, I'll fix that with and without --porcelain in another series.\n>\n> Thanks for an update.  Will queue.  I think this iteration is good\n> to merge to 'next', but let's wait for a few days to see what others\n> think.\n\nAgreed. I think this version addresses my review comments on earlier\nrounds and seems to be in good shape.\n\n(Sorry for the late response.)\n"}]}