Re: [PATCH 2/2] object-file: flush transaction packfile before migrating objects
- From
Justin Tobler <jltobler@gmail.com>
- Date
- Sep 23, 2026, 21:17 UTC
- Message-ID
- <arQ_Uz2R_sE4yzXu@denethor>
- In-Reply-To
- <arPRM191URNQGu7V@pks.im>
On 26/09/23 03:16PM, Patrick Steinhardt wrote:
Show 32 quoted lines
> 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.
Show 14 quoted lines
> > 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.
Show 11 quoted lines
> > + 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