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

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