Re: [PATCH 2/6] object-file: propagate files transaction errors
From: Patrick Steinhardt <hidden>
Date: 2026-06-24 11:26:43
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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
@@ -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?
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
{ 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?
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
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