sequencer: leave auto maintenance to the end of a sequence #2217
There are no files selected for viewing
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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,
ThomasThere was a problem hiding this comment. Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Fri, Sep 04, 2026 at 03:51:26PM +0000, Thomas Bachem via GitGitGadget wrote:
> diff --git a/sequencer.c b/sequencer.c
> index 67e1c38762..5df07750a7 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -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.
> + */
> +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);
> +}
> +
Why do you set both "maintenance.auto" and "gc.auto"? Setting only the
former should be sufficient, as maintenance uses git-maintenance(1)
exclusively nowadays. Sure, it may trigger git-gc(1) internally. But it
won't ever do so if auto-maintenance is completely disabled.
PatrickThere was a problem hiding this comment. Thomas Bachem wrote on the Git mailing list (how to reply to this email): Hi Patrick,
On 07/09/2026 10:14, Patrick Steinhardt wrote:
> Why do you set both "maintenance.auto" and "gc.auto"? Setting only the
> former should be sufficient, as maintenance uses git-maintenance(1)
> exclusively nowadays. Sure, it may trigger git-gc(1) internally. But it
> won't ever do so if auto-maintenance is completely disabled.
You're right. prepare_auto_maintenance() looks at gc.auto only when
maintenance.auto is unset, so maintenance.auto alone is enough.
gc.auto=0 would still stop an exec that runs "git gc --auto" itself,
but I don't think we need to guard against that. I'll drop it in v3.
Thanks,
ThomasThere was a problem hiding this comment. Phillip Wood wrote on the Git mailing list (how to reply to this email): On 04/09/2026 16:51, Thomas Bachem via GitGitGadget wrote:
> 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.
> 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 <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?
> +'
> +
> 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_doneThere was a problem hiding this comment. Thomas Bachem wrote on the Git mailing list (how to reply to this email): Hi Phillip,
On 07/09/2026 15:24, Phillip Wood wrote:
> 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.
I'll use that. The repack case is a bit different, though. It can
delete a pack the sequencer still has open, which 65cda10d5b had to
work around, so I'll keep one sentence on it.
> 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.
I meant the "git commit" that "git rebase --continue" spawns for a
resolved conflict. Its maintenance run can still hold MERGE_RR.lock
when the next pick conflicts and rerere needs it. I'll write it like
that.
> What does that mean?
Once the spawned commands no longer run maintenance, a long sequence
can pile up loose objects, and nothing packs them before the run at
the end. I don't know whether a sequence can get long enough for that
to matter. I'll say it like this, or drop it.
> 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()
Right, the environment reaches the command either way. I'll drop the
shell from the message.
> This comment isn't wrong but sounds like an LLM, rather than something a
> person would write.
I've rewritten it:
/*
* Don't let the commands we spawn run auto maintenance. It would
* race us for MERGE_RR.lock or delete packs we still have open,
* so it runs once at the end of the sequence instead.
*/
> 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?
Yes, I'll extend both tests from the previous patch instead.
Thanks,
ThomasThere was a problem hiding this comment. Phillip Wood wrote on the Git mailing list (how to reply to this email): Hi Thomas
On 09/09/2026 09:25, Thomas Bachem via GitGitGadget wrote:
> From: Thomas Bachem <mail@thomasbachem.com>
> > Sequencer-spawned commands like 'commit' and 'merge' run
> background auto maintenance, which interferes with ongoing
> operations (e.g. 'rerere gc' holding MERGE_RR.lock or repacks
> deleting active packs).
This is much more concise, but still sounds a bit strange to me. I'd suggest
When the sequencer spawns "git commit", or "git merge", those commands
run "git maintenance --auto" in the background which can interfere with
the sequencer (e.g. 'rerere gc' holding MERGE_RR.lock or repacks deleting active packs).
> Pass maintenance.auto=false via GIT_CONFIG_PARAMETERS to the
> spawned commit, merge and exec commands. Appending it after the
> user's own settings ensures it wins, and the environment reaches
> whatever they spawn in turn.
> > Auto maintenance now runs exactly once when the sequence
> completes. Commands run manually by the user while stopped are
> unaffected and continue to run auto maintenance normally.
Good, and the implementation changes look correct.
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index 025787b5f2..16def261b0 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -398,13 +398,18 @@ test_orig_head --merge
> test_expect_success 'rebase runs auto maintenance once it is done' '
> git checkout -b auto-maintenance topic &&
> test_must_fail env GIT_TRACE2_EVENT="$(pwd)/stop.txt" \
> - git rebase -x false main &&
> + git rebase -x "git commit --allow-empty -m exec && false" main &&
> test_subcommand_flex ! git maintenance run --auto <stop.txt &&
> echo resolved >F2 &&
> git add F2 &&
> - test_must_fail git rebase --continue &&
> + test_must_fail env GIT_TRACE2_EVENT="$(pwd)/mid.txt" \
> + git rebase --continue &&
What's this trying to check - there wasn't a conflict so commit_staged_changes() will error out without trying to commit anything. This series is looking pretty good now. Just to let you know I'll be off the list from tomorrow until the middle of next week so it will be a few days before I look at any new versions of this patch.
Thanks
Phillip
> + test_subcommand_flex git commit <mid.txt &&
> + test_subcommand_flex ! git maintenance run --auto <mid.txt &&
> GIT_TRACE2_EVENT="$(pwd)/end.txt" git rebase --continue &&
> - test_subcommand_flex git maintenance run --auto <end.txt
> + test_subcommand_flex git maintenance run --auto <end.txt &&
> + grep "\"child_start\".*\"maintenance\"" end.txt >maintenance &&
> + test_line_count = 1 maintenance
> '
> > test_done
> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> index 2bea55c3b6..1e3fa1803c 100755
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -723,8 +723,11 @@ test_expect_success 'commit descriptions in insn sheet are optional' '
> > test_expect_success 'cherry-pick runs auto maintenance once it is done' '
> pristine_detach base &&
> - GIT_TRACE2_EVENT="$(pwd)/single.txt" git cherry-pick picked &&
> + GIT_TRACE2_EVENT="$(pwd)/single.txt" git cherry-pick --edit picked &&
> + test_subcommand_flex git commit <single.txt &&
> test_subcommand_flex git maintenance run --auto <single.txt &&
> + grep "\"child_start\".*\"maintenance\"" single.txt >maintenance &&
> + test_line_count = 1 maintenance &&
> GIT_TRACE2_EVENT="$(pwd)/sequence.txt" \
> git cherry-pick anotherpick yetanotherpick &&
> test_subcommand_flex git maintenance run --auto <sequence.txt &&
> @@ -739,9 +742,14 @@ test_expect_success 'cherry-pick runs auto maintenance once a stopped sequence i
> test_subcommand_flex ! git maintenance run --auto <stop.txt &&
> echo resolved >foo &&
> git add foo &&
> - test_must_fail git cherry-pick --continue &&
> + 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 &&
> GIT_TRACE2_EVENT="$(pwd)/end.txt" git cherry-pick --skip &&
> - test_subcommand_flex git maintenance run --auto <end.txt
> + test_subcommand_flex git maintenance run --auto <end.txt &&
> + grep "\"child_start\".*\"maintenance\"" end.txt >maintenance &&
> + test_line_count = 1 maintenance
> '
> > test_done |
||
| */ | ||
| unsigned have_message :1; | ||
| /* | ||
| * GIT_CONFIG_PARAMETERS for the commands we spawn, with auto | ||
| * maintenance turned off. Built on first use. | ||
| */ | ||
| char *config_parameters; | ||
| }; | ||
|
|
||
| struct replay_ctx* replay_ctx_new(void) | ||
|
|
@@ -407,6 +412,7 @@ static void replay_ctx_release(struct replay_ctx *ctx) | |
| { | ||
| strbuf_release(&ctx->current_fixups); | ||
| strbuf_release(&ctx->message); | ||
| free(ctx->config_parameters); | ||
| } | ||
|
|
||
| void replay_opts_release(struct replay_opts *opts) | ||
|
|
@@ -1107,6 +1113,27 @@ 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 |
||
| } | ||
|
|
||
| /* | ||
| * Don't let the commands we spawn run auto maintenance. It would race | ||
| * us for MERGE_RR.lock or delete packs we still have open. Our caller | ||
| * runs it once the sequence is done. | ||
| */ | ||
| static void disable_auto_maintenance(struct replay_opts *opts, | ||
| struct child_process *cmd) | ||
| { | ||
| if (!opts->ctx->config_parameters) { | ||
| const char *old = getenv(CONFIG_DATA_ENVIRONMENT); | ||
| struct strbuf buf = STRBUF_INIT; | ||
|
|
||
| if (old && *old) | ||
| strbuf_addstr(&buf, old); | ||
| git_config_append_parameter(&buf, "maintenance.auto", "false"); | ||
| opts->ctx->config_parameters = strbuf_detach(&buf, NULL); | ||
| } | ||
| strvec_pushf(&cmd->env, "%s=%s", CONFIG_DATA_ENVIRONMENT, | ||
| opts->ctx->config_parameters); | ||
| } | ||
|
|
||
| /* | ||
| * 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 +1175,7 @@ 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="); | ||
| disable_auto_maintenance(opts, &cmd); | ||
|
|
||
| strvec_push(&cmd.args, "commit"); | ||
|
|
||
|
|
@@ -3924,16 +3952,18 @@ static int error_failed_squash(struct repository *r, | |
| return error_with_patch(r, commit, subject, subject_len, opts, 1, 1); | ||
| } | ||
|
|
||
| static int do_exec(struct repository *r, const char *command_line, | ||
| struct replay_opts *opts) | ||
| { | ||
| struct child_process cmd = CHILD_PROCESS_INIT; | ||
| int dirty, status; | ||
|
|
||
| if (!opts->quiet) | ||
| fprintf(stderr, _("Executing: %s\n"), command_line); | ||
| cmd.use_shell = 1; | ||
| strvec_push(&cmd.args, command_line); | ||
| strvec_push(&cmd.env, "GIT_CHERRY_PICK_HELP"); | ||
| disable_auto_maintenance(opts, &cmd); | ||
| status = run_command(&cmd); | ||
|
|
||
| /* force re-reading of the cache */ | ||
|
|
@@ -4342,6 +4372,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(opts, &cmd); | ||
|
|
||
| cmd.git_cmd = 1; | ||
| strvec_push(&cmd.args, "merge"); | ||
|
|
@@ -5158,7 +5189,7 @@ static int pick_commits(struct repository *r, | |
| if (!opts->verbose) | ||
| term_clear_line(); | ||
| *end_of_arg = '\0'; | ||
| res = do_exec(r, arg, opts); | ||
| *end_of_arg = saved; | ||
|
|
||
| if (res) { | ||
|
|
@@ -5329,6 +5360,7 @@ static int continue_single_pick(struct repository *r, struct replay_opts *opts) | |
| return error(_("no cherry-pick or revert in progress")); | ||
|
|
||
| cmd.git_cmd = 1; | ||
| disable_auto_maintenance(opts, &cmd); | ||
| strvec_push(&cmd.args, "commit"); | ||
|
|
||
| /* | ||
|
|
||
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phillip Wood wrote on the Git mailing list (how to reply to this email):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thomas Bachem wrote on the Git mailing list (how to reply to this email):