Re: [PATCH 2/2] worktree list: quote paths
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Nov 21, 2025, 16:20 UTC
- Message-ID
- <7583e2aa-ccd4-4316-b5ff-bcba0fc84898@gmail.com>
- In-Reply-To
- <CAPig+cSptp+a7jnUp3Tg=7D8WYKFNz4xWU2eaH+X5uy2mWjvgg@mail.gmail.com>
Hi Eric
On 19/11/2025 07:09, Eric Sunshine wrote:
Show 12 quoted lines
> On Tue, Nov 18, 2025 at 11:07 AM Phillip Wood <phillip.wood123@gmail.com> wrote: >> If a worktree path contains newlines or other control characters >> it messes up the output of "git worktree list". Fix this by using >> quote_path() to display the worktree path. The output of "git worktree >> list" is designed for human consumption, scripts should be using the >> "--porcelain" option so this change should not break them. > > I believe that it would be more accurate to say "--porcelain -z" since > that is the safe combination. Without -z, the output of --porcelain > will be gobbledygook if names contain newlines or other control > characters, but that's a long-standing problem[*] outside the scope of > this series. Anyhow, probably not worth a reroll.
I agree that scripts should be using "-z" as well but I was just trying to make the point that the changes here wont affect sensibly written scripts.
Show 42 quoted lines
> [*]: There has been talk about correcting the oversight that
> --porcelain alone (without -z) fails to call quote_path(), but such a
> fix never materialized due to backward-compatibility concerns. We
> would probably need to introduce --porcelain=v2 to finally fix the
> case when -z isn't used with --porcelain.
>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> @@ -1028,11 +1029,14 @@ static void measure_widths(struct worktree **wt, int *abbrev,
>> struct worktree_display *display = NULL;
>> + struct strbuf buf = STRBUF_INIT;
>>
>> for (i = 0; wt[i]; i++) {
>> int sha1_len;
>> ALLOC_GROW(display, i + 1, display_alloc);
>> - display[i].width = utf8_strwidth(wt[i]->path);
>> + quote_path(wt[i]->path, NULL, &buf, 0);
>> + display[i].width = utf8_strwidth(buf.buf);
>> + display[i].path = strbuf_detach(&buf, NULL);
>
> The strbuf is unconditionally detached on each iteration.
>
>> if (display[i].width > *maxwidth)
>> *maxwidth = display[i].width;
>> @@ -1104,6 +1108,8 @@ static int list(int ac, const char **av, const char *prefix,
>> show_worktree(worktrees[i],
>> &display[i], path_maxwidth, abbrev);
>> }
>> + for (i = 0; display && worktrees[i]; i++)
>> + free(display[i].path);
>
> And the detached buffers are correctly freed.
>
>> free(display);
>> free_worktrees(worktrees);
>
> Although not technically required because the strbuf is
> unconditionally detached each time through the loop, I wonder if it
> would reduce the cognitive load slightly for future readers to also
> strbuf_release(&buf) here at the end of the function. Probably not
> worth a reroll, though.I think the counterargument is that it adds cognitive load for anyone who wonders why we're calling strbuf_release() after strbuf_detach() so I'm inclined to leave it as is.
Thanks for the thorough review
Phillip