Show 37 quoted lines
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 3a71e314c9..d866c19b92 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1810,10 +1810,33 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
> TASK_PHASE_FOREGROUND))
> result = 1;
>
> - /* Failure to daemonize is ok, we'll continue in foreground. */
> if (opts->detach > 0) {
> + pid_t child_pid;
> +
> trace2_region_enter("maintenance", "detach", the_repository);
> - daemonize();
> +
> + child_pid = daemonize_without_exit();
> + if (!child_pid) {
> + /*
> + * We're in the child process, so we take ownership of
> + * the lockfile.
> + */
> + lock_file_reassign_owner(&lk, getpid());
> + } else if (child_pid > 0) {
> + /*
> + * We're in the parent process, so we drop ownership of
> + * the lockfile to prevent us from removing it upon
> + * exit.
> + */
> + lock_file_reassign_owner(&lk, child_pid);
> + exit(0);
> + } else {
> + /*
> + * Failure to daemonize is ok, we'll continue in
> + * foreground.
> + */
> + }
> +This works almost identically as the original implementation I shared[1] original thread discussing this issue, but it takes a slightly different approach.
Instead of doing this automatically as a part of daemonize(), this patch only adjusts this singular call-site, and forces us to introduce a new function daemonize_without_exit() in order to facilitate it.
I personally prefer the other approach, for many of the reasons that Peff wrote about in [2, 3]. There are two questions in my mind that I would be curious of your thoughts on:
* Should we transfer ownership of all tempfiles, or a subset of them?
* Should we perform that transfer automatically via daemonize(), or
manually via its callers?
Judging from the other parts of the original thread, I think the answer to the first question is an unambiguous "yes". The second question I think there is less of a consensus on, but I'd like to advocate for doing this automatically via daemonize().
To summarize some of my thoughts after reading [2], I think that because daemonize() is about letting a process continue on in the background rather than fork()-ing children and then continuing on ourselves, *not* automatically handing those off feels like an accident waiting to happen.
I can't think of a situation where someone would want to daemonize() but not have the new process assume ownership of its locks and tempfiles. I think an alternative approach here would be to document the behavior I'm proposing above clearly in daemonize(). Should a caller arise in the future that *doesn't* want to this behavior, then we could introduce a function similar to your daemonize_without_exit() (maybe in this case it would be daemonize_without_transfer(), since not exiting in a function that is supposed to daemonize feels awkward).
I guess what I'm saying is that I feel that future daemonize() callers are much more likely to want this behavior than not, and putting this in daemonize() feels like the safer option between the two.
FWIW, I feel fairly strongly about ^ this, but I'm of course happy to discuss more if you feel differently.
(If you do end up taking that approach, that makes the C changes identical to the ones I shared in [1], so you are free to forge my Co-authored-by and Signed-off-by lines if you want to take that approach instead.)
Very nice. The fifo-maint idea is much more robust than what I was able to come up with in my original sketch, and it is a nice compliment to fifo-uploadpack.
The rest of the test is nearly identical to mine and looks good to me.
Thanks, Taylor
[1]: https://lore.kernel.org/git/af+snTGFeoUUyfPU@nand.local/ [2]: https://lore.kernel.org/git/20260511200112.GA22912@coredump.intra.peff.net/ [3]: https://lore.kernel.org/git/20260511202258.GD22912@coredump.intra.peff.net/