| Message ID | pull.1164.git.1645801727732.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | worktree: add -z option for list subcommand | expand |
"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); > }
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/
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.
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
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 >
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/
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
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.
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.
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 &&