{"thread":{"id":"65615","subject":"[PATCH 1/2] builtin/maintenance: fix locking with \"--detach\"","startedAt":"2026-05-11T12:30:08Z","lastAt":"2026-05-21T07:45:18Z","messageCount":18,"participants":["Patrick Steinhardt","Jeff King","Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"543047","messageId":"20260511-pks-maintenance-fix-lock-with-detach-v1-1-ccd7d62c9a40@pks.im","threadId":"65615","inReplyTo":"20260511-pks-maintenance-fix-lock-with-detach-v1-0-ccd7d62c9a40@pks.im","subject":"[PATCH 1/2] builtin/maintenance: fix locking with \"--detach\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-05-11T12:29:55Z","receivedAt":"2026-05-11T12:30:08Z","isPatch":true,"body":"When running git-maintenance(1), we create a lockfile that is supposed\nto keep other maintenance processes from running at the same time. This\nlockfile is broken though in case the \"--detach\" flag is passed: the\nlockfile is created by the parent process and will be cleaned up either\nmanually or on exit. But when detaching, the parent will exit before all\nof the background maintenance tasks have been ran, and consequently the\nlock only covers a smaller part of the whole maintenance process.\n\nFix this bug by introducing two new functions:\n\n  - `daemonize_without_exit()` is the same as `daemonize()`, but doesn't\n    call exit(3p) for the parent process.\n\n  - `lock_file_reassign_owner()` reassigns the owner of its owned\n    tempfiles so that they don't get unlinked anymore when the previous\n    owner exits.\n\nTogether this allows us to reassign ownership of the lockfile after we\nhave daemonized so that the lockfile is now owned by the child process.\n\nReported-by: Jean-Christophe Manciot <actionmystique@gmail.com>\nHelped-by: Jeff King <peff@peff.net>\nHelped-by: Taylor Blau <me@ttaylorr.com>\nHelped-by: Derrick Stolee <stolee@gmail.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/gc.c           | 26 ++++++++++++++++++++--\n lockfile.c             |  9 ++++++++\n lockfile.h             | 10 +++++++++\n setup.c                | 31 +++++++++++++++++++--------\n setup.h                |  1 +\n t/t7900-maintenance.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 124 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 3a71e314c9..09cb92ac97 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1810,10 +1810,32 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,\n \t\t\t\t   TASK_PHASE_FOREGROUND))\n \t\t\tresult = 1;\n \n-\t/* Failure to daemonize is ok, we'll continue in foreground. */\n \tif (opts->detach > 0) {\n+\t\tpid_t child_pid;\n+\n \t\ttrace2_region_enter(\"maintenance\", \"detach\", the_repository);\n-\t\tdaemonize();\n+\n+\t\tchild_pid = daemonize_without_exit();\n+\t\tif (!child_pid) {\n+\t\t\t/*\n+\t\t\t * We're in the child process, so we take ownership of\n+\t\t\t * the lockfile.\n+\t\t\t */\n+\t\t\tlock_file_reassign_owner(&lk, getpid());\n+\t\t} else if (child_pid > 0) {\n+\t\t\t/*\n+\t\t\t * We're in the parent process, so we assign ownership\n+\t\t\t * of the lockfile to the child and then exit immediately.\n+\t\t\t */\n+\t\t\tlock_file_reassign_owner(&lk, child_pid);\n+\t\t\texit(0);\n+\t\t} else {\n+\t\t\t/*\n+\t\t\t * Failure to daemonize is ok, we'll continue in\n+\t\t\t * foreground.\n+\t\t\t */\n+\t\t}\n+\n \t\ttrace2_region_leave(\"maintenance\", \"detach\", the_repository);\n \t}\n \ndiff --git a/lockfile.c b/lockfile.c\nindex 7add2f136a..96aab3c885 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -356,3 +356,12 @@ int rollback_lock_file(struct lock_file *lk)\n \tdelete_tempfile(&lk->pid_tempfile);\n \treturn delete_tempfile(&lk->tempfile);\n }\n+\n+void lock_file_reassign_owner(struct lock_file *lk, pid_t owner)\n+{\n+\tif (!is_lock_file_locked(lk))\n+\t\tBUG(\"cannot reassign ownership of unlocked lockfile\");\n+\tlk->tempfile->owner = owner;\n+\tif (lk->pid_tempfile)\n+\t\tlk->pid_tempfile->owner = owner;\n+}\ndiff --git a/lockfile.h b/lockfile.h\nindex e7233f28de..0b10b624fa 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -341,4 +341,14 @@ static inline int commit_lock_file_to(struct lock_file *lk, const char *path)\n  */\n int rollback_lock_file(struct lock_file *lk);\n \n+/*\n+ * Reassign ownership of the lockfile to a different process.\n+ *\n+ * This is intended for use after `fork(2)`-ing. The parent transfers ownership\n+ * to the daemonized child so that its atexit handler does not unlink the lock\n+ * that should outlive it, and the child claims the inherited tempfiles so that\n+ * they are cleaned up when the daemon exits.\n+ */\n+void lock_file_reassign_owner(struct lock_file *lk, pid_t owner);\n+\n #endif /* LOCKFILE_H */\ndiff --git a/setup.c b/setup.c\nindex 7ec4427368..34deb6e985 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -2156,20 +2156,18 @@ void sanitize_stdfds(void)\n \t\tclose(fd);\n }\n \n-int daemonize(void)\n+pid_t daemonize_without_exit(void)\n {\n #ifdef NO_POSIX_GOODIES\n \terrno = ENOSYS;\n \treturn -1;\n #else\n-\tswitch (fork()) {\n-\t\tcase 0:\n-\t\t\tbreak;\n-\t\tcase -1:\n-\t\t\tdie_errno(_(\"fork failed\"));\n-\t\tdefault:\n-\t\t\texit(0);\n-\t}\n+\tpid_t pid = fork();\n+\tif (pid < 0)\n+\t\treturn -1;\n+\tif (pid > 0)\n+\t\treturn pid;\n+\n \tif (setsid() == -1)\n \t\tdie_errno(_(\"setsid failed\"));\n \tclose(0);\n@@ -2180,6 +2178,21 @@ int daemonize(void)\n #endif\n }\n \n+int daemonize(void)\n+{\n+#ifdef NO_POSIX_GOODIES\n+\terrno = ENOSYS;\n+\treturn -1;\n+#else\n+\tpid_t pid = daemonize_without_exit();\n+\tif (pid < 0)\n+\t\tdie_errno(_(\"fork failed\"));\n+\tif (pid > 0)\n+\t\texit(0);\n+\treturn 0;\n+#endif\n+}\n+\n struct template_dir_cb_data {\n \tchar *path;\n \tint initialized;\ndiff --git a/setup.h b/setup.h\nindex 80bc6e5f07..396af8d808 100644\n--- a/setup.h\n+++ b/setup.h\n@@ -150,6 +150,7 @@ int path_inside_repo(const char *prefix, const char *path);\n \n void sanitize_stdfds(void);\n int daemonize(void);\n+pid_t daemonize_without_exit(void);\n \n /*\n  * GIT_REPO_VERSION is the version we write by default. The\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 4700beacc1..df0bbc1669 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -1438,6 +1438,64 @@ test_expect_success '--no-detach causes maintenance to not run in background' '\n \t)\n '\n \n+test_expect_success PIPE '--detach holds maintenance lock until daemonized child exits' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\tgit config maintenance.auto false &&\n+\t\tgit config core.lockfilepid true &&\n+\n+\t\tgit remote add origin /does/not/exist &&\n+\t\tgit config set remote.origin.uploadpack \"cat fifo-uploadpack\" &&\n+\n+\t\tmkfifo fifo-uploadpack fifo-maint &&\n+\n+\t\t# Open the maintenance FIFO, as otherwise spawning\n+\t\t# git-maintenance(1) would block. Note that we need to open it\n+\t\t# as read-write, as otherwise we would block here already.\n+\t\texec 9<>fifo-maint &&\n+\n+\t\t{ git maintenance run --task=prefetch --detach 7>&9 & } &&\n+\t\tparent=\"$!\" &&\n+\n+\t\t# Reap the parent process so that the exec call below will not\n+\t\t# get SIGCHLD.\n+\t\twait \"$parent\" &&\n+\n+\t\t# Open the git-upload-pack(1) FIFO for writing, which will\n+\t\t# block until the upload-pack script opens it for reading. Once\n+\t\t# exec returns, we know that the daemonized child is alive and\n+\t\t# pinned.\n+\t\texec 8>fifo-uploadpack &&\n+\n+\t\ttest_path_is_file .git/objects/maintenance.lock &&\n+\t\ttest_path_is_file .git/objects/\"maintenance~pid.lock\" &&\n+\n+\t\t# Verify that the maintenance.lock still exists, and\n+\t\t# that it was created by the parent process, not the\n+\t\t# child.\n+\t\techo \"pid $parent\" >expect &&\n+\t\ttest_cmp expect .git/objects/\"maintenance~pid.lock\" &&\n+\n+\t\t# Reopen the maintenance FIFO as read-only so that\n+\t\t# git-maintenance(1) is the only writer. This will cause it to\n+\t\t# close the FIFO once the process exits.\n+\t\texec 9<&- &&\n+\t\texec 9<fifo-maint &&\n+\n+\t\t# Close the FIFO used by git-upload-pack(1) to unblock it and\n+\t\t# then wait until the maintenance FIFO is closed by\n+\t\t# git-maintenance(1), indicating that it has exited.\n+\t\texec 8>&- &&\n+\t\tcat <&9 &&\n+\n+\t\ttest_path_is_missing .git/objects/maintenance.lock &&\n+\t\ttest_path_is_missing .git/objects/\"maintenance~pid.lock\"\n+\t)\n+'\n+\n test_expect_success '--detach causes maintenance to run in background' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n\n-- \n2.54.0.545.g6539524ca2.dirty\n\n"},{"id":"543048","messageId":"20260511-pks-maintenance-fix-lock-with-detach-v1-0-ccd7d62c9a40@pks.im","threadId":"65615","inReplyTo":null,"subject":"[PATCH 0/2] builtin/maintenance: fix locking and respect \"gc.auto\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-05-11T12:29:54Z","receivedAt":"2026-05-11T12:30:09Z","isPatch":true,"body":"Hi,\n\nthis patch series addresses the issues reported in [1]. The series is\nbuilt on top of Git 2.54.0.\n\nThanks!\n\nPatrick\n\n[1]: <CAKcFC3arsYExb5dCMQspo4V9UFDadFaj8Q4PUsMWZJw_eYrMzA@mail.gmail.com>\n\n---\nPatrick Steinhardt (2):\n      builtin/maintenance: fix locking with \"--detach\"\n      run-command: honor \"gc.auto\" for auto-maintenance\n\n builtin/gc.c           | 26 ++++++++++++--\n lockfile.c             |  9 +++++\n lockfile.h             | 10 ++++++\n run-command.c          |  6 ++--\n setup.c                | 31 +++++++++++-----\n setup.h                |  1 +\n t/t7900-maintenance.sh | 95 ++++++++++++++++++++++++++++++++++++++++++++------\n 7 files changed, 154 insertions(+), 24 deletions(-)\n\n\n---\nbase-commit: 13ef77ce6e222bef3ab145642e6ef1486075211c\nchange-id: 20260511-pks-maintenance-fix-lock-with-detach-a608e9b6adeb\n\n"},{"id":"543049","messageId":"20260511-pks-maintenance-fix-lock-with-detach-v1-2-ccd7d62c9a40@pks.im","threadId":"65615","inReplyTo":"20260511-pks-maintenance-fix-lock-with-detach-v1-0-ccd7d62c9a40@pks.im","subject":"[PATCH 2/2] run-command: honor \"gc.auto\" for auto-maintenance","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-05-11T12:29:56Z","receivedAt":"2026-05-11T12:30:10Z","isPatch":true,"body":"The \"gc.auto\" configuration has traditionally been used to turn off\nrunning git-gc(1) as part of our auto-maintenance. We have eventually\nswitched over to git-maintenance(1) in a95ce12430 (maintenance: replace\nrun_auto_gc(), 2020-09-17), and with 1942d48380 (maintenance: optionally\nskip --auto process, 2020-08-28) we have introduced \"maintenance.auto\"\nto control whether or not to run auto-maintenance.\n\nAt that point though we still shelled out to git-gc(1) internally. So\nif \"gc.auto=0\" was set we would still _execute_ git-maintenance(1), but\nthe command would have exited fast because git-gc(1) itself knew to\nhonor the config key.\n\nThis has recently changed though, as we have adapted the default\nmaintenance strategy to not use git-gc(1) anymore. The consequence is\nthat \"gc.auto=0\" doesn't have an effect anymore, which is a somewhat\nsurprising change in behaviour for our users.\n\nAdapt `run_auto_maintenance()` so that it knows to also read \"gc.auto\",\nsimilar to how it also reads both \"maintenance.autoDetach\" and\n\"gc.autoDetach\".\n\nReported-by: Jean-Christophe Manciot <actionmystique@gmail.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n run-command.c          |  6 ++++--\n t/t7900-maintenance.sh | 37 ++++++++++++++++++++++++++-----------\n 2 files changed, 30 insertions(+), 13 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex c146a56532..1e7b789010 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1946,8 +1946,10 @@ int prepare_auto_maintenance(struct repository *r, int quiet,\n {\n \tint enabled, auto_detach;\n \n-\tif (!repo_config_get_bool(r, \"maintenance.auto\", &enabled) &&\n-\t    !enabled)\n+\tif (repo_config_get_bool(r, \"maintenance.auto\", &enabled) &&\n+\t    repo_config_get_bool(r, \"gc.auto\", &enabled))\n+\t\tenabled = 1;\n+\tif (!enabled)\n \t\treturn 0;\n \n \t/*\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex df0bbc1669..1f70462678 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -60,17 +60,32 @@ test_expect_success 'run [--auto|--quiet] with gc strategy' '\n \ttest_subcommand git gc --no-quiet --no-detach --skip-foreground-tasks <run-no-quiet.txt\n '\n \n-test_expect_success 'maintenance.auto config option' '\n-\tGIT_TRACE2_EVENT=\"$(pwd)/default\" git commit --quiet --allow-empty -m 1 &&\n-\ttest_subcommand git maintenance run --auto --quiet --detach <default &&\n-\tGIT_TRACE2_EVENT=\"$(pwd)/true\" \\\n-\t\tgit -c maintenance.auto=true \\\n-\t\tcommit --quiet --allow-empty -m 2 &&\n-\ttest_subcommand git maintenance run --auto --quiet --detach <true &&\n-\tGIT_TRACE2_EVENT=\"$(pwd)/false\" \\\n-\t\tgit -c maintenance.auto=false \\\n-\t\tcommit --quiet --allow-empty -m 3 &&\n-\ttest_subcommand ! git maintenance run --auto --quiet --detach <false\n+for cfg in maintenance.auto gc.auto\n+do\n+\ttest_expect_success \"$cfg config option\" '\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/default\" git commit --quiet --allow-empty -m 1 &&\n+\t\ttest_subcommand git maintenance run --auto --quiet --detach <default &&\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/true\" \\\n+\t\t\tgit -c $cfg=true commit --quiet --allow-empty -m 2 &&\n+\t\ttest_subcommand git maintenance run --auto --quiet --detach <true &&\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/false\" \\\n+\t\t\tgit -c $cfg=false commit --quiet --allow-empty -m 3 &&\n+\t\ttest_subcommand ! git maintenance run --auto --quiet --detach <false\n+\t'\n+done\n+\n+test_expect_success \"maintenance.auto overrides gc.auto\" '\n+\ttest_when_finished \"rm -f trace\" &&\n+\n+\ttest_config maintenance.auto false &&\n+\ttest_config gc.auto true &&\n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace\" git commit --quiet --allow-empty -m 1 &&\n+\ttest_subcommand ! git maintenance run --auto --quiet --detach <trace &&\n+\n+\ttest_config maintenance.auto true &&\n+\ttest_config gc.auto false &&\n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace\" git commit --quiet --allow-empty -m 1 &&\n+\ttest_subcommand git maintenance run --auto --quiet --detach <trace\n '\n \n for cfg in maintenance.autoDetach gc.autoDetach\n\n-- \n2.54.0.545.g6539524ca2.dirty\n\n"},{"id":"543090","messageId":"20260511201800.GC22912@coredump.intra.peff.net","threadId":"65615","inReplyTo":"20260511-pks-maintenance-fix-lock-with-detach-v1-2-ccd7d62c9a40@pks.im","subject":"Re: [PATCH 2/2] run-command: honor \"gc.auto\" for auto-maintenance","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-11T20:18:00Z","receivedAt":"2026-05-11T20:18:02Z","isPatch":true,"body":"On Mon, May 11, 2026 at 02:29:56PM +0200, Patrick Steinhardt wrote:\n\n> @@ -1946,8 +1946,10 @@ int prepare_auto_maintenance(struct repository *r, int quiet,\n>  {\n>  \tint enabled, auto_detach;\n>  \n> -\tif (!repo_config_get_bool(r, \"maintenance.auto\", &enabled) &&\n> -\t    !enabled)\n> +\tif (repo_config_get_bool(r, \"maintenance.auto\", &enabled) &&\n> +\t    repo_config_get_bool(r, \"gc.auto\", &enabled))\n> +\t\tenabled = 1;\n> +\tif (!enabled)\n>  \t\treturn 0;\n\ngc.auto isn't a bool; it's the count of loose objects after which to run\nmaintenance. So \"0\" works in both contexts, but will we complain if\ngc.auto is set to 100? I think maybe not, because we fall back to\ngit_parse_int(), but it feels kind of fragile.\n\nThe gc code uses repo_config_get_int() here.\n\n-Peff\n"},{"id":"543119","messageId":"xmqq4ikdqvad.fsf@gitster.g","threadId":"65615","inReplyTo":"20260511-pks-maintenance-fix-lock-with-detach-v1-1-ccd7d62c9a40@pks.im","subject":"Re: [PATCH 1/2] builtin/maintenance: fix locking with \"--detach\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-05-12T01:19:22Z","receivedAt":"2026-05-12T01:19:25Z","isPatch":true,"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 3a71e314c9..09cb92ac97 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -1810,10 +1810,32 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,\n>  \t\t\t\t   TASK_PHASE_FOREGROUND))\n>  \t\t\tresult = 1;\n>  \n> -\t/* Failure to daemonize is ok, we'll continue in foreground. */\n>  \tif (opts->detach > 0) {\n> +\t\tpid_t child_pid;\n> +\n>  \t\ttrace2_region_enter(\"maintenance\", \"detach\", the_repository);\n> -\t\tdaemonize();\n> +\n> +\t\tchild_pid = daemonize_without_exit();\n> +\t\tif (!child_pid) {\n> +\t\t\t/*\n> +\t\t\t * We're in the child process, so we take ownership of\n> +\t\t\t * the lockfile.\n> +\t\t\t */\n> +\t\t\tlock_file_reassign_owner(&lk, getpid());\n> +\t\t} else if (child_pid > 0) {\n> +\t\t\t/*\n> +\t\t\t * We're in the parent process, so we assign ownership\n> +\t\t\t * of the lockfile to the child and then exit immediately.\n> +\t\t\t */\n> +\t\t\tlock_file_reassign_owner(&lk, child_pid);\n> +\t\t\texit(0);\n\nThe point of reassigning the owner to somebody else is so that we\nwon't clean them when we exit as the tempfile.c::remove_tempfile()\nfunction checks the \"owner\" is \"me\" and refrains from unlinking\nthose that do not belong to us, so there is nothing wrong in this\ncode, but this somehow felt awkward.  In a sense, child_pid here\ndoes not have to be what fork() returned but anything that is not\nour own pid.  Perhaps \"we assign ... to the child\" -> \"we relinquish\n... to prevent us removing upon exiting\" would convey the intention\nbetter?  I dunno.\n\n> -int daemonize(void)\n> +pid_t daemonize_without_exit(void)\n>  {\n>  #ifdef NO_POSIX_GOODIES\n>  \terrno = ENOSYS;\n>  \treturn -1;\n>  #else\n> -\tswitch (fork()) {\n> -\t\tcase 0:\n> -\t\t\tbreak;\n> -\t\tcase -1:\n> -\t\t\tdie_errno(_(\"fork failed\"));\n> -\t\tdefault:\n> -\t\t\texit(0);\n> -\t}\n> +\tpid_t pid = fork();\n> +\tif (pid < 0)\n> +\t\treturn -1;\n> +\tif (pid > 0)\n> +\t\treturn pid;\n> +\n>  \tif (setsid() == -1)\n>  \t\tdie_errno(_(\"setsid failed\"));\n>  \tclose(0);\n> @@ -2180,6 +2178,21 @@ int daemonize(void)\n>  #endif\n>  }\n>  \n> +int daemonize(void)\n> +{\n> +#ifdef NO_POSIX_GOODIES\n> +\terrno = ENOSYS;\n> +\treturn -1;\n> +#else\n> +\tpid_t pid = daemonize_without_exit();\n> +\tif (pid < 0)\n> +\t\tdie_errno(_(\"fork failed\"));\n> +\tif (pid > 0)\n> +\t\texit(0);\n> +\treturn 0;\n> +#endif\n> +}\n\nI was hoping that we can do without the #ifdef in this caller as\ndaemonize_without_exit() already has exactly the same condtional\ncompilation.  If the NO_POSIX_GOODIES side can just return silently\nwit ENOSYS, shouldn't the callers be also fine if we return failure\ninstead of calling die_errno(_(\"fork failed\")), I have to wonder.\n\nBut because (1) as long as we have to call die_errno() here, we must\nkeep the conditional compilation in daemonize() as well as\ndaemonize_without_exit(), and (2) changing what the callers get when\nfork failed here is totally outside of this topic, I would say that\nthe code around here is good as-is.\n\n"},{"id":"543120","messageId":"xmqqzf25pgm0.fsf@gitster.g","threadId":"65615","inReplyTo":"20260511201800.GC22912@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] run-command: honor \"gc.auto\" for auto-maintenance","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-05-12T01:21:43Z","receivedAt":"2026-05-12T01:21:46Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, May 11, 2026 at 02:29:56PM +0200, Patrick Steinhardt wrote:\n>\n>> @@ -1946,8 +1946,10 @@ int prepare_auto_maintenance(struct repository *r, int quiet,\n>>  {\n>>  \tint enabled, auto_detach;\n>>  \n>> -\tif (!repo_config_get_bool(r, \"maintenance.auto\", &enabled) &&\n>> -\t    !enabled)\n>> +\tif (repo_config_get_bool(r, \"maintenance.auto\", &enabled) &&\n>> +\t    repo_config_get_bool(r, \"gc.auto\", &enabled))\n>> +\t\tenabled = 1;\n>> +\tif (!enabled)\n>>  \t\treturn 0;\n>\n> gc.auto isn't a bool; it's the count of loose objects after which to run\n> maintenance. So \"0\" works in both contexts, but will we complain if\n> gc.auto is set to 100? I think maybe not, because we fall back to\n> git_parse_int(), but it feels kind of fragile.\n>\n> The gc code uses repo_config_get_int() here.\n>\n> -Peff\n\nVery good point.  I was about to send the same message ;-)\n"},{"id":"543137","messageId":"agLBsD3y6x2ehO7G@pks.im","threadId":"65615","inReplyTo":"xmqqzf25pgm0.fsf@gitster.g","subject":"Re: [PATCH 2/2] run-command: honor \"gc.auto\" for auto-maintenance","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-05-12T05:59:12Z","receivedAt":"2026-05-12T05:59:20Z","isPatch":true,"body":"On Tue, May 12, 2026 at 10:21:43AM +0900, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n> > On Mon, May 11, 2026 at 02:29:56PM +0200, Patrick Steinhardt wrote:\n> >\n> >> @@ -1946,8 +1946,10 @@ int prepare_auto_maintenance(struct repository *r, int quiet,\n> >>  {\n> >>  \tint enabled, auto_detach;\n> >>  \n> >> -\tif (!repo_config_get_bool(r, \"maintenance.auto\", &enabled) &&\n> >> -\t    !enabled)\n> >> +\tif (repo_config_get_bool(r, \"maintenance.auto\", &enabled) &&\n> >> +\t    repo_config_get_bool(r, \"gc.auto\", &enabled))\n> >> +\t\tenabled = 1;\n> >> +\tif (!enabled)\n> >>  \t\treturn 0;\n> >\n> > gc.auto isn't a bool; it's the count of loose objects after which to run\n> > maintenance. So \"0\" works in both contexts, but will we complain if\n> > gc.auto is set to 100? I think maybe not, because we fall back to\n> > git_parse_int(), but it feels kind of fragile.\n> >\n> > The gc code uses repo_config_get_int() here.\n> >\n> > -Peff\n> \n> Very good point.  I was about to send the same message ;-)\n\nUgh, true indeed. Will fix, thanks!\n\nPatrick\n"},{"id":"543138","messageId":"agLBtvKtvWFn64-Y@pks.im","threadId":"65615","inReplyTo":"xmqq4ikdqvad.fsf@gitster.g","subject":"Re: [PATCH 1/2] builtin/maintenance: fix locking with \"--detach\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-05-12T05:59:18Z","receivedAt":"2026-05-12T05:59:27Z","isPatch":true,"body":"On Tue, May 12, 2026 at 10:19:22AM +0900, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > diff --git a/builtin/gc.c b/builtin/gc.c\n> > index 3a71e314c9..09cb92ac97 100644\n> > --- a/builtin/gc.c\n> > +++ b/builtin/gc.c\n> > @@ -1810,10 +1810,32 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,\n> >  \t\t\t\t   TASK_PHASE_FOREGROUND))\n> >  \t\t\tresult = 1;\n> >  \n> > -\t/* Failure to daemonize is ok, we'll continue in foreground. */\n> >  \tif (opts->detach > 0) {\n> > +\t\tpid_t child_pid;\n> > +\n> >  \t\ttrace2_region_enter(\"maintenance\", \"detach\", the_repository);\n> > -\t\tdaemonize();\n> > +\n> > +\t\tchild_pid = daemonize_without_exit();\n> > +\t\tif (!child_pid) {\n> > +\t\t\t/*\n> > +\t\t\t * We're in the child process, so we take ownership of\n> > +\t\t\t * the lockfile.\n> > +\t\t\t */\n> > +\t\t\tlock_file_reassign_owner(&lk, getpid());\n> > +\t\t} else if (child_pid > 0) {\n> > +\t\t\t/*\n> > +\t\t\t * We're in the parent process, so we assign ownership\n> > +\t\t\t * of the lockfile to the child and then exit immediately.\n> > +\t\t\t */\n> > +\t\t\tlock_file_reassign_owner(&lk, child_pid);\n> > +\t\t\texit(0);\n> \n> The point of reassigning the owner to somebody else is so that we\n> won't clean them when we exit as the tempfile.c::remove_tempfile()\n> function checks the \"owner\" is \"me\" and refrains from unlinking\n> those that do not belong to us, so there is nothing wrong in this\n> code, but this somehow felt awkward.  In a sense, child_pid here\n> does not have to be what fork() returned but anything that is not\n> our own pid.  Perhaps \"we assign ... to the child\" -> \"we relinquish\n> ... to prevent us removing upon exiting\" would convey the intention\n> better?  I dunno.\n\nFair. This is what I got now:\n\n\t/*\n\t * We're in the parent process, so we drop ownership of\n\t * the lockfile to prevent us from removing it upon\n\t * exit.\n\t */\n\n> > -int daemonize(void)\n> > +pid_t daemonize_without_exit(void)\n> >  {\n> >  #ifdef NO_POSIX_GOODIES\n> >  \terrno = ENOSYS;\n> >  \treturn -1;\n> >  #else\n> > -\tswitch (fork()) {\n> > -\t\tcase 0:\n> > -\t\t\tbreak;\n> > -\t\tcase -1:\n> > -\t\t\tdie_errno(_(\"fork failed\"));\n> > -\t\tdefault:\n> > -\t\t\texit(0);\n> > -\t}\n> > +\tpid_t pid = fork();\n> > +\tif (pid < 0)\n> > +\t\treturn -1;\n> > +\tif (pid > 0)\n> > +\t\treturn pid;\n> > +\n> >  \tif (setsid() == -1)\n> >  \t\tdie_errno(_(\"setsid failed\"));\n> >  \tclose(0);\n> > @@ -2180,6 +2178,21 @@ int daemonize(void)\n> >  #endif\n> >  }\n> >  \n> > +int daemonize(void)\n> > +{\n> > +#ifdef NO_POSIX_GOODIES\n> > +\terrno = ENOSYS;\n> > +\treturn -1;\n> > +#else\n> > +\tpid_t pid = daemonize_without_exit();\n> > +\tif (pid < 0)\n> > +\t\tdie_errno(_(\"fork failed\"));\n> > +\tif (pid > 0)\n> > +\t\texit(0);\n> > +\treturn 0;\n> > +#endif\n> > +}\n> \n> I was hoping that we can do without the #ifdef in this caller as\n> daemonize_without_exit() already has exactly the same condtional\n> compilation.  If the NO_POSIX_GOODIES side can just return silently\n> wit ENOSYS, shouldn't the callers be also fine if we return failure\n> instead of calling die_errno(_(\"fork failed\")), I have to wonder.\n> \n> But because (1) as long as we have to call die_errno() here, we must\n> keep the conditional compilation in daemonize() as well as\n> daemonize_without_exit(), and (2) changing what the callers get when\n> fork failed here is totally outside of this topic, I would say that\n> the code around here is good as-is.\n\nYeah, I was also pondering whether I can drop the additional ifdef. But\nI eventually decided to aim for the most minimal fix that has the least\npotential for additional regressions. So I aimed to keep `daemonize()`\nsemantically the same as before, and I aimed to only fix the issue with\nthe lockfile we know about.\n\nI agree though that this is something we should probably clean up in a\nsubsequent series. We don't have that many callers of `daemonize()`\nafter all, so it shouldn't be that involved, either.\n\nThanks!\n\nPatrick\n"},{"id":"543231","messageId":"20260513-pks-maintenance-fix-lock-with-detach-v3-0-f27a1ac82891@pks.im","threadId":"65615","inReplyTo":"20260511-pks-maintenance-fix-lock-with-detach-v1-0-ccd7d62c9a40@pks.im","subject":"[PATCH v3 0/2] builtin/maintenance: fix locking and respect \"gc.auto\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-05-13T07:31:12Z","receivedAt":"2026-05-13T07:31:20Z","isPatch":true,"body":"Hi,\n\nthis patch series addresses the issues reported in [1]. The series is\nbuilt on top of Git 2.54.0.\n\nChanges in v3:\n  - Use Taylor's approach to reassign all tempfiles when daemonizing.\n  - Link to v2: https://patch.msgid.link/20260512-pks-maintenance-fix-lock-with-detach-v2-0-dc6f2d284b6d@pks.im\n\nChanges in v2:\n  - Clarify comment when dropping ownership of the lock in the parent\n    process.\n  - Properly treat \"gc.auto\" as an integer, not a boolean.\n  - Link to v1: https://patch.msgid.link/20260511-pks-maintenance-fix-lock-with-detach-v1-0-ccd7d62c9a40@pks.im\n\nThanks!\n\nPatrick\n\n[1]: <CAKcFC3arsYExb5dCMQspo4V9UFDadFaj8Q4PUsMWZJw_eYrMzA@mail.gmail.com>\n\n---\nPatrick Steinhardt (2):\n      builtin/maintenance: fix locking with \"--detach\"\n      run-command: honor \"gc.auto\" for auto-maintenance\n\n run-command.c          | 10 ++++--\n setup.c                | 16 +++++++++-\n setup.h                | 15 +++++++++\n t/t7900-maintenance.sh | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++\n tempfile.c             | 12 ++++++++\n tempfile.h             | 11 +++++++\n 6 files changed, 143 insertions(+), 4 deletions(-)\n\nRange-diff versus v2:\n\n1:  5eb608ad43 < -:  ---------- builtin/maintenance: fix locking with \"--detach\"\n-:  ---------- > 1:  3fc8872f36 builtin/maintenance: fix locking with \"--detach\"\n2:  15e2ae8104 = 2:  665a6c9a50 run-command: honor \"gc.auto\" for auto-maintenance\n\n---\nbase-commit: 13ef77ce6e222bef3ab145642e6ef1486075211c\nchange-id: 20260511-pks-maintenance-fix-lock-with-detach-a608e9b6adeb\n\n"},{"id":"543232","messageId":"20260513-pks-maintenance-fix-lock-with-detach-v3-1-f27a1ac82891@pks.im","threadId":"65615","inReplyTo":"20260513-pks-maintenance-fix-lock-with-detach-v3-0-f27a1ac82891@pks.im","subject":"[PATCH v3 1/2] builtin/maintenance: fix locking with \"--detach\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-05-13T07:31:13Z","receivedAt":"2026-05-13T07:31:22Z","isPatch":true,"body":"When running git-maintenance(1), we create a lockfile that is supposed\nto keep other maintenance processes from running at the same time. This\nlockfile is broken though in case the \"--detach\" flag is passed: the\nlockfile is created by the parent process and will be cleaned up either\nmanually or on exit. But when detaching, the parent will exit before all\nof the background maintenance tasks have been run, and consequently the\nlock only covers a smaller part of the whole maintenance process.\n\nFix this bug by reassigning all tempfiles from the parent process to the\nchild process when daemonizing so that it becomes the responsibility of\nthe child to clean them up.\n\nNote that this is a broader fix, as we now always reassign tempfiles\nwhen daemonizing. This is a natural consequence of the semantics of\n`daemonize()` though, as it essentially promises to continue running the\ncurrent process in the background. It is thus sensible to have that\nfunction perform the whole dance of assigning resources to the child\nprocess, including tempfiles.\n\nThere's only a single other caller in \"daemon.c\", but that process\ndoesn't create any tempfiles before the call to `daemonize()` and is\nthus not impacted by this change.\n\nReported-by: Jean-Christophe Manciot <actionmystique@gmail.com>\nHelped-by: Jeff King <peff@peff.net>\nHelped-by: Derrick Stolee <stolee@gmail.com>\nCo-authored-by: Taylor Blau <me@ttaylorr.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n setup.c                | 16 +++++++++++++-\n setup.h                | 15 +++++++++++++\n t/t7900-maintenance.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++\n tempfile.c             | 12 +++++++++++\n tempfile.h             | 11 ++++++++++\n 5 files changed, 111 insertions(+), 1 deletion(-)\n\ndiff --git a/setup.c b/setup.c\nindex 7ec4427368..14445a71a4 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -2162,12 +2162,26 @@ int daemonize(void)\n \terrno = ENOSYS;\n \treturn -1;\n #else\n-\tswitch (fork()) {\n+\tpid_t parent_pid = getpid();\n+\tpid_t child_pid = fork();\n+\n+\tswitch (child_pid) {\n \t\tcase 0:\n+\t\t\t/*\n+\t\t\t * We're in the child process, so we take ownership of\n+\t\t\t * all tempfiles.\n+\t\t\t */\n+\t\t\treassign_tempfile_ownership(parent_pid, getpid());\n \t\t\tbreak;\n \t\tcase -1:\n \t\t\tdie_errno(_(\"fork failed\"));\n \t\tdefault:\n+\t\t\t/*\n+\t\t\t * We're in the parent process, so we drop ownership of\n+\t\t\t * all tempfiles to prevent us from removing them upon\n+\t\t\t * exit.\n+\t\t\t */\n+\t\t\treassign_tempfile_ownership(parent_pid, child_pid);\n \t\t\texit(0);\n \t}\n \tif (setsid() == -1)\ndiff --git a/setup.h b/setup.h\nindex 80bc6e5f07..b5bc5f280c 100644\n--- a/setup.h\n+++ b/setup.h\n@@ -149,6 +149,21 @@ void verify_non_filename(const char *prefix, const char *name);\n int path_inside_repo(const char *prefix, const char *path);\n \n void sanitize_stdfds(void);\n+\n+/*\n+ * Daemonize the current process by forking and then exiting the parent\n+ * process. Returns 0 when successful, in which case the parent process will\n+ * have exited and it's the child process that continues to run the code.\n+ * Otherwise, a negative error code is returned and the parent process will\n+ * continue execution.\n+ *\n+ * Note that this function will also perform the following changes:\n+ *\n+ *   - Standard file descriptors in the child process are closed.\n+ *   - The child process is made a session leader via setsid(3p).\n+ *   - All tempfiles owned by the parent process are reassigned to the\n+ *     daemonized child process.\n+ */\n int daemonize(void);\n \n /*\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 4700beacc1..df0bbc1669 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -1438,6 +1438,64 @@ test_expect_success '--no-detach causes maintenance to not run in background' '\n \t)\n '\n \n+test_expect_success PIPE '--detach holds maintenance lock until daemonized child exits' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\tgit config maintenance.auto false &&\n+\t\tgit config core.lockfilepid true &&\n+\n+\t\tgit remote add origin /does/not/exist &&\n+\t\tgit config set remote.origin.uploadpack \"cat fifo-uploadpack\" &&\n+\n+\t\tmkfifo fifo-uploadpack fifo-maint &&\n+\n+\t\t# Open the maintenance FIFO, as otherwise spawning\n+\t\t# git-maintenance(1) would block. Note that we need to open it\n+\t\t# as read-write, as otherwise we would block here already.\n+\t\texec 9<>fifo-maint &&\n+\n+\t\t{ git maintenance run --task=prefetch --detach 7>&9 & } &&\n+\t\tparent=\"$!\" &&\n+\n+\t\t# Reap the parent process so that the exec call below will not\n+\t\t# get SIGCHLD.\n+\t\twait \"$parent\" &&\n+\n+\t\t# Open the git-upload-pack(1) FIFO for writing, which will\n+\t\t# block until the upload-pack script opens it for reading. Once\n+\t\t# exec returns, we know that the daemonized child is alive and\n+\t\t# pinned.\n+\t\texec 8>fifo-uploadpack &&\n+\n+\t\ttest_path_is_file .git/objects/maintenance.lock &&\n+\t\ttest_path_is_file .git/objects/\"maintenance~pid.lock\" &&\n+\n+\t\t# Verify that the maintenance.lock still exists, and\n+\t\t# that it was created by the parent process, not the\n+\t\t# child.\n+\t\techo \"pid $parent\" >expect &&\n+\t\ttest_cmp expect .git/objects/\"maintenance~pid.lock\" &&\n+\n+\t\t# Reopen the maintenance FIFO as read-only so that\n+\t\t# git-maintenance(1) is the only writer. This will cause it to\n+\t\t# close the FIFO once the process exits.\n+\t\texec 9<&- &&\n+\t\texec 9<fifo-maint &&\n+\n+\t\t# Close the FIFO used by git-upload-pack(1) to unblock it and\n+\t\t# then wait until the maintenance FIFO is closed by\n+\t\t# git-maintenance(1), indicating that it has exited.\n+\t\texec 8>&- &&\n+\t\tcat <&9 &&\n+\n+\t\ttest_path_is_missing .git/objects/maintenance.lock &&\n+\t\ttest_path_is_missing .git/objects/\"maintenance~pid.lock\"\n+\t)\n+'\n+\n test_expect_success '--detach causes maintenance to run in background' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\ndiff --git a/tempfile.c b/tempfile.c\nindex 82dfa3d82f..f0fdf58279 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -373,3 +373,15 @@ int delete_tempfile(struct tempfile **tempfile_p)\n \n \treturn err ? -1 : 0;\n }\n+\n+void reassign_tempfile_ownership(pid_t from, pid_t to)\n+{\n+\tvolatile struct volatile_list_head *pos;\n+\n+\tlist_for_each(pos, &tempfile_list) {\n+\t\tstruct tempfile *p = list_entry(pos, struct tempfile, list);\n+\n+\t\tif (is_tempfile_active(p) && p->owner == from)\n+\t\t\tp->owner = to;\n+\t}\n+}\ndiff --git a/tempfile.h b/tempfile.h\nindex 2d2ae5b657..2227a095fd 100644\n--- a/tempfile.h\n+++ b/tempfile.h\n@@ -282,4 +282,15 @@ int delete_tempfile(struct tempfile **tempfile_p);\n  */\n int rename_tempfile(struct tempfile **tempfile_p, const char *path);\n \n+/*\n+ * Reassign ownership of all active tempfiles whose `owner` field matches\n+ * `from` to `to`.\n+ *\n+ * This is intended for use by `daemonize()`; after `fork(2)`-ing, the parent\n+ * transfers ownership to the daemonized child so that its atexit handler does\n+ * not unlink tempfiles that should outlive it, and the child claims the\n+ * inherited tempfiles so that they are cleaned up when the daemon exits.\n+ */\n+void reassign_tempfile_ownership(pid_t from, pid_t to);\n+\n #endif /* TEMPFILE_H */\n\n-- \n2.54.0.709.gd731d7959a.dirty\n\n"},{"id":"543233","messageId":"20260513-pks-maintenance-fix-lock-with-detach-v3-2-f27a1ac82891@pks.im","threadId":"65615","inReplyTo":"20260513-pks-maintenance-fix-lock-with-detach-v3-0-f27a1ac82891@pks.im","subject":"[PATCH v3 2/2] run-command: honor \"gc.auto\" for auto-maintenance","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-05-13T07:31:14Z","receivedAt":"2026-05-13T07:31:24Z","isPatch":true,"body":"The \"gc.auto\" configuration has traditionally been used to turn off\nrunning git-gc(1) as part of our auto-maintenance. We have eventually\nswitched over to git-maintenance(1) in a95ce12430 (maintenance: replace\nrun_auto_gc(), 2020-09-17), and with 1942d48380 (maintenance: optionally\nskip --auto process, 2020-08-28) we have introduced \"maintenance.auto\"\nto control whether or not to run auto-maintenance.\n\nAt that point though we still shelled out to git-gc(1) internally. So\nif \"gc.auto=0\" was set we would still _execute_ git-maintenance(1), but\nthe command would have exited fast because git-gc(1) itself knew to\nhonor the config key.\n\nThis has recently changed though, as we have adapted the default\nmaintenance strategy to not use git-gc(1) anymore. The consequence is\nthat \"gc.auto=0\" doesn't have an effect anymore, which is a somewhat\nsurprising change in behaviour for our users.\n\nAdapt `run_auto_maintenance()` so that it knows to also read \"gc.auto\",\nsimilar to how it also reads both \"maintenance.autoDetach\" and\n\"gc.autoDetach\".\n\nReported-by: Jean-Christophe Manciot <actionmystique@gmail.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n run-command.c          | 10 +++++++---\n t/t7900-maintenance.sh | 25 +++++++++++++++++++++++++\n 2 files changed, 32 insertions(+), 3 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex c146a56532..28202a81d8 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1944,10 +1944,14 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)\n int prepare_auto_maintenance(struct repository *r, int quiet,\n \t\t\t     struct child_process *maint)\n {\n-\tint enabled, auto_detach;\n+\tint enabled = 1, auto_detach;\n \n-\tif (!repo_config_get_bool(r, \"maintenance.auto\", &enabled) &&\n-\t    !enabled)\n+\tif (repo_config_get_bool(r, \"maintenance.auto\", &enabled)) {\n+\t\tint gc_threshold;\n+\t\tif (!repo_config_get_int(r, \"gc.auto\", &gc_threshold))\n+\t\t\tenabled = gc_threshold > 0;\n+\t}\n+\tif (!enabled)\n \t\treturn 0;\n \n \t/*\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex df0bbc1669..97c8c701bb 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -73,6 +73,31 @@ test_expect_success 'maintenance.auto config option' '\n \ttest_subcommand ! git maintenance run --auto --quiet --detach <false\n '\n \n+test_expect_success 'gc.auto config option' '\n+\tGIT_TRACE2_EVENT=\"$(pwd)/default\" git commit --quiet --allow-empty -m 1 &&\n+\ttest_subcommand git maintenance run --auto --quiet --detach <default &&\n+\tGIT_TRACE2_EVENT=\"$(pwd)/true\" \\\n+\t\tgit -c gc.auto=1 commit --quiet --allow-empty -m 2 &&\n+\ttest_subcommand git maintenance run --auto --quiet --detach <true &&\n+\tGIT_TRACE2_EVENT=\"$(pwd)/false\" \\\n+\t\tgit -c gc.auto=0 commit --quiet --allow-empty -m 3 &&\n+\ttest_subcommand ! git maintenance run --auto --quiet --detach <false\n+'\n+\n+test_expect_success 'maintenance.auto overrides gc.auto' '\n+\ttest_when_finished \"rm -f trace\" &&\n+\n+\ttest_config maintenance.auto false &&\n+\ttest_config gc.auto 1 &&\n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace\" git commit --quiet --allow-empty -m 1 &&\n+\ttest_subcommand ! git maintenance run --auto --quiet --detach <trace &&\n+\n+\ttest_config maintenance.auto true &&\n+\ttest_config gc.auto 0 &&\n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace\" git commit --quiet --allow-empty -m 1 &&\n+\ttest_subcommand git maintenance run --auto --quiet --detach <trace\n+'\n+\n for cfg in maintenance.autoDetach gc.autoDetach\n do\n \ttest_expect_success \"$cfg=true config option\" '\n\n-- \n2.54.0.709.gd731d7959a.dirty\n\n"},{"id":"543239","messageId":"xmqqy0hnipy4.fsf@gitster.g","threadId":"65615","inReplyTo":"20260513-pks-maintenance-fix-lock-with-detach-v3-1-f27a1ac82891@pks.im","subject":"Re: [PATCH v3 1/2] builtin/maintenance: fix locking with \"--detach\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-05-13T10:06:27Z","receivedAt":"2026-05-13T10:06:30Z","isPatch":true,"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Note that this is a broader fix, as we now always reassign tempfiles\n> when daemonizing. This is a natural consequence of the semantics of\n> `daemonize()` though, as it essentially promises to continue running the\n> current process in the background.\n\nExactly.  I do agree that it is the right wy to look at it.  The\nprocess that daemonise creates and leaves in the background is\nlogically the process that continues to execute the service the\nprocess the user started, and unless the original process explicitly\nsays \"we are done serving this thing\" and cleans up tempfile or\nlockfile it needed to serve that thing, it is natural to make the\nsurviving process to take over the responsibility.\n\n"},{"id":"543717","messageId":"agz78jjYEAif4lZt@nand.local","threadId":"65615","inReplyTo":"xmqqy0hnipy4.fsf@gitster.g","subject":"Re: [PATCH v3 1/2] builtin/maintenance: fix locking with \"--detach\"","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-05-20T00:10:26Z","receivedAt":"2026-05-20T00:10:28Z","isPatch":true,"body":"On Wed, May 13, 2026 at 07:06:27PM +0900, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n>\n> > Note that this is a broader fix, as we now always reassign tempfiles\n> > when daemonizing. This is a natural consequence of the semantics of\n> > `daemonize()` though, as it essentially promises to continue running the\n> > current process in the background.\n>\n> Exactly.  I do agree that it is the right wy to look at it.  The\n> process that daemonise creates and leaves in the background is\n> logically the process that continues to execute the service the\n> process the user started, and unless the original process explicitly\n> says \"we are done serving this thing\" and cleans up tempfile or\n> lockfile it needed to serve that thing, it is natural to make the\n> surviving process to take over the responsibility.\n\nYeah, this is how I had been thinking about it as well.\n\nThanks, Patrick, for making the change. I think that this series is in a\ngood spot, though I'd like to hear from Peff who had some comments on\nthe second patch from the previous round.\n\nOnce this is merged, I would suggest that we consider tagging a v2.54.1\nwith this in it, as the failure mode is pretty significant for users who\nhave concurrent maintenance processes running.\n\nThanks,\nTaylor\n"},{"id":"543729","messageId":"20260520054716.GB3849892@coredump.intra.peff.net","threadId":"65615","inReplyTo":"agz78jjYEAif4lZt@nand.local","subject":"Re: [PATCH v3 1/2] builtin/maintenance: fix locking with \"--detach\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-20T05:47:16Z","receivedAt":"2026-05-20T05:47:18Z","isPatch":true,"body":"On Tue, May 19, 2026 at 08:10:26PM -0400, Taylor Blau wrote:\n\n> Thanks, Patrick, for making the change. I think that this series is in a\n> good spot, though I'd like to hear from Peff who had some comments on\n> the second patch from the previous round.\n\nWhat's in v3 of the series looks good to me (both patches).\n\n-Peff\n"},{"id":"543730","messageId":"ag1MHje6-C6nmcO4@pks.im","threadId":"65615","inReplyTo":"20260520054716.GB3849892@coredump.intra.peff.net","subject":"Re: [PATCH v3 1/2] builtin/maintenance: fix locking with \"--detach\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-05-20T05:52:30Z","receivedAt":"2026-05-20T05:52:37Z","isPatch":true,"body":"On Wed, May 20, 2026 at 01:47:16AM -0400, Jeff King wrote:\n> On Tue, May 19, 2026 at 08:10:26PM -0400, Taylor Blau wrote:\n> \n> > Thanks, Patrick, for making the change. I think that this series is in a\n> > good spot, though I'd like to hear from Peff who had some comments on\n> > the second patch from the previous round.\n> \n> What's in v3 of the series looks good to me (both patches).\n\nThanks, both!\n\nPatrick\n"},{"id":"543772","messageId":"ag6ahXA104_70g3e@pks.im","threadId":"65615","inReplyTo":"20260511-pks-maintenance-fix-lock-with-detach-v1-0-ccd7d62c9a40@pks.im","subject":"Re: [PATCH 0/2] builtin/maintenance: fix locking and respect \"gc.auto\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-05-21T05:39:17Z","receivedAt":"2026-05-21T05:39:24Z","isPatch":true,"body":"Hi,\n\nOn Mon, May 11, 2026 at 02:29:54PM +0200, Patrick Steinhardt wrote:\n> this patch series addresses the issues reported in [1]. The series is\n> built on top of Git 2.54.0.\n\nJunio: I saw that you are starting to prep for Git 2.54.1, and\na89346e34a (Start preparing for 2.54.1, 2026-05-21) explicitly mentions\na couple of additional topics that should land in that bugfix release.\nThis topic here isn't mentioned though, but I very much think that these\nfixes should be included.\n\nThanks!\n\nPatrick\n"},{"id":"543773","messageId":"xmqq33zl2tok.fsf@gitster.g","threadId":"65615","inReplyTo":"ag6ahXA104_70g3e@pks.im","subject":"Re: [PATCH 0/2] builtin/maintenance: fix locking and respect \"gc.auto\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-05-21T05:55:07Z","receivedAt":"2026-05-21T05:55:10Z","isPatch":true,"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Hi,\n>\n> On Mon, May 11, 2026 at 02:29:54PM +0200, Patrick Steinhardt wrote:\n>> this patch series addresses the issues reported in [1]. The series is\n>> built on top of Git 2.54.0.\n>\n> Junio: I saw that you are starting to prep for Git 2.54.1, and\n> a89346e34a (Start preparing for 2.54.1, 2026-05-21) explicitly mentions\n> a couple of additional topics that should land in that bugfix release.\n> This topic here isn't mentioned though, but I very much think that these\n> fixes should be included.\n\nSure.  As of https://lore.kernel.org/git/ag1MHje6-C6nmcO4@pks.im/ I\nthink it can be merged to 'next', which will allow me to list it in\nthere?\n\nAre there other topics that should be fast-tracked?\n\n\n"},{"id":"543784","messageId":"ag64B4w4vLiJTwdb@pks.im","threadId":"65615","inReplyTo":"xmqq33zl2tok.fsf@gitster.g","subject":"Re: [PATCH 0/2] builtin/maintenance: fix locking and respect \"gc.auto\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-05-21T07:45:11Z","receivedAt":"2026-05-21T07:45:18Z","isPatch":true,"body":"On Thu, May 21, 2026 at 02:55:07PM +0900, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> > On Mon, May 11, 2026 at 02:29:54PM +0200, Patrick Steinhardt wrote:\n> >> this patch series addresses the issues reported in [1]. The series is\n> >> built on top of Git 2.54.0.\n> >\n> > Junio: I saw that you are starting to prep for Git 2.54.1, and\n> > a89346e34a (Start preparing for 2.54.1, 2026-05-21) explicitly mentions\n> > a couple of additional topics that should land in that bugfix release.\n> > This topic here isn't mentioned though, but I very much think that these\n> > fixes should be included.\n> \n> Sure.  As of https://lore.kernel.org/git/ag1MHje6-C6nmcO4@pks.im/ I\n> think it can be merged to 'next', which will allow me to list it in\n> there?\n\nYeah, both Taylor and Peff ACK'd this series, so I think it should be\nready to go.\n\n> Are there other topics that should be fast-tracked?\n\nNone that I'm currently aware of. Thanks!\n\nPatrick\n"}]}