Re: [PATCH] worktree: add -z option for list subcommand
From: Eric Sunshine <hidden>
Date: 2022-02-28 08:17:01
On Fri, Feb 25, 2022 at 10:08 AM Phillip Wood via GitGitGadget [off-list ref] 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 <redacted>
---
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/ (local)Thanks for resubmitting.
quoted hunk ↗ jump to hunk
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(?).
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
@@ -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`.
quoted hunk ↗ jump to hunk
@@ -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 hunk ↗ jump to hunk
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/ (local)