Re: [PATCH v3 5/7] builtin/maintenance: introduce "worktree-prune" task

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 5, 2025, 08:59 UTC
Message-ID: <CAPig+cScor=E2i4w99NCrsaUd-Po=FqhkrLSyT69PSo1+h2dRw@mail.gmail.com>
In-Reply-To: <20250502-pks-maintenance-missing-tasks-v3-5-13e130d36640@pks.im>

On Fri, May 2, 2025 at 4:44 AM Patrick Steinhardt <ps@pks.im> wrote:

Show 25 quoted lines

> While git-gc(1) knows to prune stale worktrees, git-maintenance(1) does
> not yet have a task for this cleanup. Introduce a new "worktree-prune"
> task to plug this gap.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/builtin/gc.c b/builtin/gc.c
> @@ -346,6 +347,45 @@ static int maintenance_task_worktree_prune(struct maintenance_run_opts *opts UNU
> +static int worktree_prune_condition(struct gc_config *cfg)
> +{
> + [...]
> +       git_config_get_int("maintenance.worktree-prune.auto", &limit);
> +       if (limit <= 0) {
> +               should_prune = limit < 0;
> +               goto out;
> +       }
> +
> +       if (parse_expiry_date(cfg->prune_worktrees_expire, &expiry_date) ||
> +           get_worktree_names(the_repository, &worktrees) < 0)
> +               goto out;
> +
> +       for (size_t i = 0; i < worktrees.nr; i++) {
> +               char *wtpath;
> +               strbuf_reset(&reason);
> +               if (should_prune_worktree(worktrees.v[i], &reason, &wtpath, expiry_date)) {

As I was reading the commit message, I had concerns that if this implementation was rolling its own pruning logic, it might overlook conditions such as a worktree being locked, so I'm glad to see that you're using should_prune_worktree() here rather than reinventing the wheel.

Show 8 quoted lines

> +                       limit--;
> +
> +                       if (!limit) {
> +                               should_prune = 1;
> +                               goto out;
> +                       }
> +               }
> +               free(wtpath);

This leaks `wtpath` when it takes the `goto out` arm.

Show 21 quoted lines

> +       }
> +
> +out:
> +       strvec_clear(&worktrees);
> +       strbuf_release(&reason);
> +       return should_prune;
> +}
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> @@ -493,6 +493,77 @@ test_expect_success 'reflog-expire task --auto only packs when exceeding limits'
> +test_expect_success 'worktree-prune task with --auto honors maintenance.worktree-prune.auto' '
> +       # A negative value should always prune.
> +       test_expect_worktree_prune git -c maintenance.worktree-prune.auto=-1 maintenance run --auto --task=worktree-prune &&
> +
> +       mkdir .git/worktrees &&
> +       : >.git/worktrees/first &&
> +       : >.git/worktrees/second &&
> +       : >.git/worktrees/third &&
> +
> +       # Zero should never prune.
> +       test_expect_worktree_prune ! git -c maintenance.worktree-prune.auto=0 maintenance run --auto --task=worktree-prune &&
> +       # A positive value should require at least this man prunable worktrees.

s/man/many/