Re: [PATCH 1/2] object-file: lift ODB reprepare out of packfile flush

On 26/09/23 03:16PM, Patrick Steinhardt wrote:

On Sun, Sep 13, 2026 at 03:26:21PM -0500, Justin Tobler wrote:

quoted

diff --git a/object-file.c b/object-file.c
index a4cbf8b081df..0f123b79fad1 100644
--- a/object-file.c
+++ b/object-file.c

@@ -909,8 +907,10 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas
 	 * to zlib compression and is sufficient for this check.
 	 */
 	if (state->nr_written && pack_size_limit_cfg &&
-	    pack_size_limit_cfg < state->offset + stream->size)
+	    pack_size_limit_cfg < state->offset + stream->size) {
 		flush_packfile_transaction(transaction);
+		odb_reprepare(transaction->base.source->odb);
+	}
 
 	CALLOC_ARRAY(idx, 1);
 	prepare_packfile_transaction(transaction);

@@ -1260,6 +1260,7 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
 {
 	struct odb_transaction_files *transaction =
 		container_of(base, struct odb_transaction_files, base);
+	int have_packfile = !!transaction->packfile.f;
 
 	if (transaction->objdir) {
 		struct strbuf temp_path = STRBUF_INIT;

@@ -1293,6 +1294,9 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
 
 	flush_packfile_transaction(transaction);
 
+	if (have_packfile)
+		odb_reprepare(transaction->base.source->odb);
+
 	return 0;
 }

One thing that I'm curious about: we don't have any error checking for
flushing the object directory at alll. So there is actually a change in
behaviour here, where we now also reprepare in case flushing has failed.
It probably doesn't matter much, but it does raise the question whether
we may want to start checking for errors.

Regarding the behavior change, I'm not entirely sure I follow.
`flush_packfile_transaction()` only returns early in the case where
there is nothing to flush. In both of the above call sites,
`odb_reprepare()` is only invoked in the same circumstance.

I do agree with the sentiment that error handling could be improve here
as most errors are simply handled by die()'ing in place. I'll probably
defer doing that as part of this series though.

Thanks,
-Justin