# [PATCH 0/2] object-file: fix packfile flush during transaction commit 18 messages from 2026-09-13 to 2026-09-24. Participants: Justin Tobler, Karthik Nayak, Patrick Steinhardt. Thread: https://gitlist.dev/t/66322 ## Justin Tobler, 2026-09-13 20:26 Subject: [PATCH 0/2] object-file: fix packfile flush during transaction commit Message-ID: ``` Greetings, This short series fixes a bug I found related to committing an ODB transaction that contains both a loose object and "large" blob when also configured to batch fsync loose objects. The issue can be reproduced with the following: git init git config core.fsync loose-object git config core.fsyncMethod batch git config core.bigFileThreshold 5 echo foo >1-foo && echo foobar >2-foobar git add 1-foo 2-foobar and produces the following error: error: unable to write file .git/objects/pack/pack-2b7c2470289822070687e8d64186093a710eaed3.pack: No such file or directory fatal: unable to rename temporary file to '.git/objects/pack/pack-2b7c2470289822070687e8d64186093a710eaed3.pack' If a "large" blob packfile is written to the transaction temporary directory, it is unable to be flushed during transaction commit because the underlying transaction is migrated to the main ODB before the packfile is finalized. To avoid this, this series ensures any pending packfile in the transaction is flushed first. Thanks, -Justin Justin Tobler (2): object-file: lift ODB reprepare out of packfile flush object-file: flush transaction packfile before migrating objects object-file.c | 12 ++++++++---- t/t1050-large.sh | 16 ++++++++++++++++ 2 files changed, 24 insertions(+), 4 deletions(-) base-commit: 47ce80527c56f462cb97db4ca8125342204d3783 -- 2.55.0 ``` ## Justin Tobler, 2026-09-13 20:26 Subject: [PATCH 1/2] object-file: lift ODB reprepare out of packfile flush Message-ID: In-Reply-To: ``` When flushing a packfile via `flush_packfile_transaction()`, `odb_reprepare()` is invoked so the written packfile becomes visible in the current process. In a subsequent commit, repreparing the ODB is slightly deferred when committing a "files" ODB transaction. Lift ODB reprepare out of `flush_packfile_transaction()` and instead require callers to explicitly invoke `odb_reprepare()` if required. Signed-off-by: Justin Tobler --- object-file.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/object-file.c b/object-file.c index a4cbf8b081df..0f123b79fad1 100644 --- a/object-file.c +++ b/object-file.c @@ -857,8 +857,6 @@ static void flush_packfile_transaction(struct odb_transaction_files *transaction memset(state, 0, sizeof(*state)); strbuf_release(&packname); - /* Make objects we just wrote available to ourselves */ - odb_reprepare(repo->objects); } /* @@ -909,8 +907,10 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas * to zlib compression and is sufficient for this check. */ if (state->nr_written && pack_size_limit_cfg && - pack_size_limit_cfg < state->offset + stream->size) + pack_size_limit_cfg < state->offset + stream->size) { flush_packfile_transaction(transaction); + odb_reprepare(transaction->base.source->odb); + } CALLOC_ARRAY(idx, 1); prepare_packfile_transaction(transaction); @@ -1260,6 +1260,7 @@ static int odb_transaction_files_commit(struct odb_transaction *base) { struct odb_transaction_files *transaction = container_of(base, struct odb_transaction_files, base); + int have_packfile = !!transaction->packfile.f; if (transaction->objdir) { struct strbuf temp_path = STRBUF_INIT; @@ -1293,6 +1294,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base) flush_packfile_transaction(transaction); + if (have_packfile) + odb_reprepare(transaction->base.source->odb); + return 0; } -- 2.55.0 ``` ## Justin Tobler, 2026-09-13 20:26 Subject: [PATCH 2/2] object-file: flush transaction packfile before migrating objects Message-ID: <18a1798d958d7f089614ec588346096c10b0666a.1789328612.git.jltobler@gmail.com> In-Reply-To: ``` A "files" ODB transaction creates a temporary directory to stage newly written objects in when configured to batch fsync loose objects. Once the temporary directory is created, it is configured as the primary ODB and all object are written to it accordingly. This also includes packfiles containing blobs that exceed `core.bigFileThreshold` written via `odb_transaction_files_write_object_stream()`. If a "large" blob packfile is written to the ODB transaction temporary directory after other loose objects, the ODB transaction fails to commit as a result of the temporary directory being migrated prior to the packfile being flushed. Fix this bug by always flushing the packfile transaction before objects are migrated to the main ODB. Signed-off-by: Justin Tobler --- object-file.c | 4 ++-- t/t1050-large.sh | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/object-file.c b/object-file.c index 0f123b79fad1..210984f82532 100644 --- a/object-file.c +++ b/object-file.c @@ -1262,6 +1262,8 @@ static int odb_transaction_files_commit(struct odb_transaction *base) container_of(base, struct odb_transaction_files, base); int have_packfile = !!transaction->packfile.f; + flush_packfile_transaction(transaction); + if (transaction->objdir) { struct strbuf temp_path = STRBUF_INIT; struct tempfile *temp; @@ -1292,8 +1294,6 @@ static int odb_transaction_files_commit(struct odb_transaction *base) transaction->objdir = NULL; } - flush_packfile_transaction(transaction); - if (have_packfile) odb_reprepare(transaction->base.source->odb); diff --git a/t/t1050-large.sh b/t/t1050-large.sh index d295c265c75c..fb83c8fba619 100755 --- a/t/t1050-large.sh +++ b/t/t1050-large.sh @@ -87,6 +87,22 @@ test_expect_success 'add a large file or two' ' test $count = 1 ' +test_expect_success 'add large file with loose object in batch fsync' ' + test_when_finished "rm -rf batch" && + git init batch && + + git -C batch config core.bigFileThreshold 5 && + echo foo >batch/1-small && + echo foobar >batch/2-large && + + git -C batch -c core.fsync=loose-object -c core.fsyncMethod=batch \ + add 1-small 2-large && + + # Neither object may be left behind in a temporary location. + git -C batch cat-file -e :1-small && + git -C batch cat-file -e :2-large +' + test_expect_success 'checkout a large file' ' large1=$(git rev-parse :large1) && git update-index --add --cacheinfo 100644 $large1 another && -- 2.55.0 ``` ## Karthik Nayak, 2026-09-15 04:40 Subject: Re: [PATCH 1/2] object-file: lift ODB reprepare out of packfile flush Message-ID: In-Reply-To: ``` Justin Tobler writes: > When flushing a packfile via `flush_packfile_transaction()`, > `odb_reprepare()` is invoked so the written packfile becomes visible in > the current process. In a subsequent commit, repreparing the ODB is > slightly deferred when committing a "files" ODB transaction. > > Lift ODB reprepare out of `flush_packfile_transaction()` and instead > require callers to explicitly invoke `odb_reprepare()` if required. > > Signed-off-by: Justin Tobler > --- > object-file.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/object-file.c b/object-file.c > index a4cbf8b081df..0f123b79fad1 100644 > --- a/object-file.c > +++ b/object-file.c > @@ -857,8 +857,6 @@ static void flush_packfile_transaction(struct odb_transaction_files *transaction > memset(state, 0, sizeof(*state)); > > strbuf_release(&packname); > - /* Make objects we just wrote available to ourselves */ > - odb_reprepare(repo->objects); > } > > /* > @@ -909,8 +907,10 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas > * to zlib compression and is sufficient for this check. > */ > if (state->nr_written && pack_size_limit_cfg && > - pack_size_limit_cfg < state->offset + stream->size) > + pack_size_limit_cfg < state->offset + stream->size) { > flush_packfile_transaction(transaction); > + odb_reprepare(transaction->base.source->odb); > + } > > CALLOC_ARRAY(idx, 1); > prepare_packfile_transaction(transaction); > @@ -1260,6 +1260,7 @@ static int odb_transaction_files_commit(struct odb_transaction *base) > { > struct odb_transaction_files *transaction = > container_of(base, struct odb_transaction_files, base); > + int have_packfile = !!transaction->packfile.f; > > if (transaction->objdir) { > struct strbuf temp_path = STRBUF_INIT; > @@ -1293,6 +1294,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base) > > flush_packfile_transaction(transaction); > Earlier this would unconditionally call `odb_reprepare()` within, now we only call if needed. Which makes sense. Would it also make sense to only call `flush_packfile_transaction(transaction)` if we have a packfile? > + if (have_packfile) > + odb_reprepare(transaction->base.source->odb); > + > return 0; > } > > -- > 2.55.0 ``` ## Karthik Nayak, 2026-09-15 04:46 Subject: Re: [PATCH 2/2] object-file: flush transaction packfile before migrating objects Message-ID: In-Reply-To: <18a1798d958d7f089614ec588346096c10b0666a.1789328612.git.jltobler@gmail.com> ``` Justin Tobler writes: > A "files" ODB transaction creates a temporary directory to stage newly > written objects in when configured to batch fsync loose objects. Once > the temporary directory is created, it is configured as the primary ODB > and all object are written to it accordingly. This also includes > packfiles containing blobs that exceed `core.bigFileThreshold` written > via `odb_transaction_files_write_object_stream()`. > > If a "large" blob packfile is written to the ODB transaction temporary > directory after other loose objects, the ODB transaction fails to commit > as a result of the temporary directory being migrated prior to the > packfile being flushed. Fix this bug by always flushing the packfile > transaction before objects are migrated to the main ODB. Okay this makes sense. > > Signed-off-by: Justin Tobler > --- > object-file.c | 4 ++-- > t/t1050-large.sh | 16 ++++++++++++++++ > 2 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/object-file.c b/object-file.c > index 0f123b79fad1..210984f82532 100644 > --- a/object-file.c > +++ b/object-file.c > @@ -1262,6 +1262,8 @@ static int odb_transaction_files_commit(struct odb_transaction *base) > container_of(base, struct odb_transaction_files, base); > int have_packfile = !!transaction->packfile.f; > > + flush_packfile_transaction(transaction); > + > if (transaction->objdir) { > struct strbuf temp_path = STRBUF_INIT; > struct tempfile *temp; > @@ -1292,8 +1294,6 @@ static int odb_transaction_files_commit(struct odb_transaction *base) > transaction->objdir = NULL; > } > > - flush_packfile_transaction(transaction); > - > if (have_packfile) > odb_reprepare(transaction->base.source->odb); > > diff --git a/t/t1050-large.sh b/t/t1050-large.sh > index d295c265c75c..fb83c8fba619 100755 > --- a/t/t1050-large.sh > +++ b/t/t1050-large.sh > @@ -87,6 +87,22 @@ test_expect_success 'add a large file or two' ' > test $count = 1 > ' > > +test_expect_success 'add large file with loose object in batch fsync' ' > + test_when_finished "rm -rf batch" && > + git init batch && > + > + git -C batch config core.bigFileThreshold 5 && Nit: we have `test_config` which automatically unsets after the test. Perhaps not really needed here, as we drop 'batch' anyways. > + echo foo >batch/1-small && > + echo foobar >batch/2-large && > + > + git -C batch -c core.fsync=loose-object -c core.fsyncMethod=batch \ > + add 1-small 2-large && > + > + # Neither object may be left behind in a temporary location. > + git -C batch cat-file -e :1-small && > + git -C batch cat-file -e :2-large > +' > Looks good. > + > test_expect_success 'checkout a large file' ' > large1=$(git rev-parse :large1) && > git update-index --add --cacheinfo 100644 $large1 another && > -- > 2.55.0 ``` ## Justin Tobler, 2026-09-15 08:54 Subject: Re: [PATCH 1/2] object-file: lift ODB reprepare out of packfile flush Message-ID: In-Reply-To: ``` On 26/09/15 12:40AM, Karthik Nayak wrote: >Justin Tobler writes: >> @@ -1260,6 +1260,7 @@ static int odb_transaction_files_commit(struct odb_transaction *base) >> { >> struct odb_transaction_files *transaction = >> container_of(base, struct odb_transaction_files, base); >> + int have_packfile = !!transaction->packfile.f; >> >> if (transaction->objdir) { >> struct strbuf temp_path = STRBUF_INIT; >> @@ -1293,6 +1294,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base) >> >> flush_packfile_transaction(transaction); >> > >Earlier this would unconditionally call `odb_reprepare()` within, now we >only call if needed. Which makes sense. Would it also make sense to only >call `flush_packfile_transaction(transaction)` if we have a packfile? `flush_packfile_transaction()` already returns early if there is nothing to do. We could make it more explicit here, but I think it is probably fine to leave it as-is. -Justin ``` ## Justin Tobler, 2026-09-15 08:59 Subject: Re: [PATCH 2/2] object-file: flush transaction packfile before migrating objects Message-ID: In-Reply-To: ``` On 26/09/15 12:46AM, Karthik Nayak wrote: >Justin Tobler writes: >> +test_expect_success 'add large file with loose object in batch fsync' ' >> + test_when_finished "rm -rf batch" && >> + git init batch && >> + >> + git -C batch config core.bigFileThreshold 5 && > >Nit: we have `test_config` which automatically unsets after the test. >Perhaps not really needed here, as we drop 'batch' anyways. Ya, since we are deleting the repo here as part of this test, I don't think there is much need to cleanup the config right before deleting. I'll leave it as-is. >> + echo foo >batch/1-small && >> + echo foobar >batch/2-large && >> + >> + git -C batch -c core.fsync=loose-object -c core.fsyncMethod=batch \ >> + add 1-small 2-large && >> + >> + # Neither object may be left behind in a temporary location. >> + git -C batch cat-file -e :1-small && >> + git -C batch cat-file -e :2-large >> +' >> > >Looks good. Thanks for the review, -Justin ``` ## Patrick Steinhardt, 2026-09-23 13:16 Subject: Re: [PATCH 1/2] object-file: lift ODB reprepare out of packfile flush Message-ID: In-Reply-To: ``` On Sun, Sep 13, 2026 at 03:26:21PM -0500, Justin Tobler wrote: > diff --git a/object-file.c b/object-file.c > index a4cbf8b081df..0f123b79fad1 100644 > --- a/object-file.c > +++ b/object-file.c > @@ -909,8 +907,10 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas > * to zlib compression and is sufficient for this check. > */ > if (state->nr_written && pack_size_limit_cfg && > - pack_size_limit_cfg < state->offset + stream->size) > + pack_size_limit_cfg < state->offset + stream->size) { > flush_packfile_transaction(transaction); > + odb_reprepare(transaction->base.source->odb); > + } > > CALLOC_ARRAY(idx, 1); > prepare_packfile_transaction(transaction); > @@ -1260,6 +1260,7 @@ static int odb_transaction_files_commit(struct odb_transaction *base) > { > struct odb_transaction_files *transaction = > container_of(base, struct odb_transaction_files, base); > + int have_packfile = !!transaction->packfile.f; > > if (transaction->objdir) { > struct strbuf temp_path = STRBUF_INIT; > @@ -1293,6 +1294,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base) > > flush_packfile_transaction(transaction); > > + if (have_packfile) > + odb_reprepare(transaction->base.source->odb); > + > return 0; > } One thing that I'm curious about: we don't have any error checking for flushing the object directory at alll. So there is actually a change in behaviour here, where we now also reprepare in case flushing has failed. It probably doesn't matter much, but it does raise the question whether we may want to start checking for errors. Patrick ``` ## Patrick Steinhardt, 2026-09-23 13:16 Subject: Re: [PATCH 2/2] object-file: flush transaction packfile before migrating objects Message-ID: In-Reply-To: <18a1798d958d7f089614ec588346096c10b0666a.1789328612.git.jltobler@gmail.com> ``` On Sun, Sep 13, 2026 at 03:26:22PM -0500, Justin Tobler wrote: > diff --git a/object-file.c b/object-file.c > index 0f123b79fad1..210984f82532 100644 > --- a/object-file.c > +++ b/object-file.c > @@ -1262,6 +1262,8 @@ static int odb_transaction_files_commit(struct odb_transaction *base) > container_of(base, struct odb_transaction_files, base); > int have_packfile = !!transaction->packfile.f; > > + flush_packfile_transaction(transaction); > + > if (transaction->objdir) { > struct strbuf temp_path = STRBUF_INIT; > struct tempfile *temp; > @@ -1292,8 +1294,6 @@ static int odb_transaction_files_commit(struct odb_transaction *base) > transaction->objdir = NULL; > } > > - flush_packfile_transaction(transaction); > - > if (have_packfile) > odb_reprepare(transaction->base.source->odb); > In the preceding commit you wrote: In a subsequent commit, repreparing the ODB is slightly deferred when committing a "files" ODB transaction. But that's not really true -- you don't delay repreparing the object database, but instead only flush earlier. The reprepare still happens at the same point in time. > diff --git a/t/t1050-large.sh b/t/t1050-large.sh > index d295c265c75c..fb83c8fba619 100755 > --- a/t/t1050-large.sh > +++ b/t/t1050-large.sh > @@ -87,6 +87,22 @@ test_expect_success 'add a large file or two' ' > test $count = 1 > ' > > +test_expect_success 'add large file with loose object in batch fsync' ' > + test_when_finished "rm -rf batch" && > + git init batch && I feel like using a subshell might've helped here for readability. But, oh well, it saves us an extra process. > + git -C batch config core.bigFileThreshold 5 && > + echo foo >batch/1-small && > + echo foobar >batch/2-large && > + > + git -C batch -c core.fsync=loose-object -c core.fsyncMethod=batch \ > + add 1-small 2-large && > + > + # Neither object may be left behind in a temporary location. You don't really verify whether they are left behind, but rather verify that the can be read. Which is a bit of a different thing. Sorry, feels like I'm in a nitpicky mood today :) Thanks! Patrick ``` ## Justin Tobler, 2026-09-23 21:04 Subject: Re: [PATCH 1/2] object-file: lift ODB reprepare out of packfile flush Message-ID: In-Reply-To: ``` On 26/09/23 03:16PM, Patrick Steinhardt wrote: > On Sun, Sep 13, 2026 at 03:26:21PM -0500, Justin Tobler wrote: > > diff --git a/object-file.c b/object-file.c > > index a4cbf8b081df..0f123b79fad1 100644 > > --- a/object-file.c > > +++ b/object-file.c > > @@ -909,8 +907,10 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas > > * to zlib compression and is sufficient for this check. > > */ > > if (state->nr_written && pack_size_limit_cfg && > > - pack_size_limit_cfg < state->offset + stream->size) > > + pack_size_limit_cfg < state->offset + stream->size) { > > flush_packfile_transaction(transaction); > > + odb_reprepare(transaction->base.source->odb); > > + } > > > > CALLOC_ARRAY(idx, 1); > > prepare_packfile_transaction(transaction); > > @@ -1260,6 +1260,7 @@ static int odb_transaction_files_commit(struct odb_transaction *base) > > { > > struct odb_transaction_files *transaction = > > container_of(base, struct odb_transaction_files, base); > > + int have_packfile = !!transaction->packfile.f; > > > > if (transaction->objdir) { > > struct strbuf temp_path = STRBUF_INIT; > > @@ -1293,6 +1294,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base) > > > > flush_packfile_transaction(transaction); > > > > + if (have_packfile) > > + odb_reprepare(transaction->base.source->odb); > > + > > return 0; > > } > > One thing that I'm curious about: we don't have any error checking for > flushing the object directory at alll. So there is actually a change in > behaviour here, where we now also reprepare in case flushing has failed. > It probably doesn't matter much, but it does raise the question whether > we may want to start checking for errors. Regarding the behavior change, I'm not entirely sure I follow. `flush_packfile_transaction()` only returns early in the case where there is nothing to flush. In both of the above call sites, `odb_reprepare()` is only invoked in the same circumstance. I do agree with the sentiment that error handling could be improve here as most errors are simply handled by die()'ing in place. I'll probably defer doing that as part of this series though. Thanks, -Justin ``` ## Justin Tobler, 2026-09-23 21:17 Subject: Re: [PATCH 2/2] object-file: flush transaction packfile before migrating objects Message-ID: In-Reply-To: ``` On 26/09/23 03:16PM, Patrick Steinhardt wrote: > On Sun, Sep 13, 2026 at 03:26:22PM -0500, Justin Tobler wrote: > > diff --git a/object-file.c b/object-file.c > > index 0f123b79fad1..210984f82532 100644 > > --- a/object-file.c > > +++ b/object-file.c > > @@ -1262,6 +1262,8 @@ static int odb_transaction_files_commit(struct odb_transaction *base) > > container_of(base, struct odb_transaction_files, base); > > int have_packfile = !!transaction->packfile.f; > > > > + flush_packfile_transaction(transaction); > > + > > if (transaction->objdir) { > > struct strbuf temp_path = STRBUF_INIT; > > struct tempfile *temp; > > @@ -1292,8 +1294,6 @@ static int odb_transaction_files_commit(struct odb_transaction *base) > > transaction->objdir = NULL; > > } > > > > - flush_packfile_transaction(transaction); > > - > > if (have_packfile) > > odb_reprepare(transaction->base.source->odb); > > > > In the preceding commit you wrote: > > In a subsequent commit, repreparing the ODB is slightly deferred > when committing a "files" ODB transaction. > > But that's not really true -- you don't delay repreparing the object > database, but instead only flush earlier. The reprepare still happens at > the same point in time. That's fair. When I said "deferred" I really meant that `odb_reprepare()` was now happening after and outside of `flush_packfile_transaction()`, but logically it is really in the same place. I will adapt the commit message accordingly. > > diff --git a/t/t1050-large.sh b/t/t1050-large.sh > > index d295c265c75c..fb83c8fba619 100755 > > --- a/t/t1050-large.sh > > +++ b/t/t1050-large.sh > > @@ -87,6 +87,22 @@ test_expect_success 'add a large file or two' ' > > test $count = 1 > > ' > > > > +test_expect_success 'add large file with loose object in batch fsync' ' > > + test_when_finished "rm -rf batch" && > > + git init batch && > > I feel like using a subshell might've helped here for readability. But, > oh well, it saves us an extra process. Ya, using a subshell is probably a bit easier on the eyes. Since I'm making some small changes anyways I'll go ahead and make this change too. > > + git -C batch config core.bigFileThreshold 5 && > > + echo foo >batch/1-small && > > + echo foobar >batch/2-large && > > + > > + git -C batch -c core.fsync=loose-object -c core.fsyncMethod=batch \ > > + add 1-small 2-large && > > + > > + # Neither object may be left behind in a temporary location. > > You don't really verify whether they are left behind, but rather verify > that the can be read. Which is a bit of a different thing. That fair, I'm not sure this comment is really that useful anyways so I'll just go ahead and remove it in the next version. > Sorry, feels like I'm in a nitpicky mood today :) It is always welcome and appreciated! :) Thanks, -Justin ``` ## Justin Tobler, 2026-09-23 22:03 Subject: [PATCH v2 0/2] object-file: fix packfile flush during transaction commit Message-ID: <20260923220315.569424-1-jltobler@gmail.com> In-Reply-To: ``` Greetings, This short series fixes a bug I found related to committing an ODB transaction that contains both a loose object and "large" blob when also configured to batch fsync loose objects. The issue can be reproduced with the following: git init git config core.fsync loose-object git config core.fsyncMethod batch git config core.bigFileThreshold 5 echo foo >1-foo && echo foobar >2-foobar git add 1-foo 2-foobar and produces the following error: error: unable to write file .git/objects/pack/pack-2b7c2470289822070687e8d64186093a710eaed3.pack: No such file or directory fatal: unable to rename temporary file to '.git/objects/pack/pack-2b7c2470289822070687e8d64186093a710eaed3.pack' If a "large" blob packfile is written to the transaction temporary directory, it is unable to be flushed during transaction commit because the underlying transaction is migrated to the main ODB before the packfile is finalized. To avoid this, this series ensures any pending packfile in the transaction is flushed first. Changes since V1: - Updated a commit message of first patch. - Improved test readability in second patch. Thanks, -Justin Justin Tobler (2): object-file: lift ODB reprepare out of packfile flush object-file: flush transaction packfile before migrating objects object-file.c | 12 ++++++++---- t/t1050-large.sh | 17 +++++++++++++++++ 2 files changed, 25 insertions(+), 4 deletions(-) Range-diff against v1: 1: cf14416f22 ! 1: 6f74391ae8 object-file: lift ODB reprepare out of packfile flush @@ Commit message When flushing a packfile via `flush_packfile_transaction()`, `odb_reprepare()` is invoked so the written packfile becomes visible in - the current process. In a subsequent commit, repreparing the ODB is - slightly deferred when committing a "files" ODB transaction. + the current process. In a subsequent commit, flushing the packfile is + performed earlier when committing a "files" ODB transaction, but the ODB + reprepare needs to remain the last step. Lift ODB reprepare out of `flush_packfile_transaction()` and instead require callers to explicitly invoke `odb_reprepare()` if required. 2: 18a1798d95 ! 2: ad2fa8ee3f object-file: flush transaction packfile before migrating objects @@ t/t1050-large.sh: test_expect_success 'add a large file or two' ' +test_expect_success 'add large file with loose object in batch fsync' ' + test_when_finished "rm -rf batch" && + git init batch && ++ ( ++ cd batch && ++ git config core.bigFileThreshold 5 && ++ echo foo >1-small && ++ echo foobar >2-large && + -+ git -C batch config core.bigFileThreshold 5 && -+ echo foo >batch/1-small && -+ echo foobar >batch/2-large && ++ git -c core.fsync=loose-object -c core.fsyncMethod=batch \ ++ add 1-small 2-large && + -+ git -C batch -c core.fsync=loose-object -c core.fsyncMethod=batch \ -+ add 1-small 2-large && -+ -+ # Neither object may be left behind in a temporary location. -+ git -C batch cat-file -e :1-small && -+ git -C batch cat-file -e :2-large ++ git cat-file -e :1-small && ++ git cat-file -e :2-large ++ ) +' + test_expect_success 'checkout a large file' ' base-commit: 47ce80527c56f462cb97db4ca8125342204d3783 -- 2.55.0.424.g13c7afec21 ``` ## Justin Tobler, 2026-09-23 22:03 Subject: [PATCH v2 1/2] object-file: lift ODB reprepare out of packfile flush Message-ID: <20260923220315.569424-2-jltobler@gmail.com> In-Reply-To: <20260923220315.569424-1-jltobler@gmail.com> ``` When flushing a packfile via `flush_packfile_transaction()`, `odb_reprepare()` is invoked so the written packfile becomes visible in the current process. In a subsequent commit, flushing the packfile is performed earlier when committing a "files" ODB transaction, but the ODB reprepare needs to remain the last step. Lift ODB reprepare out of `flush_packfile_transaction()` and instead require callers to explicitly invoke `odb_reprepare()` if required. Signed-off-by: Justin Tobler --- object-file.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/object-file.c b/object-file.c index a4cbf8b081..0f123b79fa 100644 --- a/object-file.c +++ b/object-file.c @@ -857,8 +857,6 @@ static void flush_packfile_transaction(struct odb_transaction_files *transaction memset(state, 0, sizeof(*state)); strbuf_release(&packname); - /* Make objects we just wrote available to ourselves */ - odb_reprepare(repo->objects); } /* @@ -909,8 +907,10 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas * to zlib compression and is sufficient for this check. */ if (state->nr_written && pack_size_limit_cfg && - pack_size_limit_cfg < state->offset + stream->size) + pack_size_limit_cfg < state->offset + stream->size) { flush_packfile_transaction(transaction); + odb_reprepare(transaction->base.source->odb); + } CALLOC_ARRAY(idx, 1); prepare_packfile_transaction(transaction); @@ -1260,6 +1260,7 @@ static int odb_transaction_files_commit(struct odb_transaction *base) { struct odb_transaction_files *transaction = container_of(base, struct odb_transaction_files, base); + int have_packfile = !!transaction->packfile.f; if (transaction->objdir) { struct strbuf temp_path = STRBUF_INIT; @@ -1293,6 +1294,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base) flush_packfile_transaction(transaction); + if (have_packfile) + odb_reprepare(transaction->base.source->odb); + return 0; } -- 2.55.0.424.g13c7afec21 ``` ## Justin Tobler, 2026-09-23 22:03 Subject: [PATCH v2 2/2] object-file: flush transaction packfile before migrating objects Message-ID: <20260923220315.569424-3-jltobler@gmail.com> In-Reply-To: <20260923220315.569424-1-jltobler@gmail.com> ``` A "files" ODB transaction creates a temporary directory to stage newly written objects in when configured to batch fsync loose objects. Once the temporary directory is created, it is configured as the primary ODB and all object are written to it accordingly. This also includes packfiles containing blobs that exceed `core.bigFileThreshold` written via `odb_transaction_files_write_object_stream()`. If a "large" blob packfile is written to the ODB transaction temporary directory after other loose objects, the ODB transaction fails to commit as a result of the temporary directory being migrated prior to the packfile being flushed. Fix this bug by always flushing the packfile transaction before objects are migrated to the main ODB. Signed-off-by: Justin Tobler --- object-file.c | 4 ++-- t/t1050-large.sh | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/object-file.c b/object-file.c index 0f123b79fa..210984f825 100644 --- a/object-file.c +++ b/object-file.c @@ -1262,6 +1262,8 @@ static int odb_transaction_files_commit(struct odb_transaction *base) container_of(base, struct odb_transaction_files, base); int have_packfile = !!transaction->packfile.f; + flush_packfile_transaction(transaction); + if (transaction->objdir) { struct strbuf temp_path = STRBUF_INIT; struct tempfile *temp; @@ -1292,8 +1294,6 @@ static int odb_transaction_files_commit(struct odb_transaction *base) transaction->objdir = NULL; } - flush_packfile_transaction(transaction); - if (have_packfile) odb_reprepare(transaction->base.source->odb); diff --git a/t/t1050-large.sh b/t/t1050-large.sh index d295c265c7..95233458b4 100755 --- a/t/t1050-large.sh +++ b/t/t1050-large.sh @@ -87,6 +87,23 @@ test_expect_success 'add a large file or two' ' test $count = 1 ' +test_expect_success 'add large file with loose object in batch fsync' ' + test_when_finished "rm -rf batch" && + git init batch && + ( + cd batch && + git config core.bigFileThreshold 5 && + echo foo >1-small && + echo foobar >2-large && + + git -c core.fsync=loose-object -c core.fsyncMethod=batch \ + add 1-small 2-large && + + git cat-file -e :1-small && + git cat-file -e :2-large + ) +' + test_expect_success 'checkout a large file' ' large1=$(git rev-parse :large1) && git update-index --add --cacheinfo 100644 $large1 another && -- 2.55.0.424.g13c7afec21 ``` ## Patrick Steinhardt, 2026-09-24 05:58 Subject: Re: [PATCH 1/2] object-file: lift ODB reprepare out of packfile flush Message-ID: In-Reply-To: ``` On Wed, Sep 23, 2026 at 04:04:55PM -0500, Justin Tobler wrote: > On 26/09/23 03:16PM, Patrick Steinhardt wrote: > > On Sun, Sep 13, 2026 at 03:26:21PM -0500, Justin Tobler wrote: > > > diff --git a/object-file.c b/object-file.c > > > index a4cbf8b081df..0f123b79fad1 100644 > > > --- a/object-file.c > > > +++ b/object-file.c > > > @@ -909,8 +907,10 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas > > > * to zlib compression and is sufficient for this check. > > > */ > > > if (state->nr_written && pack_size_limit_cfg && > > > - pack_size_limit_cfg < state->offset + stream->size) > > > + pack_size_limit_cfg < state->offset + stream->size) { > > > flush_packfile_transaction(transaction); > > > + odb_reprepare(transaction->base.source->odb); > > > + } > > > > > > CALLOC_ARRAY(idx, 1); > > > prepare_packfile_transaction(transaction); > > > @@ -1260,6 +1260,7 @@ static int odb_transaction_files_commit(struct odb_transaction *base) > > > { > > > struct odb_transaction_files *transaction = > > > container_of(base, struct odb_transaction_files, base); > > > + int have_packfile = !!transaction->packfile.f; > > > > > > if (transaction->objdir) { > > > struct strbuf temp_path = STRBUF_INIT; > > > @@ -1293,6 +1294,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base) > > > > > > flush_packfile_transaction(transaction); > > > > > > + if (have_packfile) > > > + odb_reprepare(transaction->base.source->odb); > > > + > > > return 0; > > > } > > > > One thing that I'm curious about: we don't have any error checking for > > flushing the object directory at alll. So there is actually a change in > > behaviour here, where we now also reprepare in case flushing has failed. > > It probably doesn't matter much, but it does raise the question whether > > we may want to start checking for errors. > > Regarding the behavior change, I'm not entirely sure I follow. > `flush_packfile_transaction()` only returns early in the case where > there is nothing to flush. In both of the above call sites, > `odb_reprepare()` is only invoked in the same circumstance. There's a second early return when `tmp_objdir_migrate()` fails, and that early return causes us to not flush. Patrick ``` ## Patrick Steinhardt, 2026-09-24 06:00 Subject: Re: [PATCH 1/2] object-file: lift ODB reprepare out of packfile flush Message-ID: In-Reply-To: ``` On Thu, Sep 24, 2026 at 07:58:07AM +0200, Patrick Steinhardt wrote: > On Wed, Sep 23, 2026 at 04:04:55PM -0500, Justin Tobler wrote: > > On 26/09/23 03:16PM, Patrick Steinhardt wrote: > > > On Sun, Sep 13, 2026 at 03:26:21PM -0500, Justin Tobler wrote: > > > > diff --git a/object-file.c b/object-file.c > > > > index a4cbf8b081df..0f123b79fad1 100644 > > > > --- a/object-file.c > > > > +++ b/object-file.c > > > > @@ -909,8 +907,10 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas > > > > * to zlib compression and is sufficient for this check. > > > > */ > > > > if (state->nr_written && pack_size_limit_cfg && > > > > - pack_size_limit_cfg < state->offset + stream->size) > > > > + pack_size_limit_cfg < state->offset + stream->size) { > > > > flush_packfile_transaction(transaction); > > > > + odb_reprepare(transaction->base.source->odb); > > > > + } > > > > > > > > CALLOC_ARRAY(idx, 1); > > > > prepare_packfile_transaction(transaction); > > > > @@ -1260,6 +1260,7 @@ static int odb_transaction_files_commit(struct odb_transaction *base) > > > > { > > > > struct odb_transaction_files *transaction = > > > > container_of(base, struct odb_transaction_files, base); > > > > + int have_packfile = !!transaction->packfile.f; > > > > > > > > if (transaction->objdir) { > > > > struct strbuf temp_path = STRBUF_INIT; > > > > @@ -1293,6 +1294,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base) > > > > > > > > flush_packfile_transaction(transaction); > > > > > > > > + if (have_packfile) > > > > + odb_reprepare(transaction->base.source->odb); > > > > + > > > > return 0; > > > > } > > > > > > One thing that I'm curious about: we don't have any error checking for > > > flushing the object directory at alll. So there is actually a change in > > > behaviour here, where we now also reprepare in case flushing has failed. > > > It probably doesn't matter much, but it does raise the question whether > > > we may want to start checking for errors. > > > > Regarding the behavior change, I'm not entirely sure I follow. > > `flush_packfile_transaction()` only returns early in the case where > > there is nothing to flush. In both of the above call sites, > > `odb_reprepare()` is only invoked in the same circumstance. > > There's a second early return when `tmp_objdir_migrate()` fails, and > that early return causes us to not flush. Oh, never mind. I think I've been confusing the fact that what you're changing is actually `odb_transaction_files_commit()` itself, and that early return of course still exists in there. So this looks good to me. Patrick ``` ## Patrick Steinhardt, 2026-09-24 06:01 Subject: Re: [PATCH v2 0/2] object-file: fix packfile flush during transaction commit Message-ID: In-Reply-To: <20260923220315.569424-1-jltobler@gmail.com> ``` On Wed, Sep 23, 2026 at 05:03:13PM -0500, Justin Tobler wrote: > Changes since V1: > - Updated a commit message of first patch. > - Improved test readability in second patch. Thanks, this version looks good to me. Patrick ``` ## Patrick Steinhardt, 2026-09-24 06:13 Subject: Re: [PATCH v2 1/2] object-file: lift ODB reprepare out of packfile flush Message-ID: In-Reply-To: <20260923220315.569424-2-jltobler@gmail.com> ``` On Wed, Sep 23, 2026 at 05:03:14PM -0500, Justin Tobler wrote: > When flushing a packfile via `flush_packfile_transaction()`, > `odb_reprepare()` is invoked so the written packfile becomes visible in > the current process. In a subsequent commit, flushing the packfile is > performed earlier when committing a "files" ODB transaction, but the ODB > reprepare needs to remain the last step. Yup, this is more in line with what that second commit will do. Patrick ```