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

[Justin Tobler <jltobler@gmail.com>]

On 26/06/30 10:45AM, Patrick Steinhardt wrote:
> On Mon, Jun 29, 2026 at 02:04:08PM -0500, Justin Tobler wrote:
> > On 26/06/29 01:58PM, 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:
> > > > > @@ -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.
> > 
> > I think I got a bit ahead of myself. The existing callers of
> > odb_transaction_files_prepare() still continue to ignore this error. So
> > the behavior already does remain the same here.
> 
> Oh, well, okay. I think this behaviour is plain bad -- if the caller
> wants to have a transaction, then we should bail in case we cannot
> create one. But this doesn't need to be fixed in this patch series.

Ya, I tend to agree. The problem here is that
odb_transaction_files_prepare() is being invoked lazily during the
object write. This would be another argument against lazily creating the
temporary directory though. In a followup series I'll explore removing
this and investigate if it has any meaninful performance implications.

-Justin