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

[Patrick Steinhardt <ps@pks.im>]

On Tue, Apr 29, 2025 at 04:02:03PM -0400, Derrick Stolee wrote:
> 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.

> 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