patchworktree: add -z option for list subcommand

> @@ -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

>  	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);

>  	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);
>  }

> +	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);
>  		}

> "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?

> > +             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.

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

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

> [...]
>>   	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.

> 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/

> 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);
>         }

>         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);

> @@ -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'"));

> 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 &&

> +       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
> +'

> 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
>        '

> 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;

> 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
> 

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Add a -z option to be used in conjunction with --porcelain that gives
> NUL-terminated output. As 'worktree list --porcelain' does not quote
> worktree paths this enables it to handle worktree paths that contain
> newlines.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>     worktree: add -z option for list subcommand
>     
>     Thanks to Eric, Jean-Noël and Junio for their comments on V1. I've
>     reworded the docs and option help and tweaked the tests as suggested by
>     Eric, fixed the error messages as suggested by Eric/Jean-Noël and
>     changed the implementation to use write_name_quoted() as suggested by
>     Junio. I've punted doing anything about quoting the output without -z
>     for now, I'll fix that with and without --porcelain in another series.

> It also made me wonder if "-z" alone should be made to imply
> "--porcelain" (in other words, is there a good reason to ask for
> NUL-terminated output when you are producing a human-readable
> output?), but we can start stricter like this patch does; we can
> later loosen it if needed.

> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > Add a -z option to be used in conjunction with --porcelain that gives
> > NUL-terminated output. As 'worktree list --porcelain' does not quote
> > worktree paths this enables it to handle worktree paths that contain
> > newlines.
> >
> > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> > ---
> >     worktree: add -z option for list subcommand
> >
> >     Thanks to Eric, Jean-Noël and Junio for their comments on V1. I've
> >     reworded the docs and option help and tweaked the tests as suggested by
> >     Eric, fixed the error messages as suggested by Eric/Jean-Noël and
> >     changed the implementation to use write_name_quoted() as suggested by
> >     Junio. I've punted doing anything about quoting the output without -z
> >     for now, I'll fix that with and without --porcelain in another series.
>
> Thanks for an update.  Will queue.  I think this iteration is good
> to merge to 'next', but let's wait for a few days to see what others
> think.