Merge branch 'ps/maintenance-daemonize-lockfix'

"git maintenance" that goes background did not use the lockfile to
prevent multiple maintenance processes from running at the same
time, which has been corrected.

* ps/maintenance-daemonize-lockfix:
  run-command: honor "gc.auto" for auto-maintenance
  builtin/maintenance: fix locking with "--detach"

run-command.c

@@ -1944,10 +1944,14 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)
 int prepare_auto_maintenance(struct repository *r, int quiet,
 			     struct child_process *maint)
 {
-	int enabled, auto_detach;
+	int enabled = 1, auto_detach;
 
-	if (!repo_config_get_bool(r, "maintenance.auto", &enabled) &&
-	    !enabled)
+	if (repo_config_get_bool(r, "maintenance.auto", &enabled)) {
+		int gc_threshold;
+		if (!repo_config_get_int(r, "gc.auto", &gc_threshold))
+			enabled = gc_threshold > 0;
+	}
+	if (!enabled)
 		return 0;
 
 	/*

setup.c

@@ -2166,12 +2166,26 @@ int daemonize(void)
 	errno = ENOSYS;
 	return -1;
 #else
-	switch (fork()) {
+	pid_t parent_pid = getpid();
+	pid_t child_pid = fork();
+
+	switch (child_pid) {
 		case 0:
+			/*
+			 * We're in the child process, so we take ownership of
+			 * all tempfiles.
+			 */
+			reassign_tempfile_ownership(parent_pid, getpid());
 			break;
 		case -1:
 			die_errno(_("fork failed"));
 		default:
+			/*
+			 * We're in the parent process, so we drop ownership of
+			 * all tempfiles to prevent us from removing them upon
+			 * exit.
+			 */
+			reassign_tempfile_ownership(parent_pid, child_pid);
 			exit(0);
 	}
 	if (setsid() == -1)

setup.h

@@ -149,6 +149,21 @@ void verify_non_filename(const char *prefix, const char *name);
 int path_inside_repo(const char *prefix, const char *path);
 
 void sanitize_stdfds(void);
+
+/*
+ * Daemonize the current process by forking and then exiting the parent
+ * process. Returns 0 when successful, in which case the parent process will
+ * have exited and it's the child process that continues to run the code.
+ * Otherwise, a negative error code is returned and the parent process will
+ * continue execution.
+ *
+ * Note that this function will also perform the following changes:
+ *
+ *   - Standard file descriptors in the child process are closed.
+ *   - The child process is made a session leader via setsid(3p).
+ *   - All tempfiles owned by the parent process are reassigned to the
+ *     daemonized child process.
+ */
 int daemonize(void);
 
 /*

t/t7900-maintenance.sh

@@ -73,6 +73,31 @@ test_expect_success 'maintenance.auto config option' '
 	test_subcommand ! git maintenance run --auto --quiet --detach <false
 '
 
+test_expect_success 'gc.auto config option' '
+	GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
+	test_subcommand git maintenance run --auto --quiet --detach <default &&
+	GIT_TRACE2_EVENT="$(pwd)/true" \
+		git -c gc.auto=1 commit --quiet --allow-empty -m 2 &&
+	test_subcommand git maintenance run --auto --quiet --detach <true &&
+	GIT_TRACE2_EVENT="$(pwd)/false" \
+		git -c gc.auto=0 commit --quiet --allow-empty -m 3 &&
+	test_subcommand ! git maintenance run --auto --quiet --detach <false
+'
+
+test_expect_success 'maintenance.auto overrides gc.auto' '
+	test_when_finished "rm -f trace" &&
+
+	test_config maintenance.auto false &&
+	test_config gc.auto 1 &&
+	GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
+	test_subcommand ! git maintenance run --auto --quiet --detach <trace &&
+
+	test_config maintenance.auto true &&
+	test_config gc.auto 0 &&
+	GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
+	test_subcommand git maintenance run --auto --quiet --detach <trace
+'
+
 for cfg in maintenance.autoDetach gc.autoDetach
 do
 	test_expect_success "$cfg=true config option" '
@@ -1466,6 +1491,64 @@ test_expect_success '--no-detach causes maintenance to not run in background' '
 	)
 '
 
+test_expect_success PIPE '--detach holds maintenance lock until daemonized child exits' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+
+		git config maintenance.auto false &&
+		git config core.lockfilepid true &&
+
+		git remote add origin /does/not/exist &&
+		git config set remote.origin.uploadpack "cat fifo-uploadpack" &&
+
+		mkfifo fifo-uploadpack fifo-maint &&
+
+		# Open the maintenance FIFO, as otherwise spawning
+		# git-maintenance(1) would block. Note that we need to open it
+		# as read-write, as otherwise we would block here already.
+		exec 9<>fifo-maint &&
+
+		{ git maintenance run --task=prefetch --detach 7>&9 & } &&
+		parent="$!" &&
+
+		# Reap the parent process so that the exec call below will not
+		# get SIGCHLD.
+		wait "$parent" &&
+
+		# Open the git-upload-pack(1) FIFO for writing, which will
+		# block until the upload-pack script opens it for reading. Once
+		# exec returns, we know that the daemonized child is alive and
+		# pinned.
+		exec 8>fifo-uploadpack &&
+
+		test_path_is_file .git/objects/maintenance.lock &&
+		test_path_is_file .git/objects/"maintenance~pid.lock" &&
+
+		# Verify that the maintenance.lock still exists, and
+		# that it was created by the parent process, not the
+		# child.
+		echo "pid $parent" >expect &&
+		test_cmp expect .git/objects/"maintenance~pid.lock" &&
+
+		# Reopen the maintenance FIFO as read-only so that
+		# git-maintenance(1) is the only writer. This will cause it to
+		# close the FIFO once the process exits.
+		exec 9<&- &&
+		exec 9<fifo-maint &&
+
+		# Close the FIFO used by git-upload-pack(1) to unblock it and
+		# then wait until the maintenance FIFO is closed by
+		# git-maintenance(1), indicating that it has exited.
+		exec 8>&- &&
+		cat <&9 &&
+
+		test_path_is_missing .git/objects/maintenance.lock &&
+		test_path_is_missing .git/objects/"maintenance~pid.lock"
+	)
+'
+
 test_expect_success '--detach causes maintenance to run in background' '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&

tempfile.c

@@ -373,3 +373,15 @@ int delete_tempfile(struct tempfile **tempfile_p)
 
 	return err ? -1 : 0;
 }
+
+void reassign_tempfile_ownership(pid_t from, pid_t to)
+{
+	volatile struct volatile_list_head *pos;
+
+	list_for_each(pos, &tempfile_list) {
+		struct tempfile *p = list_entry(pos, struct tempfile, list);
+
+		if (is_tempfile_active(p) && p->owner == from)
+			p->owner = to;
+	}
+}

tempfile.h

@@ -282,4 +282,15 @@ int delete_tempfile(struct tempfile **tempfile_p);
  */
 int rename_tempfile(struct tempfile **tempfile_p, const char *path);
 
+/*
+ * Reassign ownership of all active tempfiles whose `owner` field matches
+ * `from` to `to`.
+ *
+ * This is intended for use by `daemonize()`; after `fork(2)`-ing, the parent
+ * transfers ownership to the daemonized child so that its atexit handler does
+ * not unlink tempfiles that should outlive it, and the child claims the
+ * inherited tempfiles so that they are cleaned up when the daemon exits.
+ */
+void reassign_tempfile_ownership(pid_t from, pid_t to);
+
 #endif /* TEMPFILE_H */