diff mbox series

[v2,7/7] run-command: fix detaching when running auto maintenance

Message ID 6bc170ff05d38845a012ce57e580a0ddd18f1143.1723712608.git.ps@pks.im (mailing list archive)
State Superseded
Headers show
Series builtin/maintenance: fix auto-detach with non-standard tasks | expand

Commit Message

Patrick Steinhardt Aug. 15, 2024, 9:12 a.m. UTC 
In the past, we used to execute `git gc --auto` as part of our automatic
housekeeping routines. As git-gc(1) may require quite some time to
perform the housekeeping, it knows to detach itself and run in the
background so that the user can continue their work.

Eventually, we refactored our automatic housekeeping to instead use the
more flexible git-maintenance(1) command. The upside of this new infra
is that the user can configure which maintenance tasks are performed, at
least to a certain degree. So while it continues to run git-gc(1) by
default, it can also be adapted to e.g. use git-multi-pack-index(1) for
maintenance of the object database.

The auto-detach of the new infra is somewhat broken though once the user
configures non-standard tasks. The problem is essentially that we detach
at the wrong level in the process hierarchy: git-maintenance(1) never
detaches itself, but instead it continues to be git-gc(1) which does.

When configured to only run the git-gc(1) maintenance task, then the
result is basically the same as before. But when configured to run other
tasks, then git-maintenance(1) will wait for these to run to completion.
Even worse, it may be that git-gc(1) runs concurrently with other
housekeeping tasks, stomping on each others feet.

Fix this bug by asking git-gc(1) to not detach when it is being invoked
via git-maintenance(1). Instead, git-maintenance(1) now respects a new
config "maintenance.autoDetach", the equivalent of "gc.autoDetach", and
detaches itself into the background when running as part of our auto
maintenance. This should continue to behave the same for all users which
use the git-gc(1) task, only. For others though, it means that we now
properly perform all tasks in the background. The default behaviour of
git-maintenance(1) when executed by the user does not change, it will
remain in the foreground unless they pass the `--detach` option.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c             |  1 +
 run-command.c            | 12 ++++++++++-
 t/t5616-partial-clone.sh |  6 +++---
 t/t7900-maintenance.sh   | 43 +++++++++++++++++++++++++++++++---------
 4 files changed, 49 insertions(+), 13 deletions(-)

Comments

Junio C Hamano Aug. 15, 2024, 4:13 p.m. UTC | #1 
Patrick Steinhardt <ps@pks.im> writes:

> diff --git a/run-command.c b/run-command.c
> index 45ba544932..94f2f3079f 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1808,16 +1808,26 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)
>  
>  int prepare_auto_maintenance(int quiet, struct child_process *maint)
>  {
> -	int enabled;
> +	int enabled, auto_detach;
>  
>  	if (!git_config_get_bool("maintenance.auto", &enabled) &&
>  	    !enabled)
>  		return 0;
>  
> +	/*
> +	 * When `maintenance.autoDetach` isn't set, then we fall back to
> +	 * honoring `gc.autoDetach`. This is somewhat weird, but required to
> +	 * retain behaviour from when we used to run git-gc(1) here.
> +	 */
> +	if (git_config_get_bool("maintenance.autodetach", &auto_detach) &&
> +	    git_config_get_bool("gc.autodetach", &auto_detach))
> +		auto_detach = 1;

I think this needs somehow documented.  Something like this,
perhaps?

 Documentation/config/gc.txt          | 2 ++
 Documentation/config/maintenance.txt | 9 +++++++++
 2 files changed, 11 insertions(+)

diff --git c/Documentation/config/gc.txt w/Documentation/config/gc.txt
index 664a3c2874..6506ccb87f 100644
--- c/Documentation/config/gc.txt
+++ w/Documentation/config/gc.txt
@@ -41,6 +41,8 @@ use, it'll affect how the auto pack limit works.
 gc.autoDetach::
 	Make `git gc --auto` return immediately and run in the background
 	if the system supports it. Default is true.
+	It also acts as a fallback setting for the `maintenance.autoDetach`
+	configuration variable.
 
 gc.bigPackThreshold::
 	If non-zero, all non-cruft packs larger than this limit are kept
diff --git c/Documentation/config/maintenance.txt w/Documentation/config/maintenance.txt
index 69a4f05153..7a481a494a 100644
--- c/Documentation/config/maintenance.txt
+++ w/Documentation/config/maintenance.txt
@@ -3,6 +3,15 @@ maintenance.auto::
 	`git maintenance run --auto` after doing their normal work. Defaults
 	to true.
 
+maintenance.autoDetach::
+	Tasks that are run via `git maintenance run --auto` by
+	default runs in the background, if the system supports it.
+	Setting this configuration variable to `true` explicitly
+	asks them to run in the background, and setting it to
+	`false` forces them to run in the foreground.  If this
+	variable is not set, `gc.autoDetach` works as a fallback
+	variable and behaves the same way.
+
 maintenance.strategy::
 	This string config option provides a way to specify one of a few
 	recommended schedules for background maintenance. This only affects
Patrick Steinhardt Aug. 16, 2024, 8:06 a.m. UTC | #2 
On Thu, Aug 15, 2024 at 09:13:28AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > diff --git a/run-command.c b/run-command.c
> > index 45ba544932..94f2f3079f 100644
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -1808,16 +1808,26 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)
> >  
> >  int prepare_auto_maintenance(int quiet, struct child_process *maint)
> >  {
> > -	int enabled;
> > +	int enabled, auto_detach;
> >  
> >  	if (!git_config_get_bool("maintenance.auto", &enabled) &&
> >  	    !enabled)
> >  		return 0;
> >  
> > +	/*
> > +	 * When `maintenance.autoDetach` isn't set, then we fall back to
> > +	 * honoring `gc.autoDetach`. This is somewhat weird, but required to
> > +	 * retain behaviour from when we used to run git-gc(1) here.
> > +	 */
> > +	if (git_config_get_bool("maintenance.autodetach", &auto_detach) &&
> > +	    git_config_get_bool("gc.autodetach", &auto_detach))
> > +		auto_detach = 1;
> 
> I think this needs somehow documented.  Something like this,
> perhaps?

Indeed, I totally forgot doing that.

> --- c/Documentation/config/maintenance.txt
> +++ w/Documentation/config/maintenance.txt
> @@ -3,6 +3,15 @@ maintenance.auto::
>  	`git maintenance run --auto` after doing their normal work. Defaults
>  	to true.
>  
> +maintenance.autoDetach::
> +	Tasks that are run via `git maintenance run --auto` by
> +	default runs in the background, if the system supports it.
> +	Setting this configuration variable to `true` explicitly
> +	asks them to run in the background, and setting it to
> +	`false` forces them to run in the foreground.  If this
> +	variable is not set, `gc.autoDetach` works as a fallback
> +	variable and behaves the same way.

This isn't entirely true. `git maintenance run --auto` will not
background, because that'd change preexisting behaviour. It also would
not make a lot of sense, because here the `--auto` trigger tells the
command to do maintenance as-needed. Coupling that with whether or not
to detach was a misdesign of git-gc(1), I think. What it does control is
whether we detach or not when automatically executing maintenance via
`prepare_auto_maintenance()`.

Anyway, my fault for not documenting it, not yours for getting it
slightly wrong.

Patrick
diff mbox series

Patch

diff --git a/builtin/gc.c b/builtin/gc.c
index 63106e2028..bafee330a2 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1063,6 +1063,7 @@  static int maintenance_task_gc(struct maintenance_run_opts *opts,
 		strvec_push(&child.args, "--quiet");
 	else
 		strvec_push(&child.args, "--no-quiet");
+	strvec_push(&child.args, "--no-detach");
 
 	return run_command(&child);
 }
diff --git a/run-command.c b/run-command.c
index 45ba544932..94f2f3079f 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1808,16 +1808,26 @@  void run_processes_parallel(const struct run_process_parallel_opts *opts)
 
 int prepare_auto_maintenance(int quiet, struct child_process *maint)
 {
-	int enabled;
+	int enabled, auto_detach;
 
 	if (!git_config_get_bool("maintenance.auto", &enabled) &&
 	    !enabled)
 		return 0;
 
+	/*
+	 * When `maintenance.autoDetach` isn't set, then we fall back to
+	 * honoring `gc.autoDetach`. This is somewhat weird, but required to
+	 * retain behaviour from when we used to run git-gc(1) here.
+	 */
+	if (git_config_get_bool("maintenance.autodetach", &auto_detach) &&
+	    git_config_get_bool("gc.autodetach", &auto_detach))
+		auto_detach = 1;
+
 	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");
+	strvec_push(&maint->args, auto_detach ? "--detach" : "--no-detach");
 
 	return 1;
 }
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 2da7291e37..8415884754 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -229,7 +229,7 @@  test_expect_success 'fetch --refetch triggers repacking' '
 
 	GIT_TRACE2_EVENT="$PWD/trace1.event" \
 	git -C pc1 fetch --refetch origin &&
-	test_subcommand git maintenance run --auto --no-quiet <trace1.event &&
+	test_subcommand git maintenance run --auto --no-quiet --detach <trace1.event &&
 	grep \"param\":\"gc.autopacklimit\",\"value\":\"1\" trace1.event &&
 	grep \"param\":\"maintenance.incremental-repack.auto\",\"value\":\"-1\" trace1.event &&
 
@@ -238,7 +238,7 @@  test_expect_success 'fetch --refetch triggers repacking' '
 		-c gc.autoPackLimit=0 \
 		-c maintenance.incremental-repack.auto=1234 \
 		-C pc1 fetch --refetch origin &&
-	test_subcommand git maintenance run --auto --no-quiet <trace2.event &&
+	test_subcommand git maintenance run --auto --no-quiet --detach <trace2.event &&
 	grep \"param\":\"gc.autopacklimit\",\"value\":\"0\" trace2.event &&
 	grep \"param\":\"maintenance.incremental-repack.auto\",\"value\":\"-1\" trace2.event &&
 
@@ -247,7 +247,7 @@  test_expect_success 'fetch --refetch triggers repacking' '
 		-c gc.autoPackLimit=1234 \
 		-c maintenance.incremental-repack.auto=0 \
 		-C pc1 fetch --refetch origin &&
-	test_subcommand git maintenance run --auto --no-quiet <trace3.event &&
+	test_subcommand git maintenance run --auto --no-quiet --detach <trace3.event &&
 	grep \"param\":\"gc.autopacklimit\",\"value\":\"1\" trace3.event &&
 	grep \"param\":\"maintenance.incremental-repack.auto\",\"value\":\"0\" trace3.event
 '
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 771525aa4b..06ab43cfb5 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -49,22 +49,47 @@  test_expect_success 'run [--auto|--quiet]' '
 		git maintenance run --auto 2>/dev/null &&
 	GIT_TRACE2_EVENT="$(pwd)/run-no-quiet.txt" \
 		git maintenance run --no-quiet 2>/dev/null &&
-	test_subcommand git gc --quiet <run-no-auto.txt &&
-	test_subcommand ! git gc --auto --quiet <run-auto.txt &&
-	test_subcommand git gc --no-quiet <run-no-quiet.txt
+	test_subcommand git gc --quiet --no-detach <run-no-auto.txt &&
+	test_subcommand ! git gc --auto --quiet --no-detach <run-auto.txt &&
+	test_subcommand git gc --no-quiet --no-detach <run-no-quiet.txt
 '
 
 test_expect_success 'maintenance.auto config option' '
 	GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
-	test_subcommand git maintenance run --auto --quiet <default &&
+	test_subcommand git maintenance run --auto --quiet --detach <default &&
 	GIT_TRACE2_EVENT="$(pwd)/true" \
 		git -c maintenance.auto=true \
 		commit --quiet --allow-empty -m 2 &&
-	test_subcommand git maintenance run --auto --quiet  <true &&
+	test_subcommand git maintenance run --auto --quiet --detach <true &&
 	GIT_TRACE2_EVENT="$(pwd)/false" \
 		git -c maintenance.auto=false \
 		commit --quiet --allow-empty -m 3 &&
-	test_subcommand ! git maintenance run --auto --quiet  <false
+	test_subcommand ! git maintenance run --auto --quiet --detach <false
+'
+
+for cfg in maintenance.autoDetach gc.autoDetach
+do
+	test_expect_success "$cfg=true config option" '
+		test_when_finished "rm -f trace" &&
+		test_config $cfg true &&
+		GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
+		test_subcommand git maintenance run --auto --quiet --detach <trace
+	'
+
+	test_expect_success "$cfg=false config option" '
+		test_when_finished "rm -f trace" &&
+		test_config $cfg false &&
+		GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
+		test_subcommand git maintenance run --auto --quiet --no-detach <trace
+	'
+done
+
+test_expect_success "maintenance.autoDetach overrides gc.autoDetach" '
+	test_when_finished "rm -f trace" &&
+	test_config maintenance.autoDetach false &&
+	test_config gc.autoDetach true &&
+	GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
+	test_subcommand git maintenance run --auto --quiet --no-detach <trace
 '
 
 test_expect_success 'register uses XDG_CONFIG_HOME config if it exists' '
@@ -129,9 +154,9 @@  test_expect_success 'run --task=<task>' '
 		git maintenance run --task=commit-graph 2>/dev/null &&
 	GIT_TRACE2_EVENT="$(pwd)/run-both.txt" \
 		git maintenance run --task=commit-graph --task=gc 2>/dev/null &&
-	test_subcommand ! git gc --quiet <run-commit-graph.txt &&
-	test_subcommand git gc --quiet <run-gc.txt &&
-	test_subcommand git gc --quiet <run-both.txt &&
+	test_subcommand ! git gc --quiet --no-detach <run-commit-graph.txt &&
+	test_subcommand git gc --quiet --no-detach <run-gc.txt &&
+	test_subcommand git gc --quiet --no-detach <run-both.txt &&
 	test_subcommand git commit-graph write --split --reachable --no-progress <run-commit-graph.txt &&
 	test_subcommand ! git commit-graph write --split --reachable --no-progress <run-gc.txt &&
 	test_subcommand git commit-graph write --split --reachable --no-progress <run-both.txt