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

[Justin Tobler <jltobler@gmail.com>]

On 26/06/24 11:35AM, Junio C Hamano wrote:
> Justin Tobler <jltobler@gmail.com> writes:
> 
> > The "files" transaction backend may encounter errors related to managing
> > the temporary directory used to stage objects, but silently ignores
> > these errors. Instead return errors encountered in the
> > `odb_transaction_files_{prepare,begin,commit}()` interfaces to allow
> > callers to handle as needed.
> 
> "handle them as needed", perhaps.

Will fix, thanks

[snip]
> The caller of this function does react to a failure of it, ...
> 
> > @@ -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;
> >  }
> 
> ... like this, which is good.  Do we need an explicit "abort-transaction",
> or is that implicit?

So this is currently handled implicitly via
`tmp-objdir.c:remove_tmp_objdir()` which gets registered as an atexit()
handler. As long as the tmp_objdir global remains set, it will
automatically get cleaned up.

In a subsequent series, I do plan to add `odb_transaction_abort()` to
the transaction interface. It may make sense to also use that here to
make the cleanup a bit more explicit though.

-Justin