# [PATCH v2 1/8] builtin/gc: fix indentation of `cmd_gc()` parameters 14 messages from 2025-04-30 to 2025-05-05. Participants: Patrick Steinhardt, Derrick Stolee, Junio C Hamano. Thread: https://gitlist.dev/t/63373 ## Patrick Steinhardt, 2025-04-30 10:25 Subject: [PATCH v2 1/8] builtin/gc: fix indentation of `cmd_gc()` parameters Message-ID: <20250430-pks-maintenance-missing-tasks-v2-1-2580b7b8ca3a@pks.im> URL: https://gitlist.dev/e/20250430-pks-maintenance-missing-tasks-v2-1-2580b7b8ca3a%40pks.im In-Reply-To: <20250430-pks-maintenance-missing-tasks-v2-0-2580b7b8ca3a@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-04-30 10:25 Subject: [PATCH v2 0/8] builtin/maintenance: implement missing tasks compared to git-gc(1) Message-ID: <20250430-pks-maintenance-missing-tasks-v2-0-2580b7b8ca3a@pks.im> URL: https://gitlist.dev/e/20250430-pks-maintenance-missing-tasks-v2-0-2580b7b8ca3a%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 Thanks! Patrick --- Patrick Steinhardt (8): 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 rerere: provide function to collect stale entries builtin/gc: move rerere garbage collection into separate function builtin/maintenance: introduce "rerere-gc" task Documentation/config/maintenance.adoc | 16 ++++ Documentation/git-maintenance.adoc | 8 ++ builtin/gc.c | 153 +++++++++++++++++++++++++++------- builtin/worktree.c | 25 +++--- rerere.c | 92 +++++++++++++------- rerere.h | 14 ++++ t/t7900-maintenance.sh | 125 +++++++++++++++++++++++++++ worktree.c | 30 +++++++ worktree.h | 8 ++ 9 files changed, 399 insertions(+), 72 deletions(-) Range-diff versus v1: 1: 0304b81df0b = 1: 9c62b493297 builtin/gc: fix indentation of `cmd_gc()` parameters 2: 22c499601ee = 2: 9ae42b139fa builtin/gc: remove global variables where it trivial to do 3: db9622a408f = 3: 50a5305b6d2 builtin/gc: move pruning of worktrees into a separate function 4: f42205e1b6b = 4: b71dcb0debc worktree: expose function to retrieve worktree names 5: eade37df904 ! 5: 47d31f41c2e builtin/maintenance: introduce "worktree-prune" task @@ Commit message Signed-off-by: Patrick Steinhardt + ## Documentation/config/maintenance.adoc ## +@@ Documentation/config/maintenance.adoc: 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. + ## Documentation/git-maintenance.adoc ## @@ Documentation/git-maintenance.adoc: reflog-expire:: The `reflog-expire` task deletes any entries in the reflog older than the @@ builtin/gc.c: static int maintenance_task_worktree_prune(struct maintenance_run_ + 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) @@ builtin/gc.c: static int maintenance_task_worktree_prune(struct maintenance_run_ + + strbuf_reset(&reason); + if (should_prune_worktree(worktrees.v[i], &reason, &wtpath, expiry_date)) { -+ should_prune = 1; -+ goto out; ++ limit--; ++ ++ if (!limit) { ++ should_prune = 1; ++ goto out; ++ } + } + free(wtpath); + } @@ t/t7900-maintenance.sh: test_expect_success 'reflog-expire task --auto only pack 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 .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 6: 5550c115e84 rerere: provide function to collect stale entries 6: 66b2b033743 = 7: f5b234c859e builtin/gc: move rerere garbage collection into separate function 7: 9604fc4fc6b ! 8: 092e57cce01 builtin/maintenance: introduce "rerere-gc" task @@ Commit message Signed-off-by: Patrick Steinhardt + ## Documentation/config/maintenance.adoc ## +@@ Documentation/config/maintenance.adoc: 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, a positive ++ value implies the command should run when the number of prunable rerere ++ entries exceeds the value. The default value is 20. ++ + 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 + ## Documentation/git-maintenance.adoc ## @@ Documentation/git-maintenance.adoc: reflog-expire:: The `reflog-expire` task deletes any entries in the reflog older than the @@ builtin/gc.c #include "environment.h" #include "hex.h" #include "config.h" +@@ + #include "pack-objects.h" + #include "path.h" + #include "reflog.h" ++#include "rerere.h" + #include "blob.h" + #include "tree.h" + #include "promisor-remote.h" @@ builtin/gc.c: static int maintenance_task_rerere_gc(struct maintenance_run_opts *opts UNUSED, return run_command(&rerere_cmd); } @@ builtin/gc.c: static int maintenance_task_rerere_gc(struct maintenance_run_opts +static int rerere_gc_condition(struct gc_config *cfg UNUSED) +{ + struct strbuf path = STRBUF_INIT; ++ struct string_list prunable_dirs = STRING_LIST_INIT_DUP; ++ struct rerere_id *prunable_entries = NULL; ++ size_t prunable_entries_nr; + int should_gc = 0; -+ DIR *dir; ++ int limit = 20; ++ ++ git_config_get_int("maintenance.rerere-gc.auto", &limit); ++ if (limit <= 0) { ++ should_gc = limit < 0; ++ goto out; ++ } + + /* Skip garbage collecting the rerere cache in case rerere is disabled. */ + repo_git_path_replace(the_repository, &path, "rr-cache"); ++ if (!is_directory(path.buf)) ++ goto out; + -+ dir = opendir(path.buf); -+ if (!dir) ++ if (rerere_collect_stale_entries(the_repository, &prunable_dirs, ++ &prunable_entries, &prunable_entries_nr) < 0) + goto out; -+ should_gc = !!readdir_skip_dot_and_dotdot(dir); ++ ++ should_gc = prunable_entries_nr >= limit; + +out: ++ string_list_clear(&prunable_dirs, 0); ++ free(prunable_entries); + strbuf_release(&path); -+ closedir(dir); + return should_gc; +} + @@ builtin/gc.c: static struct maintenance_task tasks[] = { static int compare_tasks_by_selection(const void *a_, const void *b_) ## t/t7900-maintenance.sh ## -@@ t/t7900-maintenance.sh: test_expect_success 'worktree-prune task --auto only prunes with prunable worktr - test_subcommand git worktree prune --expire 3.months.ago "$rr/preimage" && ++ >"$rr/postimage" && ++ ++ test-tool chmtime ="$((-61 * 86400))" "$rr/preimage" && ++ test-tool chmtime ="$((-61 * 86400))" "$rr/postimage" ++} ++ ++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 && -+ GIT_TRACE2_EVENT="$(pwd)/rerere-gc-auto.txt" \ -+ git maintenance run --auto --task=rerere-gc && -+ test_subcommand git rerere gc URL: https://gitlist.dev/e/20250430-pks-maintenance-missing-tasks-v2-2-2580b7b8ca3a%40pks.im In-Reply-To: <20250430-pks-maintenance-missing-tasks-v2-0-2580b7b8ca3a@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-04-30 10:25 Subject: [PATCH v2 3/8] builtin/gc: move pruning of worktrees into a separate function Message-ID: <20250430-pks-maintenance-missing-tasks-v2-3-2580b7b8ca3a@pks.im> URL: https://gitlist.dev/e/20250430-pks-maintenance-missing-tasks-v2-3-2580b7b8ca3a%40pks.im In-Reply-To: <20250430-pks-maintenance-missing-tasks-v2-0-2580b7b8ca3a@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-04-30 10:25 Subject: [PATCH v2 4/8] worktree: expose function to retrieve worktree names Message-ID: <20250430-pks-maintenance-missing-tasks-v2-4-2580b7b8ca3a@pks.im> URL: https://gitlist.dev/e/20250430-pks-maintenance-missing-tasks-v2-4-2580b7b8ca3a%40pks.im In-Reply-To: <20250430-pks-maintenance-missing-tasks-v2-0-2580b7b8ca3a@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-04-30 10:25 Subject: [PATCH v2 5/8] builtin/maintenance: introduce "worktree-prune" task Message-ID: <20250430-pks-maintenance-missing-tasks-v2-5-2580b7b8ca3a@pks.im> URL: https://gitlist.dev/e/20250430-pks-maintenance-missing-tasks-v2-5-2580b7b8ca3a%40pks.im In-Reply-To: <20250430-pks-maintenance-missing-tasks-v2-0-2580b7b8ca3a@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-04-30 10:25 Subject: [PATCH v2 6/8] rerere: provide function to collect stale entries Message-ID: <20250430-pks-maintenance-missing-tasks-v2-6-2580b7b8ca3a@pks.im> URL: https://gitlist.dev/e/20250430-pks-maintenance-missing-tasks-v2-6-2580b7b8ca3a%40pks.im In-Reply-To: <20250430-pks-maintenance-missing-tasks-v2-0-2580b7b8ca3a@pks.im> ``` We're about to add another task for git-maintenance(1) that prunes stale rerere entries via `git rerere gc`. The condition of when to run this subcommand will be configurable so that the subcommand is only executed when a certain number of stale rerere entries exists. This requires us to know about the number of stale rerere entries in the first place, which is non-trivial to figure out. Refactor `rerere_gc()` and `prune_one()` so that garbage collection is split into three phases: 1. We collect any stale rerere entries and directories that are about to become empty. 2. Prune all stale rerere entries. 3. Remove all directories that should have become empty in (2). By splitting out the collection of stale entries we can trivially expose this function to external callers and thus reuse it in later steps. This refactoring is not expected to result in a user-visible change in behaviour. Signed-off-by: Patrick Steinhardt --- rerere.c | 92 ++++++++++++++++++++++++++++++++++++++++++++-------------------- rerere.h | 14 ++++++++++ 2 files changed, 78 insertions(+), 28 deletions(-) diff --git a/rerere.c b/rerere.c index 740e8ad1a0b..eb06e5f8bea 100644 --- a/rerere.c +++ b/rerere.c @@ -1202,8 +1202,8 @@ static void unlink_rr_item(struct rerere_id *id) strbuf_release(&buf); } -static void prune_one(struct rerere_id *id, - timestamp_t cutoff_resolve, timestamp_t cutoff_noresolve) +static int is_stale(struct rerere_id *id, + timestamp_t cutoff_resolve, timestamp_t cutoff_noresolve) { timestamp_t then; timestamp_t cutoff; @@ -1214,11 +1214,11 @@ static void prune_one(struct rerere_id *id, else { then = rerere_created_at(id); if (!then) - return; + return 0; cutoff = cutoff_noresolve; } - if (then < cutoff) - unlink_rr_item(id); + + return then < cutoff; } /* Does the basename in "path" look plausibly like an rr-cache entry? */ @@ -1229,29 +1229,35 @@ static int is_rr_cache_dirname(const char *path) return !parse_oid_hex(path, &oid, &end) && !*end; } -void rerere_gc(struct repository *r, struct string_list *rr) +int rerere_collect_stale_entries(struct repository *r, + struct string_list *prunable_dirs, + struct rerere_id **prunable_entries, + size_t *prunable_entries_nr) { - struct string_list to_remove = STRING_LIST_INIT_DUP; - DIR *dir; - struct dirent *e; - int i; timestamp_t now = time(NULL); timestamp_t cutoff_noresolve = now - 15 * 86400; timestamp_t cutoff_resolve = now - 60 * 86400; struct strbuf buf = STRBUF_INIT; + size_t prunable_entries_alloc; + struct dirent *e; + DIR *dir = NULL; + int ret; - if (setup_rerere(r, rr, 0) < 0) - return; + *prunable_entries = NULL; + *prunable_entries_nr = 0; + prunable_entries_alloc = 0; - repo_config_get_expiry_in_days(the_repository, "gc.rerereresolved", + repo_config_get_expiry_in_days(r, "gc.rerereresolved", &cutoff_resolve, now); - repo_config_get_expiry_in_days(the_repository, "gc.rerereunresolved", + repo_config_get_expiry_in_days(r, "gc.rerereunresolved", &cutoff_noresolve, now); - git_config(git_default_config, NULL); - dir = opendir(repo_git_path_replace(the_repository, &buf, "rr-cache")); - if (!dir) - die_errno(_("unable to open rr-cache directory")); - /* Collect stale conflict IDs ... */ + + dir = opendir(repo_git_path_replace(r, &buf, "rr-cache")); + if (!dir) { + ret = error_errno(_("unable to open rr-cache directory")); + goto out; + } + while ((e = readdir_skip_dot_and_dotdot(dir))) { struct rerere_dir *rr_dir; struct rerere_id id; @@ -1266,23 +1272,53 @@ void rerere_gc(struct repository *r, struct string_list *rr) for (id.variant = 0, id.collection = rr_dir; id.variant < id.collection->status_nr; id.variant++) { - prune_one(&id, cutoff_resolve, cutoff_noresolve); - if (id.collection->status[id.variant]) + if (is_stale(&id, cutoff_resolve, cutoff_noresolve)) { + ALLOC_GROW(*prunable_entries, *prunable_entries_nr + 1, + prunable_entries_alloc); + (*prunable_entries)[(*prunable_entries_nr)++] = id; + } else { now_empty = 0; + } } if (now_empty) - string_list_append(&to_remove, e->d_name); + string_list_append(prunable_dirs, e->d_name); } - closedir(dir); - /* ... and then remove the empty directories */ - for (i = 0; i < to_remove.nr; i++) - rmdir(repo_git_path_replace(the_repository, &buf, - "rr-cache/%s", to_remove.items[i].string)); + ret = 0; + +out: + strbuf_release(&buf); + if (dir) + closedir(dir); + return ret; +} + +void rerere_gc(struct repository *r, struct string_list *rr) +{ + struct string_list prunable_dirs = STRING_LIST_INIT_DUP; + struct rerere_id *prunable_entries; + struct strbuf buf = STRBUF_INIT; + size_t prunable_entries_nr; + + if (setup_rerere(r, rr, 0) < 0) + return; + + git_config(git_default_config, NULL); + + if (rerere_collect_stale_entries(r, &prunable_dirs, &prunable_entries, + &prunable_entries_nr) < 0) + exit(127); + + for (size_t i = 0; i < prunable_entries_nr; i++) + unlink_rr_item(&prunable_entries[i]); + for (size_t i = 0; i < prunable_dirs.nr; i++) + rmdir(repo_git_path_replace(r, &buf, "rr-cache/%s", + prunable_dirs.items[i].string)); - string_list_clear(&to_remove, 0); + string_list_clear(&prunable_dirs, 0); rollback_lock_file(&write_lock); strbuf_release(&buf); + free(prunable_entries); } /* diff --git a/rerere.h b/rerere.h index d4b5f7c9320..fd5a2388b06 100644 --- a/rerere.h +++ b/rerere.h @@ -37,6 +37,20 @@ const char *rerere_path(struct strbuf *buf, const struct rerere_id *, int rerere_forget(struct repository *, struct pathspec *); int rerere_remaining(struct repository *, struct string_list *); void rerere_clear(struct repository *, struct string_list *); + +/* + * Collect prunable rerere entries that would be garbage collected via + * `rerere_gc()`. Whether or not an entry is prunable depends on both + * "gc.rerereResolved" and "gc.rerereUnresolved". + * + * Returns 0 on success, a negative error code in case entries could not be + * collected. + */ +int rerere_collect_stale_entries(struct repository *r, + struct string_list *prunable_dirs, + struct rerere_id **prunable_entries, + size_t *prunable_entries_nr); + void rerere_gc(struct repository *, struct string_list *); #define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \ -- 2.49.0.987.g0cc8ee98dc.dirty ``` ## Patrick Steinhardt, 2025-04-30 10:25 Subject: [PATCH v2 7/8] builtin/gc: move rerere garbage collection into separate function Message-ID: <20250430-pks-maintenance-missing-tasks-v2-7-2580b7b8ca3a@pks.im> URL: https://gitlist.dev/e/20250430-pks-maintenance-missing-tasks-v2-7-2580b7b8ca3a%40pks.im In-Reply-To: <20250430-pks-maintenance-missing-tasks-v2-0-2580b7b8ca3a@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-04-30 10:25 Subject: [PATCH v2 8/8] builtin/maintenance: introduce "rerere-gc" task Message-ID: <20250430-pks-maintenance-missing-tasks-v2-8-2580b7b8ca3a@pks.im> URL: https://gitlist.dev/e/20250430-pks-maintenance-missing-tasks-v2-8-2580b7b8ca3a%40pks.im In-Reply-To: <20250430-pks-maintenance-missing-tasks-v2-0-2580b7b8ca3a@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 | 8 ++++++ Documentation/git-maintenance.adoc | 4 +++ builtin/gc.c | 41 ++++++++++++++++++++++++++ t/t7900-maintenance.sh | 54 +++++++++++++++++++++++++++++++++++ 4 files changed, 107 insertions(+) diff --git a/Documentation/config/maintenance.adoc b/Documentation/config/maintenance.adoc index b36b62c1c47..9c333d42b19 100644 --- a/Documentation/config/maintenance.adoc +++ b/Documentation/config/maintenance.adoc @@ -84,6 +84,14 @@ 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, a positive + value implies the command should run when the number of prunable rerere + entries exceeds the value. The default value is 20. + 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..e56d85ea3bc 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,39 @@ 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; + struct string_list prunable_dirs = STRING_LIST_INIT_DUP; + struct rerere_id *prunable_entries = NULL; + size_t prunable_entries_nr; + int should_gc = 0; + int limit = 20; + + git_config_get_int("maintenance.rerere-gc.auto", &limit); + if (limit <= 0) { + should_gc = limit < 0; + goto out; + } + + /* Skip garbage collecting the rerere cache in case rerere is disabled. */ + repo_git_path_replace(the_repository, &path, "rr-cache"); + if (!is_directory(path.buf)) + goto out; + + if (rerere_collect_stale_entries(the_repository, &prunable_dirs, + &prunable_entries, &prunable_entries_nr) < 0) + goto out; + + should_gc = prunable_entries_nr >= limit; + +out: + string_list_clear(&prunable_dirs, 0); + free(prunable_entries); + strbuf_release(&path); + return should_gc; +} + static int too_many_loose_objects(struct gc_config *cfg) { /* @@ -1502,6 +1537,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 +1584,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..78da81eeb24 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -564,6 +564,60 @@ test_expect_success 'worktree-prune task honors gc.worktreePruneExpire' ' test_path_is_missing .git/worktrees/worktree ' +setup_stale_rerere_entry () { + rr=.git/rr-cache/"$(printf "%0$(test_oid hexsz)d" "$1")" && + mkdir -p "$rr" && + >"$rr/preimage" && + >"$rr/postimage" && + + test-tool chmtime ="$((-61 * 86400))" "$rr/preimage" && + test-tool chmtime ="$((-61 * 86400))" "$rr/postimage" +} + +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 err && test_grep "at most one" err -- 2.49.0.987.g0cc8ee98dc.dirty ``` ## Derrick Stolee, 2025-04-30 10:37 Subject: Re: [PATCH v2 0/8] builtin/maintenance: implement missing tasks compared to git-gc(1) Message-ID: <732db882-f27c-4619-91cb-2cc291a7c7d5@gmail.com> URL: https://gitlist.dev/e/732db882-f27c-4619-91cb-2cc291a7c7d5%40gmail.com In-Reply-To: <20250430-pks-maintenance-missing-tasks-v2-0-2580b7b8ca3a@pks.im> ``` On 4/30/2025 6:25 AM, Patrick Steinhardt wrote: > 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 This version satisfies my comments from v1 and builds further by including the maintenance..auto config options. LGTM. Thanks, -Stolee ``` ## Junio C Hamano, 2025-04-30 16:58 Subject: Re: [PATCH v2 6/8] rerere: provide function to collect stale entries Message-ID: URL: https://gitlist.dev/e/xmqqy0vh1t96.fsf%40gitster.g In-Reply-To: <20250430-pks-maintenance-missing-tasks-v2-6-2580b7b8ca3a@pks.im> ``` Patrick Steinhardt writes: > We're about to add another task for git-maintenance(1) that prunes stale > rerere entries via `git rerere gc`. The condition of when to run this > subcommand will be configurable so that the subcommand is only executed > when a certain number of stale rerere entries exists. This requires us > to know about the number of stale rerere entries in the first place, > which is non-trivial to figure out. > > Refactor `rerere_gc()` and `prune_one()` so that garbage collection is > split into three phases: > > 1. We collect any stale rerere entries and directories that are about > to become empty. > > 2. Prune all stale rerere entries. > > 3. Remove all directories that should have become empty in (2). > > By splitting out the collection of stale entries we can trivially expose > this function to external callers and thus reuse it in later steps. > > This refactoring is not expected to result in a user-visible change in > behaviour. I have no objection against the goal of allowing "git maintenance" drive "git rerere gc", and as the primary author of this code path I do not see anything wrong, in the "correctness" sense, in the updated code. I however am not sure if "count what we would prune, and remove only when there are too many" would work well for this subsystem, because I expect that the cost to enumerate existing rerere entries and check each of them for staleness would be the dominant part, relative to actual "rm -fr ", of the cost you are paying when you run "git rerere gc". And if my suspicion is correct, all this change does to the plain vanilla user of "git rerere gc" is to have them pay the extra cost of allocating and deallocating the list of names of paths in string lists. We need to see some performance measurement to show that the we pay for collection and counting is a lot smaller compared to the whole pruning operation to justify the "auto" thing. > Signed-off-by: Patrick Steinhardt > --- > rerere.c | 92 ++++++++++++++++++++++++++++++++++++++++++++-------------------- > rerere.h | 14 ++++++++++ > 2 files changed, 78 insertions(+), 28 deletions(-) > > diff --git a/rerere.c b/rerere.c > index 740e8ad1a0b..eb06e5f8bea 100644 > --- a/rerere.c > +++ b/rerere.c > @@ -1202,8 +1202,8 @@ static void unlink_rr_item(struct rerere_id *id) > strbuf_release(&buf); > } > > -static void prune_one(struct rerere_id *id, > - timestamp_t cutoff_resolve, timestamp_t cutoff_noresolve) > +static int is_stale(struct rerere_id *id, > + timestamp_t cutoff_resolve, timestamp_t cutoff_noresolve) > { > timestamp_t then; > timestamp_t cutoff; > @@ -1214,11 +1214,11 @@ static void prune_one(struct rerere_id *id, > else { > then = rerere_created_at(id); > if (!then) > - return; > + return 0; > cutoff = cutoff_noresolve; > } > - if (then < cutoff) > - unlink_rr_item(id); > + > + return then < cutoff; > } > > /* Does the basename in "path" look plausibly like an rr-cache entry? */ > @@ -1229,29 +1229,35 @@ static int is_rr_cache_dirname(const char *path) > return !parse_oid_hex(path, &oid, &end) && !*end; > } > > -void rerere_gc(struct repository *r, struct string_list *rr) > +int rerere_collect_stale_entries(struct repository *r, > + struct string_list *prunable_dirs, > + struct rerere_id **prunable_entries, > + size_t *prunable_entries_nr) > { > - struct string_list to_remove = STRING_LIST_INIT_DUP; > - DIR *dir; > - struct dirent *e; > - int i; > timestamp_t now = time(NULL); > timestamp_t cutoff_noresolve = now - 15 * 86400; > timestamp_t cutoff_resolve = now - 60 * 86400; > struct strbuf buf = STRBUF_INIT; > + size_t prunable_entries_alloc; > + struct dirent *e; > + DIR *dir = NULL; > + int ret; > > - if (setup_rerere(r, rr, 0) < 0) > - return; > + *prunable_entries = NULL; > + *prunable_entries_nr = 0; > + prunable_entries_alloc = 0; > > - repo_config_get_expiry_in_days(the_repository, "gc.rerereresolved", > + repo_config_get_expiry_in_days(r, "gc.rerereresolved", > &cutoff_resolve, now); > - repo_config_get_expiry_in_days(the_repository, "gc.rerereunresolved", > + repo_config_get_expiry_in_days(r, "gc.rerereunresolved", > &cutoff_noresolve, now); > - git_config(git_default_config, NULL); > - dir = opendir(repo_git_path_replace(the_repository, &buf, "rr-cache")); > - if (!dir) > - die_errno(_("unable to open rr-cache directory")); > - /* Collect stale conflict IDs ... */ > + > + dir = opendir(repo_git_path_replace(r, &buf, "rr-cache")); > + if (!dir) { > + ret = error_errno(_("unable to open rr-cache directory")); > + goto out; > + } > + > while ((e = readdir_skip_dot_and_dotdot(dir))) { > struct rerere_dir *rr_dir; > struct rerere_id id; > @@ -1266,23 +1272,53 @@ void rerere_gc(struct repository *r, struct string_list *rr) > for (id.variant = 0, id.collection = rr_dir; > id.variant < id.collection->status_nr; > id.variant++) { > - prune_one(&id, cutoff_resolve, cutoff_noresolve); > - if (id.collection->status[id.variant]) > + if (is_stale(&id, cutoff_resolve, cutoff_noresolve)) { > + ALLOC_GROW(*prunable_entries, *prunable_entries_nr + 1, > + prunable_entries_alloc); > + (*prunable_entries)[(*prunable_entries_nr)++] = id; > + } else { > now_empty = 0; > + } > } > if (now_empty) > - string_list_append(&to_remove, e->d_name); > + string_list_append(prunable_dirs, e->d_name); > } > - closedir(dir); > > - /* ... and then remove the empty directories */ > - for (i = 0; i < to_remove.nr; i++) > - rmdir(repo_git_path_replace(the_repository, &buf, > - "rr-cache/%s", to_remove.items[i].string)); > + ret = 0; > + > +out: > + strbuf_release(&buf); > + if (dir) > + closedir(dir); > + return ret; > +} > + > +void rerere_gc(struct repository *r, struct string_list *rr) > +{ > + struct string_list prunable_dirs = STRING_LIST_INIT_DUP; > + struct rerere_id *prunable_entries; > + struct strbuf buf = STRBUF_INIT; > + size_t prunable_entries_nr; > + > + if (setup_rerere(r, rr, 0) < 0) > + return; > + > + git_config(git_default_config, NULL); > + > + if (rerere_collect_stale_entries(r, &prunable_dirs, &prunable_entries, > + &prunable_entries_nr) < 0) > + exit(127); > + > + for (size_t i = 0; i < prunable_entries_nr; i++) > + unlink_rr_item(&prunable_entries[i]); > + for (size_t i = 0; i < prunable_dirs.nr; i++) > + rmdir(repo_git_path_replace(r, &buf, "rr-cache/%s", > + prunable_dirs.items[i].string)); > > - string_list_clear(&to_remove, 0); > + string_list_clear(&prunable_dirs, 0); > rollback_lock_file(&write_lock); > strbuf_release(&buf); > + free(prunable_entries); > } > > /* > diff --git a/rerere.h b/rerere.h > index d4b5f7c9320..fd5a2388b06 100644 > --- a/rerere.h > +++ b/rerere.h > @@ -37,6 +37,20 @@ const char *rerere_path(struct strbuf *buf, const struct rerere_id *, > int rerere_forget(struct repository *, struct pathspec *); > int rerere_remaining(struct repository *, struct string_list *); > void rerere_clear(struct repository *, struct string_list *); > + > +/* > + * Collect prunable rerere entries that would be garbage collected via > + * `rerere_gc()`. Whether or not an entry is prunable depends on both > + * "gc.rerereResolved" and "gc.rerereUnresolved". > + * > + * Returns 0 on success, a negative error code in case entries could not be > + * collected. > + */ > +int rerere_collect_stale_entries(struct repository *r, > + struct string_list *prunable_dirs, > + struct rerere_id **prunable_entries, > + size_t *prunable_entries_nr); > + > void rerere_gc(struct repository *, struct string_list *); > > #define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \ ``` ## Patrick Steinhardt, 2025-05-02 08:07 Subject: Re: [PATCH v2 6/8] rerere: provide function to collect stale entries Message-ID: URL: https://gitlist.dev/e/aBR9R3PxSCuLON6G%40pks.im In-Reply-To: ``` On Wed, Apr 30, 2025 at 09:58:13AM -0700, Junio C Hamano wrote: > Patrick Steinhardt writes: > > > We're about to add another task for git-maintenance(1) that prunes stale > > rerere entries via `git rerere gc`. The condition of when to run this > > subcommand will be configurable so that the subcommand is only executed > > when a certain number of stale rerere entries exists. This requires us > > to know about the number of stale rerere entries in the first place, > > which is non-trivial to figure out. > > > > Refactor `rerere_gc()` and `prune_one()` so that garbage collection is > > split into three phases: > > > > 1. We collect any stale rerere entries and directories that are about > > to become empty. > > > > 2. Prune all stale rerere entries. > > > > 3. Remove all directories that should have become empty in (2). > > > > By splitting out the collection of stale entries we can trivially expose > > this function to external callers and thus reuse it in later steps. > > > > This refactoring is not expected to result in a user-visible change in > > behaviour. > > I have no objection against the goal of allowing "git maintenance" > drive "git rerere gc", and as the primary author of this code path I > do not see anything wrong, in the "correctness" sense, in the > updated code. > > I however am not sure if "count what we would prune, and remove only > when there are too many" would work well for this subsystem, because > I expect that the cost to enumerate existing rerere entries and > check each of them for staleness would be the dominant part, > relative to actual "rm -fr ", of the cost you are paying > when you run "git rerere gc". > > And if my suspicion is correct, all this change does to the plain > vanilla user of "git rerere gc" is to have them pay the extra cost > of allocating and deallocating the list of names of paths in string > lists. Yeah, I think this concern makes sense indeed. I was a bit sceptical myself whether this is going too far. Maybe a simpler solution would be to just count the number of directories in ".git/rr-cache", without checking whether those actually are prunable? We could also adapt this to be closer to the original version, where we only verified that ".git/rr-cache" exists and contains at least one subdirectory. This can even be combined with the above approach if we set "maintenance.rerere-gc.auto=1" by default. > We need to see some performance measurement to show that the we pay > for collection and counting is a lot smaller compared to the whole > pruning operation to justify the "auto" thing. Hm. I guess ultimately the answer is going to be "it depends". The performance implication on Windows is going to be quite different compared to the performance on Linux/macOS. In any case, let's go with the simpler model for now. We can still iterate as needed if we eventually see that the heuristic is too dumb to be useful. Patrick ``` ## Junio C Hamano, 2025-05-02 16:35 Subject: Re: [PATCH v2 6/8] rerere: provide function to collect stale entries Message-ID: URL: https://gitlist.dev/e/xmqqwmazug0i.fsf%40gitster.g In-Reply-To: ``` Patrick Steinhardt writes: >> And if my suspicion is correct, all this change does to the plain >> vanilla user of "git rerere gc" is to have them pay the extra cost >> of allocating and deallocating the list of names of paths in string >> lists. > > Yeah, I think this concern makes sense indeed. I was a bit sceptical > myself whether this is going too far. Maybe a simpler solution would be > to just count the number of directories in ".git/rr-cache", without > checking whether those actually are prunable? I dunno. It is not like you are guestimating files under .git/objects/??/ without couting all of them, just to see if you have too many of them and reduce them by repacking and/or pruning. Having too many existing rerere entries does not correleate you would have many prunable ones. If your development is nearly linear and you do not rebuild the same merges over and over every day, majority of rerere entries may be created and recorded without ever getting used even once. On the other hand, in my tree, where the same merges along master..jch first-parent history are recreated multiple times every day, the picture would be different. > We could also adapt this to be closer to the original version, where we > only verified that ".git/rr-cache" exists and contains at least one > subdirectory. This can even be combined with the above approach if we > set "maintenance.rerere-gc.auto=1" by default. But wouldn't that be like always running "rerere gc"? What would you save by checking the existence of at least one rerere entry? Cost to spawn "rerere gc" as a subprocess? Wouldn't it be a better use of the engineering effort to libify the logic of that process and make it internally callable in the longer term (or are you already doing that in this series)? >> We need to see some performance measurement to show that the we pay >> for collection and counting is a lot smaller compared to the whole >> pruning operation to justify the "auto" thing. > > Hm. I guess ultimately the answer is going to be "it depends". The > performance implication on Windows is going to be quite different > compared to the performance on Linux/macOS. Yes, but we need to start somewhere ;-) ``` ## Patrick Steinhardt, 2025-05-05 07:22 Subject: Re: [PATCH v2 6/8] rerere: provide function to collect stale entries Message-ID: URL: https://gitlist.dev/e/aBhnOiHgfycVJhHQ%40pks.im In-Reply-To: ``` On Fri, May 02, 2025 at 09:35:57AM -0700, Junio C Hamano wrote: > Patrick Steinhardt writes: > > We could also adapt this to be closer to the original version, where we > > only verified that ".git/rr-cache" exists and contains at least one > > subdirectory. This can even be combined with the above approach if we > > set "maintenance.rerere-gc.auto=1" by default. > > But wouldn't that be like always running "rerere gc"? What would > you save by checking the existence of at least one rerere entry? > Cost to spawn "rerere gc" as a subprocess? What you save is executing the rerere logic in case the feature isn't enabled at all, but also in the case where you have eventually disabled the rerere cache but still have a now-empty ".git/rr-cache" directory. > Wouldn't it be a better use of the engineering effort to libify the > logic of that process and make it internally callable in the longer > term (or are you already doing that in this series)? The logic itself already is quite encapsulated, but I decided to not use it for now because we don't do so for any of the other tasks, either. But I do agree that we should eventually refactor all of our tasks so that they don't need to spawn an external process at all. Patrick ```