> Le 27 mai 2025 à 11:29, Patrick Steinhardt <ps@xxxxxx> a écrit :
>
> The "gc" task has a similar locking race as the one that we have fixed
> for the "pack-refs" and "reflog-expire" tasks in preceding commits. Fix
> this by splitting up the logic of the "gc" task:
>
> - Before detaching we execute `gc_before_repack()`, which contains the
> logic that git-gc(1) itself would execute before detaching.
>
> - After detaching we spawn git-gc(1), but with a new hidden flag that
> suppresses calling `gc_before_repack()`.
>
> Like this we have roughly the same logic as git-gc(1) itself and know to
> repack refs and reflogs before detaching, thus fixing the race.
>
> Note that `gc_before_repack()` is renamed to `gc_before_detach()` to
> better reflect what this function does.
>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> builtin/gc.c | 39 ++++++++++++++++++++++++++-------------
> t/t7900-maintenance.sh | 12 ++++++------
> 2 files changed, 32 insertions(+), 19 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 174357b9c25..2cf61efcee9 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -816,7 +816,7 @@ static int report_last_gc_error(void)
> return ret;
> }
>
> -static int gc_before_repack(struct maintenance_run_opts *opts,
> +static int gc_before_detach(struct maintenance_run_opts *opts,
> struct gc_config *cfg)
> {
> if (cfg->pack_refs && maintenance_task_pack_refs(opts, cfg))
> @@ -837,6 +837,7 @@ int cmd_gc(int argc,
> pid_t pid;
> int daemonized = 0;
> int keep_largest_pack = -1;
> + int skip_maintenance_before_detach = 0;
> timestamp_t dummy;
> struct maintenance_run_opts opts = MAINTENANCE_RUN_OPTS_INIT;
> struct gc_config cfg = GC_CONFIG_INIT;
> @@ -869,6 +870,8 @@ int cmd_gc(int argc,
> N_("repack all other packs except the largest pack")),
> OPT_STRING(0, "expire-to", &cfg.repack_expire_to, N_("dir"),
> N_("pack prefix to store a pack containing pruned objects")),
> + OPT_HIDDEN_BOOL(0, "skip-maintenance-before-detach", &skip_maintenance_before_detach,
> + N_("skip maintenance steps typically done before detaching")),
> OPT_END()
> };
>
> @@ -952,14 +955,16 @@ int cmd_gc(int argc,
> goto out;
> }
>
> - if (lock_repo_for_gc(force, &pid)) {
> - ret = 0;
> - goto out;
> - }
> + if (!skip_maintenance_before_detach) {
> + if (lock_repo_for_gc(force, &pid)) {
> + ret = 0;
> + goto out;
> + }
>
> - if (gc_before_repack(&opts, &cfg) < 0)
> - exit(127);
> - delete_tempfile(&pidfile);
> + if (gc_before_detach(&opts, &cfg) < 0)
> + exit(127);
> + delete_tempfile(&pidfile);
> + }
>
> /*
> * failure to daemonize is ok, we'll continue
> @@ -988,8 +993,8 @@ int cmd_gc(int argc,
> free(path);
> }
>
> - if (opts.detach <= 0)
> - gc_before_repack(&opts, &cfg);
> + if (opts.detach <= 0 && !skip_maintenance_before_detach)
> + gc_before_detach(&opts, &cfg);
>
> if (!repository_format_precious_objects) {
> struct child_process repack_cmd = CHILD_PROCESS_INIT;
> @@ -1225,8 +1230,14 @@ static int maintenance_task_prefetch(struct maintenance_run_opts *opts,
> return 0;
> }
>
> -static int maintenance_task_gc(struct maintenance_run_opts *opts,
> - struct gc_config *cfg UNUSED)
> +static int maintenance_task_gc_before_detach(struct maintenance_run_opts *opts,
> + struct gc_config *cfg)
> +{
> + return gc_before_detach(opts, cfg);
> +}
> +
> +static int maintenance_task_gc_after_detach(struct maintenance_run_opts *opts,
> + struct gc_config *cfg UNUSED)
> {
> struct child_process child = CHILD_PROCESS_INIT;
>
> @@ -1240,6 +1251,7 @@ static int maintenance_task_gc(struct maintenance_run_opts *opts,
> else
> strvec_push(&child.args, "--no-quiet");
> strvec_push(&child.args, "--no-detach");
> + strvec_push(&child.args, "--skip-maintenance-before-detach");
I suspect this would be more obvious to me if I had the manual available right now, but if we are not detaching (« --no-detach ») why do we need to skip something before detaching (that presumably won’t happen)?
>
> return run_command(&child);
> }
> @@ -1561,7 +1573,8 @@ static const struct maintenance_task tasks[] = {
> },
> [TASK_GC] = {
> .name = "gc",
> - .after_detach = maintenance_task_gc,
> + .before_detach = maintenance_task_gc_before_detach,
> + .after_detach = maintenance_task_gc_after_detach,
> .auto_condition = need_to_gc,
> },
> [TASK_COMMIT_GRAPH] = {
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 1ada5246606..e09a36ab021 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -49,9 +49,9 @@ test_expect_success 'run [--auto|--quiet]' '
> git maintenance run --auto 2>/dev/null &&
> GIT_TRACE2_EVENT="$(pwd)/run-no-quiet.txt" \
> git maintenance run --no-quiet 2>/dev/null &&
> - test_subcommand git gc --quiet --no-detach <run-no-auto.txt &&
> - test_subcommand ! git gc --auto --quiet --no-detach <run-auto.txt &&
> - test_subcommand git gc --no-quiet --no-detach <run-no-quiet.txt
> + test_subcommand git gc --quiet --no-detach --skip-maintenance-before-detach <run-no-auto.txt &&
> + test_subcommand ! git gc --auto --quiet --no-detach --skip-maintenance-before-detach <run-auto.txt &&
> + test_subcommand git gc --no-quiet --no-detach --skip-maintenance-before-detach <run-no-quiet.txt
> '
>
> test_expect_success 'maintenance.auto config option' '
> @@ -154,9 +154,9 @@ test_expect_success 'run --task=<task>' '
> git maintenance run --task=commit-graph 2>/dev/null &&
> GIT_TRACE2_EVENT="$(pwd)/run-both.txt" \
> git maintenance run --task=commit-graph --task=gc 2>/dev/null &&
> - test_subcommand ! git gc --quiet --no-detach <run-commit-graph.txt &&
> - test_subcommand git gc --quiet --no-detach <run-gc.txt &&
> - test_subcommand git gc --quiet --no-detach <run-both.txt &&
> + test_subcommand ! git gc --quiet --no-detach --skip-maintenance-before-detach <run-commit-graph.txt &&
> + test_subcommand git gc --quiet --no-detach --skip-maintenance-before-detach <run-gc.txt &&
> + test_subcommand git gc --quiet --no-detach --skip-maintenance-before-detach <run-both.txt &&
> test_subcommand git commit-graph write --split --reachable --no-progress <run-commit-graph.txt &&
> test_subcommand ! git commit-graph write --split --reachable --no-progress <run-gc.txt &&
> test_subcommand git commit-graph write --split --reachable --no-progress <run-both.txt
>
> --
> 2.49.0.1266.g31b7d2e469.dirty
