From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 21 Nov 2025 16:20:39 GMT
Subject: Re: [PATCH 2/2] worktree list: quote paths
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:
> 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.

> [*]: 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