sequencer: release the ODB before spawning git commit
PATCH 1/1 sequencer: release the ODB before spawning git commit
From: Johannes Schindelin <[email protected]>
As of 4557f1add261 (rebase--helper: add a builtin helper for interactive rebases, 2017-02-09), continuing an interactive rebase uses the builtin sequencer, which spawns git commit.
The child may trigger auto-maintenance, which may need to replace files for which the sequencer still holds resources. See
https://github.com/git-for-windows/git/issues/6315: on Windows, this produces unlink retry prompts that cannot succeed while the sequencer waits for the child.
Resources such as file handles or memory mappings must be released before spawning a command that may run auto-maintenance, as established by 28d04e1ec197 (run-command: offer to close the object store before running, 2021-09-09).
Release the sequencer's ODB before spawning git commit. The regression test uses the legacy-delete trick introduced by 69ed0e35a754 (mingw:
optionally use legacy (non-POSIX) delete semantics, 2026-05-07) to trigger the failure on modern Windows.
---
sequencer: release the ODB before spawning git commit
This fixes https://github.com/git-for-windows/git/issues/6315
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2198%2Fgit-for-windows%2Frebase-release-odb-before-commit-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2198/git-for-windows/rebase-release-odb-before-commit-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2198
sequencer.c | 1 + t/t3404-rebase-interactive.sh | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+)
sequencer.c +1 -0
@@ -1127,6 +1127,7 @@ static int run_git_commit(const char *defmsg,struct child_process cmd =CHILD_PROCESS_INIT ; cmd.git_cmd =1 ; + cmd.odb_to_close = the_repository->objects; if (is_rebase_i(opts) && ((opts->committer_date_is_author_date && !opts->ignore_date) ||
t/t3404-rebase-interactive.sh +18 -0
@@ -65,6 +65,24 @@ test_expect_success 'setup' ' test_commit P fileP ' +test_expect_success MINGW 'rebase releases object database before committing' ' + test_when_finished "rm -f .git/hooks/post-commit repacked packs" && + git switch -C repack-rewrite primary && + git repack -ad && + write_script .git/hooks/post-commit <<-\EOF && + git repack -ad && + >repacked + EOF + ( + set_fake_editor && + FAKE_LINES="reword 1" GIT_TEST_LEGACY_DELETE=1 \ + git -c core.commitGraph=false rebase -i HEAD^ + ) && + test_path_is_file repacked && + ls .git/objects/pack/*.pack >packs && + test_line_count = 1 packs +' + # "exec" commands are run with the user shell by default, but this may # be non-POSIX. For example, if SHELL=zsh then ">file" doesn't work # to create a file. Unsetting SHELL avoids such non-portable behavior
base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
Signature · 1 line
gitgitgadget
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 12 lines · writes:
From: Johannes Schindelin <[email protected]>
As of
4557f1add261(rebase--helper: add a builtin helper for interactive rebases, 2017-02-09), continuing an interactive rebase uses the builtin sequencer, which spawnsgit commit.The child may trigger auto-maintenance, which may need to replace files for which the sequencer still holds resources. See
https://github.com/git-for-windows/git/issues/6315: on Windows, this produces unlink retry prompts that cannot succeed while the sequencer waits for the child.
Quoted 4 lines
Resources such as file handles or memory mappings must be released before spawning a command that may run auto-maintenance, as established by
28d04e1ec197(run-command: offer to close the object store before running, 2021-09-09).
The sequencer is holding some resources, presumably because it needs them to continue, and yet auto-maintenance wants to remove them? Whether or not we face a Windows-specific limitation when removing them, it is concerning to imagine what would happen if auto-maintenance were allowed to do so and the sequencer then resumed its work, only to find that the resources needed for its operation were gone.
I think what the proposed commit log message lacks after 'must be released' is 'and after auto-maintenance finishes and we regain control, we will automatically reacquire these resources in a refreshed state to proceed'. Such an explanation would allay the unease I expressed in the previous paragraph.
Perhaps issues/6315 talks about what exactly are held, but not spelling it out in the log message is not helping readers.
Quoted 1 line
Release the sequencer's ODB before spawning
git commit.
Makes sense.
Quoted 48 lines
The regression
test uses the legacy-delete trick introduced by69ed0e35a754(mingw:
optionally use legacy (non-POSIX) delete semantics, 2026-05-07) to trigger the failure on modern Windows.sequencer.c +1
-0@@ -1127,6 +1127,7 @@ static int run_git_commit(const char *defmsg,struct child_process cmd =CHILD_PROCESS_INIT ; cmd.git_cmd =1 ; + cmd.odb_to_close = the_repository->objects; if (is_rebase_i(opts) && ((opts->committer_date_is_author_date && !opts->ignore_date) ||t/t3404-rebase-interactive.sh +18
-0@@ -65,6 +65,24 @@ test_expect_success 'setup' ' test_commit P fileP ' +test_expect_success MINGW 'rebase releases object database before committing' ' + test_when_finished "rm -f .git/hooks/post-commit repacked packs" && + git switch -C repack-rewrite primary && + git repack -ad && + write_script .git/hooks/post-commit <<-\EOF && + git repack -ad && + >repacked + EOF + ( + set_fake_editor && + FAKE_LINES="reword 1" GIT_TEST_LEGACY_DELETE=1 \ + git -c core.commitGraph=false rebase -i HEAD^ + ) && + test_path_is_file repacked && + ls .git/objects/pack/*.pack >packs && + test_line_count = 1 packs +' + # "exec" commands are run with the user shell by default, but this may # be non-POSIX. For example, if SHELL=zsh then ">file" doesn't work # to create a file. Unsetting SHELL avoids such non-portable behaviorbase-commit:
e9019fcafe0040228b8631c30f97ae1adb61bcdc
PATCH v2 1/1 sequencer: release the ODB before spawning git commit
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
As of 4557f1add261 (rebase--helper: add a builtin helper for interactive rebases, 2017-02-09), continuing an interactive rebase uses the builtin sequencer, which spawns git commit.
The child may trigger auto-maintenance, which may need to replace files for which the sequencer still holds resources. See
https://github.com/git-for-windows/git/issues/6315: on Windows, this produces unlink retry prompts that cannot succeed while the sequencer waits for the child.
Resources such as file handles or memory mappings must be released before spawning a command that may run auto-maintenance, as established by 28d04e1ec197 (run-command: offer to close the object store before running, 2021-09-09): release the ODB file handles and memory mappings, so that auto-gc can repack (potentially deleting existing packfiles in the process); If the sequencer needs to access the ODB afterwards, it will gracefully (re-)open the ODB.
Release the sequencer's ODB before spawning git commit. The regression test uses the legacy-delete trick introduced by 69ed0e35a754 (mingw:
optionally use legacy (non-POSIX) delete semantics, 2026-05-07) to trigger the failure on modern Windows.
---
sequencer: release the ODB before spawning git commit
This fixes https://github.com/git-for-windows/git/issues/6315
Changes since v1:
* Clarify in the commit message what the strategy introduced in 28d04e1ec197 (run-command: offer to close the object store before running, 2021-09-09) is all about.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2198%2Fgit-for-windows%2Frebase-release-odb-before-commit-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2198/git-for-windows/rebase-release-odb-before-commit-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2198
Range-diff vs v1:
1: 904d65e8cb ! 1: 039fd29039 sequencer: release the ODB before spawning git commit
@@ Commit message
Resources such as file handles or memory mappings must be released before spawning a command that may run auto-maintenance, as established by 28d04e1ec197 (run-command: offer to close the object store before
- running, 2021-09-09).
+ running, 2021-09-09): release the ODB file handles and memory mappings,
+ so that auto-gc can repack (potentially deleting existing packfiles in
+ the process); If the sequencer needs to access the ODB afterwards, it
+ will gracefully (re-)open the ODB.
Release the sequencer's ODB before spawning git commit. The regression test uses the legacy-delete trick introduced by 69ed0e35a754 (mingw:
sequencer.c | 1 + t/t3404-rebase-interactive.sh | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+)
sequencer.c +1 -0
@@ -1127,6 +1127,7 @@ static int run_git_commit(const char *defmsg,struct child_process cmd =CHILD_PROCESS_INIT ; cmd.git_cmd =1 ; + cmd.odb_to_close = the_repository->objects; if (is_rebase_i(opts) && ((opts->committer_date_is_author_date && !opts->ignore_date) ||
t/t3404-rebase-interactive.sh +18 -0
@@ -65,6 +65,24 @@ test_expect_success 'setup' ' test_commit P fileP ' +test_expect_success MINGW 'rebase releases object database before committing' ' + test_when_finished "rm -f .git/hooks/post-commit repacked packs" && + git switch -C repack-rewrite primary && + git repack -ad && + write_script .git/hooks/post-commit <<-\EOF && + git repack -ad && + >repacked + EOF + ( + set_fake_editor && + FAKE_LINES="reword 1" GIT_TEST_LEGACY_DELETE=1 \ + git -c core.commitGraph=false rebase -i HEAD^ + ) && + test_path_is_file repacked && + ls .git/objects/pack/*.pack >packs && + test_line_count = 1 packs +' + # "exec" commands are run with the user shell by default, but this may # be non-POSIX. For example, if SHELL=zsh then ">file" doesn't work # to create a file. Unsetting SHELL avoids such non-portable behavior
base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
Signature · 1 line
gitgitgadget
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 25 lines · writes:
From: Johannes Schindelin <[email protected]>
As of
4557f1add261(rebase--helper: add a builtin helper for interactive rebases, 2017-02-09), continuing an interactive rebase uses the builtin sequencer, which spawnsgit commit.The child may trigger auto-maintenance, which may need to replace files for which the sequencer still holds resources. See
https://github.com/git-for-windows/git/issues/6315: on Windows, this produces unlink retry prompts that cannot succeed while the sequencer waits for the child.Resources such as file handles or memory mappings must be released before spawning a command that may run auto-maintenance, as established by
28d04e1ec197(run-command: offer to close the object store before running, 2021-09-09): release the ODB file handles and memory mappings, so that auto-gc can repack (potentially deleting existing packfiles in the process); If the sequencer needs to access the ODB afterwards, it will gracefully (re-)open the ODB.Release the sequencer's ODB before spawning
git commit. The regression test uses the legacy-delete trick introduced by69ed0e35a754(mingw:
optionally use legacy (non-POSIX) delete semantics, 2026-05-07) to trigger the failure on modern Windows.
Quoted 6 lines
Assisted-by GPT-5.6 SolSigned-off-by Johannes Schindelin <[email protected]>---
sequencer: release the ODB before spawning git commitThis fixes https://github.com/git-for-windows/git/issues/6315
Thanks. Let me mark the topic for 'next'.
Quoted 43 lines
sequencer.c +1
-0@@ -1127,6 +1127,7 @@ static int run_git_commit(const char *defmsg,struct child_process cmd =CHILD_PROCESS_INIT ; cmd.git_cmd =1 ; + cmd.odb_to_close = the_repository->objects; if (is_rebase_i(opts) && ((opts->committer_date_is_author_date && !opts->ignore_date) ||t/t3404-rebase-interactive.sh +18
-0@@ -65,6 +65,24 @@ test_expect_success 'setup' ' test_commit P fileP ' +test_expect_success MINGW 'rebase releases object database before committing' ' + test_when_finished "rm -f .git/hooks/post-commit repacked packs" && + git switch -C repack-rewrite primary && + git repack -ad && + write_script .git/hooks/post-commit <<-\EOF && + git repack -ad && + >repacked + EOF + ( + set_fake_editor && + FAKE_LINES="reword 1" GIT_TEST_LEGACY_DELETE=1 \ + git -c core.commitGraph=false rebase -i HEAD^ + ) && + test_path_is_file repacked && + ls .git/objects/pack/*.pack >packs && + test_line_count = 1 packs +' + # "exec" commands are run with the user shell by default, but this may # be non-POSIX. For example, if SHELL=zsh then ">file" doesn't work # to create a file. Unsetting SHELL avoids such non-portable behaviorbase-commit:
e9019fcafe0040228b8631c30f97ae1adb61bcdc
Hi Johannes
Quoted 20 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
As of
4557f1add261(rebase--helper: add a builtin helper for interactive rebases, 2017-02-09), continuing an interactive rebase uses the builtin sequencer, which spawnsgit commit.The child may trigger auto-maintenance, which may need to replace files for which the sequencer still holds resources. See
https://github.com/git-for-windows/git/issues/6315: on Windows, this produces unlink retry prompts that cannot succeed while the sequencer waits for the child.Resources such as file handles or memory mappings must be released before spawning a command that may run auto-maintenance, as established by
28d04e1ec197(run-command: offer to close the object store before running, 2021-09-09): release the ODB file handles and memory mappings, so that auto-gc can repack (potentially deleting existing packfiles in the process); If the sequencer needs to access the ODB afterwards, it will gracefully (re-)open the ODB.
Quoted 4 lines
Release the sequencer's ODB before spawning
git commit. The regression test uses the legacy-delete trick introduced by69ed0e35a754(mingw:
optionally use legacy (non-POSIX) delete semantics, 2026-05-07) to trigger the failure on modern Windows.
This looks fine as an immediate fix for the bug but I wonder if we should pass "-c gc.auto=false" when we fork "git commit" from the sequencer. We call run_auto_maintenance() at the end of the rebase and its not clear to me that repacking during the rebase is helpful. Another thought I had was whether we should automatically close the object database when forking another git command. I'm not sure how easy that is to implement but it would prevent future regressions and I assuming re-opening the object store is cheap compared to forking another git command.
Thanks
Phillip
Quoted 81 lines
Assisted-by GPT-5.6 SolSigned-off-by Johannes Schindelin <[email protected]>---
sequencer: release the ODB before spawning git commitThis fixes https://github.com/git-for-windows/git/issues/6315
Changes since v1:
* Clarify in the commit message what the strategy introduced in
28d04e1ec197(run-command: offer to close the object store before running, 2021-09-09) is all about.Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2198%2Fgit-for-windows%2Frebase-release-odb-before-commit-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2198/git-for-windows/rebase-release-odb-before-commit-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2198Range-diff vs v1:
1:
904d65e8cb! 1:039fd29039sequencer: release the ODB before spawning git commit
@@ Commit message
Resources such as file handles or memory mappings must be released before spawning a command that may run auto-maintenance, as established by28d04e1ec197(run-command: offer to close the object store before
- running, 2021-09-09).
+ running, 2021-09-09): release the ODB file handles and memory mappings,
+ so that auto-gc can repack (potentially deleting existing packfiles in
+ the process); If the sequencer needs to access the ODB afterwards, it
+ will gracefully (re-)open the ODB.Release the sequencer's ODB before spawning
git commit. The regression test uses the legacy-delete trick introduced by69ed0e35a754(mingw:sequencer.c | 1 + t/t3404-rebase-interactive.sh | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+)sequencer.c +1
-0@@ -1127,6 +1127,7 @@ static int run_git_commit(const char *defmsg,struct child_process cmd =CHILD_PROCESS_INIT ; cmd.git_cmd =1 ; + cmd.odb_to_close = the_repository->objects; if (is_rebase_i(opts) && ((opts->committer_date_is_author_date && !opts->ignore_date) ||t/t3404-rebase-interactive.sh +18
-0@@ -65,6 +65,24 @@ test_expect_success 'setup' ' test_commit P fileP ' +test_expect_success MINGW 'rebase releases object database before committing' ' + test_when_finished "rm -f .git/hooks/post-commit repacked packs" && + git switch -C repack-rewrite primary && + git repack -ad && + write_script .git/hooks/post-commit <<-\EOF && + git repack -ad && + >repacked + EOF + ( + set_fake_editor && + FAKE_LINES="reword 1" GIT_TEST_LEGACY_DELETE=1 \ + git -c core.commitGraph=false rebase -i HEAD^ + ) && + test_path_is_file repacked && + ls .git/objects/pack/*.pack >packs && + test_line_count = 1 packs +' + # "exec" commands are run with the user shell by default, but this may # be non-POSIX. For example, if SHELL=zsh then ">file" doesn't work # to create a file. Unsetting SHELL avoids such non-portable behaviorbase-commit:
e9019fcafe0040228b8631c30f97ae1adb61bcdc
Quoted 4 lines · Phillip Wood
This looks fine as an immediate fix for the bug but I wonder if we should pass "-c gc.auto=false" when we fork "git commit" from the sequencer. We call
run_auto_maintenance()at the end of the rebase and its not clear to me that repacking during the rebase is helpful.
This is a bit amusing as I was reading old discussion the other day around the constant 6700 [*] and saw it discused that because "commit" triggers auto-gc, there was no point in doing so in "rebase", which does series of "commit" invocations [*]. With small projects and short rebases I tend to agree with you that gc at the end of a rebase session should be plenty, but given the widespread use of Git, itt may not apply to everybody.
Quoted 6 lines
Another
thought I had was whether we should automatically close the object database when forking another git command. I'm not sure how easy that is to implement but it would prevent future regressions and I assuming re-opening the object store is cheap compared to forking another git command.
I think it is a great approach to study how feasible it is, as we will not have to sprinkle fixes like the one proposed to many code paths. Thanks for raising it.
[References]
* https://lore.kernel.org/git/?q=6700+d:..20071231
* https://lore.kernel.org/git/[email protected]/
Quoted 7 lines · Junio C Hamano
Quoted 4 lines · Phillip Wood
This looks fine as an immediate fix for the bug but I wonder if we should pass "-c gc.auto=false" when we fork "git commit" from the sequencer. We call
run_auto_maintenance()at the end of the rebase and its not clear to me that repacking during the rebase is helpful.
Quoted 7 lines
This is a bit amusing as I was reading old discussion the other day around the constant 6700 [*] and saw it discused that because "commit" triggers auto-gc, there was no point in doing so in "rebase", which does series of "commit" invocations [*]. With small projects and short rebases I tend to agree with you that gc at the end of a rebase session should be plenty, but given the widespread use of Git, itt may not apply to everybody.
That's a coincidence. These days we avoid running "git commit" for simple picks, but we do run it when rebasing a merge and I suspect large rebases probably include merges. So we will probably do trigger gc during large rebases even if there are no conflicts or commits being reworded (which I think are the only two other cases where we fork "git commit").
Thanks
Phillip
Quoted 16 lines
Quoted 6 lines
Another
thought I had was whether we should automatically close the object database when forking another git command. I'm not sure how easy that is to implement but it would prevent future regressions and I assuming re-opening the object store is cheap compared to forking another git command.I think it is a great approach to study how feasible it is, as we will not have to sprinkle fixes like the one proposed to many code paths. Thanks for raising it.
[References]
* https://lore.kernel.org/git/?q=6700+d:..20071231
* https://lore.kernel.org/git/[email protected]/