From: Eric Sunshine <sunshine@sunshineco.com>
Date: Wed, 19 Nov 2025 06:55:11 GMT
Subject: Re: [PATCH 1/2] worktree list: fix column spacing
Message-ID: <CAPig+cQaOx7yptQT=eDfVcsv_NbRseR+5Dvpm4E95z2HMpEKag@mail.gmail.com>
In-Reply-To: <9417c73b3c4b89ed7c4cb823f3f68e994a968021.1763482051.git.phillip.wood@dunelm.org.uk>

On Tue, Nov 18, 2025 at 11:07 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> The output of "git worktree list" displays a table containing the
> worktree path, HEAD OID and branch name for each worktree. The code
> aligns the columns by measuring the visual width of the worktree path
> when it is printed. Unfortunately it fails to use the visual width
> when calculating the width of the column so, if any of the paths
> contain a multibyte character, we can end up with excess padding
> between columns. The simplest fix would be to replace strlen() with
> utf8_strwidth() in measure_widths(). However that leaves us measuring
> the visual width twice and the byte length once. By caching the visual
> width and printing the padding separately to the worktree path, we only
> need to calculate the visual width once and do not need the byte length
> at all. The visual widths are stored in an arrays of structs rather
> than an array of ints as the next commit will add more struct members.
> [...]
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -1020,20 +1023,24 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
> +static void measure_widths(struct worktree **wt, int *abbrev,
> +                          struct worktree_display **d, int *maxwidth)
>  {
> -       int i;
> +       int i, display_alloc = 0;
> +       struct worktree_display *display = NULL;
>
>         for (i = 0; wt[i]; i++) {
>                 int sha1_len;
> -               int path_len = strlen(wt[i]->path);
> +               ALLOC_GROW(display, i + 1, display_alloc);
> +               display[i].width = utf8_strwidth(wt[i]->path);
>
> -               if (path_len > *maxlen)
> -                       *maxlen = path_len;
> +               if (display[i].width > *maxwidth)
> +                       *maxwidth = display[i].width;
>                 sha1_len = strlen(repo_find_unique_abbrev(the_repository, &wt[i]->head_oid, *abbrev));
>                 if (sha1_len > *abbrev)
>                         *abbrev = sha1_len;
>         }
> +       *d = display;
>  }

The reason you're using ALLOC_GROW() rather than simply allocating the
entire `display` array at the start is that `wt` is a NULL-terminated
array, thus you don't know its length ahead of time. Makes sense.

> @@ -1079,21 +1086,25 @@ static int list(int ac, const char **av, const char *prefix,
> +               struct worktree_display *display = NULL;
>                 if (!porcelain)
> +                       measure_widths(worktrees, &abbrev,
> +                                      &display, &path_maxwidth);
> [...]
> +               free(display);
>                 free_worktrees(worktrees);

`display` is correctly freed. Good.