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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 28, 2022, 08:35 UTC
Message-ID: <CAPig+cQF3y64bPN8h4XvAYGX-g0LZedh-cAM+JLKdW_DPAWJaw@mail.gmail.com>
In-Reply-To: <xmqqh78mesj6.fsf@gitster.g>

On Fri, Feb 25, 2022 at 12:59 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 14 quoted lines

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

Show 8 quoted lines

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