From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 24 Sep 2026 05:58:07 GMT
Subject: Re: [PATCH 1/2] object-file: lift ODB reprepare out of packfile flush
Message-ID: <arS77zUvWKdjEeS-@pks.im>
In-Reply-To: <arQ8nsUzg9atdCeD@denethor>

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