builtin/maintenance: fix locking race when handling "gc" task

Git fork

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:

- We execute `gc_before_repack()` in the foreground, which contains
the logic that git-gc(1) itself would execute in the foreground, as
well.

- We spawn git-gc(1) after detaching, 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_foreground_tasks()` to
better reflect what this function does.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

authored by

Patrick Steinhardt and committed by

Junio C Hamano 9 months ago 1b5074e6 d2b084c6

+33 -20

2 changed files

expand all

unified split

builtin

gc.c

t7900-maintenance.sh

+27 -14

builtin/gc.c

··· 816 816 return ret; 817 817 } 818 818 819 - static int gc_before_repack(struct maintenance_run_opts *opts, 820 - struct gc_config *cfg) 819 + static int gc_foreground_tasks(struct maintenance_run_opts *opts, 820 + struct gc_config *cfg) 821 821 { 822 822 if (cfg->pack_refs && maintenance_task_pack_refs(opts, cfg)) 823 823 return error(FAILED_RUN, "pack-refs"); ··· 837 837 pid_t pid; 838 838 int daemonized = 0; 839 839 int keep_largest_pack = -1; 840 + int skip_foreground_tasks = 0; 840 841 timestamp_t dummy; 841 842 struct maintenance_run_opts opts = MAINTENANCE_RUN_OPTS_INIT; 842 843 struct gc_config cfg = GC_CONFIG_INIT; ··· 869 870 N_("repack all other packs except the largest pack")), 870 871 OPT_STRING(0, "expire-to", &cfg.repack_expire_to, N_("dir"), 871 872 N_("pack prefix to store a pack containing pruned objects")), 873 + OPT_HIDDEN_BOOL(0, "skip-foreground-tasks", &skip_foreground_tasks, 874 + N_("skip maintenance tasks typically done in the foreground")), 872 875 OPT_END() 873 876 }; 874 877 ··· 952 955 goto out; 953 956 } 954 957 955 - if (lock_repo_for_gc(force, &pid)) { 956 - ret = 0; 957 - goto out; 958 + if (!skip_foreground_tasks) { 959 + if (lock_repo_for_gc(force, &pid)) { 960 + ret = 0; 961 + goto out; 962 + } 963 + 964 + if (gc_foreground_tasks(&opts, &cfg) < 0) 965 + die(NULL); 966 + delete_tempfile(&pidfile); 958 967 } 959 968 960 - if (gc_before_repack(&opts, &cfg) < 0) 961 - die(NULL); 962 - delete_tempfile(&pidfile); 963 - 964 969 /* 965 970 * failure to daemonize is ok, we'll continue 966 971 * in foreground ··· 988 993 free(path); 989 994 } 990 995 991 - if (opts.detach <= 0) 992 - gc_before_repack(&opts, &cfg); 996 + if (opts.detach <= 0 && !skip_foreground_tasks) 997 + gc_foreground_tasks(&opts, &cfg); 993 998 994 999 if (!repository_format_precious_objects) { 995 1000 struct child_process repack_cmd = CHILD_PROCESS_INIT; ··· 1225 1230 return 0; 1226 1231 } 1227 1232 1228 - static int maintenance_task_gc(struct maintenance_run_opts *opts, 1229 - struct gc_config *cfg UNUSED) 1233 + static int maintenance_task_gc_foreground(struct maintenance_run_opts *opts, 1234 + struct gc_config *cfg) 1235 + { 1236 + return gc_foreground_tasks(opts, cfg); 1237 + } 1238 + 1239 + static int maintenance_task_gc_background(struct maintenance_run_opts *opts, 1240 + struct gc_config *cfg UNUSED) 1230 1241 { 1231 1242 struct child_process child = CHILD_PROCESS_INIT; 1232 1243 ··· 1240 1251 else 1241 1252 strvec_push(&child.args, "--no-quiet"); 1242 1253 strvec_push(&child.args, "--no-detach"); 1254 + strvec_push(&child.args, "--skip-foreground-tasks"); 1243 1255 1244 1256 return run_command(&child); 1245 1257 } ··· 1571 1583 }, 1572 1584 [TASK_GC] = { 1573 1585 .name = "gc", 1574 - .background = maintenance_task_gc, 1586 + .foreground = maintenance_task_gc_foreground, 1587 + .background = maintenance_task_gc_background, 1575 1588 .auto_condition = need_to_gc, 1576 1589 }, 1577 1590 [TASK_COMMIT_GRAPH] = {

+6 -6

t/t7900-maintenance.sh

··· 49 49 git maintenance run --auto 2>/dev/null && 50 50 GIT_TRACE2_EVENT="$(pwd)/run-no-quiet.txt" \ 51 51 git maintenance run --no-quiet 2>/dev/null && 52 - test_subcommand git gc --quiet --no-detach <run-no-auto.txt && 53 - test_subcommand ! git gc --auto --quiet --no-detach <run-auto.txt && 54 - test_subcommand git gc --no-quiet --no-detach <run-no-quiet.txt 52 + test_subcommand git gc --quiet --no-detach --skip-foreground-tasks <run-no-auto.txt && 53 + test_subcommand ! git gc --auto --quiet --no-detach --skip-foreground-tasks <run-auto.txt && 54 + test_subcommand git gc --no-quiet --no-detach --skip-foreground-tasks <run-no-quiet.txt 55 55 ' 56 56 57 57 test_expect_success 'maintenance.auto config option' ' ··· 154 154 git maintenance run --task=commit-graph 2>/dev/null && 155 155 GIT_TRACE2_EVENT="$(pwd)/run-both.txt" \ 156 156 git maintenance run --task=commit-graph --task=gc 2>/dev/null && 157 - test_subcommand ! git gc --quiet --no-detach <run-commit-graph.txt && 158 - test_subcommand git gc --quiet --no-detach <run-gc.txt && 159 - test_subcommand git gc --quiet --no-detach <run-both.txt && 157 + test_subcommand ! git gc --quiet --no-detach --skip-foreground-tasks <run-commit-graph.txt && 158 + test_subcommand git gc --quiet --no-detach --skip-foreground-tasks <run-gc.txt && 159 + test_subcommand git gc --quiet --no-detach --skip-foreground-tasks <run-both.txt && 160 160 test_subcommand git commit-graph write --split --reachable --no-progress <run-commit-graph.txt && 161 161 test_subcommand ! git commit-graph write --split --reachable --no-progress <run-gc.txt && 162 162 test_subcommand git commit-graph write --split --reachable --no-progress <run-both.txt