Re: [PATCH v2 3/3] sequencer: keep auto maintenance out of the commands a sequence spawns

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sep 7, 2026, 13:24 UTC
Message-ID: <7493f0b7-a6cb-4b7d-bfd4-f4a318ff7e32@gmail.com>
In-Reply-To: <9a6fc0427a8bc7e7abcc0518214b1dafc2efaa6a.1788537086.git.gitgitgadget@gmail.com>

On 04/09/2026 16:51, Thomas Bachem via GitGitGadget wrote:

Show 6 quoted lines

> From: Thomas Bachem <mail@thomasbachem.com>
> 
> The "git commit" and "git merge" the sequencer spawns, and the git
> commands an exec runs, each start "git maintenance run --auto
> --detach", which then works in the background against the sequence
> itself.

I don't think maintenance is actively working against other commands, it just creates lock contention. Maybe something like

     When the sequencer runs "git commit" or "git merge", either directly
     or via a user supplied exec command, those commands run "git
     maintenance --auto --detach" which can cause lock contention with
     the sequencer.

Show 5 quoted lines

> A "rerere gc" started by the commit of one "git rebase
> --continue" holds MERGE_RR.lock when the next pick needs it, and a
> repack deletes packs the sequencer still has open, which 65cda10d5b
> (sequencer: release the ODB before spawning git commit, 2026-08-12)
> had to work around.

This is pretty hard to understand. What does 'the commit of one "git rebase --continue"' mean? Also whether the next pick needs to take MERGE_RR.lock is conditional on there being conflicts which isn't at all clear.

> The loose objects a sequence creates wait for the run at its end that
> the previous commit added.

What does that mean?

Show 5 quoted lines

> Whether a sequence can be long enough to
> suffer from them before that remains to be seen. Pass
> maintenance.auto=false and gc.auto=0 to the spawned commands through
> GIT_CONFIG_PARAMETERS, which the shell of an exec command hands on to
> whatever it runs,

Talking about the shell here is unnecessarily confusing as the command is not necessarily run by the shell: if it is a single word that does not contain any shell metacharacters it is passed directly to exec()

Show 5 quoted lines

> appended after the user's own -c settings so that
> ours win, and built once per run. A command the user runs while the
> sequence is stopped, like "git commit --amend" at an edit, is not the
> sequencer's to control and still runs maintenance.
>

Show 9 quoted lines

> @@ -1107,6 +1114,27 @@ static int run_command_silent_on_success(struct child_process *cmd)
>   	return rc;
>   }
>   
> +/*
> + * A sequence runs auto maintenance once it is done, not from every command
> + * it spawns along the way: their background "rerere gc" or repack would
> + * race the sequencer for locks and files it still holds.
> + */

This comment isn't wrong but sounds like an LLM, rather than something a person would write.

Show 12 quoted lines

> +static void disable_auto_maintenance(struct replay_opts *opts,
> +				     struct child_process *cmd)
> +{
> +	struct strbuf *params = &opts->ctx->config_parameters;
> +
> +	if (!params->len) {
> +		const char *old = getenv(CONFIG_DATA_ENVIRONMENT);
> +
> +		if (old && *old)
> +			strbuf_addstr(params, old);
> +		git_config_append_parameter(params, "maintenance.auto", "false");
> +		git_config_append_parameter(params, "gc.auto", "0");

This is much nicer now we have the helper function and the calls to disable_auto_maintenance() that I've trimmed all look good.

Show 24 quoted lines

> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index 2c34cf8a01..cf6d20ce79 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -403,4 +403,22 @@ test_expect_success 'rebase runs auto maintenance at its end' '
>   	test_subcommand_flex git maintenance run --auto <finish.txt
>   '
>   
> +test_expect_success 'rebase spawns no auto maintenance before its end' '
> +	git checkout -b two-conflicts topic &&
> +	test_commit F2-again F2 222 &&
> +	test_must_fail git rebase -x "git commit --allow-empty -m exec" main &&
> +	echo resolved >F2 &&
> +	git add F2 &&
> +	test_must_fail env GIT_TRACE2_EVENT="$(pwd)/mid.txt" \
> +		git rebase --continue &&
> +	test_subcommand_flex git commit <mid.txt &&
> +	test_subcommand_flex ! git maintenance run --auto <mid.txt &&
> +	echo resolved >F2 &&
> +	git add F2 &&
> +	GIT_TRACE2_EVENT="$(pwd)/end.txt" git rebase --continue &&
> +	test_subcommand_flex git maintenance run --auto <end.txt &&
> +	grep "\"child_start\".*\"maintenance\"" end.txt >maintenance &&
> +	test_line_count = 1 maintenance

Shouldn't this just extend the test added in the previous patch, rather than duplicating the coverage for auto maintenance being run at the end of a rebase?

Show 26 quoted lines

> +'
> +
>   test_done
> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> index 304981ccd6..57a77d91bd 100755
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -731,4 +731,21 @@ test_expect_success 'cherry-pick runs auto maintenance once it is done' '
>   	test_line_count = 1 maintenance
>   '
>   
> +test_expect_success 'cherry-pick spawns no auto maintenance before it is done' '
> +	pristine_detach initial &&
> +	test_must_fail git cherry-pick base..anotherpick &&
> +	echo resolved >foo &&
> +	git add foo &&
> +	test_must_fail env GIT_TRACE2_EVENT="$(pwd)/mid.txt" \
> +		git cherry-pick --continue &&
> +	test_subcommand_flex git commit <mid.txt &&
> +	test_subcommand_flex ! git maintenance run --auto <mid.txt &&
> +	echo d >foo &&
> +	git add foo &&
> +	GIT_TRACE2_EVENT="$(pwd)/end.txt" git cherry-pick --continue &&
> +	test_subcommand_flex git commit <end.txt &&
> +	grep "\"child_start\".*\"maintenance\"" end.txt >maintenance &&
> +	test_line_count = 1 maintenance

Again why do we need a separate test, rather than extending the one we've just added in the previous commit?

Thanks

Phillip

> +'
> +
>   test_done