{"thread":{"id":"65621","subject":"[PATCH v2 1/2] builtin/maintenance: fix locking with \"--detach\"","startedAt":"2026-05-12T08:30:42Z","lastAt":"2026-05-13T06:23:10Z","messageCount":5,"participants":["Patrick Steinhardt","Taylor Blau"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"543159","messageId":"20260512-pks-maintenance-fix-lock-with-detach-v2-0-dc6f2d284b6d@pks.im","threadId":"65621","inReplyTo":"20260511-pks-maintenance-fix-lock-with-detach-v1-0-ccd7d62c9a40@pks.im","subject":"[PATCH v2 0/2] builtin/maintenance: fix locking and respect \"gc.auto\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-05-12T08:30:29Z","receivedAt":"2026-05-12T08:30:40Z","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 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]: \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 | 27 ++++++++++++++--\n lockfile.c | 9 ++++++\n lockfile.h | 10 ++++++\n run-command.c | 10 ++++--\n setup.c | 31 +++++++++++++------\n setup.h | 1 +\n t/t7900-maintenance.sh | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 7 files changed, 157 insertions(+), 14 deletions(-)\n\nRange-diff versus v1:\n\n1: d0609c03b4 ! 1: 8dda16ec8d builtin/maintenance: fix locking with \"--detach\"\n @@ builtin/gc.c: static int maintenance_run_tasks(struct maintenance_run_opts *opts\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 * We're in the parent process, so we drop ownership of\n ++\t\t\t * the lockfile to prevent us from removing it upon\n ++\t\t\t * exit.\n +\t\t\t */\n +\t\t\tlock_file_reassign_owner(&lk, child_pid);\n +\t\t\texit(0);\n2: 959ce46f7d < -: ---------- run-command: honor \"gc.auto\" for auto-maintenance\n-: ---------- > 2: d5507b5dd2 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":"543158","messageId":"20260512-pks-maintenance-fix-lock-with-detach-v2-1-dc6f2d284b6d@pks.im","threadId":"65621","inReplyTo":"20260512-pks-maintenance-fix-lock-with-detach-v2-0-dc6f2d284b6d@pks.im","subject":"[PATCH v2 1/2] builtin/maintenance: fix locking with \"--detach\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-05-12T08:30:30Z","receivedAt":"2026-05-12T08:30:42Z","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 \nHelped-by: Jeff King \nHelped-by: Taylor Blau \nHelped-by: Derrick Stolee \nSigned-off-by: Patrick Steinhardt \n---\n builtin/gc.c | 27 +++++++++++++++++++++--\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, 125 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 3a71e314c9..d866c19b92 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1810,10 +1810,33 @@ 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 drop ownership of\n+\t\t\t * the lockfile to prevent us from removing it upon\n+\t\t\t * exit.\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&- &&\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":"543160","messageId":"20260512-pks-maintenance-fix-lock-with-detach-v2-2-dc6f2d284b6d@pks.im","threadId":"65621","inReplyTo":"20260512-pks-maintenance-fix-lock-with-detach-v2-0-dc6f2d284b6d@pks.im","subject":"[PATCH v2 2/2] run-command: honor \"gc.auto\" for auto-maintenance","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-05-12T08:30:31Z","receivedAt":"2026-05-12T08:30:44Z","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 \nSigned-off-by: Patrick Steinhardt \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 diff --git a/builtin/gc.c b/builtin/gc.c\n> index 3a71e314c9..d866c19b92 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -1810,10 +1810,33 @@ 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 drop ownership of\n> +\t\t\t * the lockfile to prevent us from removing it upon\n> +\t\t\t * exit.\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\nThis works almost identically as the original implementation I shared[1]\noriginal thread discussing this issue, but it takes a slightly different\napproach.\n\nInstead of doing this automatically as a part of daemonize(), this patch\nonly adjusts this singular call-site, and forces us to introduce a new\nfunction daemonize_without_exit() in order to facilitate it.\n\nI personally prefer the other approach, for many of the reasons that\nPeff wrote about in [2, 3]. There are two questions in my mind that I\nwould be curious of your thoughts on:\n\n * Should we transfer ownership of all tempfiles, or a subset of them?\n\n * Should we perform that transfer automatically via daemonize(), or\n manually via its callers?\n\nJudging from the other parts of the original thread, I think the answer\nto the first question is an unambiguous \"yes\". The second question I\nthink there is less of a consensus on, but I'd like to advocate for\ndoing this automatically via daemonize().\n\nTo summarize some of my thoughts after reading [2], I think that because\ndaemonize() is about letting a process continue on in the background\nrather than fork()-ing children and then continuing on ourselves, *not*\nautomatically handing those off feels like an accident waiting to\nhappen.\n\nI can't think of a situation where someone would want to daemonize() but\nnot have the new process assume ownership of its locks and tempfiles. I\nthink an alternative approach here would be to document the behavior I'm\nproposing above clearly in daemonize(). Should a caller arise in the\nfuture that *doesn't* want to this behavior, then we could introduce a\nfunction similar to your daemonize_without_exit() (maybe in this case it\nwould be daemonize_without_transfer(), since not exiting in a function\nthat is supposed to daemonize feels awkward).\n\nI guess what I'm saying is that I feel that future daemonize() callers\nare much more likely to want this behavior than not, and putting this in\ndaemonize() feels like the safer option between the two.\n\nFWIW, I feel fairly strongly about ^ this, but I'm of course happy to\ndiscuss more if you feel differently.\n\n(If you do end up taking that approach, that makes the C changes\nidentical to the ones I shared in [1], so you are free to forge my\nCo-authored-by and Signed-off-by lines if you want to take that approach\ninstead.)\n\n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> index 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\nVery nice. The fifo-maint idea is much more robust than what I was able\nto come up with in my original sketch, and it is a nice compliment to\nfifo-uploadpack.\n\nThe rest of the test is nearly identical to mine and looks good to me.\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/af+snTGFeoUUyfPU@nand.local/\n[2]: https://lore.kernel.org/git/20260511200112.GA22912@coredump.intra.peff.net/\n[3]: https://lore.kernel.org/git/20260511202258.GD22912@coredump.intra.peff.net/\n"},{"id":"543229","messageId":"agQYx08NnEqN1snT@pks.im","threadId":"65621","inReplyTo":"agOYLsJF4rHESk9k@nand.local","subject":"Re: [PATCH v2 1/2] builtin/maintenance: fix locking with \"--detach\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-05-13T06:23:03Z","receivedAt":"2026-05-13T06:23:10Z","isPatch":true,"body":"On Tue, May 12, 2026 at 05:14:22PM -0400, Taylor Blau wrote:\n> On Tue, May 12, 2026 at 10:30:30AM +0200, Patrick Steinhardt wrote:\n> > diff --git a/builtin/gc.c b/builtin/gc.c\n> > index 3a71e314c9..d866c19b92 100644\n> > --- a/builtin/gc.c\n> > +++ b/builtin/gc.c\n> > @@ -1810,10 +1810,33 @@ 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 drop ownership of\n> > +\t\t\t * the lockfile to prevent us from removing it upon\n> > +\t\t\t * exit.\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> \n> This works almost identically as the original implementation I shared[1]\n> original thread discussing this issue, but it takes a slightly different\n> approach.\n> \n> Instead of doing this automatically as a part of daemonize(), this patch\n> only adjusts this singular call-site, and forces us to introduce a new\n> function daemonize_without_exit() in order to facilitate it.\n> \n> I personally prefer the other approach, for many of the reasons that\n> Peff wrote about in [2, 3]. There are two questions in my mind that I\n> would be curious of your thoughts on:\n> \n> * Should we transfer ownership of all tempfiles, or a subset of them?\n> \n> * Should we perform that transfer automatically via daemonize(), or\n> manually via its callers?\n> \n> Judging from the other parts of the original thread, I think the answer\n> to the first question is an unambiguous \"yes\". The second question I\n> think there is less of a consensus on, but I'd like to advocate for\n> doing this automatically via daemonize().\n> \n> To summarize some of my thoughts after reading [2], I think that because\n> daemonize() is about letting a process continue on in the background\n> rather than fork()-ing children and then continuing on ourselves, *not*\n> automatically handing those off feels like an accident waiting to\n> happen.\n> \n> I can't think of a situation where someone would want to daemonize() but\n> not have the new process assume ownership of its locks and tempfiles. I\n> think an alternative approach here would be to document the behavior I'm\n> proposing above clearly in daemonize(). Should a caller arise in the\n> future that *doesn't* want to this behavior, then we could introduce a\n> function similar to your daemonize_without_exit() (maybe in this case it\n> would be daemonize_without_transfer(), since not exiting in a function\n> that is supposed to daemonize feels awkward).\n> \n> I guess what I'm saying is that I feel that future daemonize() callers\n> are much more likely to want this behavior than not, and putting this in\n> daemonize() feels like the safer option between the two.\n> \n> FWIW, I feel fairly strongly about ^ this, but I'm of course happy to\n> discuss more if you feel differently.\n> \n> (If you do end up taking that approach, that makes the C changes\n> identical to the ones I shared in [1], so you are free to forge my\n> Co-authored-by and Signed-off-by lines if you want to take that approach\n> instead.)\n\nHmm. I was initially aiming for a more minimal fix, and that's why I\ndecided to make only this one callsite reassign tempfiles. But there's\nonly two callsites right now, and thinking a bit more about the problem\nmakes me agree with your take on it.\n\nWill adapt, thanks for pushing back!\n\nPatrick\n"}]}