builtin/receive-pack: convert to use git-maintenance(1)

Hi,

this small patch series adapts git-receive-pack(1) to spawn git maintenance run --auto instead of git gc --auto like all the other parts of our codebase do nowadays. This removes the last internal user of git gc --auto.

Patrick

Patrick Steinhardt (2):
run-command: introduce function to prepare auto-maintenance process builtin/receive-pack: convert to use git-maintenance(1)

 Documentation/config/receive.txt |  2 +-
 builtin/receive-pack.c           | 21 ++++++++++-----------
 run-command.c                    | 19 +++++++++++++------
 run-command.h                    |  7 +++++++
 4 files changed, 31 insertions(+), 18 deletions(-)

2.44.GIT

The run_auto_maintenance() function is responsible for spawning a new git maintenance run --auto process. To do so, it sets up the sturct child_process and then runs it by executing run_command() directly. This is rather inflexible in case callers want to modify the child process somewhat, e.g. to redirect stderr or stdout.

Introduce a new prepare_auto_maintenance() function to plug this gap.

---
 run-command.c | 19 +++++++++++++------
 run-command.h |  7 +++++++
 2 files changed, 20 insertions(+), 6 deletions(-)

index 0e7435718a..1b821042b4 100644
@@ -1793,20 +1793,27 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)
 		trace2_region_leave(tr2_category, tr2_label, NULL);
 }
 
-int run_auto_maintenance(int quiet)
+int prepare_auto_maintenance(int quiet, struct child_process *maint)
 {
 	int enabled;
-	struct child_process maint = CHILD_PROCESS_INIT;
 
 	if (!git_config_get_bool("maintenance.auto", &enabled) &&
 	    !enabled)
 		return 0;
 
-	maint.git_cmd = 1;
-	maint.close_object_store = 1;
-	strvec_pushl(&maint.args, "maintenance", "run", "--auto", NULL);
-	strvec_push(&maint.args, quiet ? "--quiet" : "--no-quiet");
+	maint->git_cmd = 1;
+	maint->close_object_store = 1;
+	strvec_pushl(&maint->args, "maintenance", "run", "--auto", NULL);
+	strvec_push(&maint->args, quiet ? "--quiet" : "--no-quiet");
+
+	return 1;
+}
 
+int run_auto_maintenance(int quiet)
+{
+	struct child_process maint = CHILD_PROCESS_INIT;
+	if (!prepare_auto_maintenance(quiet, &maint))
+		return 0;
 	return run_command(&maint);
 }
 

index 1f22cc3827..55f6631a2a 100644
@@ -217,6 +217,13 @@ int finish_command_in_signal(struct child_process *);
  */
 int run_command(struct child_process *);
 
+/*
+ * Prepare a `struct child_process` to run auto-maintenance. Returns 1 if the
+ * process has been prepared and is ready to run, or 0 in case auto-maintenance
+ * should be skipped.
+ */
+int prepare_auto_maintenance(int quiet, struct child_process *maint);
+
 /*
  * Trigger an auto-gc
  */

2.44.GIT

In 850b6edefa (auto-gc: extract a reusable helper from "git fetch", 2020-05-06), we have introduced a helper function run_auto_gc() that kicks off git gc --auto. The intent of this function was to pass down the "--quiet" flag to git-gc(1) as required without duplicating this at all callsites. In 7c3e9e8cfb (auto-gc: pass --quiet down from am, commit, merge and rebase, 2020-05-06) we then converted callsites that need to pass down this flag to use the new helper function. This has the notable omission of git-receive-pack(1), which is the only remaining user of git gc --auto that sets up the proccess manually. This is probably because it unconditionally passes down the --quiet flag and thus didn't benefit much from the new helper function.

In a95ce12430 (maintenance: replace run_auto_gc(), 2020-09-17) we then replaced run_auto_gc() with run_auto_maintenance() which invokes git-maintenance(1) instead of git-gc(1). This command is the modern replacement for git-gc(1) and is both more thorough and also more flexible because administrators can configure which tasks exactly to run during maintenance.

But due to git-receive-pack(1) not using run_auto_gc() in the first place it did not get converted to use git-maintenance(1) like we do everywhere else now. Address this oversight and start to use the newly introduced function prepare_auto_maintenance(). This will also make it easier for us to adapt this code together with all the other callsites that invoke auto-maintenance in the future.

This removes the last internal user of git gc --auto.

---
 Documentation/config/receive.txt |  2 +-
 builtin/receive-pack.c           | 21 ++++++++++-----------
 2 files changed, 11 insertions(+), 12 deletions(-)

index c77e55b1cd..36a1e6f2d2 100644
@@ -8,7 +8,7 @@ receive.advertisePushOptions::
 	capability to its clients. False by default.
 
 receive.autogc::
-	By default, git-receive-pack will run "git-gc --auto" after
+	By default, git-receive-pack will run "git maintenance run --auto" after
 	receiving data from git-push and updating refs.  You can stop
 	it by setting this variable to false.
 

index 56d8a77ed7..e8d7df14b6 100644
@@ -2585,17 +2585,16 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 		if (auto_gc) {
 			struct child_process proc = CHILD_PROCESS_INIT;
 
-			proc.no_stdin = 1;
-			proc.stdout_to_stderr = 1;
-			proc.err = use_sideband ? -1 : 0;
-			proc.git_cmd = proc.close_object_store = 1;
-			strvec_pushl(&proc.args, "gc", "--auto", "--quiet",
-				     NULL);
-
-			if (!start_command(&proc)) {
-				if (use_sideband)
-					copy_to_sideband(proc.err, -1, NULL);
-				finish_command(&proc);
+			if (prepare_auto_maintenance(1, &proc)) {
+				proc.no_stdin = 1;
+				proc.stdout_to_stderr = 1;
+				proc.err = use_sideband ? -1 : 0;
+
+				if (!start_command(&proc)) {
+					if (use_sideband)
+						copy_to_sideband(proc.err, -1, NULL);
+					finish_command(&proc);
+				}
 			}
 		}
 		if (auto_update_server_info)

2.44.GIT

I guess the mention of "inflexible" and "redirection" above refers to some incompatibile behaviour we would introduce if we just replaced the manual spawning of "gc --auto" with a call to run_auto_maintenance(), but I would have expected that will be solved by making the interface to run_auto_maintenance() richer, not forcing the callers that would want to deviate from the norm to write the second half of the run_auto_maintenance() themselves.

I don't have enough context here, so why do this?

This commit explains my earlier question. Thanks.

I think the intent of a95ce124 (maintenance: replace run_auto_gc(), 2020-09-17) was to update all codepaths that run "git gc --auto" to instead run "git maintenance --auto", but only updated the ones that used to call run_auto_gc(). The codepath Patrick found runs "git gc --auto" without using run_auto_gc() and was left behind when the others were converted.

So why do this? I think "To follow through a95ce124 started" would probably be a good enough reason, if a reader is on board with what a95ce124 wanted to do.

Do we have a handy reference that compares "gc --auto" and "maintenance --auto"? Are they essentially the same thing these days? What are the things that is done by one but not by the other?

THanks.

Fair enough, I was a bit lazy with the cover letter as I thought that the second patch already explains it well enough. But true, I should've set the stage a bit better.

Yup, that was triggered this patch series. I was puzzled that we still run git gc --auto at all.

git-maintenance(1) is a superset of what git-gc(1) does. It is structured around different "tasks" that the user can enable or disable at a whim. By default, only a single "gc" task is enabled, which makes it a drop-in replacement for what we had before the conversion.

But users can ask git-maintenance(1) to perform housekeeping to their will. Instead of running git-gc(1), they can configure indididual tasks to repack objects via multi-pack indices, pack only loose objects, update the commit-graph or repack references.

So overall, git-maintenance(1) is the modern and more flexible replacement for git-gc(1) that gives more tuning knobs.

Patrick

git-receive-pack(1) needs to do some magic with file descriptors and needs to copy output of the command to the sideband. I first thought about extending run_auto_maintenance() to support this, but found it to be messy as this really is quite a specific usecase. So I figured that prepping the struct child_process like this is the nicer way to approach it.

Thanks.

Patrick