# [PATCH 0/7] builtin/maintenance: implement missing tasks compared to git-gc(1) 37 messages from 2025-04-25 to 2025-05-07. Participants: Patrick Steinhardt, Derrick Stolee, Christian Couder, Eric Sunshine. Thread: https://gitlist.dev/t/63348 ## Patrick Steinhardt, 2025-04-25 07:29 Subject: [PATCH 0/7] builtin/maintenance: implement missing tasks compared to git-gc(1) Message-ID: <20250425-pks-maintenance-missing-tasks-v1-0-972ed6ab2c0d@pks.im> URL: https://gitlist.dev/e/20250425-pks-maintenance-missing-tasks-v1-0-972ed6ab2c0d%40pks.im ``` Hi, this small patch series implements the last couple of remaining tasks that are missing compared to the functionality git-gc(1) provides. Right now, git-maintenance(1) still executes git-gc(1). With these last gaps plugged though we can in theory fully replace git-gc(1) with finer grained tasks without losing any functionality. The benefit is that it becomes possible for users to have finer-grained control over what exactly the maintenance does. This patch series doesn't do that yet, but only implements whatever is needed to get there. Thanks! Patrick --- Patrick Steinhardt (7): builtin/gc: fix indentation of `cmd_gc()` parameters builtin/gc: remove global variables where it trivial to do builtin/gc: move pruning of worktrees into a separate function worktree: expose function to retrieve worktree names builtin/maintenance: introduce "worktree-prune" task builtin/gc: move rerere garbage collection into separate function builtin/maintenance: introduce "rerere-gc" task Documentation/git-maintenance.adoc | 8 +++ builtin/gc.c | 128 ++++++++++++++++++++++++++++--------- builtin/worktree.c | 25 ++++---- t/t7900-maintenance.sh | 34 ++++++++++ worktree.c | 30 +++++++++ worktree.h | 8 +++ 6 files changed, 189 insertions(+), 44 deletions(-) --- base-commit: a2955b34f48265d240ab8c7deb0a929ec2d65fd0 change-id: 20250424-pks-maintenance-missing-tasks-8ffcdd596b73 ``` ## Patrick Steinhardt, 2025-04-25 07:29 Subject: [PATCH 1/7] builtin/gc: fix indentation of `cmd_gc()` parameters Message-ID: <20250425-pks-maintenance-missing-tasks-v1-1-972ed6ab2c0d@pks.im> URL: https://gitlist.dev/e/20250425-pks-maintenance-missing-tasks-v1-1-972ed6ab2c0d%40pks.im In-Reply-To: <20250425-pks-maintenance-missing-tasks-v1-0-972ed6ab2c0d@pks.im> ``` The parameters of `cmd_gc()` aren't indented properly. Fix this. Signed-off-by: Patrick Steinhardt --- builtin/gc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index d5c75be2522..a73ec22fb18 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -724,9 +724,9 @@ static void gc_before_repack(struct maintenance_run_opts *opts, } int cmd_gc(int argc, -const char **argv, -const char *prefix, -struct repository *repo UNUSED) + const char **argv, + const char *prefix, + struct repository *repo UNUSED) { int aggressive = 0; int quiet = 0; -- 2.49.0.901.g37484f566f.dirty ``` ## Patrick Steinhardt, 2025-04-25 07:29 Subject: [PATCH 2/7] builtin/gc: remove global variables where it trivial to do Message-ID: <20250425-pks-maintenance-missing-tasks-v1-2-972ed6ab2c0d@pks.im> URL: https://gitlist.dev/e/20250425-pks-maintenance-missing-tasks-v1-2-972ed6ab2c0d%40pks.im In-Reply-To: <20250425-pks-maintenance-missing-tasks-v1-0-972ed6ab2c0d@pks.im> ``` We use a couple of global variables to assemble command line arguments for subprocesses we execute in git-gc(1). All of these variables except the one for git-repack(1) are only used in a single place though, so they don't really add anything but confusion. Remove those variables. Signed-off-by: Patrick Steinhardt --- builtin/gc.c | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index a73ec22fb18..ada36e210f0 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -53,15 +53,9 @@ static const char * const builtin_gc_usage[] = { }; static timestamp_t gc_log_expire_time; - static struct strvec repack = STRVEC_INIT; -static struct strvec prune = STRVEC_INIT; -static struct strvec prune_worktrees = STRVEC_INIT; -static struct strvec rerere = STRVEC_INIT; - static struct tempfile *pidfile; static struct lock_file log_lock; - static struct string_list pack_garbage = STRING_LIST_INIT_DUP; static void clean_pack_garbage(void) @@ -769,9 +763,6 @@ int cmd_gc(int argc, builtin_gc_usage, builtin_gc_options); strvec_pushl(&repack, "repack", "-d", "-l", NULL); - strvec_pushl(&prune, "prune", "--expire", NULL); - strvec_pushl(&prune_worktrees, "worktree", "prune", "--expire", NULL); - strvec_pushl(&rerere, "rerere", "gc", NULL); gc_config(&cfg); @@ -897,34 +888,36 @@ int cmd_gc(int argc, if (cfg.prune_expire) { struct child_process prune_cmd = CHILD_PROCESS_INIT; + strvec_pushl(&prune_cmd.args, "prune", "--expire", NULL); /* run `git prune` even if using cruft packs */ - strvec_push(&prune, cfg.prune_expire); + strvec_push(&prune_cmd.args, cfg.prune_expire); if (quiet) - strvec_push(&prune, "--no-progress"); + strvec_push(&prune_cmd.args, "--no-progress"); if (repo_has_promisor_remote(the_repository)) - strvec_push(&prune, + strvec_push(&prune_cmd.args, "--exclude-promisor-objects"); prune_cmd.git_cmd = 1; - strvec_pushv(&prune_cmd.args, prune.v); + if (run_command(&prune_cmd)) - die(FAILED_RUN, prune.v[0]); + die(FAILED_RUN, prune_cmd.args.v[0]); } } if (cfg.prune_worktrees_expire) { struct child_process prune_worktrees_cmd = CHILD_PROCESS_INIT; - strvec_push(&prune_worktrees, cfg.prune_worktrees_expire); prune_worktrees_cmd.git_cmd = 1; - strvec_pushv(&prune_worktrees_cmd.args, prune_worktrees.v); + strvec_pushl(&prune_worktrees_cmd.args, "worktree", "prune", "--expire", NULL); + strvec_push(&prune_worktrees_cmd.args, cfg.prune_worktrees_expire); + if (run_command(&prune_worktrees_cmd)) - die(FAILED_RUN, prune_worktrees.v[0]); + die(FAILED_RUN, prune_worktrees_cmd.args.v[0]); } rerere_cmd.git_cmd = 1; - strvec_pushv(&rerere_cmd.args, rerere.v); + strvec_pushl(&rerere_cmd.args, "rerere", "gc", NULL); if (run_command(&rerere_cmd)) - die(FAILED_RUN, rerere.v[0]); + die(FAILED_RUN, rerere_cmd.args.v[0]); report_garbage = report_pack_garbage; reprepare_packed_git(the_repository); -- 2.49.0.901.g37484f566f.dirty ``` ## Patrick Steinhardt, 2025-04-25 07:29 Subject: [PATCH 3/7] builtin/gc: move pruning of worktrees into a separate function Message-ID: <20250425-pks-maintenance-missing-tasks-v1-3-972ed6ab2c0d@pks.im> URL: https://gitlist.dev/e/20250425-pks-maintenance-missing-tasks-v1-3-972ed6ab2c0d%40pks.im In-Reply-To: <20250425-pks-maintenance-missing-tasks-v1-0-972ed6ab2c0d@pks.im> ``` Move pruning of worktrees into a separate function. This prepares for a subsequent commit where we introduce a new "worktree-prune" task for git-maintenance(1). Signed-off-by: Patrick Steinhardt --- builtin/gc.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index ada36e210f0..005ecc3f192 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -334,6 +334,18 @@ static int maintenance_task_reflog_expire(struct maintenance_run_opts *opts UNUS return run_command(&cmd); } +static int maintenance_task_worktree_prune(struct maintenance_run_opts *opts UNUSED, + struct gc_config *cfg) +{ + struct child_process prune_worktrees_cmd = CHILD_PROCESS_INIT; + + prune_worktrees_cmd.git_cmd = 1; + strvec_pushl(&prune_worktrees_cmd.args, "worktree", "prune", "--expire", NULL); + strvec_push(&prune_worktrees_cmd.args, cfg->prune_worktrees_expire); + + return run_command(&prune_worktrees_cmd); +} + static int too_many_loose_objects(struct gc_config *cfg) { /* @@ -903,16 +915,9 @@ int cmd_gc(int argc, } } - if (cfg.prune_worktrees_expire) { - struct child_process prune_worktrees_cmd = CHILD_PROCESS_INIT; - - prune_worktrees_cmd.git_cmd = 1; - strvec_pushl(&prune_worktrees_cmd.args, "worktree", "prune", "--expire", NULL); - strvec_push(&prune_worktrees_cmd.args, cfg.prune_worktrees_expire); - - if (run_command(&prune_worktrees_cmd)) - die(FAILED_RUN, prune_worktrees_cmd.args.v[0]); - } + if (cfg.prune_worktrees_expire && + maintenance_task_worktree_prune(&opts, &cfg)) + die(FAILED_RUN, "worktree"); rerere_cmd.git_cmd = 1; strvec_pushl(&rerere_cmd.args, "rerere", "gc", NULL); -- 2.49.0.901.g37484f566f.dirty ``` ## Patrick Steinhardt, 2025-04-25 07:29 Subject: [PATCH 4/7] worktree: expose function to retrieve worktree names Message-ID: <20250425-pks-maintenance-missing-tasks-v1-4-972ed6ab2c0d@pks.im> URL: https://gitlist.dev/e/20250425-pks-maintenance-missing-tasks-v1-4-972ed6ab2c0d%40pks.im In-Reply-To: <20250425-pks-maintenance-missing-tasks-v1-0-972ed6ab2c0d@pks.im> ``` Introduce a function that retrieves worktree names as present in ".git/worktrees". This function will be used in a subsequent commit. Signed-off-by: Patrick Steinhardt --- builtin/worktree.c | 25 ++++++++++++------------- worktree.c | 30 ++++++++++++++++++++++++++++++ worktree.h | 8 ++++++++ 3 files changed, 50 insertions(+), 13 deletions(-) diff --git a/builtin/worktree.c b/builtin/worktree.c index 87ccd47794c..9b00dbf1265 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -211,27 +211,24 @@ static void prune_dups(struct string_list *l) static void prune_worktrees(void) { - struct strbuf reason = STRBUF_INIT; struct strbuf main_path = STRBUF_INIT; struct string_list kept = STRING_LIST_INIT_DUP; - char *path; - DIR *dir; - struct dirent *d; + struct strvec worktrees = STRVEC_INIT; + struct strbuf reason = STRBUF_INIT; - path = repo_git_path(the_repository, "worktrees"); - dir = opendir(path); - free(path); - if (!dir) + if (get_worktree_names(the_repository, &worktrees) < 0 || + !worktrees.nr) return; - while ((d = readdir_skip_dot_and_dotdot(dir)) != NULL) { + + for (size_t i = 0; i < worktrees.nr; i++) { char *path; + strbuf_reset(&reason); - if (should_prune_worktree(d->d_name, &reason, &path, expire)) - prune_worktree(d->d_name, reason.buf); + if (should_prune_worktree(worktrees.v[i], &reason, &path, expire)) + prune_worktree(worktrees.v[i], reason.buf); else if (path) - string_list_append_nodup(&kept, path)->util = xstrdup(d->d_name); + string_list_append_nodup(&kept, path)->util = xstrdup(worktrees.v[i]); } - closedir(dir); strbuf_add_absolute_path(&main_path, repo_get_common_dir(the_repository)); /* massage main worktree absolute path to match 'gitdir' content */ @@ -242,6 +239,8 @@ static void prune_worktrees(void) if (!show_only) delete_worktrees_dir_if_empty(); + + strvec_clear(&worktrees); strbuf_release(&reason); } diff --git a/worktree.c b/worktree.c index c34b9eb74e5..947b7a82209 100644 --- a/worktree.c +++ b/worktree.c @@ -988,6 +988,36 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, return rc; } +int get_worktree_names(struct repository *repo, struct strvec *out) +{ + char *worktrees_dir; + struct dirent *d; + DIR *dir; + int ret; + + worktrees_dir = repo_git_path(repo, "worktrees"); + dir = opendir(worktrees_dir); + if (!dir) { + if (errno == ENOENT) { + ret = 0; + goto out; + } + + ret = -1; + goto out; + } + + while ((d = readdir_skip_dot_and_dotdot(dir)) != NULL) + strvec_push(out, d->d_name); + + ret = 0; +out: + if (dir) + closedir(dir); + free(worktrees_dir); + return ret; +} + static int move_config_setting(const char *key, const char *value, const char *from_file, const char *to_file) { diff --git a/worktree.h b/worktree.h index e4bcccdc0ae..59825c37881 100644 --- a/worktree.h +++ b/worktree.h @@ -38,6 +38,14 @@ struct worktree **get_worktrees(void); */ struct worktree **get_worktrees_without_reading_head(void); +/* + * Retrieve all worktree names. Not all names may correspond to a fully + * functional worktree. Returns 0 on success, a negative error code on failure. + * Calling the function on a repository that doesn't have any worktrees is not + * considered an error. + */ +int get_worktree_names(struct repository *repo, struct strvec *out); + /* * Returns 1 if linked worktrees exist, 0 otherwise. */ -- 2.49.0.901.g37484f566f.dirty ``` ## Patrick Steinhardt, 2025-04-25 07:29 Subject: [PATCH 5/7] builtin/maintenance: introduce "worktree-prune" task Message-ID: <20250425-pks-maintenance-missing-tasks-v1-5-972ed6ab2c0d@pks.im> URL: https://gitlist.dev/e/20250425-pks-maintenance-missing-tasks-v1-5-972ed6ab2c0d%40pks.im In-Reply-To: <20250425-pks-maintenance-missing-tasks-v1-0-972ed6ab2c0d@pks.im> ``` 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 --- Documentation/git-maintenance.adoc | 4 ++++ builtin/gc.c | 35 +++++++++++++++++++++++++++++++++++ t/t7900-maintenance.sh | 17 +++++++++++++++++ 3 files changed, 56 insertions(+) diff --git a/Documentation/git-maintenance.adoc b/Documentation/git-maintenance.adoc index 3a1e2a69b6b..6f085a9cf8c 100644 --- a/Documentation/git-maintenance.adoc +++ b/Documentation/git-maintenance.adoc @@ -166,6 +166,10 @@ reflog-expire:: The `reflog-expire` task deletes any entries in the reflog older than the expiry threshold. See linkgit:git-reflog[1] for more information. +worktree-prune:: + The `worktree-prune` task deletes stale or broken worktrees. See + linkit:git-worktree[1] for more information. + OPTIONS ------- --auto:: diff --git a/builtin/gc.c b/builtin/gc.c index 005ecc3f192..46c64c852dc 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -44,6 +44,7 @@ #include "hook.h" #include "setup.h" #include "trace2.h" +#include "worktree.h" #define FAILED_RUN "failed to run %s" @@ -346,6 +347,34 @@ static int maintenance_task_worktree_prune(struct maintenance_run_opts *opts UNU return run_command(&prune_worktrees_cmd); } +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; +} + static int too_many_loose_objects(struct gc_config *cfg) { /* @@ -1455,6 +1484,7 @@ enum maintenance_task_label { TASK_COMMIT_GRAPH, TASK_PACK_REFS, TASK_REFLOG_EXPIRE, + TASK_WORKTREE_PRUNE, /* Leave as final value */ TASK__COUNT @@ -1496,6 +1526,11 @@ static struct maintenance_task tasks[] = { maintenance_task_reflog_expire, reflog_expire_condition, }, + [TASK_WORKTREE_PRUNE] = { + "worktree-prune", + maintenance_task_worktree_prune, + worktree_prune_condition, + }, }; static int compare_tasks_by_selection(const void *a_, const void *b_) diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 9b82e11c100..d21feda271f 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -493,6 +493,23 @@ test_expect_success 'reflog-expire task --auto only packs when exceeding limits' test_subcommand git reflog expire --all .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 err && test_grep "at most one" err -- 2.49.0.901.g37484f566f.dirty ``` ## Patrick Steinhardt, 2025-04-25 07:29 Subject: [PATCH 6/7] builtin/gc: move rerere garbage collection into separate function Message-ID: <20250425-pks-maintenance-missing-tasks-v1-6-972ed6ab2c0d@pks.im> URL: https://gitlist.dev/e/20250425-pks-maintenance-missing-tasks-v1-6-972ed6ab2c0d%40pks.im In-Reply-To: <20250425-pks-maintenance-missing-tasks-v1-0-972ed6ab2c0d@pks.im> ``` Move garbage collection of cached rerere entries into a separate function. This prepares us for a subsequent commit where we introduce a new "rerere-gc" task for git-maintenance(1). Signed-off-by: Patrick Steinhardt --- builtin/gc.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index 46c64c852dc..a0816bcf302 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -375,6 +375,15 @@ static int worktree_prune_condition(struct gc_config *cfg) return should_prune; } +static int maintenance_task_rerere_gc(struct maintenance_run_opts *opts UNUSED, + struct gc_config *cfg UNUSED) +{ + struct child_process rerere_cmd = CHILD_PROCESS_INIT; + rerere_cmd.git_cmd = 1; + strvec_pushl(&rerere_cmd.args, "rerere", "gc", NULL); + return run_command(&rerere_cmd); +} + static int too_many_loose_objects(struct gc_config *cfg) { /* @@ -771,7 +780,6 @@ int cmd_gc(int argc, int daemonized = 0; int keep_largest_pack = -1; timestamp_t dummy; - struct child_process rerere_cmd = CHILD_PROCESS_INIT; struct maintenance_run_opts opts = MAINTENANCE_RUN_OPTS_INIT; struct gc_config cfg = GC_CONFIG_INIT; const char *prune_expire_sentinel = "sentinel"; @@ -948,10 +956,8 @@ int cmd_gc(int argc, maintenance_task_worktree_prune(&opts, &cfg)) die(FAILED_RUN, "worktree"); - rerere_cmd.git_cmd = 1; - strvec_pushl(&rerere_cmd.args, "rerere", "gc", NULL); - if (run_command(&rerere_cmd)) - die(FAILED_RUN, rerere_cmd.args.v[0]); + if (maintenance_task_rerere_gc(&opts, &cfg)) + die(FAILED_RUN, "rerere"); report_garbage = report_pack_garbage; reprepare_packed_git(the_repository); -- 2.49.0.901.g37484f566f.dirty ``` ## Patrick Steinhardt, 2025-04-25 07:29 Subject: [PATCH 7/7] builtin/maintenance: introduce "rerere-gc" task Message-ID: <20250425-pks-maintenance-missing-tasks-v1-7-972ed6ab2c0d@pks.im> URL: https://gitlist.dev/e/20250425-pks-maintenance-missing-tasks-v1-7-972ed6ab2c0d%40pks.im In-Reply-To: <20250425-pks-maintenance-missing-tasks-v1-0-972ed6ab2c0d@pks.im> ``` While git-gc(1) knows to garbage collect the rerere cache, git-maintenance(1) does not yet have a task for this cleanup. Introduce a new "rerere-gc" task to plug this gap. Signed-off-by: Patrick Steinhardt --- Documentation/git-maintenance.adoc | 4 ++++ builtin/gc.c | 27 +++++++++++++++++++++++++++ t/t7900-maintenance.sh | 17 +++++++++++++++++ 3 files changed, 48 insertions(+) diff --git a/Documentation/git-maintenance.adoc b/Documentation/git-maintenance.adoc index 6f085a9cf8c..931f3e02e85 100644 --- a/Documentation/git-maintenance.adoc +++ b/Documentation/git-maintenance.adoc @@ -166,6 +166,10 @@ reflog-expire:: The `reflog-expire` task deletes any entries in the reflog older than the expiry threshold. See linkgit:git-reflog[1] for more information. +rerere-gc:: + The `rerere-gc` task invokes garbage collection for stale entries in + the rerere cache. See linkgit:git-rerere[1] for more information. + worktree-prune:: The `worktree-prune` task deletes stale or broken worktrees. See linkit:git-worktree[1] for more information. diff --git a/builtin/gc.c b/builtin/gc.c index a0816bcf302..d19449b33d4 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -16,6 +16,7 @@ #include "builtin.h" #include "abspath.h" #include "date.h" +#include "dir.h" #include "environment.h" #include "hex.h" #include "config.h" @@ -384,6 +385,26 @@ static int maintenance_task_rerere_gc(struct maintenance_run_opts *opts UNUSED, return run_command(&rerere_cmd); } +static int rerere_gc_condition(struct gc_config *cfg UNUSED) +{ + struct strbuf path = STRBUF_INIT; + int should_gc = 0; + DIR *dir; + + /* Skip garbage collecting the rerere cache in case rerere is disabled. */ + repo_git_path_replace(the_repository, &path, "rr-cache"); + + dir = opendir(path.buf); + if (!dir) + goto out; + should_gc = !!readdir_skip_dot_and_dotdot(dir); + +out: + strbuf_release(&path); + closedir(dir); + return should_gc; +} + static int too_many_loose_objects(struct gc_config *cfg) { /* @@ -1491,6 +1512,7 @@ enum maintenance_task_label { TASK_PACK_REFS, TASK_REFLOG_EXPIRE, TASK_WORKTREE_PRUNE, + TASK_RERERE_GC, /* Leave as final value */ TASK__COUNT @@ -1537,6 +1559,11 @@ static struct maintenance_task tasks[] = { maintenance_task_worktree_prune, worktree_prune_condition, }, + [TASK_RERERE_GC] = { + "rerere-gc", + maintenance_task_rerere_gc, + rerere_gc_condition, + }, }; static int compare_tasks_by_selection(const void *a_, const void *b_) diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index d21feda271f..9cc52f28ca4 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -510,6 +510,23 @@ test_expect_success 'worktree-prune task --auto only prunes with prunable worktr test_subcommand git worktree prune --expire 3.months.ago .git/rr-cache/entry && + GIT_TRACE2_EVENT="$(pwd)/rerere-gc-auto.txt" \ + git maintenance run --auto --task=rerere-gc && + test_subcommand git rerere gc err && test_grep "at most one" err -- 2.49.0.901.g37484f566f.dirty ``` ## Derrick Stolee, 2025-04-29 20:02 Subject: Re: [PATCH 5/7] builtin/maintenance: introduce "worktree-prune" task Message-ID: URL: https://gitlist.dev/e/c629f845-180f-4ecb-949f-a99f184f812d%40gmail.com In-Reply-To: <20250425-pks-maintenance-missing-tasks-v1-5-972ed6ab2c0d@pks.im> ``` 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). 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 +' > + > +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 + 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 +' > + > 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. Thanks, -Stolee ``` ## Derrick Stolee, 2025-04-29 20:02 Subject: Re: [PATCH 0/7] builtin/maintenance: implement missing tasks compared to git-gc(1) Message-ID: <3cc1ef7b-40d5-4802-9bf9-ff28824ce563@gmail.com> URL: https://gitlist.dev/e/3cc1ef7b-40d5-4802-9bf9-ff28824ce563%40gmail.com In-Reply-To: <20250425-pks-maintenance-missing-tasks-v1-0-972ed6ab2c0d@pks.im> ``` On 4/25/2025 3:29 AM, Patrick Steinhardt wrote: > Right now, git-maintenance(1) still executes git-gc(1). With these last > gaps plugged though we can in theory fully replace git-gc(1) with finer > grained tasks without losing any functionality. The benefit is that it > becomes possible for users to have finer-grained control over what > exactly the maintenance does. > > This patch series doesn't do that yet, but only implements whatever is > needed to get there. Thanks for putting this together. I think this is a noble goal, allowing users and system administrators more options to fine-tune the best ways to optimize their repos. I wonder if any of these fine-grained steps would be valuable to add to the default background maintenance schedule (perhaps as a follow-up)? > Patrick Steinhardt (7): > builtin/gc: fix indentation of `cmd_gc()` parameters > builtin/gc: remove global variables where it trivial to do These first two patches are simple cleanups. Thanks for isolating them. > builtin/gc: move pruning of worktrees into a separate function This is a nice refactor with a clean method body extraction. > worktree: expose function to retrieve worktree names This one is a bit messier, but still really good as it makes the prune_worktrees() method in builtin/worktree.c less complicated. There's just no way to make the removal of those variables look clean. > builtin/maintenance: introduce "worktree-prune" task This is where we start getting into new behavior. More comments on the patch itself. > builtin/gc: move rerere garbage collection into separate function > builtin/maintenance: introduce "rerere-gc" task These are a nice one-two punch for this new task. Thanks, -Stolee ``` ## Patrick Steinhardt, 2025-04-30 07:08 Subject: Re: [PATCH 0/7] builtin/maintenance: implement missing tasks compared to git-gc(1) Message-ID: URL: https://gitlist.dev/e/aBHMVm9QIKZwiaBX%40pks.im In-Reply-To: <3cc1ef7b-40d5-4802-9bf9-ff28824ce563@gmail.com> ``` On Tue, Apr 29, 2025 at 04:02:18PM -0400, Derrick Stolee wrote: > On 4/25/2025 3:29 AM, Patrick Steinhardt wrote: > > > Right now, git-maintenance(1) still executes git-gc(1). With these last > > gaps plugged though we can in theory fully replace git-gc(1) with finer > > grained tasks without losing any functionality. The benefit is that it > > becomes possible for users to have finer-grained control over what > > exactly the maintenance does. > > > > This patch series doesn't do that yet, but only implements whatever is > > needed to get there. > > Thanks for putting this together. I think this is a noble goal, allowing > users and system administrators more options to fine-tune the best ways to > optimize their repos. > > I wonder if any of these fine-grained steps would be valuable to add to > the default background maintenance schedule (perhaps as a follow-up)? I think so, yes. My immediate goal is to change `git maintenance run --auto` to not run git-gc(1) by default anymore, but to instead have it run the fine-grained steps. But we should probably at the same time also adapt the background maintenance to use the same fine-grained tasks so that the outcome matches. My idea would be that we implement all of this via high-level strategies that get honored by scheduled, auto and explicit maintenance alike. We already have the "incremental" strategy, but there are other strategies that might make sense to introduce. There should be at least one strategy that achieves the same as git-gc(1) does right now, but there's probably more opportunities here to have e.g. a "geometric" strategy. Users can then pick whatever strategy works best for them, with us providing a good default as well as rationale why we recommend one strategy over another. Furthermore, expert users can of course tweak these strategies even further by explicitly configuring which of the fine-grained tasks should run. Thinking ahead a bit, one might even be prompted to auto-select strategies based on repository properties. A tiny repository likely does not need multi-pack indices and cruft packs, but a huge behemoth like the Chromium repository probably would benefit. But that is an iteration that one can think about in the future. Patrick ``` ## Patrick Steinhardt, 2025-04-30 07:08 Subject: Re: [PATCH 5/7] builtin/maintenance: introduce "worktree-prune" task Message-ID: URL: https://gitlist.dev/e/aBHMXGNfDZQ-JVT1%40pks.im In-Reply-To: ``` 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 > +' > > + > > +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 > + 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 > +' > > + > > 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 ``` ## Patrick Steinhardt, 2025-05-05 08:51 Subject: [PATCH v4 0/7] builtin/maintenance: implement missing tasks compared to git-gc(1) Message-ID: <20250505-pks-maintenance-missing-tasks-v4-0-141f4df906a1@pks.im> URL: https://gitlist.dev/e/20250505-pks-maintenance-missing-tasks-v4-0-141f4df906a1%40pks.im In-Reply-To: <20250425-pks-maintenance-missing-tasks-v1-0-972ed6ab2c0d@pks.im> ``` Hi, this small patch series implements the last couple of remaining tasks that are missing compared to the functionality git-gc(1) provides. Right now, git-maintenance(1) still executes git-gc(1). With these last gaps plugged though we can in theory fully replace git-gc(1) with finer grained tasks without losing any functionality. The benefit is that it becomes possible for users to have finer-grained control over what exactly the maintenance does. This patch series doesn't do that yet, but only implements whatever is needed to get there. Changes in v2: - Introduce "maintenance.worktree-prune.auto", which controls how many stale worktrees need to exist before executing `git worktree prune`. - Introduce "maintenance.rerere-gc.auto", which controls how many stale rerere entries need to exist before executing `git rerere gc`. - Add tests to verify that "gc.worktreePruneExpire" works. - Remove some fragile test logic by introducing functions that check for a given maintenance subprocess. - Link to v1: https://lore.kernel.org/r/20250425-pks-maintenance-missing-tasks-v1-0-972ed6ab2c0d@pks.im Changes in v3: - Simplify the heuristic for "rerere-gc" so that we only count the number of directory entries in ".git/rr-cache", without considering staleness. - Link to v2: https://lore.kernel.org/r/20250430-pks-maintenance-missing-tasks-v2-0-2580b7b8ca3a@pks.im Changes in v4: - simplified the heuristic for "rerere-gc" even further. A positive value for "maintenance.rerere-gc.auto" now indicates that the command will run whenever there is at least one directory entry in ".rr-cache". The exact value does not matter anymore. - Link to v3: https://lore.kernel.org/r/20250502-pks-maintenance-missing-tasks-v3-0-13e130d36640@pks.im Thanks! Patrick --- Patrick Steinhardt (7): builtin/gc: fix indentation of `cmd_gc()` parameters builtin/gc: remove global variables where it trivial to do builtin/gc: move pruning of worktrees into a separate function worktree: expose function to retrieve worktree names builtin/maintenance: introduce "worktree-prune" task builtin/gc: move rerere garbage collection into separate function builtin/maintenance: introduce "rerere-gc" task Documentation/config/maintenance.adoc | 17 ++++ Documentation/git-maintenance.adoc | 8 ++ builtin/gc.c | 149 +++++++++++++++++++++++++++------- builtin/worktree.c | 25 +++--- t/t7900-maintenance.sh | 115 ++++++++++++++++++++++++++ worktree.c | 30 +++++++ worktree.h | 8 ++ 7 files changed, 308 insertions(+), 44 deletions(-) Range-diff versus v3: 1: 0cf3cb937c3 = 1: 6614504400b builtin/gc: fix indentation of `cmd_gc()` parameters 2: 5982d47cc95 = 2: 6c8c66282c5 builtin/gc: remove global variables where it trivial to do 3: afda176acd3 = 3: bfa8fc00434 builtin/gc: move pruning of worktrees into a separate function 4: b3b6a14bf1c = 4: d87b883cf62 worktree: expose function to retrieve worktree names 5: e02d59bb944 = 5: 354a1985b4b builtin/maintenance: introduce "worktree-prune" task 6: cb31ec6b8bd = 6: 6f40349b53f builtin/gc: move rerere garbage collection into separate function 7: 65e8483fe4a ! 7: e3c9da19a2c builtin/maintenance: introduce "rerere-gc" task @@ Documentation/config/maintenance.adoc: maintenance.reflog-expire.auto:: + This integer config option controls how often the `rerere-gc` task + should be run as part of `git maintenance run --auto`. If zero, then + the `rerere-gc` task will not run with the `--auto` option. A negative -+ value will force the task to run every time. Otherwise, a positive -+ value implies the command should run when there are at least this many -+ directory entries in the "rr-cache" directory. The default value is 1. ++ value will force the task to run every time. Otherwise, any positive ++ value implies the command will run when the "rr-cache" directory exists ++ and has at least one entry, regardless of whether it is stale or not. ++ This heuristic may be refined in the future. The default value is 1. + maintenance.worktree-prune.auto:: This integer config option controls how often the `worktree-prune` task @@ builtin/gc.c: static int maintenance_task_rerere_gc(struct maintenance_run_opts + + /* + * We skip garbage collection in case we either have no "rr-cache" -+ * directory or when it doesn't contain at least as many directories as -+ * indicated by "maintenance.rerere-gc.auto". ++ * directory or when it doesn't contain at least one entry. + */ + repo_git_path_replace(the_repository, &path, "rr-cache"); + dir = opendir(path.buf); + if (!dir) + goto out; -+ -+ while (readdir_skip_dot_and_dotdot(dir)) { -+ if (--limit) -+ continue; -+ -+ should_gc = 1; -+ goto out; -+ } ++ should_gc = !!readdir_skip_dot_and_dotdot(dir); + +out: + strbuf_release(&path); @@ t/t7900-maintenance.sh: test_expect_success 'worktree-prune task honors gc.workt + # A negative value should always prune. + test_expect_rerere_gc git -c maintenance.rerere-gc.auto=-1 maintenance run --auto --task=rerere-gc && + ++ # A positive value prunes when there is at least one entry. ++ test_expect_rerere_gc ! git -c maintenance.rerere-gc.auto=9000 maintenance run --auto --task=rerere-gc && + mkdir .git/rr-cache && ++ test_expect_rerere_gc ! git -c maintenance.rerere-gc.auto=9000 maintenance run --auto --task=rerere-gc && + : >.git/rr-cache/entry-1 && -+ : >.git/rr-cache/entry-2 && ++ test_expect_rerere_gc git -c maintenance.rerere-gc.auto=9000 maintenance run --auto --task=rerere-gc && + + # Zero should never prune. -+ test_expect_rerere_gc ! git -c maintenance.rerere-gc.auto=0 maintenance run --auto --task=rerere-gc && -+ # A positive value should require at least this many stale rerere entries. -+ test_expect_rerere_gc ! git -c maintenance.rerere-gc.auto=3 maintenance run --auto --task=rerere-gc && -+ test_expect_rerere_gc git -c maintenance.rerere-gc.auto=2 maintenance run --auto --task=rerere-gc ++ : >.git/rr-cache/entry-1 && ++ test_expect_rerere_gc ! git -c maintenance.rerere-gc.auto=0 maintenance run --auto --task=rerere-gc +' + test_expect_success '--auto and --schedule incompatible' ' --- base-commit: a2955b34f48265d240ab8c7deb0a929ec2d65fd0 change-id: 20250424-pks-maintenance-missing-tasks-8ffcdd596b73 ``` ## Patrick Steinhardt, 2025-05-05 08:51 Subject: [PATCH v4 1/7] builtin/gc: fix indentation of `cmd_gc()` parameters Message-ID: <20250505-pks-maintenance-missing-tasks-v4-1-141f4df906a1@pks.im> URL: https://gitlist.dev/e/20250505-pks-maintenance-missing-tasks-v4-1-141f4df906a1%40pks.im In-Reply-To: <20250505-pks-maintenance-missing-tasks-v4-0-141f4df906a1@pks.im> ``` The parameters of `cmd_gc()` aren't indented properly. Fix this. Signed-off-by: Patrick Steinhardt --- builtin/gc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index d5c75be2522..a73ec22fb18 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -724,9 +724,9 @@ static void gc_before_repack(struct maintenance_run_opts *opts, } int cmd_gc(int argc, -const char **argv, -const char *prefix, -struct repository *repo UNUSED) + const char **argv, + const char *prefix, + struct repository *repo UNUSED) { int aggressive = 0; int quiet = 0; -- 2.49.0.987.g0cc8ee98dc.dirty ``` ## Patrick Steinhardt, 2025-05-05 08:51 Subject: [PATCH v4 2/7] builtin/gc: remove global variables where it trivial to do Message-ID: <20250505-pks-maintenance-missing-tasks-v4-2-141f4df906a1@pks.im> URL: https://gitlist.dev/e/20250505-pks-maintenance-missing-tasks-v4-2-141f4df906a1%40pks.im In-Reply-To: <20250505-pks-maintenance-missing-tasks-v4-0-141f4df906a1@pks.im> ``` We use a couple of global variables to assemble command line arguments for subprocesses we execute in git-gc(1). All of these variables except the one for git-repack(1) are only used in a single place though, so they don't really add anything but confusion. Remove those variables. Signed-off-by: Patrick Steinhardt --- builtin/gc.c | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index a73ec22fb18..ada36e210f0 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -53,15 +53,9 @@ static const char * const builtin_gc_usage[] = { }; static timestamp_t gc_log_expire_time; - static struct strvec repack = STRVEC_INIT; -static struct strvec prune = STRVEC_INIT; -static struct strvec prune_worktrees = STRVEC_INIT; -static struct strvec rerere = STRVEC_INIT; - static struct tempfile *pidfile; static struct lock_file log_lock; - static struct string_list pack_garbage = STRING_LIST_INIT_DUP; static void clean_pack_garbage(void) @@ -769,9 +763,6 @@ int cmd_gc(int argc, builtin_gc_usage, builtin_gc_options); strvec_pushl(&repack, "repack", "-d", "-l", NULL); - strvec_pushl(&prune, "prune", "--expire", NULL); - strvec_pushl(&prune_worktrees, "worktree", "prune", "--expire", NULL); - strvec_pushl(&rerere, "rerere", "gc", NULL); gc_config(&cfg); @@ -897,34 +888,36 @@ int cmd_gc(int argc, if (cfg.prune_expire) { struct child_process prune_cmd = CHILD_PROCESS_INIT; + strvec_pushl(&prune_cmd.args, "prune", "--expire", NULL); /* run `git prune` even if using cruft packs */ - strvec_push(&prune, cfg.prune_expire); + strvec_push(&prune_cmd.args, cfg.prune_expire); if (quiet) - strvec_push(&prune, "--no-progress"); + strvec_push(&prune_cmd.args, "--no-progress"); if (repo_has_promisor_remote(the_repository)) - strvec_push(&prune, + strvec_push(&prune_cmd.args, "--exclude-promisor-objects"); prune_cmd.git_cmd = 1; - strvec_pushv(&prune_cmd.args, prune.v); + if (run_command(&prune_cmd)) - die(FAILED_RUN, prune.v[0]); + die(FAILED_RUN, prune_cmd.args.v[0]); } } if (cfg.prune_worktrees_expire) { struct child_process prune_worktrees_cmd = CHILD_PROCESS_INIT; - strvec_push(&prune_worktrees, cfg.prune_worktrees_expire); prune_worktrees_cmd.git_cmd = 1; - strvec_pushv(&prune_worktrees_cmd.args, prune_worktrees.v); + strvec_pushl(&prune_worktrees_cmd.args, "worktree", "prune", "--expire", NULL); + strvec_push(&prune_worktrees_cmd.args, cfg.prune_worktrees_expire); + if (run_command(&prune_worktrees_cmd)) - die(FAILED_RUN, prune_worktrees.v[0]); + die(FAILED_RUN, prune_worktrees_cmd.args.v[0]); } rerere_cmd.git_cmd = 1; - strvec_pushv(&rerere_cmd.args, rerere.v); + strvec_pushl(&rerere_cmd.args, "rerere", "gc", NULL); if (run_command(&rerere_cmd)) - die(FAILED_RUN, rerere.v[0]); + die(FAILED_RUN, rerere_cmd.args.v[0]); report_garbage = report_pack_garbage; reprepare_packed_git(the_repository); -- 2.49.0.987.g0cc8ee98dc.dirty ``` ## Patrick Steinhardt, 2025-05-05 08:51 Subject: [PATCH v4 3/7] builtin/gc: move pruning of worktrees into a separate function Message-ID: <20250505-pks-maintenance-missing-tasks-v4-3-141f4df906a1@pks.im> URL: https://gitlist.dev/e/20250505-pks-maintenance-missing-tasks-v4-3-141f4df906a1%40pks.im In-Reply-To: <20250505-pks-maintenance-missing-tasks-v4-0-141f4df906a1@pks.im> ``` Move pruning of worktrees into a separate function. This prepares for a subsequent commit where we introduce a new "worktree-prune" task for git-maintenance(1). Signed-off-by: Patrick Steinhardt --- builtin/gc.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index ada36e210f0..005ecc3f192 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -334,6 +334,18 @@ static int maintenance_task_reflog_expire(struct maintenance_run_opts *opts UNUS return run_command(&cmd); } +static int maintenance_task_worktree_prune(struct maintenance_run_opts *opts UNUSED, + struct gc_config *cfg) +{ + struct child_process prune_worktrees_cmd = CHILD_PROCESS_INIT; + + prune_worktrees_cmd.git_cmd = 1; + strvec_pushl(&prune_worktrees_cmd.args, "worktree", "prune", "--expire", NULL); + strvec_push(&prune_worktrees_cmd.args, cfg->prune_worktrees_expire); + + return run_command(&prune_worktrees_cmd); +} + static int too_many_loose_objects(struct gc_config *cfg) { /* @@ -903,16 +915,9 @@ int cmd_gc(int argc, } } - if (cfg.prune_worktrees_expire) { - struct child_process prune_worktrees_cmd = CHILD_PROCESS_INIT; - - prune_worktrees_cmd.git_cmd = 1; - strvec_pushl(&prune_worktrees_cmd.args, "worktree", "prune", "--expire", NULL); - strvec_push(&prune_worktrees_cmd.args, cfg.prune_worktrees_expire); - - if (run_command(&prune_worktrees_cmd)) - die(FAILED_RUN, prune_worktrees_cmd.args.v[0]); - } + if (cfg.prune_worktrees_expire && + maintenance_task_worktree_prune(&opts, &cfg)) + die(FAILED_RUN, "worktree"); rerere_cmd.git_cmd = 1; strvec_pushl(&rerere_cmd.args, "rerere", "gc", NULL); -- 2.49.0.987.g0cc8ee98dc.dirty ``` ## Patrick Steinhardt, 2025-05-05 08:51 Subject: [PATCH v4 4/7] worktree: expose function to retrieve worktree names Message-ID: <20250505-pks-maintenance-missing-tasks-v4-4-141f4df906a1@pks.im> URL: https://gitlist.dev/e/20250505-pks-maintenance-missing-tasks-v4-4-141f4df906a1%40pks.im In-Reply-To: <20250505-pks-maintenance-missing-tasks-v4-0-141f4df906a1@pks.im> ``` Introduce a function that retrieves worktree names as present in ".git/worktrees". This function will be used in a subsequent commit. Signed-off-by: Patrick Steinhardt --- builtin/worktree.c | 25 ++++++++++++------------- worktree.c | 30 ++++++++++++++++++++++++++++++ worktree.h | 8 ++++++++ 3 files changed, 50 insertions(+), 13 deletions(-) diff --git a/builtin/worktree.c b/builtin/worktree.c index 87ccd47794c..9b00dbf1265 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -211,27 +211,24 @@ static void prune_dups(struct string_list *l) static void prune_worktrees(void) { - struct strbuf reason = STRBUF_INIT; struct strbuf main_path = STRBUF_INIT; struct string_list kept = STRING_LIST_INIT_DUP; - char *path; - DIR *dir; - struct dirent *d; + struct strvec worktrees = STRVEC_INIT; + struct strbuf reason = STRBUF_INIT; - path = repo_git_path(the_repository, "worktrees"); - dir = opendir(path); - free(path); - if (!dir) + if (get_worktree_names(the_repository, &worktrees) < 0 || + !worktrees.nr) return; - while ((d = readdir_skip_dot_and_dotdot(dir)) != NULL) { + + for (size_t i = 0; i < worktrees.nr; i++) { char *path; + strbuf_reset(&reason); - if (should_prune_worktree(d->d_name, &reason, &path, expire)) - prune_worktree(d->d_name, reason.buf); + if (should_prune_worktree(worktrees.v[i], &reason, &path, expire)) + prune_worktree(worktrees.v[i], reason.buf); else if (path) - string_list_append_nodup(&kept, path)->util = xstrdup(d->d_name); + string_list_append_nodup(&kept, path)->util = xstrdup(worktrees.v[i]); } - closedir(dir); strbuf_add_absolute_path(&main_path, repo_get_common_dir(the_repository)); /* massage main worktree absolute path to match 'gitdir' content */ @@ -242,6 +239,8 @@ static void prune_worktrees(void) if (!show_only) delete_worktrees_dir_if_empty(); + + strvec_clear(&worktrees); strbuf_release(&reason); } diff --git a/worktree.c b/worktree.c index c34b9eb74e5..947b7a82209 100644 --- a/worktree.c +++ b/worktree.c @@ -988,6 +988,36 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, return rc; } +int get_worktree_names(struct repository *repo, struct strvec *out) +{ + char *worktrees_dir; + struct dirent *d; + DIR *dir; + int ret; + + worktrees_dir = repo_git_path(repo, "worktrees"); + dir = opendir(worktrees_dir); + if (!dir) { + if (errno == ENOENT) { + ret = 0; + goto out; + } + + ret = -1; + goto out; + } + + while ((d = readdir_skip_dot_and_dotdot(dir)) != NULL) + strvec_push(out, d->d_name); + + ret = 0; +out: + if (dir) + closedir(dir); + free(worktrees_dir); + return ret; +} + static int move_config_setting(const char *key, const char *value, const char *from_file, const char *to_file) { diff --git a/worktree.h b/worktree.h index e4bcccdc0ae..59825c37881 100644 --- a/worktree.h +++ b/worktree.h @@ -38,6 +38,14 @@ struct worktree **get_worktrees(void); */ struct worktree **get_worktrees_without_reading_head(void); +/* + * Retrieve all worktree names. Not all names may correspond to a fully + * functional worktree. Returns 0 on success, a negative error code on failure. + * Calling the function on a repository that doesn't have any worktrees is not + * considered an error. + */ +int get_worktree_names(struct repository *repo, struct strvec *out); + /* * Returns 1 if linked worktrees exist, 0 otherwise. */ -- 2.49.0.987.g0cc8ee98dc.dirty ``` ## Patrick Steinhardt, 2025-05-05 08:51 Subject: [PATCH v4 6/7] builtin/gc: move rerere garbage collection into separate function Message-ID: <20250505-pks-maintenance-missing-tasks-v4-6-141f4df906a1@pks.im> URL: https://gitlist.dev/e/20250505-pks-maintenance-missing-tasks-v4-6-141f4df906a1%40pks.im In-Reply-To: <20250505-pks-maintenance-missing-tasks-v4-0-141f4df906a1@pks.im> ``` Move garbage collection of cached rerere entries into a separate function. This prepares us for a subsequent commit where we introduce a new "rerere-gc" task for git-maintenance(1). Signed-off-by: Patrick Steinhardt --- builtin/gc.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index 93a8c856dbc..3dd1d07cca4 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -386,6 +386,15 @@ static int worktree_prune_condition(struct gc_config *cfg) return should_prune; } +static int maintenance_task_rerere_gc(struct maintenance_run_opts *opts UNUSED, + struct gc_config *cfg UNUSED) +{ + struct child_process rerere_cmd = CHILD_PROCESS_INIT; + rerere_cmd.git_cmd = 1; + strvec_pushl(&rerere_cmd.args, "rerere", "gc", NULL); + return run_command(&rerere_cmd); +} + static int too_many_loose_objects(struct gc_config *cfg) { /* @@ -782,7 +791,6 @@ int cmd_gc(int argc, int daemonized = 0; int keep_largest_pack = -1; timestamp_t dummy; - struct child_process rerere_cmd = CHILD_PROCESS_INIT; struct maintenance_run_opts opts = MAINTENANCE_RUN_OPTS_INIT; struct gc_config cfg = GC_CONFIG_INIT; const char *prune_expire_sentinel = "sentinel"; @@ -959,10 +967,8 @@ int cmd_gc(int argc, maintenance_task_worktree_prune(&opts, &cfg)) die(FAILED_RUN, "worktree"); - rerere_cmd.git_cmd = 1; - strvec_pushl(&rerere_cmd.args, "rerere", "gc", NULL); - if (run_command(&rerere_cmd)) - die(FAILED_RUN, rerere_cmd.args.v[0]); + if (maintenance_task_rerere_gc(&opts, &cfg)) + die(FAILED_RUN, "rerere"); report_garbage = report_pack_garbage; reprepare_packed_git(the_repository); -- 2.49.0.987.g0cc8ee98dc.dirty ``` ## Patrick Steinhardt, 2025-05-05 08:51 Subject: [PATCH v4 5/7] builtin/maintenance: introduce "worktree-prune" task Message-ID: <20250505-pks-maintenance-missing-tasks-v4-5-141f4df906a1@pks.im> URL: https://gitlist.dev/e/20250505-pks-maintenance-missing-tasks-v4-5-141f4df906a1%40pks.im In-Reply-To: <20250505-pks-maintenance-missing-tasks-v4-0-141f4df906a1@pks.im> ``` 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 --- Documentation/config/maintenance.adoc | 8 ++++ Documentation/git-maintenance.adoc | 4 ++ builtin/gc.c | 46 +++++++++++++++++++++++ t/t7900-maintenance.sh | 71 +++++++++++++++++++++++++++++++++++ 4 files changed, 129 insertions(+) diff --git a/Documentation/config/maintenance.adoc b/Documentation/config/maintenance.adoc index 41536162a77..b36b62c1c47 100644 --- a/Documentation/config/maintenance.adoc +++ b/Documentation/config/maintenance.adoc @@ -83,3 +83,11 @@ maintenance.reflog-expire.auto:: positive value implies the command should run when the number of expired reflog entries in the "HEAD" reflog is at least the value of `maintenance.loose-objects.auto`. The default value is 100. + +maintenance.worktree-prune.auto:: + This integer config option controls how often the `worktree-prune` task + should be run as part of `git maintenance run --auto`. If zero, then + the `worktree-prune` task will not run with the `--auto` option. A + negative value will force the task to run every time. Otherwise, a + positive value implies the command should run when the number of + prunable worktrees exceeds the value. The default value is 1. diff --git a/Documentation/git-maintenance.adoc b/Documentation/git-maintenance.adoc index 3a1e2a69b6b..6f085a9cf8c 100644 --- a/Documentation/git-maintenance.adoc +++ b/Documentation/git-maintenance.adoc @@ -166,6 +166,10 @@ reflog-expire:: The `reflog-expire` task deletes any entries in the reflog older than the expiry threshold. See linkgit:git-reflog[1] for more information. +worktree-prune:: + The `worktree-prune` task deletes stale or broken worktrees. See + linkit:git-worktree[1] for more information. + OPTIONS ------- --auto:: diff --git a/builtin/gc.c b/builtin/gc.c index 005ecc3f192..93a8c856dbc 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -44,6 +44,7 @@ #include "hook.h" #include "setup.h" #include "trace2.h" +#include "worktree.h" #define FAILED_RUN "failed to run %s" @@ -346,6 +347,45 @@ static int maintenance_task_worktree_prune(struct maintenance_run_opts *opts UNU return run_command(&prune_worktrees_cmd); } +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; + int limit = 1; + + 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)) { + limit--; + + if (!limit) { + should_prune = 1; + goto out; + } + } + free(wtpath); + } + +out: + strvec_clear(&worktrees); + strbuf_release(&reason); + return should_prune; +} + static int too_many_loose_objects(struct gc_config *cfg) { /* @@ -1455,6 +1495,7 @@ enum maintenance_task_label { TASK_COMMIT_GRAPH, TASK_PACK_REFS, TASK_REFLOG_EXPIRE, + TASK_WORKTREE_PRUNE, /* Leave as final value */ TASK__COUNT @@ -1496,6 +1537,11 @@ static struct maintenance_task tasks[] = { maintenance_task_reflog_expire, reflog_expire_condition, }, + [TASK_WORKTREE_PRUNE] = { + "worktree-prune", + maintenance_task_worktree_prune, + worktree_prune_condition, + }, }; static int compare_tasks_by_selection(const void *a_, const void *b_) diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 9b82e11c100..530c56ae91e 100755 --- 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_subcommand git reflog expire --all .git/worktrees/abc && + test_expect_worktree_prune git maintenance run --auto --task=worktree-prune +' + +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. + test_expect_worktree_prune ! git -c maintenance.worktree-prune.auto=4 maintenance run --auto --task=worktree-prune && + test_expect_worktree_prune git -c maintenance.worktree-prune.auto=3 maintenance run --auto --task=worktree-prune +' + +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 many prunable worktrees. + test_expect_worktree_prune ! git -c maintenance.worktree-prune.auto=4 maintenance run --auto --task=worktree-prune && + test_expect_worktree_prune git -c maintenance.worktree-prune.auto=3 maintenance run --auto --task=worktree-prune +' + +test_expect_success 'worktree-prune task honors gc.worktreePruneExpire' ' + git worktree add worktree && + rm -rf worktree && + + rm -f worktree-prune.txt && + GIT_TRACE2_EVENT="$(pwd)/worktree-prune.txt" git -c gc.worktreePruneExpire=1.week.ago maintenance run --auto --task=worktree-prune && + test_subcommand ! git worktree prune --expire 1.week.ago err && test_grep "at most one" err -- 2.49.0.987.g0cc8ee98dc.dirty ``` ## Patrick Steinhardt, 2025-05-05 08:51 Subject: [PATCH v4 7/7] builtin/maintenance: introduce "rerere-gc" task Message-ID: <20250505-pks-maintenance-missing-tasks-v4-7-141f4df906a1@pks.im> URL: https://gitlist.dev/e/20250505-pks-maintenance-missing-tasks-v4-7-141f4df906a1%40pks.im In-Reply-To: <20250505-pks-maintenance-missing-tasks-v4-0-141f4df906a1@pks.im> ``` While git-gc(1) knows to garbage collect the rerere cache, git-maintenance(1) does not yet have a task for this cleanup. Introduce a new "rerere-gc" task to plug this gap. Signed-off-by: Patrick Steinhardt --- Documentation/config/maintenance.adoc | 9 +++++++ Documentation/git-maintenance.adoc | 4 ++++ builtin/gc.c | 37 +++++++++++++++++++++++++++++ t/t7900-maintenance.sh | 44 +++++++++++++++++++++++++++++++++++ 4 files changed, 94 insertions(+) diff --git a/Documentation/config/maintenance.adoc b/Documentation/config/maintenance.adoc index b36b62c1c47..2f719342183 100644 --- a/Documentation/config/maintenance.adoc +++ b/Documentation/config/maintenance.adoc @@ -84,6 +84,15 @@ maintenance.reflog-expire.auto:: expired reflog entries in the "HEAD" reflog is at least the value of `maintenance.loose-objects.auto`. The default value is 100. +maintenance.rerere-gc.auto:: + This integer config option controls how often the `rerere-gc` task + should be run as part of `git maintenance run --auto`. If zero, then + the `rerere-gc` task will not run with the `--auto` option. A negative + value will force the task to run every time. Otherwise, any positive + value implies the command will run when the "rr-cache" directory exists + and has at least one entry, regardless of whether it is stale or not. + This heuristic may be refined in the future. The default value is 1. + maintenance.worktree-prune.auto:: This integer config option controls how often the `worktree-prune` task should be run as part of `git maintenance run --auto`. If zero, then diff --git a/Documentation/git-maintenance.adoc b/Documentation/git-maintenance.adoc index 6f085a9cf8c..931f3e02e85 100644 --- a/Documentation/git-maintenance.adoc +++ b/Documentation/git-maintenance.adoc @@ -166,6 +166,10 @@ reflog-expire:: The `reflog-expire` task deletes any entries in the reflog older than the expiry threshold. See linkgit:git-reflog[1] for more information. +rerere-gc:: + The `rerere-gc` task invokes garbage collection for stale entries in + the rerere cache. See linkgit:git-rerere[1] for more information. + worktree-prune:: The `worktree-prune` task deletes stale or broken worktrees. See linkit:git-worktree[1] for more information. diff --git a/builtin/gc.c b/builtin/gc.c index 3dd1d07cca4..0d4a70c7804 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -16,6 +16,7 @@ #include "builtin.h" #include "abspath.h" #include "date.h" +#include "dir.h" #include "environment.h" #include "hex.h" #include "config.h" @@ -34,6 +35,7 @@ #include "pack-objects.h" #include "path.h" #include "reflog.h" +#include "rerere.h" #include "blob.h" #include "tree.h" #include "promisor-remote.h" @@ -395,6 +397,35 @@ static int maintenance_task_rerere_gc(struct maintenance_run_opts *opts UNUSED, return run_command(&rerere_cmd); } +static int rerere_gc_condition(struct gc_config *cfg UNUSED) +{ + struct strbuf path = STRBUF_INIT; + int should_gc = 0, limit = 1; + DIR *dir = NULL; + + git_config_get_int("maintenance.rerere-gc.auto", &limit); + if (limit <= 0) { + should_gc = limit < 0; + goto out; + } + + /* + * We skip garbage collection in case we either have no "rr-cache" + * directory or when it doesn't contain at least one entry. + */ + repo_git_path_replace(the_repository, &path, "rr-cache"); + dir = opendir(path.buf); + if (!dir) + goto out; + should_gc = !!readdir_skip_dot_and_dotdot(dir); + +out: + strbuf_release(&path); + if (dir) + closedir(dir); + return should_gc; +} + static int too_many_loose_objects(struct gc_config *cfg) { /* @@ -1502,6 +1533,7 @@ enum maintenance_task_label { TASK_PACK_REFS, TASK_REFLOG_EXPIRE, TASK_WORKTREE_PRUNE, + TASK_RERERE_GC, /* Leave as final value */ TASK__COUNT @@ -1548,6 +1580,11 @@ static struct maintenance_task tasks[] = { maintenance_task_worktree_prune, worktree_prune_condition, }, + [TASK_RERERE_GC] = { + "rerere-gc", + maintenance_task_rerere_gc, + rerere_gc_condition, + }, }; static int compare_tasks_by_selection(const void *a_, const void *b_) diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 530c56ae91e..af02a551869 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -564,6 +564,50 @@ test_expect_success 'worktree-prune task honors gc.worktreePruneExpire' ' test_path_is_missing .git/worktrees/worktree ' +test_expect_rerere_gc () { + negate= + if test "$1" = "!" + then + negate="!" + shift + fi + + rm -f "rerere-gc.txt" && + GIT_TRACE2_EVENT="$(pwd)/rerere-gc.txt" "$@" && + test_subcommand $negate git rerere gc .git/rr-cache/entry && + test_expect_rerere_gc git maintenance run --auto --task=rerere-gc +' + +test_expect_success 'rerere-gc task with --auto honors maintenance.rerere-gc.auto' ' + test_when_finished "rm -rf .git/rr-cache" && + + # A negative value should always prune. + test_expect_rerere_gc git -c maintenance.rerere-gc.auto=-1 maintenance run --auto --task=rerere-gc && + + # A positive value prunes when there is at least one entry. + test_expect_rerere_gc ! git -c maintenance.rerere-gc.auto=9000 maintenance run --auto --task=rerere-gc && + mkdir .git/rr-cache && + test_expect_rerere_gc ! git -c maintenance.rerere-gc.auto=9000 maintenance run --auto --task=rerere-gc && + : >.git/rr-cache/entry-1 && + test_expect_rerere_gc git -c maintenance.rerere-gc.auto=9000 maintenance run --auto --task=rerere-gc && + + # Zero should never prune. + : >.git/rr-cache/entry-1 && + test_expect_rerere_gc ! git -c maintenance.rerere-gc.auto=0 maintenance run --auto --task=rerere-gc +' + test_expect_success '--auto and --schedule incompatible' ' test_must_fail git maintenance run --auto --schedule=daily 2>err && test_grep "at most one" err -- 2.49.0.987.g0cc8ee98dc.dirty ``` ## Christian Couder, 2025-05-06 07:40 Subject: Re: [PATCH v4 5/7] builtin/maintenance: introduce "worktree-prune" task Message-ID: URL: https://gitlist.dev/e/CAP8UFD2cuMxdpGFuBoKirw2mko6gt-djndqqZy92NypPiiFCYQ%40mail.gmail.com In-Reply-To: <20250505-pks-maintenance-missing-tasks-v4-5-141f4df906a1@pks.im> ``` On Mon, May 5, 2025 at 10:52 AM Patrick Steinhardt wrote: > +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; > + int limit = 1; > + > + 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)) { > + limit--; > + > + if (!limit) { > + should_prune = 1; > + goto out; Eric noticed in a previous round that wtpath is leaked in this `goto out` path, and it seems to me that it's still the case. > + } > + } > + free(wtpath); > + } > + > +out: > + strvec_clear(&worktrees); > + strbuf_release(&reason); > + return should_prune; > +} ``` ## Christian Couder, 2025-05-06 07:44 Subject: Re: [PATCH v4 2/7] builtin/gc: remove global variables where it trivial to do Message-ID: URL: https://gitlist.dev/e/CAP8UFD2BDv%3Dkp7W6w%3DJ00iRAj-Jqz_EjHGfH_YpNTEWC2a82QA%40mail.gmail.com In-Reply-To: <20250505-pks-maintenance-missing-tasks-v4-2-141f4df906a1@pks.im> ``` On Mon, May 5, 2025 at 10:52 AM Patrick Steinhardt wrote: > > We use a couple of global variables to assemble command line arguments > for subprocesses we execute in git-gc(1). All of these variables except > the one for git-repack(1) are only used in a single place though, so > they don't really add anything but confusion. > > Remove those variables. About the commit message it seems to me that it's missing "is", so maybe: "builtin/gc: remove global variables where it's trivial to do" or just: "builtin/gc: remove global variables where trivial to do" ? ``` ## Christian Couder, 2025-05-06 07:50 Subject: Re: [PATCH v4 3/7] builtin/gc: move pruning of worktrees into a separate function Message-ID: URL: https://gitlist.dev/e/CAP8UFD1ZtYMf0ja75AV_jGJgzAL919zpHoGy0v-G2tNV%2BOEDwA%40mail.gmail.com In-Reply-To: <20250505-pks-maintenance-missing-tasks-v4-3-141f4df906a1@pks.im> ``` On Mon, May 5, 2025 at 10:52 AM Patrick Steinhardt wrote: > > Move pruning of worktrees into a separate function. Repeating the subject at the beginning of the body part of the commit message doesn't bring much value. > This prepares for a > subsequent commit where we introduce a new "worktree-prune" task for > git-maintenance(1). I think it might help a bit to reword in the usual "describe the problem first and then say what to do about it" way. Maybe something like: "In a subsequent commit we are going to introduce a new "worktree-prune" task for git-maintenance(1). To prepare for this, refactor the code that prunes worktrees into a separate function." ``` ## Christian Couder, 2025-05-06 08:20 Subject: Re: [PATCH v4 4/7] worktree: expose function to retrieve worktree names Message-ID: URL: https://gitlist.dev/e/CAP8UFD24jwKjP6XdD_SSv7ukgyqewCThMd-wQSRsDk00X7btiQ%40mail.gmail.com In-Reply-To: <20250505-pks-maintenance-missing-tasks-v4-4-141f4df906a1@pks.im> ``` On Mon, May 5, 2025 at 10:52 AM Patrick Steinhardt wrote: > > Introduce a function that retrieves worktree names as present in > ".git/worktrees". This function will be used in a subsequent commit. > > Signed-off-by: Patrick Steinhardt > --- > builtin/worktree.c | 25 ++++++++++++------------- > worktree.c | 30 ++++++++++++++++++++++++++++++ > worktree.h | 8 ++++++++ > 3 files changed, 50 insertions(+), 13 deletions(-) > > diff --git a/builtin/worktree.c b/builtin/worktree.c > index 87ccd47794c..9b00dbf1265 100644 > --- a/builtin/worktree.c > +++ b/builtin/worktree.c > @@ -211,27 +211,24 @@ static void prune_dups(struct string_list *l) > > static void prune_worktrees(void) > { > - struct strbuf reason = STRBUF_INIT; > struct strbuf main_path = STRBUF_INIT; > struct string_list kept = STRING_LIST_INIT_DUP; > - char *path; > - DIR *dir; > - struct dirent *d; > + struct strvec worktrees = STRVEC_INIT; > + struct strbuf reason = STRBUF_INIT; > > - path = repo_git_path(the_repository, "worktrees"); > - dir = opendir(path); > - free(path); > - if (!dir) > + if (get_worktree_names(the_repository, &worktrees) < 0 || > + !worktrees.nr) > return; > - while ((d = readdir_skip_dot_and_dotdot(dir)) != NULL) { > + > + for (size_t i = 0; i < worktrees.nr; i++) { > char *path; > + > strbuf_reset(&reason); > - if (should_prune_worktree(d->d_name, &reason, &path, expire)) > - prune_worktree(d->d_name, reason.buf); > + if (should_prune_worktree(worktrees.v[i], &reason, &path, expire)) > + prune_worktree(worktrees.v[i], reason.buf); > else if (path) > - string_list_append_nodup(&kept, path)->util = xstrdup(d->d_name); > + string_list_append_nodup(&kept, path)->util = xstrdup(worktrees.v[i]); > } > - closedir(dir); > > strbuf_add_absolute_path(&main_path, repo_get_common_dir(the_repository)); > /* massage main worktree absolute path to match 'gitdir' content */ > @@ -242,6 +239,8 @@ static void prune_worktrees(void) > > if (!show_only) > delete_worktrees_dir_if_empty(); > + > + strvec_clear(&worktrees); > strbuf_release(&reason); > } > > diff --git a/worktree.c b/worktree.c > index c34b9eb74e5..947b7a82209 100644 > --- a/worktree.c > +++ b/worktree.c > @@ -988,6 +988,36 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, > return rc; > } > > +int get_worktree_names(struct repository *repo, struct strvec *out) > +{ > + char *worktrees_dir; > + struct dirent *d; > + DIR *dir; > + int ret; > + > + worktrees_dir = repo_git_path(repo, "worktrees"); > + dir = opendir(worktrees_dir); > + if (!dir) { > + if (errno == ENOENT) { > + ret = 0; > + goto out; > + } > + > + ret = -1; > + goto out; > + } > + > + while ((d = readdir_skip_dot_and_dotdot(dir)) != NULL) > + strvec_push(out, d->d_name); > + > + ret = 0; > +out: > + if (dir) > + closedir(dir); > + free(worktrees_dir); > + return ret; > +} Nit: this function seems to use "goto out" a bit too much for me. What about something like: int get_worktree_names(struct repository *repo, struct strvec *out) { int ret = 0; char *worktrees_dir = repo_git_path(repo, "worktrees"); DIR *dir = opendir(worktrees_dir); if (dir) { struct dirent *d; while ((d = readdir_skip_dot_and_dotdot(dir)) != NULL) strvec_push(out, d->d_name); closedir(dir); } else { if (errno != ENOENT) ret = -1; } free(worktrees_dir); return ret; } ? ``` ## Christian Couder, 2025-05-06 08:39 Subject: Re: [PATCH v4 6/7] builtin/gc: move rerere garbage collection into separate function Message-ID: URL: https://gitlist.dev/e/CAP8UFD0cKmL9e18bNGXOd0taahrK4Xx3CG%2BZ6BqDovAxbq0sYA%40mail.gmail.com In-Reply-To: <20250505-pks-maintenance-missing-tasks-v4-6-141f4df906a1@pks.im> ``` On Mon, May 5, 2025 at 10:52 AM Patrick Steinhardt wrote: > > Move garbage collection of cached rerere entries into a separate > function. Nit: this is also repeating the commit message subject nearly as-is. > This prepares us for a subsequent commit where we introduce a > new "rerere-gc" task for git-maintenance(1). So maybe something like: "In a subsequent commit we are going to introduce a new "rerere-gc" task for git-maintenance(1). To prepare for this, refactor the code that garbage collects cached rerere entries into a separate function." ``` ## Christian Couder, 2025-05-06 09:05 Subject: Re: [PATCH v4 0/7] builtin/maintenance: implement missing tasks compared to git-gc(1) Message-ID: URL: https://gitlist.dev/e/CAP8UFD0twBxZvEiLq8OBi2xhT%2BBnbL2k7swqru5CTQ67rdYM3Q%40mail.gmail.com In-Reply-To: <20250505-pks-maintenance-missing-tasks-v4-0-141f4df906a1@pks.im> ``` On Mon, May 5, 2025 at 10:52 AM Patrick Steinhardt wrote: > Changes in v4: > - simplified the heuristic for "rerere-gc" even further. A positive > value for "maintenance.rerere-gc.auto" now indicates that the > command will run whenever there is at least one directory entry in > ".rr-cache". The exact value does not matter anymore. > - Link to v3: https://lore.kernel.org/r/20250502-pks-maintenance-missing-tasks-v3-0-13e130d36640@pks.im Except for a few nits I commented on in my replies to specific patches, and except that some comments from Eric Sunshine in https://lore.kernel.org/git/CAPig+cScor=E2i4w99NCrsaUd-Po=FqhkrLSyT69PSo1+h2dRw@mail.gmail.com/#t seems to have been overlooked (especially one about a memory leak), this looks good to me. Thanks! ``` ## Eric Sunshine, 2025-05-06 16:08 Subject: Re: [PATCH v4 4/7] worktree: expose function to retrieve worktree names Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSCFZArGbAXe77H3__nUAt1BTwDRsM6V2emqaMvHdxVgg%40mail.gmail.com In-Reply-To: ``` On Tue, May 6, 2025 at 4:23 AM Christian Couder wrote: > On Mon, May 5, 2025 at 10:52 AM Patrick Steinhardt wrote: > > +int get_worktree_names(struct repository *repo, struct strvec *out) > > +{ > > + worktrees_dir = repo_git_path(repo, "worktrees"); > > + dir = opendir(worktrees_dir); > > + if (!dir) { > > + if (errno == ENOENT) { > > + ret = 0; > > + goto out; > > + } > > + > > + ret = -1; > > + goto out; > > + } > > + > > + while ((d = readdir_skip_dot_and_dotdot(dir)) != NULL) > > + strvec_push(out, d->d_name); > > + > > + ret = 0; > > +out: > > + if (dir) > > + closedir(dir); > > + free(worktrees_dir); > > + return ret; > > +} > > Nit: this function seems to use "goto out" a bit too much for me. What > about something like: > > int get_worktree_names(struct repository *repo, struct strvec *out) > { > int ret = 0; > char *worktrees_dir = repo_git_path(repo, "worktrees"); > DIR *dir = opendir(worktrees_dir); > > if (dir) { > struct dirent *d; > while ((d = readdir_skip_dot_and_dotdot(dir)) != NULL) > strvec_push(out, d->d_name); > closedir(dir); > } else { > if (errno != ENOENT) > ret = -1; > } > > free(worktrees_dir); > return ret; > } I had the same response and made a slightly more concise proposal[*], though I'm not convinced that this function should exist at all. [*]: https://lore.kernel.org/git/CAPig+cSDDbhGrym8j=PFKBCUxBQhZPzAHXGvKy-Z6POA4Ju3sw@mail.gmail.com/ ``` ## Patrick Steinhardt, 2025-05-07 07:06 Subject: Re: [PATCH v4 2/7] builtin/gc: remove global variables where it trivial to do Message-ID: URL: https://gitlist.dev/e/aBsGhpiLaYA8M351%40pks.im In-Reply-To: ``` On Tue, May 06, 2025 at 09:44:03AM +0200, Christian Couder wrote: > On Mon, May 5, 2025 at 10:52 AM Patrick Steinhardt wrote: > > > > We use a couple of global variables to assemble command line arguments > > for subprocesses we execute in git-gc(1). All of these variables except > > the one for git-repack(1) are only used in a single place though, so > > they don't really add anything but confusion. > > > > Remove those variables. > > About the commit message it seems to me that it's missing "is", so maybe: > > "builtin/gc: remove global variables where it's trivial to do" > > or just: > > "builtin/gc: remove global variables where trivial to do" > > ? Ah, indeed. Patrick ``` ## Patrick Steinhardt, 2025-05-07 07:06 Subject: Re: [PATCH v4 3/7] builtin/gc: move pruning of worktrees into a separate function Message-ID: URL: https://gitlist.dev/e/aBsGiu3IBIUuYuYU%40pks.im In-Reply-To: ``` On Tue, May 06, 2025 at 09:50:12AM +0200, Christian Couder wrote: > On Mon, May 5, 2025 at 10:52 AM Patrick Steinhardt wrote: > > > > Move pruning of worktrees into a separate function. > > Repeating the subject at the beginning of the body part of the commit > message doesn't bring much value. I disagree with that statement. The way I review commit messages the subject is often detached from the body, so it always helps me quite a bit when the body is fully self-contained and doesn't depend on the subject. > > This prepares for a > > subsequent commit where we introduce a new "worktree-prune" task for > > git-maintenance(1). > > I think it might help a bit to reword in the usual "describe the > problem first and then say what to do about it" way. Maybe something > like: > > "In a subsequent commit we are going to introduce a new > "worktree-prune" task for git-maintenance(1). > > To prepare for this, refactor the code that prunes worktrees into a > separate function." But this reads better regardless of that, so I'll take it. Thanks! Patrick ``` ## Patrick Steinhardt, 2025-05-07 07:06 Subject: Re: [PATCH v4 5/7] builtin/maintenance: introduce "worktree-prune" task Message-ID: URL: https://gitlist.dev/e/aBsGjVqYr2jY6oJ6%40pks.im In-Reply-To: ``` On Tue, May 06, 2025 at 09:40:55AM +0200, Christian Couder wrote: > On Mon, May 5, 2025 at 10:52 AM Patrick Steinhardt wrote: > > > +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; > > + int limit = 1; > > + > > + 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)) { > > + limit--; > > + > > + if (!limit) { > > + should_prune = 1; > > + goto out; > > Eric noticed in a previous round that wtpath is leaked in this `goto > out` path, and it seems to me that it's still the case. Yeah, our mails had crossed back then. Fixed now. Patrick ``` ## Patrick Steinhardt, 2025-05-07 07:21 Subject: [PATCH v5 0/6] builtin/maintenance: implement missing tasks compared to git-gc(1) Message-ID: <20250507-pks-maintenance-missing-tasks-v5-0-aa5fdfb82891@pks.im> URL: https://gitlist.dev/e/20250507-pks-maintenance-missing-tasks-v5-0-aa5fdfb82891%40pks.im In-Reply-To: <20250425-pks-maintenance-missing-tasks-v1-0-972ed6ab2c0d@pks.im> ``` Hi, this small patch series implements the last couple of remaining tasks that are missing compared to the functionality git-gc(1) provides. Right now, git-maintenance(1) still executes git-gc(1). With these last gaps plugged though we can in theory fully replace git-gc(1) with finer grained tasks without losing any functionality. The benefit is that it becomes possible for users to have finer-grained control over what exactly the maintenance does. This patch series doesn't do that yet, but only implements whatever is needed to get there. Changes in v2: - Introduce "maintenance.worktree-prune.auto", which controls how many stale worktrees need to exist before executing `git worktree prune`. - Introduce "maintenance.rerere-gc.auto", which controls how many stale rerere entries need to exist before executing `git rerere gc`. - Add tests to verify that "gc.worktreePruneExpire" works. - Remove some fragile test logic by introducing functions that check for a given maintenance subprocess. - Link to v1: https://lore.kernel.org/r/20250425-pks-maintenance-missing-tasks-v1-0-972ed6ab2c0d@pks.im Changes in v3: - Simplify the heuristic for "rerere-gc" so that we only count the number of directory entries in ".git/rr-cache", without considering staleness. - Link to v2: https://lore.kernel.org/r/20250430-pks-maintenance-missing-tasks-v2-0-2580b7b8ca3a@pks.im Changes in v4: - simplified the heuristic for "rerere-gc" even further. A positive value for "maintenance.rerere-gc.auto" now indicates that the command will run whenever there is at least one directory entry in ".rr-cache". The exact value does not matter anymore. - Link to v3: https://lore.kernel.org/r/20250502-pks-maintenance-missing-tasks-v3-0-13e130d36640@pks.im Changes in v5: - Drop `get_worktree_names()` in favor of an open-coded loop. - Fix a memory leak. - Simplified the logic in `worktree_prune_condition()` a bit. - Link to v4: https://lore.kernel.org/r/20250505-pks-maintenance-missing-tasks-v4-0-141f4df906a1@pks.im Thanks! Patrick --- Patrick Steinhardt (6): builtin/gc: fix indentation of `cmd_gc()` parameters builtin/gc: remove global variables where it is trivial to do builtin/gc: move pruning of worktrees into a separate function builtin/maintenance: introduce "worktree-prune" task builtin/gc: move rerere garbage collection into separate function builtin/maintenance: introduce "rerere-gc" task Documentation/config/maintenance.adoc | 17 ++++ Documentation/git-maintenance.adoc | 8 ++ builtin/gc.c | 148 +++++++++++++++++++++++++++------- t/t7900-maintenance.sh | 115 ++++++++++++++++++++++++++ 4 files changed, 257 insertions(+), 31 deletions(-) Range-diff versus v4: 1: 59edf54e3ec = 1: 815904a68a1 builtin/gc: fix indentation of `cmd_gc()` parameters 2: 9f02c33f5b9 ! 2: 91f4c304232 builtin/gc: remove global variables where it trivial to do @@ Metadata Author: Patrick Steinhardt ## Commit message ## - builtin/gc: remove global variables where it trivial to do + builtin/gc: remove global variables where it is trivial to do We use a couple of global variables to assemble command line arguments for subprocesses we execute in git-gc(1). All of these variables except 3: b280af7bbc4 ! 3: 9232c8aac1d builtin/gc: move pruning of worktrees into a separate function @@ Metadata ## Commit message ## builtin/gc: move pruning of worktrees into a separate function - Move pruning of worktrees into a separate function. This prepares for a - subsequent commit where we introduce a new "worktree-prune" task for - git-maintenance(1). + In a subsequent commit we will introduce a new "worktree-prune" task for + git-maintenance(1). To prepare for this, refactor the code that spawns + `git worktree prune` into a separate function. Signed-off-by: Patrick Steinhardt 4: 58ce12459c2 < -: ----------- worktree: expose function to retrieve worktree names 5: efeec465db0 ! 4: 24ca70b35b9 builtin/maintenance: introduce "worktree-prune" task @@ builtin/gc.c: static int maintenance_task_worktree_prune(struct maintenance_run_ +static int worktree_prune_condition(struct gc_config *cfg) +{ -+ struct strvec worktrees = STRVEC_INIT; -+ struct strbuf reason = STRBUF_INIT; ++ struct strbuf buf = STRBUF_INIT; ++ int should_prune = 0, limit = 1; + timestamp_t expiry_date; -+ int should_prune = 0; -+ int limit = 1; ++ struct dirent *d; ++ DIR *dir = NULL; + + git_config_get_int("maintenance.worktree-prune.auto", &limit); + if (limit <= 0) { @@ builtin/gc.c: static int maintenance_task_worktree_prune(struct maintenance_run_ + goto out; + } + -+ if (parse_expiry_date(cfg->prune_worktrees_expire, &expiry_date) || -+ get_worktree_names(the_repository, &worktrees) < 0) ++ if (parse_expiry_date(cfg->prune_worktrees_expire, &expiry_date)) + goto out; + -+ for (size_t i = 0; i < worktrees.nr; i++) { -+ char *wtpath; ++ dir = opendir(repo_git_path_replace(the_repository, &buf, "worktrees")); ++ if (!dir) ++ goto out; + -+ strbuf_reset(&reason); -+ if (should_prune_worktree(worktrees.v[i], &reason, &wtpath, expiry_date)) { ++ while (limit && (d = readdir_skip_dot_and_dotdot(dir))) { ++ char *wtpath; ++ strbuf_reset(&buf); ++ if (should_prune_worktree(d->d_name, &buf, &wtpath, expiry_date)) + limit--; -+ -+ if (!limit) { -+ should_prune = 1; -+ goto out; -+ } -+ } + free(wtpath); + } + ++ should_prune = !limit; ++ +out: -+ strvec_clear(&worktrees); -+ strbuf_release(&reason); ++ if (dir) ++ closedir(dir); ++ strbuf_release(&buf); + return should_prune; +} + @@ t/t7900-maintenance.sh: test_expect_success 'reflog-expire task --auto only pack + + # 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. ++ # A positive value should require at least this many prunable worktrees. + test_expect_worktree_prune ! git -c maintenance.worktree-prune.auto=4 maintenance run --auto --task=worktree-prune && + test_expect_worktree_prune git -c maintenance.worktree-prune.auto=3 maintenance run --auto --task=worktree-prune +' 6: 10ed12cc737 ! 5: 67e05501fbb builtin/gc: move rerere garbage collection into separate function @@ Metadata ## Commit message ## builtin/gc: move rerere garbage collection into separate function - Move garbage collection of cached rerere entries into a separate - function. This prepares us for a subsequent commit where we introduce a - new "rerere-gc" task for git-maintenance(1). + In a subsequent commit we are going to introduce a new "rerere-gc" task + for git-maintenance(1). To prepare for this, refactor the code that + spawns `git rerere gc` into a separate function. Signed-off-by: Patrick Steinhardt 7: be7fa13115c = 6: 818ed6b8999 builtin/maintenance: introduce "rerere-gc" task --- base-commit: a2955b34f48265d240ab8c7deb0a929ec2d65fd0 change-id: 20250424-pks-maintenance-missing-tasks-8ffcdd596b73 ``` ## Patrick Steinhardt, 2025-05-07 07:21 Subject: [PATCH v5 1/6] builtin/gc: fix indentation of `cmd_gc()` parameters Message-ID: <20250507-pks-maintenance-missing-tasks-v5-1-aa5fdfb82891@pks.im> URL: https://gitlist.dev/e/20250507-pks-maintenance-missing-tasks-v5-1-aa5fdfb82891%40pks.im In-Reply-To: <20250507-pks-maintenance-missing-tasks-v5-0-aa5fdfb82891@pks.im> ``` The parameters of `cmd_gc()` aren't indented properly. Fix this. Signed-off-by: Patrick Steinhardt --- builtin/gc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index d5c75be2522..a73ec22fb18 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -724,9 +724,9 @@ static void gc_before_repack(struct maintenance_run_opts *opts, } int cmd_gc(int argc, -const char **argv, -const char *prefix, -struct repository *repo UNUSED) + const char **argv, + const char *prefix, + struct repository *repo UNUSED) { int aggressive = 0; int quiet = 0; -- 2.49.0.1045.g170613ef41.dirty ``` ## Patrick Steinhardt, 2025-05-07 07:21 Subject: [PATCH v5 2/6] builtin/gc: remove global variables where it is trivial to do Message-ID: <20250507-pks-maintenance-missing-tasks-v5-2-aa5fdfb82891@pks.im> URL: https://gitlist.dev/e/20250507-pks-maintenance-missing-tasks-v5-2-aa5fdfb82891%40pks.im In-Reply-To: <20250507-pks-maintenance-missing-tasks-v5-0-aa5fdfb82891@pks.im> ``` We use a couple of global variables to assemble command line arguments for subprocesses we execute in git-gc(1). All of these variables except the one for git-repack(1) are only used in a single place though, so they don't really add anything but confusion. Remove those variables. Signed-off-by: Patrick Steinhardt --- builtin/gc.c | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index a73ec22fb18..ada36e210f0 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -53,15 +53,9 @@ static const char * const builtin_gc_usage[] = { }; static timestamp_t gc_log_expire_time; - static struct strvec repack = STRVEC_INIT; -static struct strvec prune = STRVEC_INIT; -static struct strvec prune_worktrees = STRVEC_INIT; -static struct strvec rerere = STRVEC_INIT; - static struct tempfile *pidfile; static struct lock_file log_lock; - static struct string_list pack_garbage = STRING_LIST_INIT_DUP; static void clean_pack_garbage(void) @@ -769,9 +763,6 @@ int cmd_gc(int argc, builtin_gc_usage, builtin_gc_options); strvec_pushl(&repack, "repack", "-d", "-l", NULL); - strvec_pushl(&prune, "prune", "--expire", NULL); - strvec_pushl(&prune_worktrees, "worktree", "prune", "--expire", NULL); - strvec_pushl(&rerere, "rerere", "gc", NULL); gc_config(&cfg); @@ -897,34 +888,36 @@ int cmd_gc(int argc, if (cfg.prune_expire) { struct child_process prune_cmd = CHILD_PROCESS_INIT; + strvec_pushl(&prune_cmd.args, "prune", "--expire", NULL); /* run `git prune` even if using cruft packs */ - strvec_push(&prune, cfg.prune_expire); + strvec_push(&prune_cmd.args, cfg.prune_expire); if (quiet) - strvec_push(&prune, "--no-progress"); + strvec_push(&prune_cmd.args, "--no-progress"); if (repo_has_promisor_remote(the_repository)) - strvec_push(&prune, + strvec_push(&prune_cmd.args, "--exclude-promisor-objects"); prune_cmd.git_cmd = 1; - strvec_pushv(&prune_cmd.args, prune.v); + if (run_command(&prune_cmd)) - die(FAILED_RUN, prune.v[0]); + die(FAILED_RUN, prune_cmd.args.v[0]); } } if (cfg.prune_worktrees_expire) { struct child_process prune_worktrees_cmd = CHILD_PROCESS_INIT; - strvec_push(&prune_worktrees, cfg.prune_worktrees_expire); prune_worktrees_cmd.git_cmd = 1; - strvec_pushv(&prune_worktrees_cmd.args, prune_worktrees.v); + strvec_pushl(&prune_worktrees_cmd.args, "worktree", "prune", "--expire", NULL); + strvec_push(&prune_worktrees_cmd.args, cfg.prune_worktrees_expire); + if (run_command(&prune_worktrees_cmd)) - die(FAILED_RUN, prune_worktrees.v[0]); + die(FAILED_RUN, prune_worktrees_cmd.args.v[0]); } rerere_cmd.git_cmd = 1; - strvec_pushv(&rerere_cmd.args, rerere.v); + strvec_pushl(&rerere_cmd.args, "rerere", "gc", NULL); if (run_command(&rerere_cmd)) - die(FAILED_RUN, rerere.v[0]); + die(FAILED_RUN, rerere_cmd.args.v[0]); report_garbage = report_pack_garbage; reprepare_packed_git(the_repository); -- 2.49.0.1045.g170613ef41.dirty ``` ## Patrick Steinhardt, 2025-05-07 07:21 Subject: [PATCH v5 3/6] builtin/gc: move pruning of worktrees into a separate function Message-ID: <20250507-pks-maintenance-missing-tasks-v5-3-aa5fdfb82891@pks.im> URL: https://gitlist.dev/e/20250507-pks-maintenance-missing-tasks-v5-3-aa5fdfb82891%40pks.im In-Reply-To: <20250507-pks-maintenance-missing-tasks-v5-0-aa5fdfb82891@pks.im> ``` In a subsequent commit we will introduce a new "worktree-prune" task for git-maintenance(1). To prepare for this, refactor the code that spawns `git worktree prune` into a separate function. Signed-off-by: Patrick Steinhardt --- builtin/gc.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index ada36e210f0..005ecc3f192 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -334,6 +334,18 @@ static int maintenance_task_reflog_expire(struct maintenance_run_opts *opts UNUS return run_command(&cmd); } +static int maintenance_task_worktree_prune(struct maintenance_run_opts *opts UNUSED, + struct gc_config *cfg) +{ + struct child_process prune_worktrees_cmd = CHILD_PROCESS_INIT; + + prune_worktrees_cmd.git_cmd = 1; + strvec_pushl(&prune_worktrees_cmd.args, "worktree", "prune", "--expire", NULL); + strvec_push(&prune_worktrees_cmd.args, cfg->prune_worktrees_expire); + + return run_command(&prune_worktrees_cmd); +} + static int too_many_loose_objects(struct gc_config *cfg) { /* @@ -903,16 +915,9 @@ int cmd_gc(int argc, } } - if (cfg.prune_worktrees_expire) { - struct child_process prune_worktrees_cmd = CHILD_PROCESS_INIT; - - prune_worktrees_cmd.git_cmd = 1; - strvec_pushl(&prune_worktrees_cmd.args, "worktree", "prune", "--expire", NULL); - strvec_push(&prune_worktrees_cmd.args, cfg.prune_worktrees_expire); - - if (run_command(&prune_worktrees_cmd)) - die(FAILED_RUN, prune_worktrees_cmd.args.v[0]); - } + if (cfg.prune_worktrees_expire && + maintenance_task_worktree_prune(&opts, &cfg)) + die(FAILED_RUN, "worktree"); rerere_cmd.git_cmd = 1; strvec_pushl(&rerere_cmd.args, "rerere", "gc", NULL); -- 2.49.0.1045.g170613ef41.dirty ``` ## Patrick Steinhardt, 2025-05-07 07:21 Subject: [PATCH v5 4/6] builtin/maintenance: introduce "worktree-prune" task Message-ID: <20250507-pks-maintenance-missing-tasks-v5-4-aa5fdfb82891@pks.im> URL: https://gitlist.dev/e/20250507-pks-maintenance-missing-tasks-v5-4-aa5fdfb82891%40pks.im In-Reply-To: <20250507-pks-maintenance-missing-tasks-v5-0-aa5fdfb82891@pks.im> ``` 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 --- Documentation/config/maintenance.adoc | 8 ++++ Documentation/git-maintenance.adoc | 4 ++ builtin/gc.c | 45 ++++++++++++++++++++++ t/t7900-maintenance.sh | 71 +++++++++++++++++++++++++++++++++++ 4 files changed, 128 insertions(+) diff --git a/Documentation/config/maintenance.adoc b/Documentation/config/maintenance.adoc index 41536162a77..b36b62c1c47 100644 --- a/Documentation/config/maintenance.adoc +++ b/Documentation/config/maintenance.adoc @@ -83,3 +83,11 @@ maintenance.reflog-expire.auto:: positive value implies the command should run when the number of expired reflog entries in the "HEAD" reflog is at least the value of `maintenance.loose-objects.auto`. The default value is 100. + +maintenance.worktree-prune.auto:: + This integer config option controls how often the `worktree-prune` task + should be run as part of `git maintenance run --auto`. If zero, then + the `worktree-prune` task will not run with the `--auto` option. A + negative value will force the task to run every time. Otherwise, a + positive value implies the command should run when the number of + prunable worktrees exceeds the value. The default value is 1. diff --git a/Documentation/git-maintenance.adoc b/Documentation/git-maintenance.adoc index 3a1e2a69b6b..6f085a9cf8c 100644 --- a/Documentation/git-maintenance.adoc +++ b/Documentation/git-maintenance.adoc @@ -166,6 +166,10 @@ reflog-expire:: The `reflog-expire` task deletes any entries in the reflog older than the expiry threshold. See linkgit:git-reflog[1] for more information. +worktree-prune:: + The `worktree-prune` task deletes stale or broken worktrees. See + linkit:git-worktree[1] for more information. + OPTIONS ------- --auto:: diff --git a/builtin/gc.c b/builtin/gc.c index 005ecc3f192..e85e313d78f 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -44,6 +44,7 @@ #include "hook.h" #include "setup.h" #include "trace2.h" +#include "worktree.h" #define FAILED_RUN "failed to run %s" @@ -346,6 +347,44 @@ static int maintenance_task_worktree_prune(struct maintenance_run_opts *opts UNU return run_command(&prune_worktrees_cmd); } +static int worktree_prune_condition(struct gc_config *cfg) +{ + struct strbuf buf = STRBUF_INIT; + int should_prune = 0, limit = 1; + timestamp_t expiry_date; + struct dirent *d; + DIR *dir = NULL; + + 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)) + goto out; + + dir = opendir(repo_git_path_replace(the_repository, &buf, "worktrees")); + if (!dir) + goto out; + + while (limit && (d = readdir_skip_dot_and_dotdot(dir))) { + char *wtpath; + strbuf_reset(&buf); + if (should_prune_worktree(d->d_name, &buf, &wtpath, expiry_date)) + limit--; + free(wtpath); + } + + should_prune = !limit; + +out: + if (dir) + closedir(dir); + strbuf_release(&buf); + return should_prune; +} + static int too_many_loose_objects(struct gc_config *cfg) { /* @@ -1455,6 +1494,7 @@ enum maintenance_task_label { TASK_COMMIT_GRAPH, TASK_PACK_REFS, TASK_REFLOG_EXPIRE, + TASK_WORKTREE_PRUNE, /* Leave as final value */ TASK__COUNT @@ -1496,6 +1536,11 @@ static struct maintenance_task tasks[] = { maintenance_task_reflog_expire, reflog_expire_condition, }, + [TASK_WORKTREE_PRUNE] = { + "worktree-prune", + maintenance_task_worktree_prune, + worktree_prune_condition, + }, }; static int compare_tasks_by_selection(const void *a_, const void *b_) diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 9b82e11c100..8f4120a0351 100755 --- 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_subcommand git reflog expire --all .git/worktrees/abc && + test_expect_worktree_prune git maintenance run --auto --task=worktree-prune +' + +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 many prunable worktrees. + test_expect_worktree_prune ! git -c maintenance.worktree-prune.auto=4 maintenance run --auto --task=worktree-prune && + test_expect_worktree_prune git -c maintenance.worktree-prune.auto=3 maintenance run --auto --task=worktree-prune +' + +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 many prunable worktrees. + test_expect_worktree_prune ! git -c maintenance.worktree-prune.auto=4 maintenance run --auto --task=worktree-prune && + test_expect_worktree_prune git -c maintenance.worktree-prune.auto=3 maintenance run --auto --task=worktree-prune +' + +test_expect_success 'worktree-prune task honors gc.worktreePruneExpire' ' + git worktree add worktree && + rm -rf worktree && + + rm -f worktree-prune.txt && + GIT_TRACE2_EVENT="$(pwd)/worktree-prune.txt" git -c gc.worktreePruneExpire=1.week.ago maintenance run --auto --task=worktree-prune && + test_subcommand ! git worktree prune --expire 1.week.ago err && test_grep "at most one" err -- 2.49.0.1045.g170613ef41.dirty ``` ## Patrick Steinhardt, 2025-05-07 07:21 Subject: [PATCH v5 5/6] builtin/gc: move rerere garbage collection into separate function Message-ID: <20250507-pks-maintenance-missing-tasks-v5-5-aa5fdfb82891@pks.im> URL: https://gitlist.dev/e/20250507-pks-maintenance-missing-tasks-v5-5-aa5fdfb82891%40pks.im In-Reply-To: <20250507-pks-maintenance-missing-tasks-v5-0-aa5fdfb82891@pks.im> ``` In a subsequent commit we are going to introduce a new "rerere-gc" task for git-maintenance(1). To prepare for this, refactor the code that spawns `git rerere gc` into a separate function. Signed-off-by: Patrick Steinhardt --- builtin/gc.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index e85e313d78f..0ae3071ec71 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -385,6 +385,15 @@ static int worktree_prune_condition(struct gc_config *cfg) return should_prune; } +static int maintenance_task_rerere_gc(struct maintenance_run_opts *opts UNUSED, + struct gc_config *cfg UNUSED) +{ + struct child_process rerere_cmd = CHILD_PROCESS_INIT; + rerere_cmd.git_cmd = 1; + strvec_pushl(&rerere_cmd.args, "rerere", "gc", NULL); + return run_command(&rerere_cmd); +} + static int too_many_loose_objects(struct gc_config *cfg) { /* @@ -781,7 +790,6 @@ int cmd_gc(int argc, int daemonized = 0; int keep_largest_pack = -1; timestamp_t dummy; - struct child_process rerere_cmd = CHILD_PROCESS_INIT; struct maintenance_run_opts opts = MAINTENANCE_RUN_OPTS_INIT; struct gc_config cfg = GC_CONFIG_INIT; const char *prune_expire_sentinel = "sentinel"; @@ -958,10 +966,8 @@ int cmd_gc(int argc, maintenance_task_worktree_prune(&opts, &cfg)) die(FAILED_RUN, "worktree"); - rerere_cmd.git_cmd = 1; - strvec_pushl(&rerere_cmd.args, "rerere", "gc", NULL); - if (run_command(&rerere_cmd)) - die(FAILED_RUN, rerere_cmd.args.v[0]); + if (maintenance_task_rerere_gc(&opts, &cfg)) + die(FAILED_RUN, "rerere"); report_garbage = report_pack_garbage; reprepare_packed_git(the_repository); -- 2.49.0.1045.g170613ef41.dirty ``` ## Patrick Steinhardt, 2025-05-07 07:21 Subject: [PATCH v5 6/6] builtin/maintenance: introduce "rerere-gc" task Message-ID: <20250507-pks-maintenance-missing-tasks-v5-6-aa5fdfb82891@pks.im> URL: https://gitlist.dev/e/20250507-pks-maintenance-missing-tasks-v5-6-aa5fdfb82891%40pks.im In-Reply-To: <20250507-pks-maintenance-missing-tasks-v5-0-aa5fdfb82891@pks.im> ``` While git-gc(1) knows to garbage collect the rerere cache, git-maintenance(1) does not yet have a task for this cleanup. Introduce a new "rerere-gc" task to plug this gap. Signed-off-by: Patrick Steinhardt --- Documentation/config/maintenance.adoc | 9 +++++++ Documentation/git-maintenance.adoc | 4 ++++ builtin/gc.c | 37 +++++++++++++++++++++++++++++ t/t7900-maintenance.sh | 44 +++++++++++++++++++++++++++++++++++ 4 files changed, 94 insertions(+) diff --git a/Documentation/config/maintenance.adoc b/Documentation/config/maintenance.adoc index b36b62c1c47..2f719342183 100644 --- a/Documentation/config/maintenance.adoc +++ b/Documentation/config/maintenance.adoc @@ -84,6 +84,15 @@ maintenance.reflog-expire.auto:: expired reflog entries in the "HEAD" reflog is at least the value of `maintenance.loose-objects.auto`. The default value is 100. +maintenance.rerere-gc.auto:: + This integer config option controls how often the `rerere-gc` task + should be run as part of `git maintenance run --auto`. If zero, then + the `rerere-gc` task will not run with the `--auto` option. A negative + value will force the task to run every time. Otherwise, any positive + value implies the command will run when the "rr-cache" directory exists + and has at least one entry, regardless of whether it is stale or not. + This heuristic may be refined in the future. The default value is 1. + maintenance.worktree-prune.auto:: This integer config option controls how often the `worktree-prune` task should be run as part of `git maintenance run --auto`. If zero, then diff --git a/Documentation/git-maintenance.adoc b/Documentation/git-maintenance.adoc index 6f085a9cf8c..931f3e02e85 100644 --- a/Documentation/git-maintenance.adoc +++ b/Documentation/git-maintenance.adoc @@ -166,6 +166,10 @@ reflog-expire:: The `reflog-expire` task deletes any entries in the reflog older than the expiry threshold. See linkgit:git-reflog[1] for more information. +rerere-gc:: + The `rerere-gc` task invokes garbage collection for stale entries in + the rerere cache. See linkgit:git-rerere[1] for more information. + worktree-prune:: The `worktree-prune` task deletes stale or broken worktrees. See linkit:git-worktree[1] for more information. diff --git a/builtin/gc.c b/builtin/gc.c index 0ae3071ec71..e79082f985d 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -16,6 +16,7 @@ #include "builtin.h" #include "abspath.h" #include "date.h" +#include "dir.h" #include "environment.h" #include "hex.h" #include "config.h" @@ -34,6 +35,7 @@ #include "pack-objects.h" #include "path.h" #include "reflog.h" +#include "rerere.h" #include "blob.h" #include "tree.h" #include "promisor-remote.h" @@ -394,6 +396,35 @@ static int maintenance_task_rerere_gc(struct maintenance_run_opts *opts UNUSED, return run_command(&rerere_cmd); } +static int rerere_gc_condition(struct gc_config *cfg UNUSED) +{ + struct strbuf path = STRBUF_INIT; + int should_gc = 0, limit = 1; + DIR *dir = NULL; + + git_config_get_int("maintenance.rerere-gc.auto", &limit); + if (limit <= 0) { + should_gc = limit < 0; + goto out; + } + + /* + * We skip garbage collection in case we either have no "rr-cache" + * directory or when it doesn't contain at least one entry. + */ + repo_git_path_replace(the_repository, &path, "rr-cache"); + dir = opendir(path.buf); + if (!dir) + goto out; + should_gc = !!readdir_skip_dot_and_dotdot(dir); + +out: + strbuf_release(&path); + if (dir) + closedir(dir); + return should_gc; +} + static int too_many_loose_objects(struct gc_config *cfg) { /* @@ -1501,6 +1532,7 @@ enum maintenance_task_label { TASK_PACK_REFS, TASK_REFLOG_EXPIRE, TASK_WORKTREE_PRUNE, + TASK_RERERE_GC, /* Leave as final value */ TASK__COUNT @@ -1547,6 +1579,11 @@ static struct maintenance_task tasks[] = { maintenance_task_worktree_prune, worktree_prune_condition, }, + [TASK_RERERE_GC] = { + "rerere-gc", + maintenance_task_rerere_gc, + rerere_gc_condition, + }, }; static int compare_tasks_by_selection(const void *a_, const void *b_) diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 8f4120a0351..8cf89e285f4 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -564,6 +564,50 @@ test_expect_success 'worktree-prune task honors gc.worktreePruneExpire' ' test_path_is_missing .git/worktrees/worktree ' +test_expect_rerere_gc () { + negate= + if test "$1" = "!" + then + negate="!" + shift + fi + + rm -f "rerere-gc.txt" && + GIT_TRACE2_EVENT="$(pwd)/rerere-gc.txt" "$@" && + test_subcommand $negate git rerere gc .git/rr-cache/entry && + test_expect_rerere_gc git maintenance run --auto --task=rerere-gc +' + +test_expect_success 'rerere-gc task with --auto honors maintenance.rerere-gc.auto' ' + test_when_finished "rm -rf .git/rr-cache" && + + # A negative value should always prune. + test_expect_rerere_gc git -c maintenance.rerere-gc.auto=-1 maintenance run --auto --task=rerere-gc && + + # A positive value prunes when there is at least one entry. + test_expect_rerere_gc ! git -c maintenance.rerere-gc.auto=9000 maintenance run --auto --task=rerere-gc && + mkdir .git/rr-cache && + test_expect_rerere_gc ! git -c maintenance.rerere-gc.auto=9000 maintenance run --auto --task=rerere-gc && + : >.git/rr-cache/entry-1 && + test_expect_rerere_gc git -c maintenance.rerere-gc.auto=9000 maintenance run --auto --task=rerere-gc && + + # Zero should never prune. + : >.git/rr-cache/entry-1 && + test_expect_rerere_gc ! git -c maintenance.rerere-gc.auto=0 maintenance run --auto --task=rerere-gc +' + test_expect_success '--auto and --schedule incompatible' ' test_must_fail git maintenance run --auto --schedule=daily 2>err && test_grep "at most one" err -- 2.49.0.1045.g170613ef41.dirty ```