From: Phillip Wood Date: Mon, 07 Sep 2026 13:24:13 GMT Subject: Re: [PATCH v2 3/3] sequencer: keep auto maintenance out of the commands a sequence spawns 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: > From: Thomas Bachem > > 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. > 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? > 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() > 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. > > @@ -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. > +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. > 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 ' > > +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 + test_subcommand_flex ! git maintenance run --auto + echo resolved >F2 && > + git add F2 && > + GIT_TRACE2_EVENT="$(pwd)/end.txt" git rebase --continue && > + test_subcommand_flex git maintenance run --auto + 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? > +' > + > 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 + test_subcommand_flex ! git maintenance run --auto + echo d >foo && > + git add foo && > + GIT_TRACE2_EVENT="$(pwd)/end.txt" git cherry-pick --continue && > + test_subcommand_flex git commit + 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