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

[Patrick Steinhardt <ps@pks.im>]

On Tue, Jun 23, 2026 at 11:19:16PM -0500, Justin Tobler wrote:
> 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.

Missing a then? "to handle as needed" -> "to handle them as needed"

Makes sense. It always felt a bit off that those functions didn't have a
way to signal errors to the caller.

> 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.

> @@ -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?

> @@ -542,13 +546,13 @@ static void fsync_loose_object_transaction(struct odb_transaction *base,
>  /*
>   * Cleanup after batch-mode fsync_object_files.
>   */
> -static void flush_loose_object_transaction(struct odb_transaction_files *transaction)
> +static int flush_loose_object_transaction(struct odb_transaction_files *transaction)

Feels like this function should've been renamed in the preceding commit,
as well.

>  {
>  	struct strbuf temp_path = STRBUF_INIT;
>  	struct tempfile *temp;
>  
>  	if (!transaction->objdir)
> -		return;
> +		return 0;
>  
>  	/*
>  	 * Issue a full hardware flush against a temporary file to ensure
> @@ -570,8 +574,12 @@ static void flush_loose_object_transaction(struct odb_transaction_files *transac

There is a call to `xmks_tempfile()` hidden that can fail, but that
failure is already handled in that function itself by dying.

>  	 * Make the object files visible in the primary ODB after their data is
>  	 * fully durable.
>  	 */
> -	tmp_objdir_migrate(transaction->objdir);
> +	if (tmp_objdir_migrate(transaction->objdir))
> +		return -1;

Feels like another case of YOLOing it. The migration could have failed,
but we just ignored that failure and never told the user about it. The
result may be silent corruption, I assume?

> @@ -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.

> diff --git a/odb/transaction.h b/odb/transaction.h
> index 854fda06f5..f4c1ebfaaa 100644
> --- a/odb/transaction.h
> +++ b/odb/transaction.h
> @@ -17,7 +17,7 @@ struct odb_transaction {
>  	struct odb_source *source;
>  
>  	/* The ODB source specific callback invoked to commit a transaction. */
> -	void (*commit)(struct odb_transaction *transaction);
> +	int (*commit)(struct odb_transaction *transaction);

We might want to document the returned error code here.

Patrick