sequencer: leave auto maintenance to the end of a sequence - #2217
Open
thomasbachem wants to merge 3 commits into
Open
sequencer: leave auto maintenance to the end of a sequence#2217thomasbachem wants to merge 3 commits into
thomasbachem wants to merge 3 commits into
Conversation
Author
|
/preview |
|
Preview email sent as pull.2217.git.1788507668.gitgitgadget@gmail.com |
Author
|
/preview |
|
Preview email sent as pull.2217.git.1788507851.gitgitgadget@gmail.com |
Author
|
/preview |
1 similar comment
Author
|
/preview |
|
Preview email sent as pull.2217.git.1788508300.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.2217.git.1788508426.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -1107,6 +1107,29 @@ static int run_command_silent_on_success(struct child_process *cmd) | |||
| return rc; | |||
There was a problem hiding this comment.
Phillip Wood wrote on the Git mailing list (how to reply to this email):
Hi Thomas
On 04/09/2026 08:53, Thomas Bachem via GitGitGadget wrote:
> From: Thomas Bachem <mail@thomasbachem.com>
> > The commands a rebase with the merge backend spawns, the "git commit"
> for a resolved, reworded or squashed pick, the "git merge" of a
> "rebase -r" for an octopus merge or with a strategy, and whatever an
> exec command runs, each kick off "git maintenance run --auto --detach",
> a background process the rebase then races for the repository: the
> "rerere gc" spawned by the commit of one "git rebase --continue" holds
> MERGE_RR.lock while the next pick wants it, and a repack wants to
> delete packs the sequencer still had open, which 65cda10d5b
> (sequencer: release the ODB before spawning git commit, 2026-08-12)
> had to fix for Windows.
This entire paragraph is a single sentence and is very hard to understand.
> Nothing a rebase creates is old enough to be pruned by the time it
s/Nothing/The objects/?
> ends, and repacking what it created can wait until then,
That's what we'll find out when this is merged. In principle it is possible someone is doing enormous rebases where the number of loose objects impacts the performance if we don't repack mid-rebase but I don't think we can know that without disabling auto maintenance and seeing if anyone complians.
> so
> maintenance in the middle of a rebase has nothing to do that a run at
> its end cannot, and a rebase to get in the way of.
That last clause is hard to parse.
> Pass
> maintenance.auto=false and gc.auto=0 to the commands a rebase spawns,
> through GIT_CONFIG_PARAMETERS so that the shell of an exec command
> passes them on too, appended to whatever -c the user gave, since the
> last entry wins. What the user runs while the rebase is stopped, say
> "git commit --amend" at an edit, is not the rebase's to control and
> still runs it. s/runs/run/
> "git commit" and "git merge" could skip it themselves
> while a rebase is in progress, which would cover that too, but that
> spreads the rebase's business over every command that runs
> maintenance and defers theirs for as long as a rebase is left lying
> around, so keep the decision with the rebase, in what it spawns. Both
> backends run maintenance once the rebase is done, the merge backend
> since the previous commit, so nothing is lost.
> > Cherry-pick and revert are left alone: they never ran maintenance at
> the end of a sequence, and the "git commit" they spawn for a
> --continue or an edited message is the only place they run it at all.
I'm inclined to think that the reasoning for running maintenance at the end of a rebase applies to cherry-pick and probably revert as well.
> > Assisted-by: Claude Fable 5.1
> Signed-off-by: Thomas Bachem <mail@thomasbachem.com>
> ---
> sequencer.c | 27 +++++++++++++++++++++++++++
> t/t3418-rebase-continue.sh | 18 ++++++++++++++++++
> 2 files changed, 45 insertions(+)
> > diff --git a/sequencer.c b/sequencer.c
> index f58ad254be..30c1a799cc 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1107,6 +1107,29 @@ static int run_command_silent_on_success(struct child_process *cmd)
> return rc;
> }
> > +/*
> + * A rebase 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 rebase for locks and files it still holds.
> + */
> +static void disable_auto_maintenance(struct child_process *cmd)
> +{
> + struct strbuf value = STRBUF_INIT;
> + const char *old = getenv(CONFIG_DATA_ENVIRONMENT);
> +
> + if (old && *old)
> + strbuf_addf(&value, "%s ", old);
> + sq_quote_buf(&value, "maintenance.auto");
> + strbuf_addch(&value, '=');
> + sq_quote_buf(&value, "false");
> + strbuf_addch(&value, ' ');
> + sq_quote_buf(&value, "gc.auto");
> + strbuf_addch(&value, '=');
> + sq_quote_buf(&value, "0");
> + strvec_pushf(&cmd->env, "%s=%s", CONFIG_DATA_ENVIRONMENT, value.buf);
> + strbuf_release(&value);
> +}
We already have a function in config.c to append parameters, but it sets them in the callers environment which we don't want to do here. I wonder if we could factor out a helper append the parameters to an strbuf passed by the caller so we don't need to know about the quoting scheme here. Also it would be nice to cache this in replay_ctx so we don't have to construct the string each time we want to disable auto maintenance.
Other than that this looks good
Thanks
Phillip
> +
> /*
> * If we are cherry-pick, and if the merge did not result in
> * hand-editing, we will hit this commit and inherit the original
> @@ -1148,6 +1171,8 @@ static int run_git_commit(const char *defmsg,
> author_date_from_env(&cmd.env));
> if (opts->ignore_date)
> strvec_push(&cmd.env, "GIT_AUTHOR_DATE=");
> + if (is_rebase_i(opts))
> + disable_auto_maintenance(&cmd);
> > strvec_push(&cmd.args, "commit");
> > @@ -3934,6 +3959,7 @@ static int do_exec(struct repository *r, const char *command_line, int quiet)
> cmd.use_shell = 1;
> strvec_push(&cmd.args, command_line);
> strvec_push(&cmd.env, "GIT_CHERRY_PICK_HELP");
> + disable_auto_maintenance(&cmd);
> status = run_command(&cmd);
> > /* force re-reading of the cache */
> @@ -4342,6 +4368,7 @@ static int do_merge(struct repository *r,
> author_date_from_env(&cmd.env));
> if (opts->ignore_date)
> strvec_push(&cmd.env, "GIT_AUTHOR_DATE=");
> + disable_auto_maintenance(&cmd);
> > cmd.git_cmd = 1;
> strvec_push(&cmd.args, "merge");
> 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
> +'
> +
> test_doneThere was a problem hiding this comment.
Thomas Bachem wrote on the Git mailing list (how to reply to this email):
Hi Phillip,
On 04/09/2026 16:03, Phillip Wood wrote:
> That's what we'll find out when this is merged.
Agreed, and the message now says so instead of claiming it.
> I'm inclined to think that the reasoning for running maintenance at the
> end of a rebase applies to cherry-pick and probably revert as well.
Agreed. In v2 all three end with one run where the sequencer finishes,
and none of the commands they spawn run it, the "git commit" of a
"cherry-pick --continue" included.
> I wonder if we could factor out a helper append the parameters to an
> strbuf passed by the caller so we don't need to know about the quoting
> scheme here. Also it would be nice to cache this in replay_ctx so we
> don't have to construct the string each time we want to disable auto
> maintenance.
Done, as a small config.c patch in front of the two, and a strbuf in
replay_ctx built on first use.
The messages are rewritten from scratch and much shorter, the sentence
you could not parse included.
Thanks,
Thomas
|
User |
|
|
||
| hook_opt.path_to_stdin = rebase_path_rewritten_list(); | ||
| strvec_push(&hook_opt.args, "rebase"); | ||
| run_hooks_opt(r, "post-rewrite", &hook_opt); |
There was a problem hiding this comment.
Phillip Wood wrote on the Git mailing list (how to reply to this email):
Hi Thomas
On 04/09/2026 08:53, Thomas Bachem via GitGitGadget wrote:
> From: Thomas Bachem <mail@thomasbachem.com>
> > The apply backend runs "git maintenance run --auto" from
> finish_rebase() once it has applied its patches, and so does "git am"
> on its own. The merge backend reaches finish_rebase() only on the
> paths both backends share in builtin/rebase.c: an abort, a branch
> that is already up to date, and a fast-forward. A rebase that
> replays commits never runs maintenance at its end. It creates most
> of its commits in process, and only the "git commit" it spawns for a
> resolved, reworded or squashed pick, the "git merge" a "rebase -r"
> spawns for an octopus merge or with a strategy, and whatever an exec
> command runs kick maintenance off, in the middle of the rebase. Run
> it where the sequencer finishes a rebase, after the autostash is
> applied, as finish_rebase() does, so that both backends end a rebase
> the same way, and so that the next commit can keep it out of the
> commands a rebase spawns. builtin/rebase.c could run it instead once
> run_sequencer_rebase() returns, but the sequencer is where the rebase
> finishes, and the autostash and the state cleanup that surround the
> run in finish_rebase() are there as well. prepare_auto_maintenance()
> closes the object database before the spawn, so the sequencer holds
> nothing a repack would need to replace.
It would be nice if the two backends shared more of the cleanup code, I don't think there is a good reason for them not to share the code that copies notes, applies the autostash and switches HEAD back to the branch, but that is outside the scope of this change. So adding a separate call to the sequencer code seems reasonable. I wonder if we should do this for cherry-pick and revert as well; certainly cherry-pick can create a lot of loose objects and does not necessarily call "git commit". If cherry-pick does call "git commit" and trigger background maintenance it is susceptible to the same problems as rebase, so we should probably treat them the same.
Thanks
Phillip
> > Assisted-by: Claude Fable 5.1
> Signed-off-by: Thomas Bachem <mail@thomasbachem.com>
> ---
> sequencer.c | 5 +++++
> t/t3418-rebase-continue.sh | 8 ++++++++
> 2 files changed, 13 insertions(+)
> > diff --git a/sequencer.c b/sequencer.c
> index 65afd100d9..f58ad254be 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5297,6 +5297,11 @@ cleanup_head_ref:
> run_hooks_opt(r, "post-rewrite", &hook_opt);
> }
> apply_autostash(rebase_path_autostash());
> + /*
> + * We ignore errors in 'git maintenance run --auto', since the
> + * user should see them.
> + */
> + run_auto_maintenance(r, opts->quiet);
> > if (!opts->quiet) {
> if (!opts->verbose)
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index cb5c3a1cb5..2c34cf8a01 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -395,4 +395,12 @@ test_orig_head () {
> test_orig_head --apply
> test_orig_head --merge
> > +test_expect_success 'rebase runs auto maintenance at its end' '
> + git checkout -b one-exec main^ &&
> + test_commit F4 &&
> + test_must_fail git rebase -x false main &&
> + GIT_TRACE2_EVENT="$(pwd)/finish.txt" git rebase --continue &&
> + test_subcommand_flex git maintenance run --auto <finish.txt
> +'
> +
> test_done
Split the part of git_config_push_split_parameter() that formats one GIT_CONFIG_PARAMETERS entry into a helper that appends it to a strbuf, so that a caller can build a value for a child's environment without knowing the quoting. The sequencer is about to do that. Assisted-by: Claude Fable 5.1 Signed-off-by: Thomas Bachem <mail@thomasbachem.com>
The apply backend of "git rebase" runs "git maintenance run --auto" from finish_rebase() once it has applied its patches. The merge backend, "git cherry-pick" and "git revert" do not run it when they finish. They create their commits in process, and only the "git commit" they spawn for an edited message or a resolved conflict, the "git merge" a "rebase -r" spawns and an exec command start it, in the middle of the sequence. Run it where the sequencer finishes, so that every sequence ends the way the apply backend does, and so that the next commit can keep it out of the commands a sequence spawns. Assisted-by: Claude Fable 5.1 Signed-off-by: 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. 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 65cda10 (sequencer: release the ODB before spawning git commit, 2026-08-12) had to work around. The loose objects a sequence creates wait for the run at its end that the previous commit added. 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, 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. Assisted-by: Claude Fable 5.1 Signed-off-by: Thomas Bachem <mail@thomasbachem.com>
thomasbachem
force-pushed
the
rebase-auto-maintenance
branch
from
September 4, 2026 15:47
06d2f0f to
9a6fc04
Compare
thomasbachem
changed the title
Author
|
/preview |
|
Preview email sent as pull.2217.v2.git.1788536934.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.2217.v2.git.1788537086.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
| @@ -234,6 +234,11 @@ struct replay_ctx { | |||
| * Whether message contains a commit message. | |||
There was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
"Thomas Bachem via GitGitGadget" <gitgitgadget@gmail.com> writes:
> + /*
> + * The GIT_CONFIG_PARAMETERS value that keeps auto maintenance out
> + * of the commands we spawn, built on first use.
> + */
> + struct strbuf config_parameters;
Does this have to be a "struct strbuf", not "const char *"? The
latter makes it clear that it will never change its value once you
built it in disable_auto_maintenance().
> +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");
> + }
> + strvec_pushf(&cmd->env, "%s=%s", CONFIG_DATA_ENVIRONMENT, params->buf);
> +}
This would then become something like
if (!opts->ctx->config_parameters) {
const char *old = getenv(CONFIG_DATA_ENVIRONMENT);
struct strbuf params = STRBUF_INIT;
if (old && *old)
strbuf_addstr(¶ms, old);
git_config_append_parameter(¶ms, "maintenance.auto", "0");
git_config_append_parameter(¶ms, "gc.auto", "0");
opts->ctx->config_parameters = strbuf_detach(¶ms, NULL);
}
strbuf_pushf(..., opts->ctx->config_parameters);
There was a problem hiding this comment.
Thomas Bachem wrote on the Git mailing list (how to reply to this email):
Hi Junio,
On 04/09/2026 23:21, Junio C Hamano wrote:
> Does this have to be a "struct strbuf", not "const char *"? The
> latter makes it clear that it will never change its value once you
> built it in disable_auto_maintenance().
It doesn't. I'll make it a char * that replay_ctx_release() frees,
built the way you sketch, in the next version.
Thanks,
Thomas
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes since v1:
too, and keep it out of the "git commit" they spawn, so the three
commands now behave the same (Phillip).
git_config_push_split_parameter() in config.c (new patch 1), built
once per run and kept in replay_ctx (Phillip).
Based on master. Independent of the rerere lock fix in [1].
[1] <pull.2214.v2.git.1788507876543.gitgitgadget@gmail.com>
Cc: Phillip Wood phillip.wood@dunelm.org.uk
Cc: Patrick Steinhardt ps@pks.im
Cc: Junio C Hamano gitster@pobox.com
Cc: Johannes Schindelin johannes.schindelin@gmx.de