Re: [PATCH 5/7] builtin/maintenance: introduce "worktree-prune" task
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Apr 30, 2025, 07:08 UTC
- Message-ID
- <aBHMXGNfDZQ-JVT1@pks.im>
- In-Reply-To
- <c629f845-180f-4ecb-949f-a99f184f812d@gmail.com>
On Tue, Apr 29, 2025 at 04:02:03PM -0400, Derrick Stolee wrote:
Show 48 quoted lines
> On 4/25/2025 3:29 AM, Patrick Steinhardt wrote:
> > 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.
>
> I initially thought that this could merge down into patch 3 (move pruning
> of worktrees into a separate function), but...
>
> > +static int worktree_prune_condition(struct gc_config *cfg)
> > +{
> > + struct strvec worktrees = STRVEC_INIT;
> > + struct strbuf reason = STRBUF_INIT;
> > + timestamp_t expiry_date;
> > + int should_prune = 0;
> > +
> > + 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)) {
> > + should_prune = 1;
> > + goto out;
> > + }
> > + free(wtpath);
> > + }
> > +
> > +out:
> > + strvec_clear(&worktrees);
> > + strbuf_release(&reason);
> > + return should_prune;
> > +}
> > +
>
> ...this implementation is new and nice to have in a separate patch. I
> initially wondered if this condition needed to exist in the maintenance
> builtin or could be relied upon by the 'git worktree prune' command that
> is called by this implementation.
>
> If we are trying to match the behavior of 'git gc --auto', then it was
> running 'git worktree prune --expire...' every time that the generic
> --auto condition was satisfied. But when 'git maintenance run --auto' is
> executed, each task is checked to see if it should run. If we can avoid a
> child process startup, then that is very valuable (especially on Windows
> where process creation is expensive).Yup, exactly. In theory, we could even make the condition configurable via "maintenance.worktree-prune.auto" so that we treat it as a limit of how many worktrees need to be prunable before we execute `git worktree prune`. Maybe I'll do just that in the next iteration.
Show 27 quoted lines
> So I think this is a good approach. Similar thoughts apply to patch 7. No > code change is needed. > > > +test_expect_success 'worktree-prune task' ' > > + GIT_TRACE2_EVENT="$(pwd)/worktree-prune.txt" \ > > + git maintenance run --task=worktree-prune && > > + test_subcommand git worktree prune --expire 3.months.ago <worktree-prune.txt > > +' > > + > > +test_expect_success 'worktree-prune task --auto only prunes with prunable worktree' ' > > + GIT_TRACE2_EVENT="$(pwd)/worktree-prune-auto.txt" \ > > + git maintenance run --auto --task=worktree-prune && > > + test_subcommand ! git worktree prune --expire 3.months.ago <worktree-prune-auto.txt && > > + mkdir .git/worktrees && > > + : >.git/worktrees/abc && > > + GIT_TRACE2_EVENT="$(pwd)/worktree-prune-auto.txt" \ > > + git maintenance run --auto --task=worktree-prune && > > + test_subcommand git worktree prune --expire 3.months.ago <worktree-prune-auto.txt > > +' > > + > > test_expect_success '--auto and --schedule incompatible' ' > > test_must_fail git maintenance run --auto --schedule=daily 2>err && > > test_grep "at most one" err > > It may be good to double-check that the gc.worktreePruneExpire config value > is being used here, especially since the prune condition is operating on > that value.
Fair, will do.
Patrick