Re: [PATCH] worktree: add -z option for list subcommand

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Jan 5, 2021 at 6:02 AM Phillip Wood <phillip.wood123@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.

Adding a -z mode makes a lot of sense. This, along with a fix to call
quote_c_style() on paths in normal (not `-z`) porcelain mode, can
easily be done after Rafael's series lands. Or they could be done
before his series, though that might make a lot of extra work for him.

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> I found an old patch that added NUL termination. I've rebased it
> but the new test fails as there seems to be another worktree thats
> been added since I wrote this, anyway I thought it might be a useful
> start for adding a `-z` option.

The test failure is fallout from a "bug" in a test added by Rafael's
earlier series[1] which was not caught by the reviewer[2]. In fact,
his current series has a drive-by fix[3] for this bug in patch [6/7].
Applying that fix (or the refinement[4] I suggested in my review)
makes your test work.

[1]: https://lore.kernel.org/git/20201011101152.5291-2-rafaeloliveira.cs@gmail.com/
[2]: https://lore.kernel.org/git/CAPig+cS8hvX4GCsnfLBnQ4Q_AkUad=bw7rjVcaOqSEqcLZvx8w@mail.gmail.com/
[3]: https://lore.kernel.org/git/20210104162128.95281-7-rafaeloliveira.cs@gmail.com/
[4]: https://lore.kernel.org/git/CAPig+cRysXpK0e1xXOuVd+EtkeyTk8FR6MUrL=Bg3W4ve8jdNA@mail.gmail.com/

> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -217,7 +217,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
> +       NUL rather than a newline. This makes it possible to parse the output
> +       when a worktree path contains a newline character.

If we fix normal-mode porcelain to call quote_c_style(), then we can
drop the last sentence or refine it to say something along the lines
of it being easier to deal with paths with embedded newlines than in
normal porcelain mode.

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -640,19 +640,25 @@ static int add(int ac, const char **av, const char *prefix)
> +static void show_worktree_porcelain(struct worktree *wt, int line_terminator)
>  {
> +       printf("worktree %s", wt->path);
> +       fputc(line_terminator, stdout);
> +       if (wt->is_bare) {
> +               printf("bare");
> +               fputc(line_terminator, stdout);
> +       } else {
> +               printf("HEAD %s", oid_to_hex(&wt->head_oid));
> +               fputc(line_terminator, stdout);
> +               if (wt->is_detached) {
> +                       printf("detached");
> +                       fputc(line_terminator, stdout);
> +               } else if (wt->head_ref) {
> +                       printf("branch %s", wt->head_ref);
> +                       fputc(line_terminator, stdout);
> +               }

Perhaps this could all be done a bit more concisely with printf()
alone rather than combining it with fputc(). For instance:

    printf("worktree %s%c", wt->path, line_terminator);

and so on.

> -       printf("\n");
> +       fputc(line_terminator, stdout);

This use of fputc() makes sense.

> @@ -720,15 +726,20 @@ static void pathsort(struct worktree **wt)
>  static int list(int ac, const char **av, const char *prefix)
>  {
> +               OPT_SET_INT('z', NULL, &line_terminator,
> +                           N_("fields are separated with NUL character"), '\0'),

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

> +       else if (!line_terminator && !porcelain)
> +               die(_("'-z' requires '--porcelain'"));

Other error messages in this file don't quote command-line options, so:

    die(_("-z requires --porcelain"));

would be more consistent.

> diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
> @@ -71,6 +71,28 @@ test_expect_success '"list" all worktrees with locked annotation' '
> +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)" \
> +               "$(git symbolic-ref HEAD)" >expect &&
> +       git worktree add --detach here master &&
> +       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 &&

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 master &&
> +       test_must_fail git worktree list -z
> +'

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
    '