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
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 branch is now known as |
|
This patch series was integrated into seen via git@e41b0e7. |
| @@ -450,18 +450,24 @@ static int git_config_include(const char *var, const char *value, | |||
| return ret; | |||
There 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:24PM +0000, Thomas Bachem via GitGitGadget wrote:
> From: Thomas Bachem <mail@thomasbachem.com>
>
> 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.
Readers who don't have any context around GIT_CONFIG_PARAMETERS and what
it does will have a bit of a hard time making much sense of this, I
think. It usually helps to give a sentence or two explaining what the
infra even does, and what this quoting looks like.
> Assisted-by: Claude Fable 5.1
> Signed-off-by: Thomas Bachem <mail@thomasbachem.com>
> ---
> config.c | 20 +++++++++++++-------
> config.h | 10 ++++++++++
> 2 files changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/config.c b/config.c
> index d9019e7e6c..e0bb29b53d 100644
> --- a/config.c
> +++ b/config.c
> @@ -450,18 +450,24 @@ static int git_config_include(const char *var, const char *value,
> return ret;
> }
>
> +void git_config_append_parameter(struct strbuf *env, const char *key,
Nit: callling this `env` assumes a bit too much about what this buffer
is going to be used for. I'd have called it just `buf`.
> diff --git a/config.h b/config.h
> index b66dd08007..fcf48f6245 100644
> --- a/config.h
> +++ b/config.h
> @@ -22,6 +22,7 @@
> */
>
> struct object_id;
> +struct strbuf;
>
> /* git_config_parse_key() returns these negated: */
> #define CONFIG_INVALID_KEY 1
> @@ -186,6 +187,15 @@ int git_config_from_blob_oid(config_fn_t fn, const char *name,
> enum config_scope scope);
> void git_config_push_parameter(const char *text);
> void git_config_push_env(const char *spec);
> +
> +/*
> + * Append `key=value` to the GIT_CONFIG_PARAMETERS value in `env`, quoted
> + * the way git_config_from_parameters() reads it, so that a child can be
> + * given configuration on top of what this process was given. A NULL
> + * `value` appends a boolean entry.
> + */
> +void git_config_append_parameter(struct strbuf *env, const char *key,
> + const char *value);
Pointing to that other function makes sense, but neither of the
functions documents the actual format that's used.
PatrickThere was a problem hiding this comment.
Phillip Wood wrote on the Git mailing list (how to reply to this email):
On 07/09/2026 09:14, Patrick Steinhardt wrote:
> On Fri, Sep 04, 2026 at 03:51:24PM +0000, Thomas Bachem via GitGitGadget wrote:
>> From: Thomas Bachem <mail@thomasbachem.com>
>>
>> diff --git a/config.c b/config.c
>> index d9019e7e6c..e0bb29b53d 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -450,18 +450,24 @@ static int git_config_include(const char *var, const char *value,
>> return ret;
>> }
>> >> +void git_config_append_parameter(struct strbuf *env, const char *key,
> > Nit: callling this `env` assumes a bit too much about what this buffer
> is going to be used for. I'd have called it just `buf`.
Are we ever likely to use this outside of GIT_CONFIG_PARAMETERS? If not then I think env is a good name because it hints at where this function is used. Isn't the whole point of this function to allow us to append settings an environment variable?
Thanks
PhillipThere was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Mon, Sep 07, 2026 at 02:24:34PM +0100, Phillip Wood wrote:
> On 07/09/2026 09:14, Patrick Steinhardt wrote:
> > On Fri, Sep 04, 2026 at 03:51:24PM +0000, Thomas Bachem via GitGitGadget wrote:
> > > From: Thomas Bachem <mail@thomasbachem.com>
> > >
> > > diff --git a/config.c b/config.c
> > > index d9019e7e6c..e0bb29b53d 100644
> > > --- a/config.c
> > > +++ b/config.c
> > > @@ -450,18 +450,24 @@ static int git_config_include(const char *var, const char *value,
> > > return ret;
> > > }
> > > +void git_config_append_parameter(struct strbuf *env, const char *key,
> >
> > Nit: callling this `env` assumes a bit too much about what this buffer
> > is going to be used for. I'd have called it just `buf`.
>
> Are we ever likely to use this outside of GIT_CONFIG_PARAMETERS? If not then
> I think env is a good name because it hints at where this function is used.
> Isn't the whole point of this function to allow us to append settings an
> environment variable?
Potentially, even though this function doesn't really require that at
all. So it may or may not be used outside this current use case.
Anway, as I've said it's only a nit, so I won't insist on a change here.
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:
> Readers who don't have any context around GIT_CONFIG_PARAMETERS and what
> it does will have a bit of a hard time making much sense of this, I
> think. It usually helps to give a sentence or two explaining what the
> infra even does, and what this quoting looks like.
Will do. It is how "git -c key=value" reaches the child processes: a
space separated list of 'key'='value' pairs, each side single quoted,
that git_config_from_parameters() reads back. I'll say that in the
message.
> Pointing to that other function makes sense, but neither of the
> functions documents the actual format that's used.
I'll put the format in the header comment as well.
Thanks,
Thomas
| strbuf_release(&head_ref); | ||
|
|
||
| if (do_update_refs(r, opts->quiet)) | ||
| return -1; |
There 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:25PM +0000, Thomas Bachem via GitGitGadget wrote:
> From: 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.
This paragraph just doesn't parse for me, it's really hard to tell what
it even wants to say.
> 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.
Besides moving stuff around to prep for the next commit, what does this
change? Like, do we now run the command in cases where we didn't before?
And if so, what are the consequences of doing so?
> diff --git a/sequencer.c b/sequencer.c
> index 65afd100d9..67e1c38762 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5313,6 +5313,12 @@ cleanup_head_ref:
> return -1;
> }
>
> + /*
> + * We ignore errors in 'git maintenance run --auto', since the
> + * user should see them.
> + */
> + run_auto_maintenance(r, opts->quiet);
> +
> /*
> * Sequence of picks finished successfully; cleanup by
> * removing the .git/sequencer directory
> @@ -5577,10 +5583,14 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
> res = -1;
> goto release_todo_list;
> }
> - } else if (!file_exists(get_todo_path(opts)))
> - return continue_single_pick(r, opts);
> - else if ((res = read_populate_todo(r, &todo_list, opts)))
> + } else if (!file_exists(get_todo_path(opts))) {
> + res = continue_single_pick(r, opts);
> + if (!res)
> + run_auto_maintenance(r, opts->quiet);
> + return res;
> + } else if ((res = read_populate_todo(r, &todo_list, opts))) {
> goto release_todo_list;
> + }
>
> if (!is_rebase_i(opts)) {
> /* Verify that the conflict has been resolved */
> @@ -5698,6 +5708,8 @@ int sequencer_pick_revisions(struct repository *r,
> BUG("unexpected extra commit from walk");
>
> res = single_pick(r, cmit, opts);
> + if (!res)
> + run_auto_maintenance(r, opts->quiet);
> goto out;
> }
>
It's surprisingly many sites where you add the call to
`run_auto_maintenance()`. My hope was that there is a single exit path
somewhere that is used by both the "apply" and "merge" strategy that we
could adapt to unify when exactly we run auto-maintenance across both
backends.
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:
> This paragraph just doesn't parse for me, it's really hard to tell what
> it even wants to say.
Sorry, I'll rewrite it. What I meant: the apply backend runs auto
maintenance from finish_rebase() when it is done. The merge backend,
cherry-pick and revert create their commits in process and never run
it themselves. It only runs when they spawn a command that runs it
anyway: "git commit" for an edited message or a resolved conflict,
"git merge" for an octopus merge or a custom strategy in "rebase -r",
or a git command in an exec. So a clean sequence never runs it, and
one with conflicts runs it after each resolution, in the middle of the
sequence.
> Besides moving stuff around to prep for the next commit, what does this
> change? Like, do we now run the command in cases where we didn't before?
> And if so, what are the consequences of doing so?
Yes. After this patch every sequence that finishes runs it at the end,
like a "git commit" or an apply backend rebase does. The next patch
stops the runs in the middle, so it ends up as one run per sequence.
The case that changes is a sequence with no conflict to resolve and no
message to edit. Such a sequence never ran it at all, so its loose
objects waited for the next commit. It is the same
"git maintenance run --auto --detach" as after a commit, so it runs in
the background and does nothing unless a threshold is met. I'll put
that in the message.
> It's surprisingly many sites where you add the call to
> `run_auto_maintenance()`. My hope was that there is a single exit path
> somewhere that is used by both the "apply" and "merge" strategy that we
> could adapt to unify when exactly we run auto-maintenance across both
> backends.
There is none inside the sequencer. run_specific_rebase() calls
finish_rebase() for the apply backend only,
"merge backend cleans up after itself" as the comment there says.
Sequences end inside pick_commits(), and a single cherry-pick or
revert never creates sequencer state at all and returns straight to
builtin/revert.c. That is where the three sites come from.
I could instead do what the apply backend already does. am.c skips
maintenance in rebasing mode and leaves it to rebase.c. If the
sequencer leaves it to its callers the same way, run_specific_rebase()
runs it for the merge backend too, once its state directory is gone,
and run_sequencer() in builtin/revert.c runs it for cherry-pick and
revert. Every entry into the merge backend returns through
run_specific_rebase(), --continue and --skip included, so nothing is
missed. The sequencer then never runs it, the change is in the two
builtins only, and the rule is short: the command runs it once when it
is done, and nothing it spawns does. Is that what you had in mind?
Thanks,
ThomasThere was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Mon, Sep 07, 2026 at 06:35:20PM +0200, Thomas Bachem wrote:
> On 07/09/2026 10:14, Patrick Steinhardt wrote:
[snip]
> > It's surprisingly many sites where you add the call to
> > `run_auto_maintenance()`. My hope was that there is a single exit path
> > somewhere that is used by both the "apply" and "merge" strategy that we
> > could adapt to unify when exactly we run auto-maintenance across both
> > backends.
>
> There is none inside the sequencer. run_specific_rebase() calls
> finish_rebase() for the apply backend only,
> "merge backend cleans up after itself" as the comment there says.
> Sequences end inside pick_commits(), and a single cherry-pick or
> revert never creates sequencer state at all and returns straight to
> builtin/revert.c. That is where the three sites come from.
>
> I could instead do what the apply backend already does. am.c skips
> maintenance in rebasing mode and leaves it to rebase.c. If the
> sequencer leaves it to its callers the same way, run_specific_rebase()
> runs it for the merge backend too, once its state directory is gone,
> and run_sequencer() in builtin/revert.c runs it for cherry-pick and
> revert. Every entry into the merge backend returns through
> run_specific_rebase(), --continue and --skip included, so nothing is
> missed. The sequencer then never runs it, the change is in the two
> builtins only, and the rule is short: the command runs it once when it
> is done, and nothing it spawns does. Is that what you had in mind?
Maybe. The question is what kind of impact it would have on other
subsystems. I think the most important part that I'm after is that the
commit message explains design decisions like this, as it gives the
reader the required context to be able to evaluate the patch.
And please stay mindful of LLM-generated commit messages. For most of
the part they are just completely useless as they tend to ramble without
conveying any useful information. The commit message is the place where
you yourself sell the change to us, and by explaining the changes well
you demonstrate that you understand what you're sending to the mailing
list.
An LLM-generated commit message on the other side demonstrates nothing
like that. So in many cases, it's actively hurting your own mission as
people do notice that it's not generated by humans.
It's fine to use LLMs to help you with drafting the commit message. But
what we're asking is that you double or even triple check what was
generated and whether the generated message (1) makes sense and (2) is
understandable by a normal human being.
Thanks!
PatrickThere was a problem hiding this comment.
Thomas Bachem wrote on the Git mailing list (how to reply to this email):
Hi Patrick,
On 08/09/2026 07:50, Patrick Steinhardt wrote:
> Maybe. The question is what kind of impact it would have on other
> subsystems. I think the most important part that I'm after is that the
> commit message explains design decisions like this, as it gives the
> reader the required context to be able to evaluate the patch.
None that I can see. Outside sequencer.c the sequencer is only entered
from builtin/rebase.c and builtin/revert.c, so those two are the only
places the call moves to. I'll put that, and why, in the commit
message.
> It's fine to use LLMs to help you with drafting the commit message. But
> what we're asking is that you double or even triple check what was
> generated and whether the generated message (1) makes sense and (2) is
> understandable by a normal human being.
Okay, I'll rewrite all three messages for v3.
Thanks,
Thomas
| @@ -234,6 +234,11 @@ struct replay_ctx { | |||
| * Whether message contains a commit message. | |||
There 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,
Thomas
| @@ -234,6 +234,11 @@ struct replay_ctx { | |||
| * Whether message contains a commit message. | |||
There 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,
Thomas
|
User |
| strbuf_release(&head_ref); | ||
|
|
||
| if (do_update_refs(r, opts->quiet)) | ||
| return -1; |
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 16:51, Thomas Bachem via GitGitGadget wrote:
> From: 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
The merge backend which is also used by "git cherry-pick" and "git
revert" does not run it when it finishes.
would be clearer to me
> 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.
Like Patrick I cannot understand what this is saying, let alone whether it is saying anything useful.
> 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.
Run "git maintenace --auto" at the end of all sequencer operations,
...
would be clearer to me
> diff --git a/sequencer.c b/sequencer.c
> index 65afd100d9..67e1c38762 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5313,6 +5313,12 @@ cleanup_head_ref:
This makes me think we should change the fragment re for c files to exclude ':', but that's a digression and would mean having different expressions for c and c++.
Anyway this change is at the end of pick_commits(), just before we finish so looks like the right place to call run_auto_maintenance()
> return -1;
> }
> > + /*
> + * We ignore errors in 'git maintenance run --auto', since the
> + * user should see them.
> + */
> + run_auto_maintenance(r, opts->quiet);
> +
> /*
> * Sequence of picks finished successfully; cleanup by
> * removing the .git/sequencer directory
> @@ -5577,10 +5583,14 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
> res = -1;
> goto release_todo_list;
> }
> - } else if (!file_exists(get_todo_path(opts)))
> - return continue_single_pick(r, opts);
> - else if ((res = read_populate_todo(r, &todo_list, opts)))
> + } else if (!file_exists(get_todo_path(opts))) {
> + res = continue_single_pick(r, opts);
It is a shame the single pick variants of "git cherry-pick" and "git revert" do not share the same code path as the multiple pick variants. continue_single_pick() runs "git commit" without calling run_git_commit() which is also unfortunate, but means that we could just rely and "git commit" to call run_auto_maintenance() for us.
> + if (!res)
> + run_auto_maintenance(r, opts->quiet);
> + return res;
> + } else if ((res = read_populate_todo(r, &todo_list, opts))) {
> goto release_todo_list;
> + }
> > if (!is_rebase_i(opts)) {
> /* Verify that the conflict has been resolved */
> @@ -5698,6 +5708,8 @@ int sequencer_pick_revisions(struct repository *r,
> BUG("unexpected extra commit from walk");
> > res = single_pick(r, cmit, opts);
> + if (!res)
> + run_auto_maintenance(r, opts->quiet);
> goto out;
> }
> > 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 &&
Do we want to assert that we don't run auto maintenance up to this point?
> + GIT_TRACE2_EVENT="$(pwd)/finish.txt" git rebase --continue &&
> + test_subcommand_flex git maintenance run --auto <finish.txt
> +'
> +
> test_done
> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> index 5777dff496..304981ccd6 100755
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -721,4 +721,14 @@ test_expect_success 'commit descriptions in insn sheet are optional' '
> test_line_count = 4 commits
> '
> > +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 &&
> + test_subcommand_flex git maintenance run --auto <single.txt &&
> + GIT_TRACE2_EVENT="$(pwd)/sequence.txt" \
> + git cherry-pick anotherpick yetanotherpick &&
> + grep "\"child_start\".*\"maintenance\"" sequence.txt >maintenance &&
Using test_grep here would mean we get some useful test output if there are not matches in the file. Without that test_line_count just says the line count didn't match and prints an empty file.
Thanks
Phillip
> + 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 07/09/2026 15:25, Phillip Wood wrote:
> The merge backend which is also used by "git cherry-pick" and "git
> revert" does not run it when it finishes.
>
> would be clearer to me
Yes, I'll use that.
> Like Patrick I cannot understand what this is saying, let alone whether
> it is saying anything useful.
I've spelled it out in my reply to Patrick and will rewrite the
message that way.
> Run "git maintenace --auto" at the end of all sequencer operations,
> ...
>
> would be clearer to me
That too.
> Anyway this change is at the end of pick_commits(), just before we
> finish so looks like the right place to call run_auto_maintenance()
Patrick would rather have one exit shared by both rebase backends. In
my reply to him I've proposed moving the call out of the sequencer
into builtin/rebase.c and builtin/revert.c, the way am.c leaves it to
rebase.c today. Say if you'd rather keep it here.
> It is a shame the single pick variants of "git cherry-pick" and "git
> revert" do not share the same code path as the multiple pick variants.
> continue_single_pick() runs "git commit" without calling
> run_git_commit() which is also unfortunate, but means that we could just
> rely and "git commit" to call run_auto_maintenance() for us.
That works until the next patch, which turns auto maintenance off in
every command the sequencer spawns, this "git commit" included. So
somebody has to run it afterwards. With the call in builtin/revert.c,
cherry-pick does that itself once the continue returns.
> Do we want to assert that we don't run auto maintenance up to this point?
Yes, I'll add that, and the next patch will extend this test instead
of adding its own.
> Using test_grep here would mean we get some useful test output if there
> are not matches in the file. Without that test_line_count just says the
> line count didn't match and prints an empty file.
Will do.
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 07/09/2026 17:36, Thomas Bachem wrote:
> >> Anyway this change is at the end of pick_commits(), just before we
>> finish so looks like the right place to call run_auto_maintenance()
> > Patrick would rather have one exit shared by both rebase backends. In
> my reply to him I've proposed moving the call out of the sequencer
> into builtin/rebase.c and builtin/revert.c, the way am.c leaves it to
> rebase.c today. Say if you'd rather keep it here.
That works for we and means we don't have to sprinkle calls to run_auto_maintenance() around to accommodate the different code paths for single and multiple picks.
>> It is a shame the single pick variants of "git cherry-pick" and "git
>> revert" do not share the same code path as the multiple pick variants.
>> continue_single_pick() runs "git commit" without calling
>> run_git_commit() which is also unfortunate, but means that we could just
>> rely and "git commit" to call run_auto_maintenance() for us.
> > That works until the next patch, which turns auto maintenance off in
> every command the sequencer spawns, this "git commit" included. So
> somebody has to run it afterwards. With the call in builtin/revert.c,
> cherry-pick does that itself once the continue returns.
Sounds good
Thanks
Phillip
> >> Do we want to assert that we don't run auto maintenance up to this point?
> > Yes, I'll add that, and the next patch will extend this test instead
> of adding its own.
> >> Using test_grep here would mean we get some useful test output if there
>> are not matches in the file. Without that test_line_count just says the
>> line count didn't match and prints an empty file.
> > Will do.
> > Thanks,
> Thomas
|
There was a status update in the "Cooking" section about the branch The sequencer machinery (used by 'git rebase', 'git cherry-pick', and 'git revert') has been updated to defer automatic maintenance tasks until the end of the operation, preventing nested 'git commit', 'git merge', and 'exec' commands from triggering GC operations that could contend for locks or delete open packs while the sequence is in progress. Expecting a reroll. cf. <CAA0xjtrherFZZBSeqE8zd6Kbwy_=f9mwiqJK832DzZRakkiwBw@mail.gmail.com> source: <pull.2217.v2.git.1788537086.gitgitgadget@gmail.com> |
thomasbachem
force-pushed
the
rebase-auto-maintenance
branch
from
September 8, 2026 10:24
9a6fc04 to
7a353df
Compare
Author
|
/preview |
|
Preview email sent as pull.2217.v3.git.1788863148.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.2217.v3.git.1788863286.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Thomas Bachem via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Changes since v2:
>
> * Auto maintenance now runs from builtin/rebase.c and builtin/revert.c,
> once the command is done, instead of from three places inside the
> sequencer. That follows the apply backend, where "git am" leaves it to
> rebase.c (Patrick, Phillip).
> * gc.auto=0 dropped, maintenance.auto=false is enough (Patrick).
> * config_parameters is a char * built once with strbuf_detach() (Junio).
> * Patch 3 extends the tests of patch 2 instead of adding its own. They also
> assert that nothing runs before a stop, and cover a single pick with
> --edit and a sequence ending in --skip (Phillip).
> * Commit messages rewritten: what GIT_CONFIG_PARAMETERS is and looks like
> (Patrick), what patch 2 changes for the user and why the call moved
> (Patrick, Phillip), and the comment on the helper (Phillip).
>
> Based on master. Independent of the rerere lock fix in [1].
Will queue.
By the way, is it just me or are proposed log messages getting
longer and longer with more irrelevant detaild these days?
If it is not just my imagination, this trend must stop.
Throw "Say the same thing in 1/N of the words used." followed by
these proposed log messages to your near-by LLM, see if the output
still makes sense, and adjust the value of N.
For example, I got the following from [3/3] with N==3 and I think
that is far easier to understand than the original.
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).
Pass maintenance.auto=false via GIT_CONFIG_PARAMETERS to all
spawned commands. Building this value after user settings
ensures it wins and propagates to child processes.
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.
Thanks. |
|
This patch series is no longer integrated into seen. |
|
"Kristoffer Haugsbakk" wrote on the Git mailing list (how to reply to this email): On Tue, Sep 8, 2026, at 17:53, Junio C Hamano wrote:
> "Thomas Bachem via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> Changes since v2:
>>
>> * Auto maintenance now runs from builtin/rebase.c and builtin/revert.c,
>> once the command is done, instead of from three places inside the
>> sequencer. That follows the apply backend, where "git am" leaves it to
>> rebase.c (Patrick, Phillip).
>> * gc.auto=0 dropped, maintenance.auto=false is enough (Patrick).
>> * config_parameters is a char * built once with strbuf_detach() (Junio).
>> * Patch 3 extends the tests of patch 2 instead of adding its own. They also
>> assert that nothing runs before a stop, and cover a single pick with
>> --edit and a sequence ending in --skip (Phillip).
>> * Commit messages rewritten: what GIT_CONFIG_PARAMETERS is and looks like
>> (Patrick), what patch 2 changes for the user and why the call moved
>> (Patrick, Phillip), and the comment on the helper (Phillip).
>>
>> Based on master. Independent of the rerere lock fix in [1].
>
> Will queue.
>
> By the way, is it just me or are proposed log messages getting
> longer and longer with more irrelevant detaild these days?
(sent from mobile)
Here is my impression. Six months ago, an on the
surface thorough commit message would
- describe the problem
- describe the solution conceptually
- maybe the concrete code solution...
- maybe the alternatives not followed
through on...
(and this is still the case overall)
That can lead to verbose commit messages.
But note. There were also many things *not*
included.
- What tests have been added, what they test
- That the docs have been updated (accordingly)
and howso
- Maybe even benchmarks when they are not
relevant?
And only in the last six months have I seen
commit messages that look like they are
explicitly filling in an imagined form with
a dozen or so reminder/mandatory points.
(Note in the cmt msg, not after it)
The most noticable ones to me are the ones
with the implied checkmarks in the last paragraph.
Those old school commit messages took
all the conventions and business as usual
(like adding a regression test alongside
the bugfix) and didn't mention “and we are
still following the project guidelines by…”
Of course, for those commit messages which look
more like code-to-English descriptions, I can't even
keep up and judge whether they are correct
and coherent.
>[snip] |
|
User |
|
This patch series was integrated into seen via git@7c39547. |
"git -c" passes its settings to the commands it spawns through GIT_CONFIG_PARAMETERS, a list of quoted 'key'='value' pairs. The only place that formats such an entry is git_config_push_split_parameter(), which writes straight into our own environment. Split the formatting out into git_config_append_parameter(), which appends one entry to a strbuf, so that a caller can build the value for a child's environment. The sequencer will use it in a later commit. Assisted-by: Claude Fable 5.1 Signed-off-by: Thomas Bachem <mail@thomasbachem.com>
"git cherry-pick", "git revert" and the merge backend of "git rebase" create their commits in process, so auto maintenance runs only when they spawn a command that runs it, like the "git commit" for a resolved conflict. A sequence thus runs it in the middle, after each resolution, or never. Run it once when the sequence is done, like the apply backend does. The sequencer has no single place where every sequence ends: a sequence of several commits ends in pick_commits(), a single pick returns as soon as its commit is made, and "--continue" and "--skip" have entry points of their own. Run it from the two builtins that start or continue a sequence instead: run_specific_rebase() once the sequencer has returned and removed its state directory, and run_sequencer() after a successful pick, "--continue" or "--skip". Assisted-by: Claude Fable 5.1 Signed-off-by: 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). 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. Assisted-by: Claude Fable 5.1 Signed-off-by: Thomas Bachem <mail@thomasbachem.com>
thomasbachem
force-pushed
the
rebase-auto-maintenance
branch
from
September 9, 2026 08:21
7a353df to
031b3bd
Compare
Author
|
/preview |
|
Preview email sent as pull.2217.v4.git.1788942170.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.2217.v4.git.1788942331.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Tue, Sep 08, 2026 at 08:53:05AM -0700, Junio C Hamano wrote:
> "Thomas Bachem via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > Changes since v2:
> >
> > * Auto maintenance now runs from builtin/rebase.c and builtin/revert.c,
> > once the command is done, instead of from three places inside the
> > sequencer. That follows the apply backend, where "git am" leaves it to
> > rebase.c (Patrick, Phillip).
> > * gc.auto=0 dropped, maintenance.auto=false is enough (Patrick).
> > * config_parameters is a char * built once with strbuf_detach() (Junio).
> > * Patch 3 extends the tests of patch 2 instead of adding its own. They also
> > assert that nothing runs before a stop, and cover a single pick with
> > --edit and a sequence ending in --skip (Phillip).
> > * Commit messages rewritten: what GIT_CONFIG_PARAMETERS is and looks like
> > (Patrick), what patch 2 changes for the user and why the call moved
> > (Patrick, Phillip), and the comment on the helper (Phillip).
> >
> > Based on master. Independent of the rerere lock fix in [1].
>
> Will queue.
>
> By the way, is it just me or are proposed log messages getting
> longer and longer with more irrelevant detaild these days?
>
> If it is not just my imagination, this trend must stop.
It's not your imagination, it's gotten quite a bit worse over the last
couple weeks. I also started to push back on this trend, see [1] for
example.
Thanks!
Patrick
[1]: <ap-iEoeY7XKjeZgL@pks.im> |
| @@ -234,6 +234,11 @@ struct replay_ctx { | |||
| * Whether message contains a commit message. | |||
There 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
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 v3:
Junio's wording, except that the setting goes to the commit, merge
and exec commands the sequencer spawns, not to all of them.
No code change.
Based on master. Independent of the rerere lock fix in [1].
[1] <pull.2214.v3.git.1788537081930.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
cc: Phillip Wood phillip.wood123@gmail.com
cc: "Kristoffer Haugsbakk" kristofferhaugsbakk@fastmail.com