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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Feb 28, 2022, 16:10 UTC
Message-ID: <bb17e9ee-d92f-40df-3069-bd86fc79c3cf@gmail.com>
In-Reply-To: <CAPig+cQF3y64bPN8h4XvAYGX-g0LZedh-cAM+JLKdW_DPAWJaw@mail.gmail.com>

On 28/02/2022 08:35, Eric Sunshine wrote:

Show 26 quoted lines

> 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

Show 13 quoted lines

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