Re: [PATCH 2/6] object-file: propagate files transaction errors

[Patrick Steinhardt <ps@pks.im>]

On Mon, Jun 29, 2026 at 01:58:54PM -0500, Justin Tobler wrote:
> On 26/06/24 01:26PM, Patrick Steinhardt wrote:
> > On Tue, Jun 23, 2026 at 11:19:16PM -0500, Justin Tobler wrote:
> > > diff --git a/object-file.c b/object-file.c
> > > index a3eb8d71dd..18c2df75fb 100644
> > > --- a/object-file.c
> > > +++ b/object-file.c
> > > @@ -499,7 +499,7 @@ struct odb_transaction_files {
> > >  	struct transaction_packfile packfile;
> > >  };
> > >  
> > > -static void odb_transaction_files_prepare(struct odb_transaction *base)
> > > +static int odb_transaction_files_prepare(struct odb_transaction *base)
> > >  {
> > >  	struct odb_transaction_files *transaction =
> > >  		container_of_or_null(base, struct odb_transaction_files, base);
> > 
> > By the way, is there any reason why those functions are still hosted in
> > "object-file.c" instead of in "odb/source-files.c"? I should probably
> > know, but I forgot.
> 
> There are currently a couple spots in the "files" object write path in
> "object-file.c" that still reach into some of these transaction function
> that are not part of the generic ODB transaction interface. I'm hoping
> in a future followup series to detangle this a bit and be able to get
> all the "files" ODB transaction related code moved into
> "odb/source-files.c".

Makes sense. I also revisited that code a couple days ago, and the
answer is "it's messy right now". Hopefully this will become easier to
detangle as we make progress on pluggifying transactions.

> > > @@ -511,11 +511,15 @@ static void odb_transaction_files_prepare(struct odb_transaction *base)
> > >  	 * added at the time they call odb_transaction_files_begin.
> > >  	 */
> > >  	if (!transaction || transaction->objdir)
> > > -		return;
> > > +		return 0;
> > >  
> > >  	transaction->objdir = tmp_objdir_create(base->source->odb->repo, "bulk-fsync");
> > > -	if (transaction->objdir)
> > > -		tmp_objdir_replace_primary_odb(transaction->objdir, 0);
> > > +	if (!transaction->objdir)
> > > +		return -1;
> > 
> > Huh. So previously we just didn't handle this error at all and just
> > continued to tag along? Did that result in anything sensible or was this
> > just YOLOing it?
> 
> Good question. Previously if there was an error, we wouldn't end up
> creating any tmpdir and would instead continue to use the primary ODB to
> write objects in. This change would make it a hard error if we fail to
> create the temp dir. This matches the behavior that git-receive-pack(1)
> expects, but I didn't consider that the existing callers could
> transparently handle there being no temp dir.
> 
> I suspect we may want existing ODB transaction users to continue being
> resilient in the same manner. In the next version, I'll maintain the
> same behavior.

Honestly I'd say that the change is a good one. I cannot think of a
single reason to just blindly not create the transaction and proceed.
But it certainly is something that should be documented as part of the
commit message.

> > > @@ -1670,27 +1678,34 @@ int read_loose_object(struct repository *repo,
> > >  	return ret;
> > >  }
> > >  
> > > -static void odb_transaction_files_commit(struct odb_transaction *base)
> > > +static int odb_transaction_files_commit(struct odb_transaction *base)
> > >  {
> > >  	struct odb_transaction_files *transaction =
> > >  		container_of(base, struct odb_transaction_files, base);
> > >  
> > > -	flush_loose_object_transaction(transaction);
> > > +	if (flush_loose_object_transaction(transaction))
> > > +		return -1;
> > >  	flush_packfile_transaction(transaction);
> > > +
> > > +	return 0;
> > >  }
> > >  
> > > -struct odb_transaction *odb_transaction_files_begin(struct odb_source *source)
> > > +int odb_transaction_files_begin(struct odb_source *source,
> > > +				struct odb_transaction **out)
> > >  {
> > >  	struct odb_transaction_files *transaction;
> > >  	struct object_database *odb = source->odb;
> > >  
> > > -	if (odb->transaction)
> > > -		return NULL;
> > > +	if (odb->transaction) {
> > > +		*out = NULL;
> > > +		return 0;
> > > +	}
> > >  
> > >  	transaction = xcalloc(1, sizeof(*transaction));
> > >  	transaction->base.source = source;
> > >  	transaction->base.commit = odb_transaction_files_commit;
> > >  	transaction->base.write_object_stream = odb_transaction_files_write_object_stream;
> > > +	*out = &transaction->base;
> > >  
> > > -	return &transaction->base;
> > > +	return 0;
> > >  }
> > 
> > It's still somewhat fishy that we have this ODB-level transaction, but
> > that's a preexisting issue and thus outside the scope of this patch
> > series. Ideally though, it would be possible for there to be multiple
> > transactions, and it would be the caller's responsibility for juggling
> > these transactions. Just as it happens with reference transactions.
> 
> I completely agree. One of the current problems preventing this is that
> only a single instance of tmp_objdir is allowed and stored globally.
> This is done to keep atexit() cleanup simple.
> 
> My plan is to eventually convert all existing tmp_objdir callsites to
> use ODB transactions which should hopefully allow us to remove the need
> for a separate tmp_objdir system. At that point, we can also work to
> change how temp dir cleanup is handled at exit.

Great.

> Another problem is that there are also a couple of ODB transaction
> callsites that require to know if there is already a pending transaction
> for the repository and the transaction has not been wired down to these
> callsites. My hope is that this can be addressed though as we expand ODB
> transaction usage for object writes.

Yeah, agreed. Making the use of transactions explicit feels sensible to
me.

Patrick