On Fri, Feb 25, 2022 at 10:08 AM Phillip Wood via GitGitGadget <gitgitgadget@xxxxxxxxx> 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@xxxxxxxxxxxxx> > --- > 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@xxxxxxxxxxxxxx/ 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@xxxxxxxxxxxxxx/
