{"thread":{"id":"64002","subject":"[PATCH 0/3] bulk-checkin: remove global transaction state","startedAt":"2025-08-20T22:55:47Z","lastAt":"2025-08-25T20:25:21Z","messageCount":27,"participants":["Justin Tobler","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"524584","messageId":"20250820225531.1212935-1-jltobler@gmail.com","threadId":"64002","inReplyTo":null,"subject":"[PATCH 0/3] bulk-checkin: remove global transaction state","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-20T22:55:28Z","receivedAt":"2025-08-20T22:55:47Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Greetings,\n\nThe bulk-checkin subsystem provides an interface to write objects to the\nobject database in a bulk transaction. The state of an ongoing\ntransaction is stored across several global variables. This series aims\nto remove this global transaction state in favor of storing state in in\n`struct object_database`. This is done in preparation for a follow-up\nchange where the goal is to eventually move these transaction interfaces\ninto \"odb.h\".\n\nThanks,\n-Justin\n\nJustin Tobler (3):\n bulk-checkin: introduce object database transaction structure\n bulk-checkin: remove global transaction state\n bulk-checkin: wire repository variable\n\n builtin/add.c | 5 +-\n builtin/unpack-objects.c | 5 +-\n builtin/update-index.c | 7 +-\n bulk-checkin.c | 136 ++++++++++++++++++++++++---------------\n bulk-checkin.h | 19 ++++--\n cache-tree.c | 5 +-\n object-file.c | 12 ++--\n odb.h | 8 +++\n read-cache.c | 5 +-\n 9 files changed, 128 insertions(+), 74 deletions(-)\n\n\nbase-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0\n-- \n2.51.0\n\n"},{"id":"524585","messageId":"20250820225531.1212935-2-jltobler@gmail.com","threadId":"64002","inReplyTo":"20250820225531.1212935-1-jltobler@gmail.com","subject":"[PATCH 1/3] bulk-checkin: introduce object database transaction structure","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-20T22:55:29Z","receivedAt":"2025-08-20T22:55:48Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Object database transaction state is stored across several global\nvariables in the bulk-checkin subsystem. Consolidate this state into a\nsingle `struct odb_transaction` global. In a subsequent commit, the\ntransactional interfaces will be updated to wire this structure instead\nof relying on a global variable.\n\nSigned-off-by: Justin Tobler \n---\n bulk-checkin.c | 46 ++++++++++++++++++++++++----------------------\n 1 file changed, 24 insertions(+), 22 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex b2809ab0398..82a73da79e8 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -19,11 +19,7 @@\n #include \"object-file.h\"\n #include \"odb.h\"\n \n-static int odb_transaction_nesting;\n-\n-static struct tmp_objdir *bulk_fsync_objdir;\n-\n-static struct bulk_checkin_packfile {\n+struct bulk_checkin_packfile {\n \tchar *pack_tmp_name;\n \tstruct hashfile *f;\n \toff_t offset;\n@@ -32,7 +28,13 @@ static struct bulk_checkin_packfile {\n \tstruct pack_idx_entry **written;\n \tuint32_t alloc_written;\n \tuint32_t nr_written;\n-} bulk_checkin_packfile;\n+};\n+\n+static struct odb_transaction {\n+\tint nesting;\n+\tstruct tmp_objdir *objdir;\n+\tstruct bulk_checkin_packfile packfile;\n+} transaction;\n \n static void finish_tmp_packfile(struct strbuf *basename,\n \t\t\t\tconst char *pack_tmp_name,\n@@ -101,7 +103,7 @@ static void flush_batch_fsync(void)\n \tstruct strbuf temp_path = STRBUF_INIT;\n \tstruct tempfile *temp;\n \n-\tif (!bulk_fsync_objdir)\n+\tif (!transaction.objdir)\n \t\treturn;\n \n \t/*\n@@ -123,8 +125,8 @@ static void flush_batch_fsync(void)\n \t * Make the object files visible in the primary ODB after their data is\n \t * fully durable.\n \t */\n-\ttmp_objdir_migrate(bulk_fsync_objdir);\n-\tbulk_fsync_objdir = NULL;\n+\ttmp_objdir_migrate(transaction.objdir);\n+\ttransaction.objdir = NULL;\n }\n \n static int already_written(struct bulk_checkin_packfile *state, struct object_id *oid)\n@@ -331,12 +333,12 @@ void prepare_loose_object_bulk_checkin(void)\n \t * callers may not know whether any objects will be\n \t * added at the time they call begin_odb_transaction.\n \t */\n-\tif (!odb_transaction_nesting || bulk_fsync_objdir)\n+\tif (!transaction.nesting || transaction.objdir)\n \t\treturn;\n \n-\tbulk_fsync_objdir = tmp_objdir_create(the_repository, \"bulk-fsync\");\n-\tif (bulk_fsync_objdir)\n-\t\ttmp_objdir_replace_primary_odb(bulk_fsync_objdir, 0);\n+\ttransaction.objdir = tmp_objdir_create(the_repository, \"bulk-fsync\");\n+\tif (transaction.objdir)\n+\t\ttmp_objdir_replace_primary_odb(transaction.objdir, 0);\n }\n \n void fsync_loose_object_bulk_checkin(int fd, const char *filename)\n@@ -348,7 +350,7 @@ void fsync_loose_object_bulk_checkin(int fd, const char *filename)\n \t * before renaming the objects to their final names as part of\n \t * flush_batch_fsync.\n \t */\n-\tif (!bulk_fsync_objdir ||\n+\tif (!transaction.objdir ||\n \t git_fsync(fd, FSYNC_WRITEOUT_ONLY) < 0) {\n \t\tif (errno == ENOSYS)\n \t\t\twarning(_(\"core.fsyncMethod = batch is unsupported on this platform\"));\n@@ -360,31 +362,31 @@ int index_blob_bulk_checkin(struct object_id *oid,\n \t\t\t int fd, size_t size,\n \t\t\t const char *path, unsigned flags)\n {\n-\tint status = deflate_blob_to_pack(&bulk_checkin_packfile, oid, fd, size,\n+\tint status = deflate_blob_to_pack(&transaction.packfile, oid, fd, size,\n \t\t\t\t\t path, flags);\n-\tif (!odb_transaction_nesting)\n-\t\tflush_bulk_checkin_packfile(&bulk_checkin_packfile);\n+\tif (!transaction.nesting)\n+\t\tflush_bulk_checkin_packfile(&transaction.packfile);\n \treturn status;\n }\n \n void begin_odb_transaction(void)\n {\n-\todb_transaction_nesting += 1;\n+\ttransaction.nesting += 1;\n }\n \n void flush_odb_transaction(void)\n {\n \tflush_batch_fsync();\n-\tflush_bulk_checkin_packfile(&bulk_checkin_packfile);\n+\tflush_bulk_checkin_packfile(&transaction.packfile);\n }\n \n void end_odb_transaction(void)\n {\n-\todb_transaction_nesting -= 1;\n-\tif (odb_transaction_nesting < 0)\n+\ttransaction.nesting -= 1;\n+\tif (transaction.nesting < 0)\n \t\tBUG(\"Unbalanced ODB transaction nesting\");\n \n-\tif (odb_transaction_nesting)\n+\tif (transaction.nesting)\n \t\treturn;\n \n \tflush_odb_transaction();\n-- \n2.51.0\n\n"},{"id":"524586","messageId":"20250820225531.1212935-3-jltobler@gmail.com","threadId":"64002","inReplyTo":"20250820225531.1212935-1-jltobler@gmail.com","subject":"[PATCH 2/3] bulk-checkin: remove global transaction state","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-20T22:55:30Z","receivedAt":"2025-08-20T22:55:49Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Object database transactions in the bulk-checkin subsystem rely on\nglobal state to track transaction status. Stop relying on global state\nand instead store the transaction in the `struct object_database`.\nFunctions that operate on transactions are updated to now wire\ntransaction state.\n\nSigned-off-by: Justin Tobler \n---\n builtin/add.c | 5 ++-\n builtin/unpack-objects.c | 5 ++-\n builtin/update-index.c | 7 ++--\n bulk-checkin.c | 82 ++++++++++++++++++++++++++--------------\n bulk-checkin.h | 18 +++++----\n cache-tree.c | 5 ++-\n object-file.c | 11 +++---\n odb.h | 8 ++++\n read-cache.c | 5 ++-\n 9 files changed, 94 insertions(+), 52 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 0235854f809..740c7c45817 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -389,6 +389,7 @@ int cmd_add(int argc,\n \tchar *seen = NULL;\n \tchar *ps_matched = NULL;\n \tstruct lock_file lock_file = LOCK_INIT;\n+\tstruct odb_transaction *transaction;\n \n \trepo_config(repo, add_config, NULL);\n \n@@ -574,7 +575,7 @@ int cmd_add(int argc,\n \t\tstring_list_clear(&only_match_skip_worktree, 0);\n \t}\n \n-\tbegin_odb_transaction();\n+\ttransaction = begin_odb_transaction(repo->objects);\n \n \tps_matched = xcalloc(pathspec.nr, 1);\n \tif (add_renormalize)\n@@ -593,7 +594,7 @@ int cmd_add(int argc,\n \n \tif (chmod_arg && pathspec.nr)\n \t\texit_status |= chmod_pathspec(repo, &pathspec, chmod_arg[0], show_only);\n-\tend_odb_transaction();\n+\tend_odb_transaction(transaction);\n \n finish:\n \tif (write_locked_index(repo->index, &lock_file,\ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex 7ae7c82b6c0..28124b324d2 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -584,6 +584,7 @@ static void unpack_all(void)\n {\n \tint i;\n \tunsigned char *hdr = fill(sizeof(struct pack_header));\n+\tstruct odb_transaction *transaction;\n \n \tif (get_be32(hdr) != PACK_SIGNATURE)\n \t\tdie(\"bad pack file\");\n@@ -599,12 +600,12 @@ static void unpack_all(void)\n \t\tprogress = start_progress(the_repository,\n \t\t\t\t\t _(\"Unpacking objects\"), nr_objects);\n \tCALLOC_ARRAY(obj_list, nr_objects);\n-\tbegin_odb_transaction();\n+\ttransaction = begin_odb_transaction(the_repository->objects);\n \tfor (i = 0; i < nr_objects; i++) {\n \t\tunpack_one(i);\n \t\tdisplay_progress(progress, i + 1);\n \t}\n-\tend_odb_transaction();\n+\tend_odb_transaction(transaction);\n \tstop_progress(&progress);\n \n \tif (delta_list)\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 2380f3ccd68..2ba2d29c959 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -77,7 +77,7 @@ static void report(const char *fmt, ...)\n \t * objects invisible while a transaction is active, so flush the\n \t * transaction here before reporting a change made by update-index.\n \t */\n-\tflush_odb_transaction();\n+\tflush_odb_transaction(the_repository->objects->transaction);\n \tva_start(vp, fmt);\n \tvprintf(fmt, vp);\n \tputchar('\\n');\n@@ -940,6 +940,7 @@ int cmd_update_index(int argc,\n \tstrbuf_getline_fn getline_fn;\n \tint parseopt_state = PARSE_OPT_UNKNOWN;\n \tstruct repository *r = the_repository;\n+\tstruct odb_transaction *transaction;\n \tstruct option options[] = {\n \t\tOPT_BIT('q', NULL, &refresh_args.flags,\n \t\t\tN_(\"continue refresh even when index needs update\"),\n@@ -1130,7 +1131,7 @@ int cmd_update_index(int argc,\n \t * Allow the object layer to optimize adding multiple objects in\n \t * a batch.\n \t */\n-\tbegin_odb_transaction();\n+\ttransaction = begin_odb_transaction(the_repository->objects);\n \twhile (ctx.argc) {\n \t\tif (parseopt_state != PARSE_OPT_DONE)\n \t\t\tparseopt_state = parse_options_step(&ctx, options,\n@@ -1213,7 +1214,7 @@ int cmd_update_index(int argc,\n \t/*\n \t * By now we have added all of the new objects\n \t */\n-\tend_odb_transaction();\n+\tend_odb_transaction(transaction);\n \n \tif (split_index > 0) {\n \t\tif (repo_config_get_split_index(the_repository) == 0)\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 82a73da79e8..53a20a2d92f 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -30,11 +30,13 @@ struct bulk_checkin_packfile {\n \tuint32_t nr_written;\n };\n \n-static struct odb_transaction {\n+struct odb_transaction {\n+\tstruct object_database *odb;\n+\n \tint nesting;\n \tstruct tmp_objdir *objdir;\n \tstruct bulk_checkin_packfile packfile;\n-} transaction;\n+};\n \n static void finish_tmp_packfile(struct strbuf *basename,\n \t\t\t\tconst char *pack_tmp_name,\n@@ -98,12 +100,12 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n /*\n * Cleanup after batch-mode fsync_object_files.\n */\n-static void flush_batch_fsync(void)\n+static void flush_batch_fsync(struct odb_transaction *transaction)\n {\n \tstruct strbuf temp_path = STRBUF_INIT;\n \tstruct tempfile *temp;\n \n-\tif (!transaction.objdir)\n+\tif (!transaction->objdir)\n \t\treturn;\n \n \t/*\n@@ -125,8 +127,8 @@ static void flush_batch_fsync(void)\n \t * Make the object files visible in the primary ODB after their data is\n \t * fully durable.\n \t */\n-\ttmp_objdir_migrate(transaction.objdir);\n-\ttransaction.objdir = NULL;\n+\ttmp_objdir_migrate(transaction->objdir);\n+\ttransaction->objdir = NULL;\n }\n \n static int already_written(struct bulk_checkin_packfile *state, struct object_id *oid)\n@@ -325,7 +327,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \treturn 0;\n }\n \n-void prepare_loose_object_bulk_checkin(void)\n+void prepare_loose_object_bulk_checkin(struct odb_transaction *transaction)\n {\n \t/*\n \t * We lazily create the temporary object directory\n@@ -333,15 +335,16 @@ void prepare_loose_object_bulk_checkin(void)\n \t * callers may not know whether any objects will be\n \t * added at the time they call begin_odb_transaction.\n \t */\n-\tif (!transaction.nesting || transaction.objdir)\n+\tif (!transaction || transaction->objdir)\n \t\treturn;\n \n-\ttransaction.objdir = tmp_objdir_create(the_repository, \"bulk-fsync\");\n-\tif (transaction.objdir)\n-\t\ttmp_objdir_replace_primary_odb(transaction.objdir, 0);\n+\ttransaction->objdir = tmp_objdir_create(the_repository, \"bulk-fsync\");\n+\tif (transaction->objdir)\n+\t\ttmp_objdir_replace_primary_odb(transaction->objdir, 0);\n }\n \n-void fsync_loose_object_bulk_checkin(int fd, const char *filename)\n+void fsync_loose_object_bulk_checkin(struct odb_transaction *transaction,\n+\t\t\t\t int fd, const char *filename)\n {\n \t/*\n \t * If we have an active ODB transaction, we issue a call that\n@@ -350,7 +353,7 @@ void fsync_loose_object_bulk_checkin(int fd, const char *filename)\n \t * before renaming the objects to their final names as part of\n \t * flush_batch_fsync.\n \t */\n-\tif (!transaction.objdir ||\n+\tif (!transaction || !transaction->objdir ||\n \t git_fsync(fd, FSYNC_WRITEOUT_ONLY) < 0) {\n \t\tif (errno == ENOSYS)\n \t\t\twarning(_(\"core.fsyncMethod = batch is unsupported on this platform\"));\n@@ -358,36 +361,57 @@ void fsync_loose_object_bulk_checkin(int fd, const char *filename)\n \t}\n }\n \n-int index_blob_bulk_checkin(struct object_id *oid,\n-\t\t\t int fd, size_t size,\n+int index_blob_bulk_checkin(struct odb_transaction *transaction,\n+\t\t\t struct object_id *oid, int fd, size_t size,\n \t\t\t const char *path, unsigned flags)\n {\n-\tint status = deflate_blob_to_pack(&transaction.packfile, oid, fd, size,\n-\t\t\t\t\t path, flags);\n-\tif (!transaction.nesting)\n-\t\tflush_bulk_checkin_packfile(&transaction.packfile);\n+\tint status;\n+\n+\tif (transaction) {\n+\t\tstatus = deflate_blob_to_pack(&transaction->packfile, oid, fd,\n+\t\t\t\t\t size, path, flags);\n+\t} else {\n+\t\tstruct bulk_checkin_packfile state = { 0 };\n+\n+\t\tstatus = deflate_blob_to_pack(&state, oid, fd, size, path, flags);\n+\t\tflush_bulk_checkin_packfile(&state);\n+\t}\n+\n \treturn status;\n }\n \n-void begin_odb_transaction(void)\n+struct odb_transaction *begin_odb_transaction(struct object_database *odb)\n {\n-\ttransaction.nesting += 1;\n+\tif (!odb->transaction) {\n+\t\tCALLOC_ARRAY(odb->transaction, 1);\n+\t\todb->transaction->odb = odb;\n+\t}\n+\n+\todb->transaction->nesting += 1;\n+\n+\treturn odb->transaction;\n }\n \n-void flush_odb_transaction(void)\n+void flush_odb_transaction(struct odb_transaction *transaction)\n {\n-\tflush_batch_fsync();\n-\tflush_bulk_checkin_packfile(&transaction.packfile);\n+\tif (!transaction)\n+\t\treturn;\n+\n+\tflush_batch_fsync(transaction);\n+\tflush_bulk_checkin_packfile(&transaction->packfile);\n }\n \n-void end_odb_transaction(void)\n+void end_odb_transaction(struct odb_transaction *transaction)\n {\n-\ttransaction.nesting -= 1;\n-\tif (transaction.nesting < 0)\n+\tif (!transaction || transaction->nesting == 0)\n \t\tBUG(\"Unbalanced ODB transaction nesting\");\n \n-\tif (transaction.nesting)\n+\ttransaction->nesting -= 1;\n+\n+\tif (transaction->nesting)\n \t\treturn;\n \n-\tflush_odb_transaction();\n+\tflush_odb_transaction(transaction);\n+\ttransaction->odb->transaction = NULL;\n+\tfree(transaction);\n }\ndiff --git a/bulk-checkin.h b/bulk-checkin.h\nindex 7246ea58dcf..16254ce6a70 100644\n--- a/bulk-checkin.h\n+++ b/bulk-checkin.h\n@@ -5,9 +5,13 @@\n #define BULK_CHECKIN_H\n \n #include \"object.h\"\n+#include \"odb.h\"\n \n-void prepare_loose_object_bulk_checkin(void);\n-void fsync_loose_object_bulk_checkin(int fd, const char *filename);\n+struct odb_transaction;\n+\n+void prepare_loose_object_bulk_checkin(struct odb_transaction *transaction);\n+void fsync_loose_object_bulk_checkin(struct odb_transaction *transaction,\n+\t\t\t\t int fd, const char *filename);\n \n /*\n * This creates one packfile per large blob unless bulk-checkin\n@@ -24,8 +28,8 @@ void fsync_loose_object_bulk_checkin(int fd, const char *filename);\n * binary blobs, they generally do not want to get any conversion, and\n * callers should avoid this code path when filters are requested.\n */\n-int index_blob_bulk_checkin(struct object_id *oid,\n-\t\t\t int fd, size_t size,\n+int index_blob_bulk_checkin(struct odb_transaction *transaction,\n+\t\t\t struct object_id *oid, int fd, size_t size,\n \t\t\t const char *path, unsigned flags);\n \n /*\n@@ -35,20 +39,20 @@ int index_blob_bulk_checkin(struct object_id *oid,\n * and objects are only visible after the outermost transaction\n * is complete or the transaction is flushed.\n */\n-void begin_odb_transaction(void);\n+struct odb_transaction *begin_odb_transaction(struct object_database *odb);\n \n /*\n * Make any objects that are currently part of a pending object\n * database transaction visible. It is valid to call this function\n * even if no transaction is active.\n */\n-void flush_odb_transaction(void);\n+void flush_odb_transaction(struct odb_transaction *transaction);\n \n /*\n * Tell the object database to make any objects from the\n * current transaction visible if this is the final nested\n * transaction.\n */\n-void end_odb_transaction(void);\n+void end_odb_transaction(struct odb_transaction *transaction);\n \n #endif\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 66ef2becbe0..d225554eedd 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -474,6 +474,7 @@ static int update_one(struct cache_tree *it,\n \n int cache_tree_update(struct index_state *istate, int flags)\n {\n+\tstruct odb_transaction *transaction;\n \tint skip, i;\n \n \ti = verify_cache(istate, flags);\n@@ -489,10 +490,10 @@ int cache_tree_update(struct index_state *istate, int flags)\n \n \ttrace_performance_enter();\n \ttrace2_region_enter(\"cache_tree\", \"update\", the_repository);\n-\tbegin_odb_transaction();\n+\ttransaction = begin_odb_transaction(the_repository->objects);\n \ti = update_one(istate->cache_tree, istate->cache, istate->cache_nr,\n \t\t \"\", 0, &skip, flags);\n-\tend_odb_transaction();\n+\tend_odb_transaction(transaction);\n \ttrace2_region_leave(\"cache_tree\", \"update\", the_repository);\n \ttrace_performance_leave(\"cache_tree_update\");\n \tif (i < 0)\ndiff --git a/object-file.c b/object-file.c\nindex 2bc36ab3ee8..1740aa2b2e3 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -674,7 +674,7 @@ static void close_loose_object(struct odb_source *source,\n \t\tgoto out;\n \n \tif (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))\n-\t\tfsync_loose_object_bulk_checkin(fd, filename);\n+\t\tfsync_loose_object_bulk_checkin(source->odb->transaction, fd, filename);\n \telse if (fsync_object_files > 0)\n \t\tfsync_or_die(fd, filename);\n \telse\n@@ -852,7 +852,7 @@ static int write_loose_object(struct odb_source *source,\n \tstatic struct strbuf filename = STRBUF_INIT;\n \n \tif (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))\n-\t\tprepare_loose_object_bulk_checkin();\n+\t\tprepare_loose_object_bulk_checkin(source->odb->transaction);\n \n \todb_loose_path(source, &filename, oid);\n \n@@ -941,7 +941,7 @@ int stream_loose_object(struct odb_source *source,\n \tint hdrlen;\n \n \tif (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))\n-\t\tprepare_loose_object_bulk_checkin();\n+\t\tprepare_loose_object_bulk_checkin(source->odb->transaction);\n \n \t/* Since oid is not determined, save tmp file to odb path. */\n \tstrbuf_addf(&filename, \"%s/\", source->path);\n@@ -1263,8 +1263,9 @@ int index_fd(struct index_state *istate, struct object_id *oid,\n \t\tret = index_core(istate, oid, fd, xsize_t(st->st_size),\n \t\t\t\t type, path, flags);\n \telse\n-\t\tret = index_blob_bulk_checkin(oid, fd, xsize_t(st->st_size), path,\n-\t\t\t\t\t flags);\n+\t\tret = index_blob_bulk_checkin(the_repository->objects->transaction,\n+\t\t\t\t\t oid, fd, xsize_t(st->st_size),\n+\t\t\t\t\t path, flags);\n \tclose(fd);\n \treturn ret;\n }\ndiff --git a/odb.h b/odb.h\nindex 3dfc66d75a3..a89b2143909 100644\n--- a/odb.h\n+++ b/odb.h\n@@ -84,6 +84,7 @@ struct odb_source {\n \n struct packed_git;\n struct cached_object_entry;\n+struct odb_transaction;\n \n /*\n * The object database encapsulates access to objects in a repository. It\n@@ -94,6 +95,13 @@ struct object_database {\n \t/* Repository that owns this database. */\n \tstruct repository *repo;\n \n+\t/*\n+\t * State of current current object database transaction. Only one\n+\t * transaction may be pending at a time. Is NULL when no transaction is\n+\t * configured.\n+\t */\n+\tstruct odb_transaction *transaction;\n+\n \t/*\n \t * Set of all object directories; the main directory is first (and\n \t * cannot be NULL after initialization). Subsequent directories are\ndiff --git a/read-cache.c b/read-cache.c\nindex 06ad74db228..229b8ef11c9 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -3947,6 +3947,7 @@ int add_files_to_cache(struct repository *repo, const char *prefix,\n \t\t const struct pathspec *pathspec, char *ps_matched,\n \t\t int include_sparse, int flags)\n {\n+\tstruct odb_transaction *transaction;\n \tstruct update_callback_data data;\n \tstruct rev_info rev;\n \n@@ -3972,9 +3973,9 @@ int add_files_to_cache(struct repository *repo, const char *prefix,\n \t * This function is invoked from commands other than 'add', which\n \t * may not have their own transaction active.\n \t */\n-\tbegin_odb_transaction();\n+\ttransaction = begin_odb_transaction(repo->objects);\n \trun_diff_files(&rev, DIFF_RACY_IS_MODIFIED);\n-\tend_odb_transaction();\n+\tend_odb_transaction(transaction);\n \n \trelease_revisions(&rev);\n \treturn !!data.add_errors;\n-- \n2.51.0\n\n"},{"id":"524587","messageId":"20250820225531.1212935-4-jltobler@gmail.com","threadId":"64002","inReplyTo":"20250820225531.1212935-1-jltobler@gmail.com","subject":"[PATCH 3/3] bulk-checkin: wire repository variable","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-20T22:55:31Z","receivedAt":"2025-08-20T22:55:49Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"The bulk-checkin subsystem depends on `the_repository`. Adapt functions\nand call sites to wire the repository variable where needed. The\n`USE_THE_REPOSITORY_VARIBALE` is still required as the\n`pack_compression_level` and `pack_size_limit_cfg` globals are still\nused.\n\nSigned-off-by: Justin Tobler \n---\n bulk-checkin.c | 58 ++++++++++++++++++++++++++++----------------------\n bulk-checkin.h | 3 ++-\n object-file.c | 3 ++-\n 3 files changed, 37 insertions(+), 27 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 53a20a2d92f..a1185883837 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -38,7 +38,8 @@ struct odb_transaction {\n \tstruct bulk_checkin_packfile packfile;\n };\n \n-static void finish_tmp_packfile(struct strbuf *basename,\n+static void finish_tmp_packfile(struct repository *repo,\n+\t\t\t\tstruct strbuf *basename,\n \t\t\t\tconst char *pack_tmp_name,\n \t\t\t\tstruct pack_idx_entry **written_list,\n \t\t\t\tuint32_t nr_written,\n@@ -47,15 +48,16 @@ static void finish_tmp_packfile(struct strbuf *basename,\n {\n \tchar *idx_tmp_name = NULL;\n \n-\tstage_tmp_packfiles(the_repository, basename, pack_tmp_name,\n+\tstage_tmp_packfiles(repo, basename, pack_tmp_name,\n \t\t\t written_list, nr_written, NULL, pack_idx_opts, hash,\n \t\t\t &idx_tmp_name);\n-\trename_tmp_packfile_idx(the_repository, basename, &idx_tmp_name);\n+\trename_tmp_packfile_idx(repo, basename, &idx_tmp_name);\n \n \tfree(idx_tmp_name);\n }\n \n-static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n+static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state,\n+\t\t\t\t\tstruct repository *repo)\n {\n \tunsigned char hash[GIT_MAX_RAWSZ];\n \tstruct strbuf packname = STRBUF_INIT;\n@@ -73,15 +75,15 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n \t\t\t\t CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);\n \t} else {\n \t\tint fd = finalize_hashfile(state->f, hash, FSYNC_COMPONENT_PACK, 0);\n-\t\tfixup_pack_header_footer(the_hash_algo, fd, hash, state->pack_tmp_name,\n+\t\tfixup_pack_header_footer(repo->hash_algo, fd, hash, state->pack_tmp_name,\n \t\t\t\t\t state->nr_written, hash,\n \t\t\t\t\t state->offset);\n \t\tclose(fd);\n \t}\n \n-\tstrbuf_addf(&packname, \"%s/pack/pack-%s.\", repo_get_object_directory(the_repository),\n-\t\t hash_to_hex(hash));\n-\tfinish_tmp_packfile(&packname, state->pack_tmp_name,\n+\tstrbuf_addf(&packname, \"%s/pack/pack-%s.\", repo_get_object_directory(repo),\n+\t\t hash_to_hex_algop(hash, repo->hash_algo));\n+\tfinish_tmp_packfile(repo, &packname, state->pack_tmp_name,\n \t\t\t state->written, state->nr_written,\n \t\t\t &state->pack_idx_opts, hash);\n \tfor (uint32_t i = 0; i < state->nr_written; i++)\n@@ -94,7 +96,7 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n \n \tstrbuf_release(&packname);\n \t/* Make objects we just wrote available to ourselves */\n-\treprepare_packed_git(the_repository);\n+\treprepare_packed_git(repo);\n }\n \n /*\n@@ -117,7 +119,8 @@ static void flush_batch_fsync(struct odb_transaction *transaction)\n \t * to ensure that the data in each new object file is durable before\n \t * the final name is visible.\n \t */\n-\tstrbuf_addf(&temp_path, \"%s/bulk_fsync_XXXXXX\", repo_get_object_directory(the_repository));\n+\tstrbuf_addf(&temp_path, \"%s/bulk_fsync_XXXXXX\",\n+\t\t repo_get_object_directory(transaction->odb->repo));\n \ttemp = xmks_tempfile(temp_path.buf);\n \tfsync_or_die(get_tempfile_fd(temp), get_tempfile_path(temp));\n \tdelete_tempfile(&temp);\n@@ -131,10 +134,11 @@ static void flush_batch_fsync(struct odb_transaction *transaction)\n \ttransaction->objdir = NULL;\n }\n \n-static int already_written(struct bulk_checkin_packfile *state, struct object_id *oid)\n+static int already_written(struct bulk_checkin_packfile *state,\n+\t\t\t struct repository *repo, struct object_id *oid)\n {\n \t/* The object may already exist in the repository */\n-\tif (odb_has_object(the_repository->objects, oid,\n+\tif (odb_has_object(repo->objects, oid,\n \t\t\t HAS_OBJECT_RECHECK_PACKED | HAS_OBJECT_FETCH_PROMISOR))\n \t\treturn 1;\n \n@@ -240,12 +244,13 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n \n /* Lazily create backing packfile for the state */\n static void prepare_to_stream(struct bulk_checkin_packfile *state,\n+\t\t\t struct repository *repo,\n \t\t\t unsigned flags)\n {\n \tif (!(flags & INDEX_WRITE_OBJECT) || state->f)\n \t\treturn;\n \n-\tstate->f = create_tmp_packfile(the_repository, &state->pack_tmp_name);\n+\tstate->f = create_tmp_packfile(repo, &state->pack_tmp_name);\n \treset_pack_idx_option(&state->pack_idx_opts);\n \n \t/* Pretend we are going to write only one object */\n@@ -255,6 +260,7 @@ static void prepare_to_stream(struct bulk_checkin_packfile *state,\n }\n \n static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n+\t\t\t\tstruct repository *repo,\n \t\t\t\tstruct object_id *result_oid,\n \t\t\t\tint fd, size_t size,\n \t\t\t\tconst char *path, unsigned flags)\n@@ -272,21 +278,21 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \n \theader_len = format_object_header((char *)obuf, sizeof(obuf),\n \t\t\t\t\t OBJ_BLOB, size);\n-\tthe_hash_algo->init_fn(&ctx);\n+\trepo->hash_algo->init_fn(&ctx);\n \tgit_hash_update(&ctx, obuf, header_len);\n \n \t/* Note: idx is non-NULL when we are writing */\n \tif ((flags & INDEX_WRITE_OBJECT) != 0) {\n \t\tCALLOC_ARRAY(idx, 1);\n \n-\t\tprepare_to_stream(state, flags);\n+\t\tprepare_to_stream(state, repo, flags);\n \t\thashfile_checkpoint_init(state->f, &checkpoint);\n \t}\n \n \talready_hashed_to = 0;\n \n \twhile (1) {\n-\t\tprepare_to_stream(state, flags);\n+\t\tprepare_to_stream(state, repo, flags);\n \t\tif (idx) {\n \t\t\thashfile_checkpoint(state->f, &checkpoint);\n \t\t\tidx->offset = state->offset;\n@@ -304,7 +310,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \t\t\tBUG(\"should not happen\");\n \t\thashfile_truncate(state->f, &checkpoint);\n \t\tstate->offset = checkpoint.offset;\n-\t\tflush_bulk_checkin_packfile(state);\n+\t\tflush_bulk_checkin_packfile(state, repo);\n \t\tif (lseek(fd, seekback, SEEK_SET) == (off_t) -1)\n \t\t\treturn error(\"cannot seek back\");\n \t}\n@@ -313,7 +319,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \t\treturn 0;\n \n \tidx->crc32 = crc32_end(state->f);\n-\tif (already_written(state, result_oid)) {\n+\tif (already_written(state, repo, result_oid)) {\n \t\thashfile_truncate(state->f, &checkpoint);\n \t\tstate->offset = checkpoint.offset;\n \t\tfree(idx);\n@@ -338,7 +344,7 @@ void prepare_loose_object_bulk_checkin(struct odb_transaction *transaction)\n \tif (!transaction || transaction->objdir)\n \t\treturn;\n \n-\ttransaction->objdir = tmp_objdir_create(the_repository, \"bulk-fsync\");\n+\ttransaction->objdir = tmp_objdir_create(transaction->odb->repo, \"bulk-fsync\");\n \tif (transaction->objdir)\n \t\ttmp_objdir_replace_primary_odb(transaction->objdir, 0);\n }\n@@ -361,20 +367,21 @@ void fsync_loose_object_bulk_checkin(struct odb_transaction *transaction,\n \t}\n }\n \n-int index_blob_bulk_checkin(struct odb_transaction *transaction,\n+int index_blob_bulk_checkin(struct repository *repo,\n+\t\t\t struct odb_transaction *transaction,\n \t\t\t struct object_id *oid, int fd, size_t size,\n \t\t\t const char *path, unsigned flags)\n {\n \tint status;\n \n \tif (transaction) {\n-\t\tstatus = deflate_blob_to_pack(&transaction->packfile, oid, fd,\n-\t\t\t\t\t size, path, flags);\n+\t\tstatus = deflate_blob_to_pack(&transaction->packfile,\n+\t\t\t\t\t repo, oid, fd, size, path, flags);\n \t} else {\n \t\tstruct bulk_checkin_packfile state = { 0 };\n \n-\t\tstatus = deflate_blob_to_pack(&state, oid, fd, size, path, flags);\n-\t\tflush_bulk_checkin_packfile(&state);\n+\t\tstatus = deflate_blob_to_pack(&state, repo, oid, fd, size, path, flags);\n+\t\tflush_bulk_checkin_packfile(&state, repo);\n \t}\n \n \treturn status;\n@@ -398,7 +405,8 @@ void flush_odb_transaction(struct odb_transaction *transaction)\n \t\treturn;\n \n \tflush_batch_fsync(transaction);\n-\tflush_bulk_checkin_packfile(&transaction->packfile);\n+\tflush_bulk_checkin_packfile(&transaction->packfile,\n+\t\t\t\t transaction->odb->repo);\n }\n \n void end_odb_transaction(struct odb_transaction *transaction)\ndiff --git a/bulk-checkin.h b/bulk-checkin.h\nindex 16254ce6a70..ac8dbf3523f 100644\n--- a/bulk-checkin.h\n+++ b/bulk-checkin.h\n@@ -28,7 +28,8 @@ void fsync_loose_object_bulk_checkin(struct odb_transaction *transaction,\n * binary blobs, they generally do not want to get any conversion, and\n * callers should avoid this code path when filters are requested.\n */\n-int index_blob_bulk_checkin(struct odb_transaction *transaction,\n+int index_blob_bulk_checkin(struct repository *repo,\n+\t\t\t struct odb_transaction *transaction,\n \t\t\t struct object_id *oid, int fd, size_t size,\n \t\t\t const char *path, unsigned flags);\n \ndiff --git a/object-file.c b/object-file.c\nindex 1740aa2b2e3..35f33e466c2 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1263,7 +1263,8 @@ int index_fd(struct index_state *istate, struct object_id *oid,\n \t\tret = index_core(istate, oid, fd, xsize_t(st->st_size),\n \t\t\t\t type, path, flags);\n \telse\n-\t\tret = index_blob_bulk_checkin(the_repository->objects->transaction,\n+\t\tret = index_blob_bulk_checkin(the_repository,\n+\t\t\t\t\t the_repository->objects->transaction,\n \t\t\t\t\t oid, fd, xsize_t(st->st_size),\n \t\t\t\t\t path, flags);\n \tclose(fd);\n-- \n2.51.0\n\n"},{"id":"524591","messageId":"xmqqcy8p7ddg.fsf@gitster.g","threadId":"64002","inReplyTo":"20250820225531.1212935-1-jltobler@gmail.com","subject":"Re: [PATCH 0/3] bulk-checkin: remove global transaction state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-21T00:00:11Z","receivedAt":"2025-08-21T00:00:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler writes:\n\n> The bulk-checkin subsystem provides an interface to write objects to the\n> object database in a bulk transaction. The state of an ongoing\n> transaction is stored across several global variables. This series aims\n> to remove this global transaction state in favor of storing state in in\n> `struct object_database`. This is done in preparation for a follow-up\n> change where the goal is to eventually move these transaction interfaces\n> into \"odb.h\".\n\nWonderful. I admit these global variables are left-over dropping of\nmy making. Thanks for cleaning after me ;-)\n\n"},{"id":"524592","messageId":"xmqq8qjd7coe.fsf@gitster.g","threadId":"64002","inReplyTo":"20250820225531.1212935-4-jltobler@gmail.com","subject":"Re: [PATCH 3/3] bulk-checkin: wire repository variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-21T00:15:13Z","receivedAt":"2025-08-21T00:15:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler writes:\n\n> The bulk-checkin subsystem depends on `the_repository`. Adapt functions\n> and call sites to wire the repository variable where needed. The\n> `USE_THE_REPOSITORY_VARIBALE` is still required as the\n> `pack_compression_level` and `pack_size_limit_cfg` globals are still\n> used.\n\nHmph.\n\nI somehow expected that odb would know what repository it was\ninstanciated to work with, or in the worst case where in-core odb is\nin theory sharable among multiple in-core repositories, at least\nbegin_odb_transaction() would take pair and the\ntransaction would know for which repository the transaction is\nworking for.\n\nDo we need bulk_checkin_packfile as a separate structure and pass it\naround, or would these internal functions be better off passing an\ninstance of odb_transaction around and learn the repository from\nodb->repo?\n"},{"id":"524683","messageId":"rhkhmaozjaogm34xfwagagy2qifsq5p4fbtb23nhdu7jfg2627@qkrv57gg2k3u","threadId":"64002","inReplyTo":"xmqq8qjd7coe.fsf@gitster.g","subject":"Re: [PATCH 3/3] bulk-checkin: wire repository variable","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-21T20:26:35Z","receivedAt":"2025-08-21T20:26:37Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/08/20 05:15PM, Junio C Hamano wrote:\n> I somehow expected that odb would know what repository it was\n> instanciated to work with, or in the worst case where in-core odb is\n> in theory sharable among multiple in-core repositories, at least\n> begin_odb_transaction() would take pair and the\n> transaction would know for which repository the transaction is\n> working for.\n\nThe `struct odb_transaction` maintains a pointer to its parent `struct\nobject_database`. This ODB also has a pointer to the `struct\nrepository`. Thus, from an ongoing transaction we should be able to get\nthe repository we want.\n\nFor `begin_odb_transaction()` we should only have to provide `struct\nobject_database` and from that the transaction can access the\ncorresponding repository. In the general cases, this is exactly what we\ndo, but this doesn't work for `index_blob_bulk_checkin()` and its\naccompanying internal functions as they may be invoked from outside of a\ntransaction.\n\n> Do we need bulk_checkin_packfile as a separate structure and pass it\n> around, or would these internal functions be better off passing an\n> instance of odb_transaction around and learn the repository from\n> odb->repo?\n\nIn its current form, the bulk-checkin mechanism for packfiles may be\nused outside of transactions. For example when calling\n`index_blob_bulk_checkin()` without a transaction, the single object is\nwritten to the packfile moved into the primary ODB. With a transaction,\nmultiple objects may be written to a single packfile. This whole setup\nis rather awkward though.\n\nThinking about this more, we should probably just require\n`index_blob_bulk_checkin()` be provided a transaction. Callers will need\nto ensure a transaction is running so that a `struct\nbulk_checkin_packfile` gets set up, but this shouldn't be a big deal.\nWith this we could easily just propagate the transaction for all these\nfunction as you suggested.\n\nI'll do this in the next version. Thanks!\n\n-Justin\n"},{"id":"524684","messageId":"xmqqms7sza95.fsf@gitster.g","threadId":"64002","inReplyTo":"rhkhmaozjaogm34xfwagagy2qifsq5p4fbtb23nhdu7jfg2627@qkrv57gg2k3u","subject":"Re: [PATCH 3/3] bulk-checkin: wire repository variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-21T20:32:22Z","receivedAt":"2025-08-21T20:32:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler writes:\n\n> Thinking about this more, we should probably just require\n> `index_blob_bulk_checkin()` be provided a transaction. Callers will need\n> to ensure a transaction is running so that a `struct\n> bulk_checkin_packfile` gets set up, but this shouldn't be a big deal.\n\nThanks for thinking this through. I think reducing the number of\noddball callers-from-sideways leads us to good code hygiene.\n\n> With this we could easily just propagate the transaction for all these\n> function as you suggested.\n>\n> I'll do this in the next version. Thanks!\n>\n> -Justin\n"},{"id":"524693","messageId":"20250821232249.319427-1-jltobler@gmail.com","threadId":"64002","inReplyTo":"20250820225531.1212935-1-jltobler@gmail.com","subject":"[PATCH v2 0/4] bulk-checkin: remove global transaction state","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-21T23:22:45Z","receivedAt":"2025-08-21T23:22:55Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Greetings,\n\nThe bulk-checkin subsystem provides an interface to write objects to the\nobject database in a bulk transaction. The state of an ongoing\ntransaction is stored across several global variables. This series aims\nto remove this global transaction state in favor of storing state in in\n`struct object_database`. This is done in preparation for a follow-up\nchange where the goal is to eventually move these transaction interfaces\ninto \"odb.h\".\n\nChanges since V1:\n\n- `index_blob_bulk_checkin()` now assumes that the caller always\n provides a setup `struct odb_transaction`. Callers are adjusted to\n ensure this.\n- Functions in bulk-checkin.c now consistently access the repository\n through the provided `odb_transaction`.\n\nThanks,\n-Justin\n\nJustin Tobler (4):\n bulk-checkin: introduce object database transaction structure\n bulk-checkin: remove global transaction state\n bulk-checkin: require transaction for index_blob_bulk_checkin()\n bulk-checkin: use repository variable from transaction\n\n builtin/add.c | 5 +-\n builtin/unpack-objects.c | 5 +-\n builtin/update-index.c | 7 +-\n bulk-checkin.c | 153 +++++++++++++++++++++------------------\n bulk-checkin.h | 25 ++++---\n cache-tree.c | 5 +-\n object-file.c | 30 +++++---\n odb.h | 8 ++\n read-cache.c | 5 +-\n 9 files changed, 142 insertions(+), 101 deletions(-)\n\nRange-diff against v1:\n1: 5c9358e0b03 = 1: 5c9358e0b03 bulk-checkin: introduce object database transaction structure\n2: 4a1b80a6baf = 2: 4a1b80a6baf bulk-checkin: remove global transaction state\n-: ----------- > 3: ce329932fdd bulk-checkin: require transaction for index_blob_bulk_checkin()\n3: 2ca78c8d343 ! 4: 08e26647915 bulk-checkin: wire repository variable\n @@ Metadata\n Author: Justin Tobler \n \n ## Commit message ##\n - bulk-checkin: wire repository variable\n + bulk-checkin: use repository variable from transaction\n \n The bulk-checkin subsystem depends on `the_repository`. Adapt functions\n - and call sites to wire the repository variable where needed. The\n - `USE_THE_REPOSITORY_VARIBALE` is still required as the\n + and call sites to access the repository through `struct odb_transaction`\n + instead. The `USE_THE_REPOSITORY_VARIBALE` is still required as the\n `pack_compression_level` and `pack_size_limit_cfg` globals are still\n used.\n \n @@ bulk-checkin.c: struct odb_transaction {\n };\n \n -static void finish_tmp_packfile(struct strbuf *basename,\n -+static void finish_tmp_packfile(struct repository *repo,\n +-\t\t\t\tconst char *pack_tmp_name,\n +-\t\t\t\tstruct pack_idx_entry **written_list,\n +-\t\t\t\tuint32_t nr_written,\n +-\t\t\t\tstruct pack_idx_option *pack_idx_opts,\n ++static void finish_tmp_packfile(struct odb_transaction *transaction,\n +\t\t\t\tstruct strbuf *basename,\n - \t\t\t\tconst char *pack_tmp_name,\n - \t\t\t\tstruct pack_idx_entry **written_list,\n - \t\t\t\tuint32_t nr_written,\n -@@ bulk-checkin.c: static void finish_tmp_packfile(struct strbuf *basename,\n + \t\t\t\tunsigned char hash[])\n {\n ++\tstruct bulk_checkin_packfile *state = &transaction->packfile;\n ++\tstruct repository *repo = transaction->odb->repo;\n \tchar *idx_tmp_name = NULL;\n \n -\tstage_tmp_packfiles(the_repository, basename, pack_tmp_name,\n -+\tstage_tmp_packfiles(repo, basename, pack_tmp_name,\n - \t\t\t written_list, nr_written, NULL, pack_idx_opts, hash,\n - \t\t\t &idx_tmp_name);\n +-\t\t\t written_list, nr_written, NULL, pack_idx_opts, hash,\n +-\t\t\t &idx_tmp_name);\n -\trename_tmp_packfile_idx(the_repository, basename, &idx_tmp_name);\n ++\tstage_tmp_packfiles(repo, basename, state->pack_tmp_name,\n ++\t\t\t state->written, state->nr_written, NULL,\n ++\t\t\t &state->pack_idx_opts, hash, &idx_tmp_name);\n +\trename_tmp_packfile_idx(repo, basename, &idx_tmp_name);\n \n \tfree(idx_tmp_name);\n }\n \n -static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n -+static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state,\n -+\t\t\t\t\tstruct repository *repo)\n ++static void flush_bulk_checkin_packfile(struct odb_transaction *transaction)\n {\n ++\tstruct bulk_checkin_packfile *state = &transaction->packfile;\n ++\tstruct repository *repo = transaction->odb->repo;\n \tunsigned char hash[GIT_MAX_RAWSZ];\n \tstruct strbuf packname = STRBUF_INIT;\n + \n @@ bulk-checkin.c: static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n \t\t\t\t CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);\n \t} else {\n @@ bulk-checkin.c: static void flush_bulk_checkin_packfile(struct bulk_checkin_pack\n -\tstrbuf_addf(&packname, \"%s/pack/pack-%s.\", repo_get_object_directory(the_repository),\n -\t\t hash_to_hex(hash));\n -\tfinish_tmp_packfile(&packname, state->pack_tmp_name,\n -+\tstrbuf_addf(&packname, \"%s/pack/pack-%s.\", repo_get_object_directory(repo),\n +-\t\t\t state->written, state->nr_written,\n +-\t\t\t &state->pack_idx_opts, hash);\n ++\tstrbuf_addf(&packname, \"%s/pack/pack-%s.\",\n ++\t\t transaction->odb->sources->path,\n +\t\t hash_to_hex_algop(hash, repo->hash_algo));\n -+\tfinish_tmp_packfile(repo, &packname, state->pack_tmp_name,\n - \t\t\t state->written, state->nr_written,\n - \t\t\t &state->pack_idx_opts, hash);\n ++\n ++\tfinish_tmp_packfile(transaction, &packname, hash);\n \tfor (uint32_t i = 0; i < state->nr_written; i++)\n + \t\tfree(state->written[i]);\n + \n @@ bulk-checkin.c: static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n \n \tstrbuf_release(&packname);\n @@ bulk-checkin.c: static void flush_batch_fsync(struct odb_transaction *transactio\n \t */\n -\tstrbuf_addf(&temp_path, \"%s/bulk_fsync_XXXXXX\", repo_get_object_directory(the_repository));\n +\tstrbuf_addf(&temp_path, \"%s/bulk_fsync_XXXXXX\",\n -+\t\t repo_get_object_directory(transaction->odb->repo));\n ++\t\t transaction->odb->sources->path);\n \ttemp = xmks_tempfile(temp_path.buf);\n \tfsync_or_die(get_tempfile_fd(temp), get_tempfile_path(temp));\n \tdelete_tempfile(&temp);\n @@ bulk-checkin.c: static void flush_batch_fsync(struct odb_transaction *transactio\n }\n \n -static int already_written(struct bulk_checkin_packfile *state, struct object_id *oid)\n -+static int already_written(struct bulk_checkin_packfile *state,\n -+\t\t\t struct repository *repo, struct object_id *oid)\n ++static int already_written(struct odb_transaction *transaction,\n ++\t\t\t struct object_id *oid)\n {\n \t/* The object may already exist in the repository */\n -\tif (odb_has_object(the_repository->objects, oid,\n -+\tif (odb_has_object(repo->objects, oid,\n ++\tif (odb_has_object(transaction->odb, oid,\n \t\t\t HAS_OBJECT_RECHECK_PACKED | HAS_OBJECT_FETCH_PROMISOR))\n \t\treturn 1;\n \n + \t/* Might want to keep the list sorted */\n +-\tfor (uint32_t i = 0; i < state->nr_written; i++)\n +-\t\tif (oideq(&state->written[i]->oid, oid))\n ++\tfor (uint32_t i = 0; i < transaction->packfile.nr_written; i++)\n ++\t\tif (oideq(&transaction->packfile.written[i]->oid, oid))\n + \t\t\treturn 1;\n + \n + \t/* This is a new object we need to keep */\n @@ bulk-checkin.c: static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n + }\n \n /* Lazily create backing packfile for the state */\n - static void prepare_to_stream(struct bulk_checkin_packfile *state,\n -+\t\t\t struct repository *repo,\n +-static void prepare_to_stream(struct bulk_checkin_packfile *state,\n ++static void prepare_to_stream(struct odb_transaction *transaction,\n \t\t\t unsigned flags)\n {\n ++\tstruct bulk_checkin_packfile *state = &transaction->packfile;\n \tif (!(flags & INDEX_WRITE_OBJECT) || state->f)\n \t\treturn;\n \n -\tstate->f = create_tmp_packfile(the_repository, &state->pack_tmp_name);\n -+\tstate->f = create_tmp_packfile(repo, &state->pack_tmp_name);\n ++\tstate->f = create_tmp_packfile(transaction->odb->repo,\n ++\t\t\t\t &state->pack_tmp_name);\n \treset_pack_idx_option(&state->pack_idx_opts);\n \n \t/* Pretend we are going to write only one object */\n @@ bulk-checkin.c: static void prepare_to_stream(struct bulk_checkin_packfile *state,\n + \t\tdie_errno(\"unable to write pack header\");\n }\n \n - static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n -+\t\t\t\tstruct repository *repo,\n - \t\t\t\tstruct object_id *result_oid,\n - \t\t\t\tint fd, size_t size,\n - \t\t\t\tconst char *path, unsigned flags)\n +-static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n +-\t\t\t\tstruct object_id *result_oid,\n +-\t\t\t\tint fd, size_t size,\n +-\t\t\t\tconst char *path, unsigned flags)\n ++int index_blob_bulk_checkin(struct odb_transaction *transaction,\n ++\t\t\t struct object_id *result_oid,\n ++\t\t\t int fd, size_t size,\n ++\t\t\t const char *path, unsigned flags)\n + {\n ++\tstruct bulk_checkin_packfile *state = &transaction->packfile;\n + \toff_t seekback, already_hashed_to;\n + \tstruct git_hash_ctx ctx;\n + \tunsigned char obuf[16384];\n @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \n \theader_len = format_object_header((char *)obuf, sizeof(obuf),\n \t\t\t\t\t OBJ_BLOB, size);\n -\tthe_hash_algo->init_fn(&ctx);\n -+\trepo->hash_algo->init_fn(&ctx);\n ++\ttransaction->odb->repo->hash_algo->init_fn(&ctx);\n \tgit_hash_update(&ctx, obuf, header_len);\n \n \t/* Note: idx is non-NULL when we are writing */\n @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st\n \t\tCALLOC_ARRAY(idx, 1);\n \n -\t\tprepare_to_stream(state, flags);\n -+\t\tprepare_to_stream(state, repo, flags);\n ++\t\tprepare_to_stream(transaction, flags);\n \t\thashfile_checkpoint_init(state->f, &checkpoint);\n \t}\n \n @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st\n \n \twhile (1) {\n -\t\tprepare_to_stream(state, flags);\n -+\t\tprepare_to_stream(state, repo, flags);\n ++\t\tprepare_to_stream(transaction, flags);\n \t\tif (idx) {\n \t\t\thashfile_checkpoint(state->f, &checkpoint);\n \t\t\tidx->offset = state->offset;\n @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st\n \t\thashfile_truncate(state->f, &checkpoint);\n \t\tstate->offset = checkpoint.offset;\n -\t\tflush_bulk_checkin_packfile(state);\n -+\t\tflush_bulk_checkin_packfile(state, repo);\n ++\t\tflush_bulk_checkin_packfile(transaction);\n \t\tif (lseek(fd, seekback, SEEK_SET) == (off_t) -1)\n \t\t\treturn error(\"cannot seek back\");\n \t}\n @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st\n \n \tidx->crc32 = crc32_end(state->f);\n -\tif (already_written(state, result_oid)) {\n -+\tif (already_written(state, repo, result_oid)) {\n ++\tif (already_written(transaction, result_oid)) {\n \t\thashfile_truncate(state->f, &checkpoint);\n \t\tstate->offset = checkpoint.offset;\n \t\tfree(idx);\n @@ bulk-checkin.c: void fsync_loose_object_bulk_checkin(struct odb_transaction *tra\n }\n \n -int index_blob_bulk_checkin(struct odb_transaction *transaction,\n -+int index_blob_bulk_checkin(struct repository *repo,\n -+\t\t\t struct odb_transaction *transaction,\n - \t\t\t struct object_id *oid, int fd, size_t size,\n - \t\t\t const char *path, unsigned flags)\n +-\t\t\t struct object_id *oid, int fd, size_t size,\n +-\t\t\t const char *path, unsigned flags)\n +-{\n +-\treturn deflate_blob_to_pack(&transaction->packfile, oid, fd, size, path,\n +-\t\t\t\t flags);\n +-}\n +-\n + struct odb_transaction *begin_odb_transaction(struct object_database *odb)\n {\n - \tint status;\n - \n - \tif (transaction) {\n --\t\tstatus = deflate_blob_to_pack(&transaction->packfile, oid, fd,\n --\t\t\t\t\t size, path, flags);\n -+\t\tstatus = deflate_blob_to_pack(&transaction->packfile,\n -+\t\t\t\t\t repo, oid, fd, size, path, flags);\n - \t} else {\n - \t\tstruct bulk_checkin_packfile state = { 0 };\n - \n --\t\tstatus = deflate_blob_to_pack(&state, oid, fd, size, path, flags);\n --\t\tflush_bulk_checkin_packfile(&state);\n -+\t\tstatus = deflate_blob_to_pack(&state, repo, oid, fd, size, path, flags);\n -+\t\tflush_bulk_checkin_packfile(&state, repo);\n - \t}\n - \n - \treturn status;\n + \tif (!odb->transaction) {\n @@ bulk-checkin.c: void flush_odb_transaction(struct odb_transaction *transaction)\n \t\treturn;\n \n \tflush_batch_fsync(transaction);\n -\tflush_bulk_checkin_packfile(&transaction->packfile);\n -+\tflush_bulk_checkin_packfile(&transaction->packfile,\n -+\t\t\t\t transaction->odb->repo);\n ++\tflush_bulk_checkin_packfile(transaction);\n }\n \n void end_odb_transaction(struct odb_transaction *transaction)\n -\n - ## bulk-checkin.h ##\n -@@ bulk-checkin.h: void fsync_loose_object_bulk_checkin(struct odb_transaction *transaction,\n - * binary blobs, they generally do not want to get any conversion, and\n - * callers should avoid this code path when filters are requested.\n - */\n --int index_blob_bulk_checkin(struct odb_transaction *transaction,\n -+int index_blob_bulk_checkin(struct repository *repo,\n -+\t\t\t struct odb_transaction *transaction,\n - \t\t\t struct object_id *oid, int fd, size_t size,\n - \t\t\t const char *path, unsigned flags);\n - \n -\n - ## object-file.c ##\n -@@ object-file.c: int index_fd(struct index_state *istate, struct object_id *oid,\n - \t\tret = index_core(istate, oid, fd, xsize_t(st->st_size),\n - \t\t\t\t type, path, flags);\n - \telse\n --\t\tret = index_blob_bulk_checkin(the_repository->objects->transaction,\n -+\t\tret = index_blob_bulk_checkin(the_repository,\n -+\t\t\t\t\t the_repository->objects->transaction,\n - \t\t\t\t\t oid, fd, xsize_t(st->st_size),\n - \t\t\t\t\t path, flags);\n - \tclose(fd);\n\nbase-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0\n-- \n2.51.0\n\n"},{"id":"524694","messageId":"20250821232249.319427-2-jltobler@gmail.com","threadId":"64002","inReplyTo":"20250821232249.319427-1-jltobler@gmail.com","subject":"[PATCH v2 1/4] bulk-checkin: introduce object database transaction structure","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-21T23:22:46Z","receivedAt":"2025-08-21T23:22:56Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Object database transaction state is stored across several global\nvariables in the bulk-checkin subsystem. Consolidate this state into a\nsingle `struct odb_transaction` global. In a subsequent commit, the\ntransactional interfaces will be updated to wire this structure instead\nof relying on a global variable.\n\nSigned-off-by: Justin Tobler \n---\n bulk-checkin.c | 46 ++++++++++++++++++++++++----------------------\n 1 file changed, 24 insertions(+), 22 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex b2809ab0398..82a73da79e8 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -19,11 +19,7 @@\n #include \"object-file.h\"\n #include \"odb.h\"\n \n-static int odb_transaction_nesting;\n-\n-static struct tmp_objdir *bulk_fsync_objdir;\n-\n-static struct bulk_checkin_packfile {\n+struct bulk_checkin_packfile {\n \tchar *pack_tmp_name;\n \tstruct hashfile *f;\n \toff_t offset;\n@@ -32,7 +28,13 @@ static struct bulk_checkin_packfile {\n \tstruct pack_idx_entry **written;\n \tuint32_t alloc_written;\n \tuint32_t nr_written;\n-} bulk_checkin_packfile;\n+};\n+\n+static struct odb_transaction {\n+\tint nesting;\n+\tstruct tmp_objdir *objdir;\n+\tstruct bulk_checkin_packfile packfile;\n+} transaction;\n \n static void finish_tmp_packfile(struct strbuf *basename,\n \t\t\t\tconst char *pack_tmp_name,\n@@ -101,7 +103,7 @@ static void flush_batch_fsync(void)\n \tstruct strbuf temp_path = STRBUF_INIT;\n \tstruct tempfile *temp;\n \n-\tif (!bulk_fsync_objdir)\n+\tif (!transaction.objdir)\n \t\treturn;\n \n \t/*\n@@ -123,8 +125,8 @@ static void flush_batch_fsync(void)\n \t * Make the object files visible in the primary ODB after their data is\n \t * fully durable.\n \t */\n-\ttmp_objdir_migrate(bulk_fsync_objdir);\n-\tbulk_fsync_objdir = NULL;\n+\ttmp_objdir_migrate(transaction.objdir);\n+\ttransaction.objdir = NULL;\n }\n \n static int already_written(struct bulk_checkin_packfile *state, struct object_id *oid)\n@@ -331,12 +333,12 @@ void prepare_loose_object_bulk_checkin(void)\n \t * callers may not know whether any objects will be\n \t * added at the time they call begin_odb_transaction.\n \t */\n-\tif (!odb_transaction_nesting || bulk_fsync_objdir)\n+\tif (!transaction.nesting || transaction.objdir)\n \t\treturn;\n \n-\tbulk_fsync_objdir = tmp_objdir_create(the_repository, \"bulk-fsync\");\n-\tif (bulk_fsync_objdir)\n-\t\ttmp_objdir_replace_primary_odb(bulk_fsync_objdir, 0);\n+\ttransaction.objdir = tmp_objdir_create(the_repository, \"bulk-fsync\");\n+\tif (transaction.objdir)\n+\t\ttmp_objdir_replace_primary_odb(transaction.objdir, 0);\n }\n \n void fsync_loose_object_bulk_checkin(int fd, const char *filename)\n@@ -348,7 +350,7 @@ void fsync_loose_object_bulk_checkin(int fd, const char *filename)\n \t * before renaming the objects to their final names as part of\n \t * flush_batch_fsync.\n \t */\n-\tif (!bulk_fsync_objdir ||\n+\tif (!transaction.objdir ||\n \t git_fsync(fd, FSYNC_WRITEOUT_ONLY) < 0) {\n \t\tif (errno == ENOSYS)\n \t\t\twarning(_(\"core.fsyncMethod = batch is unsupported on this platform\"));\n@@ -360,31 +362,31 @@ int index_blob_bulk_checkin(struct object_id *oid,\n \t\t\t int fd, size_t size,\n \t\t\t const char *path, unsigned flags)\n {\n-\tint status = deflate_blob_to_pack(&bulk_checkin_packfile, oid, fd, size,\n+\tint status = deflate_blob_to_pack(&transaction.packfile, oid, fd, size,\n \t\t\t\t\t path, flags);\n-\tif (!odb_transaction_nesting)\n-\t\tflush_bulk_checkin_packfile(&bulk_checkin_packfile);\n+\tif (!transaction.nesting)\n+\t\tflush_bulk_checkin_packfile(&transaction.packfile);\n \treturn status;\n }\n \n void begin_odb_transaction(void)\n {\n-\todb_transaction_nesting += 1;\n+\ttransaction.nesting += 1;\n }\n \n void flush_odb_transaction(void)\n {\n \tflush_batch_fsync();\n-\tflush_bulk_checkin_packfile(&bulk_checkin_packfile);\n+\tflush_bulk_checkin_packfile(&transaction.packfile);\n }\n \n void end_odb_transaction(void)\n {\n-\todb_transaction_nesting -= 1;\n-\tif (odb_transaction_nesting < 0)\n+\ttransaction.nesting -= 1;\n+\tif (transaction.nesting < 0)\n \t\tBUG(\"Unbalanced ODB transaction nesting\");\n \n-\tif (odb_transaction_nesting)\n+\tif (transaction.nesting)\n \t\treturn;\n \n \tflush_odb_transaction();\n-- \n2.51.0\n\n"},{"id":"524695","messageId":"20250821232249.319427-4-jltobler@gmail.com","threadId":"64002","inReplyTo":"20250821232249.319427-1-jltobler@gmail.com","subject":"[PATCH v2 3/4] bulk-checkin: require transaction for index_blob_bulk_checkin()","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-21T23:22:48Z","receivedAt":"2025-08-21T23:22:57Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"The bulk-checkin subsystem provides a mechanism to write blobs directly\nto a packfile via `index_blob_bulk_checkin()`. If there is an ongoing\ntransaction when invoked, objects written via this function are stored\nin the same packfile. The packfile is not flushed until the transaction\nitself is flushed. If there is no transaction, the single object is\nwritten to a packfile and immediately flushed. This complicates\n`index_blob_bulk_checkin()` as it cannot reliably use the provided\ntransaction to get the associated repository.\n\nUpdate `index_blob_bulk_checkin()` to assume that a valid transaction is\nalways provided. Callers are now expected to ensure a transaction is set\nup beforehand. The single call site in `object-file.c:index_fd()` is\nupdated accordingly. Due to how `{begin,end}_odb_transaction()` handles\nnested transactions, a new transaction is only created and committed if\nthere is not already an ongoing transaction.\n\nSigned-off-by: Justin Tobler \n---\n bulk-checkin.c | 15 ++-------------\n bulk-checkin.h | 7 +++++--\n object-file.c | 21 ++++++++++++++-------\n 3 files changed, 21 insertions(+), 22 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 53a20a2d92f..0e3747640b9 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -365,19 +365,8 @@ int index_blob_bulk_checkin(struct odb_transaction *transaction,\n \t\t\t struct object_id *oid, int fd, size_t size,\n \t\t\t const char *path, unsigned flags)\n {\n-\tint status;\n-\n-\tif (transaction) {\n-\t\tstatus = deflate_blob_to_pack(&transaction->packfile, oid, fd,\n-\t\t\t\t\t size, path, flags);\n-\t} else {\n-\t\tstruct bulk_checkin_packfile state = { 0 };\n-\n-\t\tstatus = deflate_blob_to_pack(&state, oid, fd, size, path, flags);\n-\t\tflush_bulk_checkin_packfile(&state);\n-\t}\n-\n-\treturn status;\n+\treturn deflate_blob_to_pack(&transaction->packfile, oid, fd, size, path,\n+\t\t\t\t flags);\n }\n \n struct odb_transaction *begin_odb_transaction(struct object_database *odb)\ndiff --git a/bulk-checkin.h b/bulk-checkin.h\nindex 16254ce6a70..ac8887f476b 100644\n--- a/bulk-checkin.h\n+++ b/bulk-checkin.h\n@@ -14,8 +14,11 @@ void fsync_loose_object_bulk_checkin(struct odb_transaction *transaction,\n \t\t\t\t int fd, const char *filename);\n \n /*\n- * This creates one packfile per large blob unless bulk-checkin\n- * machinery is \"plugged\".\n+ * This writes the specified object to a packfile. Objects written here\n+ * during the same transaction are written to the same packfile. The\n+ * packfile is not flushed until the transaction is flushed. The caller\n+ * is expected to ensure a valid transaction is setup for objects to be\n+ * recorded to.\n *\n * This also bypasses the usual \"convert-to-git\" dance, and that is on\n * purpose. We could write a streaming version of the converting\ndiff --git a/object-file.c b/object-file.c\nindex 1740aa2b2e3..bc15af42450 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1253,19 +1253,26 @@ int index_fd(struct index_state *istate, struct object_id *oid,\n \t * Call xsize_t() only when needed to avoid potentially unnecessary\n \t * die() for large files.\n \t */\n-\tif (type == OBJ_BLOB && path && would_convert_to_git_filter_fd(istate, path))\n+\tif (type == OBJ_BLOB && path && would_convert_to_git_filter_fd(istate, path)) {\n \t\tret = index_stream_convert_blob(istate, oid, fd, path, flags);\n-\telse if (!S_ISREG(st->st_mode))\n+\t} else if (!S_ISREG(st->st_mode)) {\n \t\tret = index_pipe(istate, oid, fd, type, path, flags);\n-\telse if ((st->st_size >= 0 && (size_t) st->st_size <= repo_settings_get_big_file_threshold(istate->repo)) ||\n-\t\t type != OBJ_BLOB ||\n-\t\t (path && would_convert_to_git(istate, path)))\n+\t} else if ((st->st_size >= 0 &&\n+\t\t (size_t)st->st_size <= repo_settings_get_big_file_threshold(istate->repo)) ||\n+\t\t type != OBJ_BLOB ||\n+\t\t (path && would_convert_to_git(istate, path))) {\n \t\tret = index_core(istate, oid, fd, xsize_t(st->st_size),\n \t\t\t\t type, path, flags);\n-\telse\n-\t\tret = index_blob_bulk_checkin(the_repository->objects->transaction,\n+\t} else {\n+\t\tstruct odb_transaction *transaction;\n+\n+\t\ttransaction = begin_odb_transaction(the_repository->objects);\n+\t\tret = index_blob_bulk_checkin(transaction,\n \t\t\t\t\t oid, fd, xsize_t(st->st_size),\n \t\t\t\t\t path, flags);\n+\t\tend_odb_transaction(transaction);\n+\t}\n+\n \tclose(fd);\n \treturn ret;\n }\n-- \n2.51.0\n\n"},{"id":"524696","messageId":"20250821232249.319427-3-jltobler@gmail.com","threadId":"64002","inReplyTo":"20250821232249.319427-1-jltobler@gmail.com","subject":"[PATCH v2 2/4] bulk-checkin: remove global transaction state","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-21T23:22:47Z","receivedAt":"2025-08-21T23:22:57Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Object database transactions in the bulk-checkin subsystem rely on\nglobal state to track transaction status. Stop relying on global state\nand instead store the transaction in the `struct object_database`.\nFunctions that operate on transactions are updated to now wire\ntransaction state.\n\nSigned-off-by: Justin Tobler \n---\n builtin/add.c | 5 ++-\n builtin/unpack-objects.c | 5 ++-\n builtin/update-index.c | 7 ++--\n bulk-checkin.c | 82 ++++++++++++++++++++++++++--------------\n bulk-checkin.h | 18 +++++----\n cache-tree.c | 5 ++-\n object-file.c | 11 +++---\n odb.h | 8 ++++\n read-cache.c | 5 ++-\n 9 files changed, 94 insertions(+), 52 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 0235854f809..740c7c45817 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -389,6 +389,7 @@ int cmd_add(int argc,\n \tchar *seen = NULL;\n \tchar *ps_matched = NULL;\n \tstruct lock_file lock_file = LOCK_INIT;\n+\tstruct odb_transaction *transaction;\n \n \trepo_config(repo, add_config, NULL);\n \n@@ -574,7 +575,7 @@ int cmd_add(int argc,\n \t\tstring_list_clear(&only_match_skip_worktree, 0);\n \t}\n \n-\tbegin_odb_transaction();\n+\ttransaction = begin_odb_transaction(repo->objects);\n \n \tps_matched = xcalloc(pathspec.nr, 1);\n \tif (add_renormalize)\n@@ -593,7 +594,7 @@ int cmd_add(int argc,\n \n \tif (chmod_arg && pathspec.nr)\n \t\texit_status |= chmod_pathspec(repo, &pathspec, chmod_arg[0], show_only);\n-\tend_odb_transaction();\n+\tend_odb_transaction(transaction);\n \n finish:\n \tif (write_locked_index(repo->index, &lock_file,\ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex 7ae7c82b6c0..28124b324d2 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -584,6 +584,7 @@ static void unpack_all(void)\n {\n \tint i;\n \tunsigned char *hdr = fill(sizeof(struct pack_header));\n+\tstruct odb_transaction *transaction;\n \n \tif (get_be32(hdr) != PACK_SIGNATURE)\n \t\tdie(\"bad pack file\");\n@@ -599,12 +600,12 @@ static void unpack_all(void)\n \t\tprogress = start_progress(the_repository,\n \t\t\t\t\t _(\"Unpacking objects\"), nr_objects);\n \tCALLOC_ARRAY(obj_list, nr_objects);\n-\tbegin_odb_transaction();\n+\ttransaction = begin_odb_transaction(the_repository->objects);\n \tfor (i = 0; i < nr_objects; i++) {\n \t\tunpack_one(i);\n \t\tdisplay_progress(progress, i + 1);\n \t}\n-\tend_odb_transaction();\n+\tend_odb_transaction(transaction);\n \tstop_progress(&progress);\n \n \tif (delta_list)\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 2380f3ccd68..2ba2d29c959 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -77,7 +77,7 @@ static void report(const char *fmt, ...)\n \t * objects invisible while a transaction is active, so flush the\n \t * transaction here before reporting a change made by update-index.\n \t */\n-\tflush_odb_transaction();\n+\tflush_odb_transaction(the_repository->objects->transaction);\n \tva_start(vp, fmt);\n \tvprintf(fmt, vp);\n \tputchar('\\n');\n@@ -940,6 +940,7 @@ int cmd_update_index(int argc,\n \tstrbuf_getline_fn getline_fn;\n \tint parseopt_state = PARSE_OPT_UNKNOWN;\n \tstruct repository *r = the_repository;\n+\tstruct odb_transaction *transaction;\n \tstruct option options[] = {\n \t\tOPT_BIT('q', NULL, &refresh_args.flags,\n \t\t\tN_(\"continue refresh even when index needs update\"),\n@@ -1130,7 +1131,7 @@ int cmd_update_index(int argc,\n \t * Allow the object layer to optimize adding multiple objects in\n \t * a batch.\n \t */\n-\tbegin_odb_transaction();\n+\ttransaction = begin_odb_transaction(the_repository->objects);\n \twhile (ctx.argc) {\n \t\tif (parseopt_state != PARSE_OPT_DONE)\n \t\t\tparseopt_state = parse_options_step(&ctx, options,\n@@ -1213,7 +1214,7 @@ int cmd_update_index(int argc,\n \t/*\n \t * By now we have added all of the new objects\n \t */\n-\tend_odb_transaction();\n+\tend_odb_transaction(transaction);\n \n \tif (split_index > 0) {\n \t\tif (repo_config_get_split_index(the_repository) == 0)\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 82a73da79e8..53a20a2d92f 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -30,11 +30,13 @@ struct bulk_checkin_packfile {\n \tuint32_t nr_written;\n };\n \n-static struct odb_transaction {\n+struct odb_transaction {\n+\tstruct object_database *odb;\n+\n \tint nesting;\n \tstruct tmp_objdir *objdir;\n \tstruct bulk_checkin_packfile packfile;\n-} transaction;\n+};\n \n static void finish_tmp_packfile(struct strbuf *basename,\n \t\t\t\tconst char *pack_tmp_name,\n@@ -98,12 +100,12 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n /*\n * Cleanup after batch-mode fsync_object_files.\n */\n-static void flush_batch_fsync(void)\n+static void flush_batch_fsync(struct odb_transaction *transaction)\n {\n \tstruct strbuf temp_path = STRBUF_INIT;\n \tstruct tempfile *temp;\n \n-\tif (!transaction.objdir)\n+\tif (!transaction->objdir)\n \t\treturn;\n \n \t/*\n@@ -125,8 +127,8 @@ static void flush_batch_fsync(void)\n \t * Make the object files visible in the primary ODB after their data is\n \t * fully durable.\n \t */\n-\ttmp_objdir_migrate(transaction.objdir);\n-\ttransaction.objdir = NULL;\n+\ttmp_objdir_migrate(transaction->objdir);\n+\ttransaction->objdir = NULL;\n }\n \n static int already_written(struct bulk_checkin_packfile *state, struct object_id *oid)\n@@ -325,7 +327,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \treturn 0;\n }\n \n-void prepare_loose_object_bulk_checkin(void)\n+void prepare_loose_object_bulk_checkin(struct odb_transaction *transaction)\n {\n \t/*\n \t * We lazily create the temporary object directory\n@@ -333,15 +335,16 @@ void prepare_loose_object_bulk_checkin(void)\n \t * callers may not know whether any objects will be\n \t * added at the time they call begin_odb_transaction.\n \t */\n-\tif (!transaction.nesting || transaction.objdir)\n+\tif (!transaction || transaction->objdir)\n \t\treturn;\n \n-\ttransaction.objdir = tmp_objdir_create(the_repository, \"bulk-fsync\");\n-\tif (transaction.objdir)\n-\t\ttmp_objdir_replace_primary_odb(transaction.objdir, 0);\n+\ttransaction->objdir = tmp_objdir_create(the_repository, \"bulk-fsync\");\n+\tif (transaction->objdir)\n+\t\ttmp_objdir_replace_primary_odb(transaction->objdir, 0);\n }\n \n-void fsync_loose_object_bulk_checkin(int fd, const char *filename)\n+void fsync_loose_object_bulk_checkin(struct odb_transaction *transaction,\n+\t\t\t\t int fd, const char *filename)\n {\n \t/*\n \t * If we have an active ODB transaction, we issue a call that\n@@ -350,7 +353,7 @@ void fsync_loose_object_bulk_checkin(int fd, const char *filename)\n \t * before renaming the objects to their final names as part of\n \t * flush_batch_fsync.\n \t */\n-\tif (!transaction.objdir ||\n+\tif (!transaction || !transaction->objdir ||\n \t git_fsync(fd, FSYNC_WRITEOUT_ONLY) < 0) {\n \t\tif (errno == ENOSYS)\n \t\t\twarning(_(\"core.fsyncMethod = batch is unsupported on this platform\"));\n@@ -358,36 +361,57 @@ void fsync_loose_object_bulk_checkin(int fd, const char *filename)\n \t}\n }\n \n-int index_blob_bulk_checkin(struct object_id *oid,\n-\t\t\t int fd, size_t size,\n+int index_blob_bulk_checkin(struct odb_transaction *transaction,\n+\t\t\t struct object_id *oid, int fd, size_t size,\n \t\t\t const char *path, unsigned flags)\n {\n-\tint status = deflate_blob_to_pack(&transaction.packfile, oid, fd, size,\n-\t\t\t\t\t path, flags);\n-\tif (!transaction.nesting)\n-\t\tflush_bulk_checkin_packfile(&transaction.packfile);\n+\tint status;\n+\n+\tif (transaction) {\n+\t\tstatus = deflate_blob_to_pack(&transaction->packfile, oid, fd,\n+\t\t\t\t\t size, path, flags);\n+\t} else {\n+\t\tstruct bulk_checkin_packfile state = { 0 };\n+\n+\t\tstatus = deflate_blob_to_pack(&state, oid, fd, size, path, flags);\n+\t\tflush_bulk_checkin_packfile(&state);\n+\t}\n+\n \treturn status;\n }\n \n-void begin_odb_transaction(void)\n+struct odb_transaction *begin_odb_transaction(struct object_database *odb)\n {\n-\ttransaction.nesting += 1;\n+\tif (!odb->transaction) {\n+\t\tCALLOC_ARRAY(odb->transaction, 1);\n+\t\todb->transaction->odb = odb;\n+\t}\n+\n+\todb->transaction->nesting += 1;\n+\n+\treturn odb->transaction;\n }\n \n-void flush_odb_transaction(void)\n+void flush_odb_transaction(struct odb_transaction *transaction)\n {\n-\tflush_batch_fsync();\n-\tflush_bulk_checkin_packfile(&transaction.packfile);\n+\tif (!transaction)\n+\t\treturn;\n+\n+\tflush_batch_fsync(transaction);\n+\tflush_bulk_checkin_packfile(&transaction->packfile);\n }\n \n-void end_odb_transaction(void)\n+void end_odb_transaction(struct odb_transaction *transaction)\n {\n-\ttransaction.nesting -= 1;\n-\tif (transaction.nesting < 0)\n+\tif (!transaction || transaction->nesting == 0)\n \t\tBUG(\"Unbalanced ODB transaction nesting\");\n \n-\tif (transaction.nesting)\n+\ttransaction->nesting -= 1;\n+\n+\tif (transaction->nesting)\n \t\treturn;\n \n-\tflush_odb_transaction();\n+\tflush_odb_transaction(transaction);\n+\ttransaction->odb->transaction = NULL;\n+\tfree(transaction);\n }\ndiff --git a/bulk-checkin.h b/bulk-checkin.h\nindex 7246ea58dcf..16254ce6a70 100644\n--- a/bulk-checkin.h\n+++ b/bulk-checkin.h\n@@ -5,9 +5,13 @@\n #define BULK_CHECKIN_H\n \n #include \"object.h\"\n+#include \"odb.h\"\n \n-void prepare_loose_object_bulk_checkin(void);\n-void fsync_loose_object_bulk_checkin(int fd, const char *filename);\n+struct odb_transaction;\n+\n+void prepare_loose_object_bulk_checkin(struct odb_transaction *transaction);\n+void fsync_loose_object_bulk_checkin(struct odb_transaction *transaction,\n+\t\t\t\t int fd, const char *filename);\n \n /*\n * This creates one packfile per large blob unless bulk-checkin\n@@ -24,8 +28,8 @@ void fsync_loose_object_bulk_checkin(int fd, const char *filename);\n * binary blobs, they generally do not want to get any conversion, and\n * callers should avoid this code path when filters are requested.\n */\n-int index_blob_bulk_checkin(struct object_id *oid,\n-\t\t\t int fd, size_t size,\n+int index_blob_bulk_checkin(struct odb_transaction *transaction,\n+\t\t\t struct object_id *oid, int fd, size_t size,\n \t\t\t const char *path, unsigned flags);\n \n /*\n@@ -35,20 +39,20 @@ int index_blob_bulk_checkin(struct object_id *oid,\n * and objects are only visible after the outermost transaction\n * is complete or the transaction is flushed.\n */\n-void begin_odb_transaction(void);\n+struct odb_transaction *begin_odb_transaction(struct object_database *odb);\n \n /*\n * Make any objects that are currently part of a pending object\n * database transaction visible. It is valid to call this function\n * even if no transaction is active.\n */\n-void flush_odb_transaction(void);\n+void flush_odb_transaction(struct odb_transaction *transaction);\n \n /*\n * Tell the object database to make any objects from the\n * current transaction visible if this is the final nested\n * transaction.\n */\n-void end_odb_transaction(void);\n+void end_odb_transaction(struct odb_transaction *transaction);\n \n #endif\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 66ef2becbe0..d225554eedd 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -474,6 +474,7 @@ static int update_one(struct cache_tree *it,\n \n int cache_tree_update(struct index_state *istate, int flags)\n {\n+\tstruct odb_transaction *transaction;\n \tint skip, i;\n \n \ti = verify_cache(istate, flags);\n@@ -489,10 +490,10 @@ int cache_tree_update(struct index_state *istate, int flags)\n \n \ttrace_performance_enter();\n \ttrace2_region_enter(\"cache_tree\", \"update\", the_repository);\n-\tbegin_odb_transaction();\n+\ttransaction = begin_odb_transaction(the_repository->objects);\n \ti = update_one(istate->cache_tree, istate->cache, istate->cache_nr,\n \t\t \"\", 0, &skip, flags);\n-\tend_odb_transaction();\n+\tend_odb_transaction(transaction);\n \ttrace2_region_leave(\"cache_tree\", \"update\", the_repository);\n \ttrace_performance_leave(\"cache_tree_update\");\n \tif (i < 0)\ndiff --git a/object-file.c b/object-file.c\nindex 2bc36ab3ee8..1740aa2b2e3 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -674,7 +674,7 @@ static void close_loose_object(struct odb_source *source,\n \t\tgoto out;\n \n \tif (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))\n-\t\tfsync_loose_object_bulk_checkin(fd, filename);\n+\t\tfsync_loose_object_bulk_checkin(source->odb->transaction, fd, filename);\n \telse if (fsync_object_files > 0)\n \t\tfsync_or_die(fd, filename);\n \telse\n@@ -852,7 +852,7 @@ static int write_loose_object(struct odb_source *source,\n \tstatic struct strbuf filename = STRBUF_INIT;\n \n \tif (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))\n-\t\tprepare_loose_object_bulk_checkin();\n+\t\tprepare_loose_object_bulk_checkin(source->odb->transaction);\n \n \todb_loose_path(source, &filename, oid);\n \n@@ -941,7 +941,7 @@ int stream_loose_object(struct odb_source *source,\n \tint hdrlen;\n \n \tif (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))\n-\t\tprepare_loose_object_bulk_checkin();\n+\t\tprepare_loose_object_bulk_checkin(source->odb->transaction);\n \n \t/* Since oid is not determined, save tmp file to odb path. */\n \tstrbuf_addf(&filename, \"%s/\", source->path);\n@@ -1263,8 +1263,9 @@ int index_fd(struct index_state *istate, struct object_id *oid,\n \t\tret = index_core(istate, oid, fd, xsize_t(st->st_size),\n \t\t\t\t type, path, flags);\n \telse\n-\t\tret = index_blob_bulk_checkin(oid, fd, xsize_t(st->st_size), path,\n-\t\t\t\t\t flags);\n+\t\tret = index_blob_bulk_checkin(the_repository->objects->transaction,\n+\t\t\t\t\t oid, fd, xsize_t(st->st_size),\n+\t\t\t\t\t path, flags);\n \tclose(fd);\n \treturn ret;\n }\ndiff --git a/odb.h b/odb.h\nindex 3dfc66d75a3..a89b2143909 100644\n--- a/odb.h\n+++ b/odb.h\n@@ -84,6 +84,7 @@ struct odb_source {\n \n struct packed_git;\n struct cached_object_entry;\n+struct odb_transaction;\n \n /*\n * The object database encapsulates access to objects in a repository. It\n@@ -94,6 +95,13 @@ struct object_database {\n \t/* Repository that owns this database. */\n \tstruct repository *repo;\n \n+\t/*\n+\t * State of current current object database transaction. Only one\n+\t * transaction may be pending at a time. Is NULL when no transaction is\n+\t * configured.\n+\t */\n+\tstruct odb_transaction *transaction;\n+\n \t/*\n \t * Set of all object directories; the main directory is first (and\n \t * cannot be NULL after initialization). Subsequent directories are\ndiff --git a/read-cache.c b/read-cache.c\nindex 06ad74db228..229b8ef11c9 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -3947,6 +3947,7 @@ int add_files_to_cache(struct repository *repo, const char *prefix,\n \t\t const struct pathspec *pathspec, char *ps_matched,\n \t\t int include_sparse, int flags)\n {\n+\tstruct odb_transaction *transaction;\n \tstruct update_callback_data data;\n \tstruct rev_info rev;\n \n@@ -3972,9 +3973,9 @@ int add_files_to_cache(struct repository *repo, const char *prefix,\n \t * This function is invoked from commands other than 'add', which\n \t * may not have their own transaction active.\n \t */\n-\tbegin_odb_transaction();\n+\ttransaction = begin_odb_transaction(repo->objects);\n \trun_diff_files(&rev, DIFF_RACY_IS_MODIFIED);\n-\tend_odb_transaction();\n+\tend_odb_transaction(transaction);\n \n \trelease_revisions(&rev);\n \treturn !!data.add_errors;\n-- \n2.51.0\n\n"},{"id":"524697","messageId":"20250821232249.319427-5-jltobler@gmail.com","threadId":"64002","inReplyTo":"20250821232249.319427-1-jltobler@gmail.com","subject":"[PATCH v2 4/4] bulk-checkin: use repository variable from transaction","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-21T23:22:49Z","receivedAt":"2025-08-21T23:22:58Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"The bulk-checkin subsystem depends on `the_repository`. Adapt functions\nand call sites to access the repository through `struct odb_transaction`\ninstead. The `USE_THE_REPOSITORY_VARIBALE` is still required as the\n`pack_compression_level` and `pack_size_limit_cfg` globals are still\nused.\n\nSigned-off-by: Justin Tobler \n---\n bulk-checkin.c | 84 ++++++++++++++++++++++++--------------------------\n 1 file changed, 41 insertions(+), 43 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 0e3747640b9..8402d9637c3 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -38,25 +38,26 @@ struct odb_transaction {\n \tstruct bulk_checkin_packfile packfile;\n };\n \n-static void finish_tmp_packfile(struct strbuf *basename,\n-\t\t\t\tconst char *pack_tmp_name,\n-\t\t\t\tstruct pack_idx_entry **written_list,\n-\t\t\t\tuint32_t nr_written,\n-\t\t\t\tstruct pack_idx_option *pack_idx_opts,\n+static void finish_tmp_packfile(struct odb_transaction *transaction,\n+\t\t\t\tstruct strbuf *basename,\n \t\t\t\tunsigned char hash[])\n {\n+\tstruct bulk_checkin_packfile *state = &transaction->packfile;\n+\tstruct repository *repo = transaction->odb->repo;\n \tchar *idx_tmp_name = NULL;\n \n-\tstage_tmp_packfiles(the_repository, basename, pack_tmp_name,\n-\t\t\t written_list, nr_written, NULL, pack_idx_opts, hash,\n-\t\t\t &idx_tmp_name);\n-\trename_tmp_packfile_idx(the_repository, basename, &idx_tmp_name);\n+\tstage_tmp_packfiles(repo, basename, state->pack_tmp_name,\n+\t\t\t state->written, state->nr_written, NULL,\n+\t\t\t &state->pack_idx_opts, hash, &idx_tmp_name);\n+\trename_tmp_packfile_idx(repo, basename, &idx_tmp_name);\n \n \tfree(idx_tmp_name);\n }\n \n-static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n+static void flush_bulk_checkin_packfile(struct odb_transaction *transaction)\n {\n+\tstruct bulk_checkin_packfile *state = &transaction->packfile;\n+\tstruct repository *repo = transaction->odb->repo;\n \tunsigned char hash[GIT_MAX_RAWSZ];\n \tstruct strbuf packname = STRBUF_INIT;\n \n@@ -73,17 +74,17 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n \t\t\t\t CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);\n \t} else {\n \t\tint fd = finalize_hashfile(state->f, hash, FSYNC_COMPONENT_PACK, 0);\n-\t\tfixup_pack_header_footer(the_hash_algo, fd, hash, state->pack_tmp_name,\n+\t\tfixup_pack_header_footer(repo->hash_algo, fd, hash, state->pack_tmp_name,\n \t\t\t\t\t state->nr_written, hash,\n \t\t\t\t\t state->offset);\n \t\tclose(fd);\n \t}\n \n-\tstrbuf_addf(&packname, \"%s/pack/pack-%s.\", repo_get_object_directory(the_repository),\n-\t\t hash_to_hex(hash));\n-\tfinish_tmp_packfile(&packname, state->pack_tmp_name,\n-\t\t\t state->written, state->nr_written,\n-\t\t\t &state->pack_idx_opts, hash);\n+\tstrbuf_addf(&packname, \"%s/pack/pack-%s.\",\n+\t\t transaction->odb->sources->path,\n+\t\t hash_to_hex_algop(hash, repo->hash_algo));\n+\n+\tfinish_tmp_packfile(transaction, &packname, hash);\n \tfor (uint32_t i = 0; i < state->nr_written; i++)\n \t\tfree(state->written[i]);\n \n@@ -94,7 +95,7 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n \n \tstrbuf_release(&packname);\n \t/* Make objects we just wrote available to ourselves */\n-\treprepare_packed_git(the_repository);\n+\treprepare_packed_git(repo);\n }\n \n /*\n@@ -117,7 +118,8 @@ static void flush_batch_fsync(struct odb_transaction *transaction)\n \t * to ensure that the data in each new object file is durable before\n \t * the final name is visible.\n \t */\n-\tstrbuf_addf(&temp_path, \"%s/bulk_fsync_XXXXXX\", repo_get_object_directory(the_repository));\n+\tstrbuf_addf(&temp_path, \"%s/bulk_fsync_XXXXXX\",\n+\t\t transaction->odb->sources->path);\n \ttemp = xmks_tempfile(temp_path.buf);\n \tfsync_or_die(get_tempfile_fd(temp), get_tempfile_path(temp));\n \tdelete_tempfile(&temp);\n@@ -131,16 +133,17 @@ static void flush_batch_fsync(struct odb_transaction *transaction)\n \ttransaction->objdir = NULL;\n }\n \n-static int already_written(struct bulk_checkin_packfile *state, struct object_id *oid)\n+static int already_written(struct odb_transaction *transaction,\n+\t\t\t struct object_id *oid)\n {\n \t/* The object may already exist in the repository */\n-\tif (odb_has_object(the_repository->objects, oid,\n+\tif (odb_has_object(transaction->odb, oid,\n \t\t\t HAS_OBJECT_RECHECK_PACKED | HAS_OBJECT_FETCH_PROMISOR))\n \t\treturn 1;\n \n \t/* Might want to keep the list sorted */\n-\tfor (uint32_t i = 0; i < state->nr_written; i++)\n-\t\tif (oideq(&state->written[i]->oid, oid))\n+\tfor (uint32_t i = 0; i < transaction->packfile.nr_written; i++)\n+\t\tif (oideq(&transaction->packfile.written[i]->oid, oid))\n \t\t\treturn 1;\n \n \t/* This is a new object we need to keep */\n@@ -239,13 +242,15 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n }\n \n /* Lazily create backing packfile for the state */\n-static void prepare_to_stream(struct bulk_checkin_packfile *state,\n+static void prepare_to_stream(struct odb_transaction *transaction,\n \t\t\t unsigned flags)\n {\n+\tstruct bulk_checkin_packfile *state = &transaction->packfile;\n \tif (!(flags & INDEX_WRITE_OBJECT) || state->f)\n \t\treturn;\n \n-\tstate->f = create_tmp_packfile(the_repository, &state->pack_tmp_name);\n+\tstate->f = create_tmp_packfile(transaction->odb->repo,\n+\t\t\t\t &state->pack_tmp_name);\n \treset_pack_idx_option(&state->pack_idx_opts);\n \n \t/* Pretend we are going to write only one object */\n@@ -254,11 +259,12 @@ static void prepare_to_stream(struct bulk_checkin_packfile *state,\n \t\tdie_errno(\"unable to write pack header\");\n }\n \n-static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n-\t\t\t\tstruct object_id *result_oid,\n-\t\t\t\tint fd, size_t size,\n-\t\t\t\tconst char *path, unsigned flags)\n+int index_blob_bulk_checkin(struct odb_transaction *transaction,\n+\t\t\t struct object_id *result_oid,\n+\t\t\t int fd, size_t size,\n+\t\t\t const char *path, unsigned flags)\n {\n+\tstruct bulk_checkin_packfile *state = &transaction->packfile;\n \toff_t seekback, already_hashed_to;\n \tstruct git_hash_ctx ctx;\n \tunsigned char obuf[16384];\n@@ -272,21 +278,21 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \n \theader_len = format_object_header((char *)obuf, sizeof(obuf),\n \t\t\t\t\t OBJ_BLOB, size);\n-\tthe_hash_algo->init_fn(&ctx);\n+\ttransaction->odb->repo->hash_algo->init_fn(&ctx);\n \tgit_hash_update(&ctx, obuf, header_len);\n \n \t/* Note: idx is non-NULL when we are writing */\n \tif ((flags & INDEX_WRITE_OBJECT) != 0) {\n \t\tCALLOC_ARRAY(idx, 1);\n \n-\t\tprepare_to_stream(state, flags);\n+\t\tprepare_to_stream(transaction, flags);\n \t\thashfile_checkpoint_init(state->f, &checkpoint);\n \t}\n \n \talready_hashed_to = 0;\n \n \twhile (1) {\n-\t\tprepare_to_stream(state, flags);\n+\t\tprepare_to_stream(transaction, flags);\n \t\tif (idx) {\n \t\t\thashfile_checkpoint(state->f, &checkpoint);\n \t\t\tidx->offset = state->offset;\n@@ -304,7 +310,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \t\t\tBUG(\"should not happen\");\n \t\thashfile_truncate(state->f, &checkpoint);\n \t\tstate->offset = checkpoint.offset;\n-\t\tflush_bulk_checkin_packfile(state);\n+\t\tflush_bulk_checkin_packfile(transaction);\n \t\tif (lseek(fd, seekback, SEEK_SET) == (off_t) -1)\n \t\t\treturn error(\"cannot seek back\");\n \t}\n@@ -313,7 +319,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \t\treturn 0;\n \n \tidx->crc32 = crc32_end(state->f);\n-\tif (already_written(state, result_oid)) {\n+\tif (already_written(transaction, result_oid)) {\n \t\thashfile_truncate(state->f, &checkpoint);\n \t\tstate->offset = checkpoint.offset;\n \t\tfree(idx);\n@@ -338,7 +344,7 @@ void prepare_loose_object_bulk_checkin(struct odb_transaction *transaction)\n \tif (!transaction || transaction->objdir)\n \t\treturn;\n \n-\ttransaction->objdir = tmp_objdir_create(the_repository, \"bulk-fsync\");\n+\ttransaction->objdir = tmp_objdir_create(transaction->odb->repo, \"bulk-fsync\");\n \tif (transaction->objdir)\n \t\ttmp_objdir_replace_primary_odb(transaction->objdir, 0);\n }\n@@ -361,14 +367,6 @@ void fsync_loose_object_bulk_checkin(struct odb_transaction *transaction,\n \t}\n }\n \n-int index_blob_bulk_checkin(struct odb_transaction *transaction,\n-\t\t\t struct object_id *oid, int fd, size_t size,\n-\t\t\t const char *path, unsigned flags)\n-{\n-\treturn deflate_blob_to_pack(&transaction->packfile, oid, fd, size, path,\n-\t\t\t\t flags);\n-}\n-\n struct odb_transaction *begin_odb_transaction(struct object_database *odb)\n {\n \tif (!odb->transaction) {\n@@ -387,7 +385,7 @@ void flush_odb_transaction(struct odb_transaction *transaction)\n \t\treturn;\n \n \tflush_batch_fsync(transaction);\n-\tflush_bulk_checkin_packfile(&transaction->packfile);\n+\tflush_bulk_checkin_packfile(transaction);\n }\n \n void end_odb_transaction(struct odb_transaction *transaction)\n-- \n2.51.0\n\n"},{"id":"524717","messageId":"xmqqv7mfxqg2.fsf@gitster.g","threadId":"64002","inReplyTo":"20250821232249.319427-3-jltobler@gmail.com","subject":"Re: [PATCH v2 2/4] bulk-checkin: remove global transaction state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-22T16:37:49Z","receivedAt":"2025-08-22T16:37:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler writes:\n\n> diff --git a/bulk-checkin.c b/bulk-checkin.c\n> index 82a73da79e8..53a20a2d92f 100644\n> --- a/bulk-checkin.c\n> +++ b/bulk-checkin.c\n> @@ -30,11 +30,13 @@ struct bulk_checkin_packfile {\n> \tuint32_t nr_written;\n> };\n> \n> -static struct odb_transaction {\n> +struct odb_transaction {\n> +\tstruct object_database *odb;\n> +\n> \tint nesting;\n> \tstruct tmp_objdir *objdir;\n> \tstruct bulk_checkin_packfile packfile;\n> -} transaction;\n> +};\n\nVery nice to see that this singleton instance is now gone. \n\nThe singleton was really handy way to allow calls into this\nsubsystem to lazily initialize bulk_checkin_packfile in the call\nchain starting from deflate_blob_to_pack() -> prepare_to_stream().\n\nWe now need to be a lot more careful to make sure that everybody has\naccess to a valid bulk_checkin_packfile struct, which makes the\nimplementation of index_blob_bulk_checkin() a bit awkward and we\nneed to invent one bulk_checkin_packfile instance right there.\nLuckily it goes away in the next step, I guess?\n\n> +int index_blob_bulk_checkin(struct odb_transaction *transaction,\n> +\t\t\t struct object_id *oid, int fd, size_t size,\n> \t\t\t const char *path, unsigned flags)\n> {\n> -\tint status = deflate_blob_to_pack(&transaction.packfile, oid, fd, size,\n> -\t\t\t\t\t path, flags);\n> -\tif (!transaction.nesting)\n> -\t\tflush_bulk_checkin_packfile(&transaction.packfile);\n> +\tint status;\n> +\n> +\tif (transaction) {\n> +\t\tstatus = deflate_blob_to_pack(&transaction->packfile, oid, fd,\n> +\t\t\t\t\t size, path, flags);\n> +\t} else {\n> +\t\tstruct bulk_checkin_packfile state = { 0 };\n> +\n> +\t\tstatus = deflate_blob_to_pack(&state, oid, fd, size, path, flags);\n> +\t\tflush_bulk_checkin_packfile(&state);\n> +\t}\n> +\n> \treturn status;\n> }\n\nOK. If we do not have a transaction (i.e. if nobody called \nbegin_odb_transaction() more times than end_odb_transaction()\ngot called), we let the underlying machinery do the right thing on\nan empty state and clean up after ourselves; otherwise we follow\nexactly the same code path as we used to.\n\n> diff --git a/cache-tree.c b/cache-tree.c\n> index 66ef2becbe0..d225554eedd 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -474,6 +474,7 @@ static int update_one(struct cache_tree *it,\n> \n> int cache_tree_update(struct index_state *istate, int flags)\n> {\n> +\tstruct odb_transaction *transaction;\n> \tint skip, i;\n\nOK, doing the odb-transaction here would cover both this one and\nwrite_index_as_tree and its friends, that are public interfaces into\nthe cache-tree subsystem. \n\n> diff --git a/odb.h b/odb.h\n> index 3dfc66d75a3..a89b2143909 100644\n> --- a/odb.h\n> +++ b/odb.h\n> @@ -84,6 +84,7 @@ struct odb_source {\n> \n> struct packed_git;\n> struct cached_object_entry;\n> +struct odb_transaction;\n> \n> /*\n> * The object database encapsulates access to objects in a repository. It\n> @@ -94,6 +95,13 @@ struct object_database {\n> \t/* Repository that owns this database. */\n> \tstruct repository *repo;\n> \n> +\t/*\n> +\t * State of current current object database transaction. Only one\n> +\t * transaction may be pending at a time. Is NULL when no transaction is\n> +\t * configured.\n> +\t */\n> +\tstruct odb_transaction *transaction;\n\nOnce dust from this topic settles, we may want to rename the\nbulk-checkin.[ch] to have \"odb\" somewhere in its name, perhaps? I\nusually do not like renaming file for the sake of renaming to make\nthe result look \"pretty\" (people may use \"consistent naming\" ),\nthough. I dunno.\n"},{"id":"524718","messageId":"xmqqqzx3xpwc.fsf@gitster.g","threadId":"64002","inReplyTo":"20250821232249.319427-4-jltobler@gmail.com","subject":"Re: [PATCH v2 3/4] bulk-checkin: require transaction for index_blob_bulk_checkin()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-22T16:49:39Z","receivedAt":"2025-08-22T16:49:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler writes:\n\n> /*\n> - * This creates one packfile per large blob unless bulk-checkin\n> - * machinery is \"plugged\".\n> + * This writes the specified object to a packfile. Objects written here\n> + * during the same transaction are written to the same packfile. The\n> + * packfile is not flushed until the transaction is flushed. The caller\n> + * is expected to ensure a valid transaction is setup for objects to be\n> + * recorded to.\n> *\n> * This also bypasses the usual \"convert-to-git\" dance, and that is on\n> * purpose. We could write a streaming version of the converting\n> diff --git a/object-file.c b/object-file.c\n> index 1740aa2b2e3..bc15af42450 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1253,19 +1253,26 @@ int index_fd(struct index_state *istate, struct object_id *oid,\n> \t * Call xsize_t() only when needed to avoid potentially unnecessary\n> \t * die() for large files.\n> \t */\n> -\tif (type == OBJ_BLOB && path && would_convert_to_git_filter_fd(istate, path))\n> +\tif (type == OBJ_BLOB && path && would_convert_to_git_filter_fd(istate, path)) {\n> \t\tret = index_stream_convert_blob(istate, oid, fd, path, flags);\n> -\telse if (!S_ISREG(st->st_mode))\n> +\t} else if (!S_ISREG(st->st_mode)) {\n> \t\tret = index_pipe(istate, oid, fd, type, path, flags);\n> -\telse if ((st->st_size >= 0 && (size_t) st->st_size <= repo_settings_get_big_file_threshold(istate->repo)) ||\n> -\t\t type != OBJ_BLOB ||\n> -\t\t (path && would_convert_to_git(istate, path)))\n> +\t} else if ((st->st_size >= 0 &&\n> +\t\t (size_t)st->st_size <= repo_settings_get_big_file_threshold(istate->repo)) ||\n> +\t\t type != OBJ_BLOB ||\n> +\t\t (path && would_convert_to_git(istate, path))) {\n> \t\tret = index_core(istate, oid, fd, xsize_t(st->st_size),\n> \t\t\t\t type, path, flags);\n> -\telse\n> -\t\tret = index_blob_bulk_checkin(the_repository->objects->transaction,\n> +\t} else {\n> +\t\tstruct odb_transaction *transaction;\n> +\n> +\t\ttransaction = begin_odb_transaction(the_repository->objects);\n> +\t\tret = index_blob_bulk_checkin(transaction,\n> \t\t\t\t\t oid, fd, xsize_t(st->st_size),\n> \t\t\t\t\t path, flags);\n> +\t\tend_odb_transaction(transaction);\n> +\t}\n\nInteresting. If the caller does the odb transaction management\nitself by calling begin/end before calling this function and fed two\nor more large objects, the original code did the right thing with\n.nesting set to 1. In the new code, it still does the right thing\neven during the call to index_blob_bulk_checkin we raise .nesting by\none, because the all non-zero .nesting values mean the same thing to\nthe machinery, thanks to lazy initialization of the .objdir.\n\nSo, isn't the comment above the function now less accurate than\nbefore? The caller of this function does not have to do anything\nand we do not expect the caller to \"ensure a valid transaction\" at\nall, no?\n\nThanks.\n\n\n\n\n"},{"id":"524719","messageId":"xmqqjz2vxp98.fsf@gitster.g","threadId":"64002","inReplyTo":"20250821232249.319427-5-jltobler@gmail.com","subject":"Re: [PATCH v2 4/4] bulk-checkin: use repository variable from transaction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-22T17:03:31Z","receivedAt":"2025-08-22T17:03:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler writes:\n\n> The bulk-checkin subsystem depends on `the_repository`. Adapt functions\n> and call sites to access the repository through `struct odb_transaction`\n> instead. The `USE_THE_REPOSITORY_VARIBALE` is still required as the\n> `pack_compression_level` and `pack_size_limit_cfg` globals are still\n> used.\n\nAlso we grab the details of the new packfile the bulk-checkin\nmachinery is building out of the transaction, which made some\nredundant parameters functions take go away, ...\n\n> -static void finish_tmp_packfile(struct strbuf *basename,\n> -\t\t\t\tconst char *pack_tmp_name,\n> -\t\t\t\tstruct pack_idx_entry **written_list,\n> -\t\t\t\tuint32_t nr_written,\n> -\t\t\t\tstruct pack_idx_option *pack_idx_opts,\n> +static void finish_tmp_packfile(struct odb_transaction *transaction,\n> +\t\t\t\tstruct strbuf *basename,\n> \t\t\t\tunsigned char hash[])\n\n... like this one. Very nice.\n\n> @@ -117,7 +118,8 @@ static void flush_batch_fsync(struct odb_transaction *transaction)\n> \t * to ensure that the data in each new object file is durable before\n> \t * the final name is visible.\n> \t */\n> -\tstrbuf_addf(&temp_path, \"%s/bulk_fsync_XXXXXX\", repo_get_object_directory(the_repository));\n> +\tstrbuf_addf(&temp_path, \"%s/bulk_fsync_XXXXXX\",\n> +\t\t transaction->odb->sources->path);\n\nThis is doing a lot more than a simple \"the_repository\" ->\n\"odb->repo\" replacement. How much confidence do we have\nthat the internal detail of repo_get_object_directory() will stay\nthe same and our developers in the future would spot that this open\ncoded copy needs to be updated if they have to change it?\n\n> -static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n> -\t\t\t\tstruct object_id *result_oid,\n> -\t\t\t\tint fd, size_t size,\n> -\t\t\t\tconst char *path, unsigned flags)\n> +int index_blob_bulk_checkin(struct odb_transaction *transaction,\n> +\t\t\t struct object_id *result_oid,\n> +\t\t\t int fd, size_t size,\n> +\t\t\t const char *path, unsigned flags)\n\nAhh, OK, with the simplification to always take transaction, there\nis no need to have the deflate_blob_to_pack() function, which had\nonly one caller, as an internal implementation detail anymore.\n\nThis change could have been in the previous step and it would have\nbeen less surprising; the above \"Ahh, OK\" is my reaction to the\nsurprise ;-)\n\n"},{"id":"524723","messageId":"njvcqkkcotesqznalieuwhzow7lxbejxlfw3knbhalmqjcb7cs@3vwea4r2zljl","threadId":"64002","inReplyTo":"xmqqv7mfxqg2.fsf@gitster.g","subject":"Re: [PATCH v2 2/4] bulk-checkin: remove global transaction state","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-22T18:07:37Z","receivedAt":"2025-08-22T18:07:39Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/08/22 09:37AM, Junio C Hamano wrote:\n> Justin Tobler writes:\n\n> We now need to be a lot more careful to make sure that everybody has\n> access to a valid bulk_checkin_packfile struct, which makes the\n> implementation of index_blob_bulk_checkin() a bit awkward and we\n> need to invent one bulk_checkin_packfile instance right there.\n> Luckily it goes away in the next step, I guess?\n\nYa, now that we cannot rely on the transaction singleton to provide a\nbulk_checkin_packfile, we have create one if there is no transaction. I\ncompletely agree that this is awkward which is why the next step\nrequires index_blob_bulk_checkin() callers to ensure a transaction is\ncreated ahead of time.\n\nIf it is preferable, I can combine these two patches together. I kept\nthem separate as I thought it would better explain the steps, but maybe\nthat isn't the best way to structure the patch as we are largely undoing\nthe change in the next patch anyway.\n\n> > diff --git a/odb.h b/odb.h\n> > index 3dfc66d75a3..a89b2143909 100644\n> > --- a/odb.h\n> > +++ b/odb.h\n> > @@ -84,6 +84,7 @@ struct odb_source {\n> > \n> > struct packed_git;\n> > struct cached_object_entry;\n> > +struct odb_transaction;\n> > \n> > /*\n> > * The object database encapsulates access to objects in a repository. It\n> > @@ -94,6 +95,13 @@ struct object_database {\n> > \t/* Repository that owns this database. */\n> > \tstruct repository *repo;\n> > \n> > +\t/*\n> > +\t * State of current current object database transaction. Only one\n> > +\t * transaction may be pending at a time. Is NULL when no transaction is\n> > +\t * configured.\n> > +\t */\n> > +\tstruct odb_transaction *transaction;\n> \n> Once dust from this topic settles, we may want to rename the\n> bulk-checkin.[ch] to have \"odb\" somewhere in its name, perhaps? I\n> usually do not like renaming file for the sake of renaming to make\n> the result look \"pretty\" (people may use \"consistent naming\" ),\n> though. I dunno.\n\nI'm hoping for an eventual state where the transactional interfaces,\n{begin,end}_odb_transaction(), are moved directly into odb.{h,c}. The\ncurrent implementation of transaction handling is specific to the\ncurrent object database source. In a pluggable object database future\nwhere we could have different types of object database sources,\ntransaction handling will likely have to implemented separately.\n\nWith this in mind, we could move the current transaction implementation\ninto something like object-files.c. This is already where the vast\nmajority of its call sites are and would enable us to further simply the\ninterface we expose.\n\nI plan do submit a followup series to do this. :)\n\n-Justin\n"},{"id":"524728","messageId":"p6bdiflg2fe7kgw47ntg46csjhmp63gkovtf4inlc42f4dga77@5oieqa4bydne","threadId":"64002","inReplyTo":"xmqqqzx3xpwc.fsf@gitster.g","subject":"Re: [PATCH v2 3/4] bulk-checkin: require transaction for index_blob_bulk_checkin()","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-22T19:13:27Z","receivedAt":"2025-08-22T19:13:29Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/08/22 09:49AM, Junio C Hamano wrote:\n> Justin Tobler writes:\n> \n> > /*\n> > - * This creates one packfile per large blob unless bulk-checkin\n> > - * machinery is \"plugged\".\n> > + * This writes the specified object to a packfile. Objects written here\n> > + * during the same transaction are written to the same packfile. The\n> > + * packfile is not flushed until the transaction is flushed. The caller\n> > + * is expected to ensure a valid transaction is setup for objects to be\n> > + * recorded to.\n> > *\n> > * This also bypasses the usual \"convert-to-git\" dance, and that is on\n> > * purpose. We could write a streaming version of the converting\n> > diff --git a/object-file.c b/object-file.c\n> > index 1740aa2b2e3..bc15af42450 100644\n> > --- a/object-file.c\n> > +++ b/object-file.c\n> > @@ -1253,19 +1253,26 @@ int index_fd(struct index_state *istate, struct object_id *oid,\n> > \t * Call xsize_t() only when needed to avoid potentially unnecessary\n> > \t * die() for large files.\n> > \t */\n> > -\tif (type == OBJ_BLOB && path && would_convert_to_git_filter_fd(istate, path))\n> > +\tif (type == OBJ_BLOB && path && would_convert_to_git_filter_fd(istate, path)) {\n> > \t\tret = index_stream_convert_blob(istate, oid, fd, path, flags);\n> > -\telse if (!S_ISREG(st->st_mode))\n> > +\t} else if (!S_ISREG(st->st_mode)) {\n> > \t\tret = index_pipe(istate, oid, fd, type, path, flags);\n> > -\telse if ((st->st_size >= 0 && (size_t) st->st_size <= repo_settings_get_big_file_threshold(istate->repo)) ||\n> > -\t\t type != OBJ_BLOB ||\n> > -\t\t (path && would_convert_to_git(istate, path)))\n> > +\t} else if ((st->st_size >= 0 &&\n> > +\t\t (size_t)st->st_size <= repo_settings_get_big_file_threshold(istate->repo)) ||\n> > +\t\t type != OBJ_BLOB ||\n> > +\t\t (path && would_convert_to_git(istate, path))) {\n> > \t\tret = index_core(istate, oid, fd, xsize_t(st->st_size),\n> > \t\t\t\t type, path, flags);\n> > -\telse\n> > -\t\tret = index_blob_bulk_checkin(the_repository->objects->transaction,\n> > +\t} else {\n> > +\t\tstruct odb_transaction *transaction;\n> > +\n> > +\t\ttransaction = begin_odb_transaction(the_repository->objects);\n> > +\t\tret = index_blob_bulk_checkin(transaction,\n> > \t\t\t\t\t oid, fd, xsize_t(st->st_size),\n> > \t\t\t\t\t path, flags);\n> > +\t\tend_odb_transaction(transaction);\n> > +\t}\n> \n> Interesting. If the caller does the odb transaction management\n> itself by calling begin/end before calling this function and fed two\n> or more large objects, the original code did the right thing with\n> .nesting set to 1. In the new code, it still does the right thing\n> even during the call to index_blob_bulk_checkin we raise .nesting by\n> one, because the all non-zero .nesting values mean the same thing to\n> the machinery, thanks to lazy initialization of the .objdir.\n\nIn the original code, if no transaction was setup prior to invoking\nindex_blob_bulk_checkin() (i.e. nesting == 0), the object would be\nwritten to its own packfile and flushed immediately. If a transaction\nhad been started further upstream (i.e nesting > 0), the object would be\nwritten to a packfile, but not flushed. This allowed for subseqent calls\nto index_blob_bulk_checkin() to write objects to the same packfile. Only\nwhen transaction ended would the packfile be flushed.\n\nWith this change, index_blob_bulk_checkin() must be provided a\ntransaction so it can write the object to the packfile. As there is now\nalways a transaction involved, there is no longer any automatic packfile\nflushing. The caller is required to ensure a transaction handling as\nappropriate.\n\n> So, isn't the comment above the function now less accurate than\n> before? The caller of this function does not have to do anything\n> and we do not expect the caller to \"ensure a valid transaction\" at\n> all, no?\n\nI'm not quite sure I follow. index_blob_bulk_checkin() now expects a\ntransaction to be setup even if we intend to only write a single object.\nThus the call site in index_fd() is adjusted to ensure there is a\ntransaction via invoking begin_odb_transaction() and\nend_odb_transaction() before/after the function respectively.\n\nJust to clarify, are we talking about the comment above\nindex_blob_bulk_checkin()?\n\n-Justin\n"},{"id":"524729","messageId":"jd667c3ef7gkxxju5vhrwnibj4guwi5nee2o6bdtc5htgbthxd@ycyasrwbkz6r","threadId":"64002","inReplyTo":"xmqqjz2vxp98.fsf@gitster.g","subject":"Re: [PATCH v2 4/4] bulk-checkin: use repository variable from transaction","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-22T19:38:34Z","receivedAt":"2025-08-22T19:38:36Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/08/22 10:03AM, Junio C Hamano wrote:\n> Justin Tobler writes:\n> \n> > The bulk-checkin subsystem depends on `the_repository`. Adapt functions\n> > and call sites to access the repository through `struct odb_transaction`\n> > instead. The `USE_THE_REPOSITORY_VARIBALE` is still required as the\n> > `pack_compression_level` and `pack_size_limit_cfg` globals are still\n> > used.\n> \n> Also we grab the details of the new packfile the bulk-checkin\n> machinery is building out of the transaction, which made some\n> redundant parameters functions take go away, ...\n\nI'll mention this in the commit message in the next version.\n\n> > @@ -117,7 +118,8 @@ static void flush_batch_fsync(struct odb_transaction *transaction)\n> > \t * to ensure that the data in each new object file is durable before\n> > \t * the final name is visible.\n> > \t */\n> > -\tstrbuf_addf(&temp_path, \"%s/bulk_fsync_XXXXXX\", repo_get_object_directory(the_repository));\n> > +\tstrbuf_addf(&temp_path, \"%s/bulk_fsync_XXXXXX\",\n> > +\t\t transaction->odb->sources->path);\n> \n> This is doing a lot more than a simple \"the_repository\" ->\n> \"odb->repo\" replacement. How much confidence do we have\n> that the internal detail of repo_get_object_directory() will stay\n> the same and our developers in the future would spot that this open\n> coded copy needs to be updated if they have to change it?\n\nThat's fair. With this change we are less defensive since we are not\nbugging out if the object source is not set up. There are other\ninstances throughout the codebase where the object directory is accessed\ndirectly through the repository in an open coded manner instead of going\nthrough repo_get_object_directory(). Regardless, it's probably best to\njust keep using repo_get_object_directory() here anyway.\n\nI'll update in the next version.\n\n> > -static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n> > -\t\t\t\tstruct object_id *result_oid,\n> > -\t\t\t\tint fd, size_t size,\n> > -\t\t\t\tconst char *path, unsigned flags)\n> > +int index_blob_bulk_checkin(struct odb_transaction *transaction,\n> > +\t\t\t struct object_id *result_oid,\n> > +\t\t\t int fd, size_t size,\n> > +\t\t\t const char *path, unsigned flags)\n> \n> Ahh, OK, with the simplification to always take transaction, there\n> is no need to have the deflate_blob_to_pack() function, which had\n> only one caller, as an internal implementation detail anymore.\n> \n> This change could have been in the previous step and it would have\n> been less surprising; the above \"Ahh, OK\" is my reaction to the\n> surprise ;-)\n\nIn the next version, I'll perform this in the previous step instead. :)\n\nThanks,\n-Justin\n"},{"id":"524731","messageId":"xmqqikifw1cl.fsf@gitster.g","threadId":"64002","inReplyTo":"njvcqkkcotesqznalieuwhzow7lxbejxlfw3knbhalmqjcb7cs@3vwea4r2zljl","subject":"Re: [PATCH v2 2/4] bulk-checkin: remove global transaction state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-22T20:25:14Z","receivedAt":"2025-08-22T20:25:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler writes:\n\n> If it is preferable, I can combine these two patches together. I kept\n> them separate as I thought it would better explain the steps, but maybe\n> that isn't the best way to structure the patch as we are largely undoing\n> the change in the next patch anyway.\n\nThis shouldn't make much difference, but I prefer the current\n\"awkared but done as two steps\" arrangement slightly better.\n\n> I'm hoping for an eventual state where the transactional interfaces,\n> {begin,end}_odb_transaction(), are moved directly into odb.{h,c}.\n\nYes.\n\n> The\n> current implementation of transaction handling is specific to the\n> current object database source. In a pluggable object database future\n> where we could have different types of object database sources,\n> transaction handling will likely have to implemented separately.\n\nYup.\n\n> With this in mind, we could move the current transaction implementation\n> into something like object-files.c. This is already where the vast\n> majority of its call sites are and would enable us to further simply the\n> interface we expose.\n\nGreat.\n"},{"id":"524733","messageId":"xmqqa53rw0ye.fsf@gitster.g","threadId":"64002","inReplyTo":"p6bdiflg2fe7kgw47ntg46csjhmp63gkovtf4inlc42f4dga77@5oieqa4bydne","subject":"Re: [PATCH v2 3/4] bulk-checkin: require transaction for index_blob_bulk_checkin()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-22T20:33:45Z","receivedAt":"2025-08-22T20:33:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler writes:\n\n> On 25/08/22 09:49AM, Junio C Hamano wrote:\n>> Justin Tobler writes:\n>> \n>> > /*\n>> > - * This creates one packfile per large blob unless bulk-checkin\n>> > - * machinery is \"plugged\".\n>> > + * This writes the specified object to a packfile. Objects written here\n>> > + * during the same transaction are written to the same packfile. The\n>> > + * packfile is not flushed until the transaction is flushed. The caller\n>> > + * is expected to ensure a valid transaction is setup for objects to be\n>> > + * recorded to.\n>> > *\n>> > * This also bypasses the usual \"convert-to-git\" dance, and that is on\n>> > * purpose. We could write a streaming version of the converting\n>> > diff --git a/object-file.c b/object-file.c\n>> > index 1740aa2b2e3..bc15af42450 100644\n>> > --- a/object-file.c\n>> > +++ b/object-file.c\n>> > @@ -1253,19 +1253,26 @@ int index_fd(struct index_state *istate, struct object_id *oid,\n>> > \t * Call xsize_t() only when needed to avoid potentially unnecessary\n>> > \t * die() for large files.\n>> > \t */\n>> > -\tif (type == OBJ_BLOB && path && would_convert_to_git_filter_fd(istate, path))\n>> > +\tif (type == OBJ_BLOB && path && would_convert_to_git_filter_fd(istate, path)) {\n>> > \t\tret = index_stream_convert_blob(istate, oid, fd, path, flags);\n>> > -\telse if (!S_ISREG(st->st_mode))\n>> > +\t} else if (!S_ISREG(st->st_mode)) {\n>> > \t\tret = index_pipe(istate, oid, fd, type, path, flags);\n>> > -\telse if ((st->st_size >= 0 && (size_t) st->st_size <= repo_settings_get_big_file_threshold(istate->repo)) ||\n>> > -\t\t type != OBJ_BLOB ||\n>> > -\t\t (path && would_convert_to_git(istate, path)))\n>> > +\t} else if ((st->st_size >= 0 &&\n>> > +\t\t (size_t)st->st_size <= repo_settings_get_big_file_threshold(istate->repo)) ||\n>> > +\t\t type != OBJ_BLOB ||\n>> > +\t\t (path && would_convert_to_git(istate, path))) {\n>> > \t\tret = index_core(istate, oid, fd, xsize_t(st->st_size),\n>> > \t\t\t\t type, path, flags);\n>> > -\telse\n>> > -\t\tret = index_blob_bulk_checkin(the_repository->objects->transaction,\n>> > +\t} else {\n>> > +\t\tstruct odb_transaction *transaction;\n>> > +\n>> > +\t\ttransaction = begin_odb_transaction(the_repository->objects);\n>> > +\t\tret = index_blob_bulk_checkin(transaction,\n>> > \t\t\t\t\t oid, fd, xsize_t(st->st_size),\n>> > \t\t\t\t\t path, flags);\n>> > +\t\tend_odb_transaction(transaction);\n>> > +\t}\n>> \n>> Interesting. If the caller does the odb transaction management\n>> itself by calling begin/end before calling this function and fed two\n>> or more large objects, the original code did the right thing with\n>> .nesting set to 1. In the new code, it still does the right thing\n>> even during the call to index_blob_bulk_checkin we raise .nesting by\n>> one, because the all non-zero .nesting values mean the same thing to\n>> the machinery, thanks to lazy initialization of the .objdir.\n>\n> In the original code, if no transaction was setup prior to invoking\n> index_blob_bulk_checkin() (i.e. nesting == 0), the object would be\n> written to its own packfile and flushed immediately. If a transaction\n> had been started further upstream (i.e nesting > 0), the object would be\n> written to a packfile, but not flushed. This allowed for subseqent calls\n> to index_blob_bulk_checkin() to write objects to the same packfile. Only\n> when transaction ended would the packfile be flushed.\n>\n> With this change, index_blob_bulk_checkin() must be provided a\n> transaction so it can write the object to the packfile. As there is now\n> always a transaction involved, there is no longer any automatic packfile\n> flushing. The caller is required to ensure a transaction handling as\n> appropriate.\n>\n>> So, isn't the comment above the function now less accurate than\n>> before? The caller of this function does not have to do anything\n>> and we do not expect the caller to \"ensure a valid transaction\" at\n>> all, no?\n>\n> I'm not quite sure I follow. index_blob_bulk_checkin() now expects a\n> transaction to be setup even if we intend to only write a single object.\n> Thus the call site in index_fd() is adjusted to ensure there is a\n> transaction via invoking begin_odb_transaction() and\n> end_odb_transaction() before/after the function respectively.\n>\n> Just to clarify, are we talking about the comment above\n> index_blob_bulk_checkin()?\n\nAhh, sorry, I misread the patch and missed the hunk/file boundary;\niow, thought the comment was about index_fd(), which was the source\nof my confusion.\n\nThanks for correcting me.\n"},{"id":"524736","messageId":"20250822213500.1488064-1-jltobler@gmail.com","threadId":"64002","inReplyTo":"20250821232249.319427-1-jltobler@gmail.com","subject":"[PATCH v3 0/4] bulk-checkin: remove global transaction state","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-22T21:34:56Z","receivedAt":"2025-08-22T21:35:06Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Greetings,\n\nThe bulk-checkin subsystem provides an interface to write objects to the\nobject database in a bulk transaction. The state of an ongoing\ntransaction is stored across several global variables. This series aims\nto remove this global transaction state in favor of storing state in in\n`struct object_database`. This is done in preparation for a follow-up\nchange where the goal is to eventually move these transaction interfaces\ninto \"odb.h\".\n\nChanges since V2:\n\n- `index_blob_bulk_checkin()` is combined with\n `deflate_blob_bulk_checkin()` in patch 3 instead of 4.\n- Continue to use `repo_get_object_directory()` instead of open coding.\n\nChanges since V1:\n\n- `index_blob_bulk_checkin()` now assumes that the caller always\n provides a setup `struct odb_transaction`. Callers are adjusted to\n ensure this.\n- Functions in bulk-checkin.c now consistently access the repository\n through the provided `odb_transaction`.\n\nThanks,\n-Justin\n\nJustin Tobler (4):\n bulk-checkin: introduce object database transaction structure\n bulk-checkin: remove global transaction state\n bulk-checkin: require transaction for index_blob_bulk_checkin()\n bulk-checkin: use repository variable from transaction\n\n builtin/add.c | 5 +-\n builtin/unpack-objects.c | 5 +-\n builtin/update-index.c | 7 +-\n bulk-checkin.c | 152 +++++++++++++++++++++------------------\n bulk-checkin.h | 25 ++++---\n cache-tree.c | 5 +-\n object-file.c | 30 +++++---\n odb.h | 8 +++\n read-cache.c | 5 +-\n 9 files changed, 141 insertions(+), 101 deletions(-)\n\nRange-diff against v2:\n1: 5c9358e0b03 = 1: 5c9358e0b03 bulk-checkin: introduce object database transaction structure\n2: 4a1b80a6baf = 2: 4a1b80a6baf bulk-checkin: remove global transaction state\n3: ce329932fdd ! 3: ae5dbd0e1af bulk-checkin: require transaction for index_blob_bulk_checkin()\n @@ Commit message\n \n Update `index_blob_bulk_checkin()` to assume that a valid transaction is\n always provided. Callers are now expected to ensure a transaction is set\n - up beforehand. The single call site in `object-file.c:index_fd()` is\n - updated accordingly. Due to how `{begin,end}_odb_transaction()` handles\n - nested transactions, a new transaction is only created and committed if\n - there is not already an ongoing transaction.\n + up beforehand. With this simplification, `deflate_blob_bulk_checkin()`\n + is no longer needed as a standalone internal function and is combined\n + with `index_blob_bulk_checkin()`. The single call site in\n + `object-file.c:index_fd()` is updated accordingly. Due to how\n + `{begin,end}_odb_transaction()` handles nested transactions, a new\n + transaction is only created and committed if there is not already an\n + ongoing transaction.\n \n Signed-off-by: Justin Tobler \n \n ## bulk-checkin.c ##\n -@@ bulk-checkin.c: int index_blob_bulk_checkin(struct odb_transaction *transaction,\n - \t\t\t struct object_id *oid, int fd, size_t size,\n - \t\t\t const char *path, unsigned flags)\n +@@ bulk-checkin.c: static void prepare_to_stream(struct bulk_checkin_packfile *state,\n + \t\tdie_errno(\"unable to write pack header\");\n + }\n + \n +-static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n +-\t\t\t\tstruct object_id *result_oid,\n +-\t\t\t\tint fd, size_t size,\n +-\t\t\t\tconst char *path, unsigned flags)\n ++int index_blob_bulk_checkin(struct odb_transaction *transaction,\n ++\t\t\t struct object_id *result_oid, int fd, size_t size,\n ++\t\t\t const char *path, unsigned flags)\n {\n ++\tstruct bulk_checkin_packfile *state = &transaction->packfile;\n + \toff_t seekback, already_hashed_to;\n + \tstruct git_hash_ctx ctx;\n + \tunsigned char obuf[16384];\n +@@ bulk-checkin.c: void fsync_loose_object_bulk_checkin(struct odb_transaction *transaction,\n + \t}\n + }\n + \n +-int index_blob_bulk_checkin(struct odb_transaction *transaction,\n +-\t\t\t struct object_id *oid, int fd, size_t size,\n +-\t\t\t const char *path, unsigned flags)\n +-{\n -\tint status;\n -\n -\tif (transaction) {\n @@ bulk-checkin.c: int index_blob_bulk_checkin(struct odb_transaction *transaction,\n -\t}\n -\n -\treturn status;\n -+\treturn deflate_blob_to_pack(&transaction->packfile, oid, fd, size, path,\n -+\t\t\t\t flags);\n - }\n - \n +-}\n +-\n struct odb_transaction *begin_odb_transaction(struct object_database *odb)\n + {\n + \tif (!odb->transaction) {\n \n ## bulk-checkin.h ##\n @@ bulk-checkin.h: void fsync_loose_object_bulk_checkin(struct odb_transaction *transaction,\n4: 08e26647915 ! 4: a05af82fddb bulk-checkin: use repository variable from transaction\n @@ Commit message\n `pack_compression_level` and `pack_size_limit_cfg` globals are still\n used.\n \n + Also adapt functions using packfile state to instead access it through\n + the transaction. This makes some function parameters redundant and go\n + away.\n +\n Signed-off-by: Justin Tobler \n \n ## bulk-checkin.c ##\n @@ bulk-checkin.c: static void flush_bulk_checkin_packfile(struct bulk_checkin_pack\n -\t\t\t state->written, state->nr_written,\n -\t\t\t &state->pack_idx_opts, hash);\n +\tstrbuf_addf(&packname, \"%s/pack/pack-%s.\",\n -+\t\t transaction->odb->sources->path,\n ++\t\t repo_get_object_directory(transaction->odb->repo),\n +\t\t hash_to_hex_algop(hash, repo->hash_algo));\n +\n +\tfinish_tmp_packfile(transaction, &packname, hash);\n @@ bulk-checkin.c: static void flush_batch_fsync(struct odb_transaction *transactio\n \t */\n -\tstrbuf_addf(&temp_path, \"%s/bulk_fsync_XXXXXX\", repo_get_object_directory(the_repository));\n +\tstrbuf_addf(&temp_path, \"%s/bulk_fsync_XXXXXX\",\n -+\t\t transaction->odb->sources->path);\n ++\t\t repo_get_object_directory(transaction->odb->repo));\n \ttemp = xmks_tempfile(temp_path.buf);\n \tfsync_or_die(get_tempfile_fd(temp), get_tempfile_path(temp));\n \tdelete_tempfile(&temp);\n @@ bulk-checkin.c: static int stream_blob_to_pack(struct bulk_checkin_packfile *sta\n \treset_pack_idx_option(&state->pack_idx_opts);\n \n \t/* Pretend we are going to write only one object */\n -@@ bulk-checkin.c: static void prepare_to_stream(struct bulk_checkin_packfile *state,\n - \t\tdie_errno(\"unable to write pack header\");\n - }\n - \n --static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n --\t\t\t\tstruct object_id *result_oid,\n --\t\t\t\tint fd, size_t size,\n --\t\t\t\tconst char *path, unsigned flags)\n -+int index_blob_bulk_checkin(struct odb_transaction *transaction,\n -+\t\t\t struct object_id *result_oid,\n -+\t\t\t int fd, size_t size,\n -+\t\t\t const char *path, unsigned flags)\n - {\n -+\tstruct bulk_checkin_packfile *state = &transaction->packfile;\n - \toff_t seekback, already_hashed_to;\n - \tstruct git_hash_ctx ctx;\n - \tunsigned char obuf[16384];\n -@@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n +@@ bulk-checkin.c: int index_blob_bulk_checkin(struct odb_transaction *transaction,\n \n \theader_len = format_object_header((char *)obuf, sizeof(obuf),\n \t\t\t\t\t OBJ_BLOB, size);\n @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st\n \t\tif (idx) {\n \t\t\thashfile_checkpoint(state->f, &checkpoint);\n \t\t\tidx->offset = state->offset;\n -@@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n +@@ bulk-checkin.c: int index_blob_bulk_checkin(struct odb_transaction *transaction,\n \t\t\tBUG(\"should not happen\");\n \t\thashfile_truncate(state->f, &checkpoint);\n \t\tstate->offset = checkpoint.offset;\n @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st\n \t\tif (lseek(fd, seekback, SEEK_SET) == (off_t) -1)\n \t\t\treturn error(\"cannot seek back\");\n \t}\n -@@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n +@@ bulk-checkin.c: int index_blob_bulk_checkin(struct odb_transaction *transaction,\n \t\treturn 0;\n \n \tidx->crc32 = crc32_end(state->f);\n @@ bulk-checkin.c: void prepare_loose_object_bulk_checkin(struct odb_transaction *t\n \tif (transaction->objdir)\n \t\ttmp_objdir_replace_primary_odb(transaction->objdir, 0);\n }\n -@@ bulk-checkin.c: void fsync_loose_object_bulk_checkin(struct odb_transaction *transaction,\n - \t}\n - }\n - \n --int index_blob_bulk_checkin(struct odb_transaction *transaction,\n --\t\t\t struct object_id *oid, int fd, size_t size,\n --\t\t\t const char *path, unsigned flags)\n --{\n --\treturn deflate_blob_to_pack(&transaction->packfile, oid, fd, size, path,\n --\t\t\t\t flags);\n --}\n --\n - struct odb_transaction *begin_odb_transaction(struct object_database *odb)\n - {\n - \tif (!odb->transaction) {\n @@ bulk-checkin.c: void flush_odb_transaction(struct odb_transaction *transaction)\n \t\treturn;\n \n\nbase-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0\n-- \n2.51.0\n\n"},{"id":"524737","messageId":"20250822213500.1488064-2-jltobler@gmail.com","threadId":"64002","inReplyTo":"20250822213500.1488064-1-jltobler@gmail.com","subject":"[PATCH v3 1/4] bulk-checkin: introduce object database transaction structure","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-22T21:34:57Z","receivedAt":"2025-08-22T21:35:06Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Object database transaction state is stored across several global\nvariables in the bulk-checkin subsystem. Consolidate this state into a\nsingle `struct odb_transaction` global. In a subsequent commit, the\ntransactional interfaces will be updated to wire this structure instead\nof relying on a global variable.\n\nSigned-off-by: Justin Tobler \n---\n bulk-checkin.c | 46 ++++++++++++++++++++++++----------------------\n 1 file changed, 24 insertions(+), 22 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex b2809ab0398..82a73da79e8 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -19,11 +19,7 @@\n #include \"object-file.h\"\n #include \"odb.h\"\n \n-static int odb_transaction_nesting;\n-\n-static struct tmp_objdir *bulk_fsync_objdir;\n-\n-static struct bulk_checkin_packfile {\n+struct bulk_checkin_packfile {\n \tchar *pack_tmp_name;\n \tstruct hashfile *f;\n \toff_t offset;\n@@ -32,7 +28,13 @@ static struct bulk_checkin_packfile {\n \tstruct pack_idx_entry **written;\n \tuint32_t alloc_written;\n \tuint32_t nr_written;\n-} bulk_checkin_packfile;\n+};\n+\n+static struct odb_transaction {\n+\tint nesting;\n+\tstruct tmp_objdir *objdir;\n+\tstruct bulk_checkin_packfile packfile;\n+} transaction;\n \n static void finish_tmp_packfile(struct strbuf *basename,\n \t\t\t\tconst char *pack_tmp_name,\n@@ -101,7 +103,7 @@ static void flush_batch_fsync(void)\n \tstruct strbuf temp_path = STRBUF_INIT;\n \tstruct tempfile *temp;\n \n-\tif (!bulk_fsync_objdir)\n+\tif (!transaction.objdir)\n \t\treturn;\n \n \t/*\n@@ -123,8 +125,8 @@ static void flush_batch_fsync(void)\n \t * Make the object files visible in the primary ODB after their data is\n \t * fully durable.\n \t */\n-\ttmp_objdir_migrate(bulk_fsync_objdir);\n-\tbulk_fsync_objdir = NULL;\n+\ttmp_objdir_migrate(transaction.objdir);\n+\ttransaction.objdir = NULL;\n }\n \n static int already_written(struct bulk_checkin_packfile *state, struct object_id *oid)\n@@ -331,12 +333,12 @@ void prepare_loose_object_bulk_checkin(void)\n \t * callers may not know whether any objects will be\n \t * added at the time they call begin_odb_transaction.\n \t */\n-\tif (!odb_transaction_nesting || bulk_fsync_objdir)\n+\tif (!transaction.nesting || transaction.objdir)\n \t\treturn;\n \n-\tbulk_fsync_objdir = tmp_objdir_create(the_repository, \"bulk-fsync\");\n-\tif (bulk_fsync_objdir)\n-\t\ttmp_objdir_replace_primary_odb(bulk_fsync_objdir, 0);\n+\ttransaction.objdir = tmp_objdir_create(the_repository, \"bulk-fsync\");\n+\tif (transaction.objdir)\n+\t\ttmp_objdir_replace_primary_odb(transaction.objdir, 0);\n }\n \n void fsync_loose_object_bulk_checkin(int fd, const char *filename)\n@@ -348,7 +350,7 @@ void fsync_loose_object_bulk_checkin(int fd, const char *filename)\n \t * before renaming the objects to their final names as part of\n \t * flush_batch_fsync.\n \t */\n-\tif (!bulk_fsync_objdir ||\n+\tif (!transaction.objdir ||\n \t git_fsync(fd, FSYNC_WRITEOUT_ONLY) < 0) {\n \t\tif (errno == ENOSYS)\n \t\t\twarning(_(\"core.fsyncMethod = batch is unsupported on this platform\"));\n@@ -360,31 +362,31 @@ int index_blob_bulk_checkin(struct object_id *oid,\n \t\t\t int fd, size_t size,\n \t\t\t const char *path, unsigned flags)\n {\n-\tint status = deflate_blob_to_pack(&bulk_checkin_packfile, oid, fd, size,\n+\tint status = deflate_blob_to_pack(&transaction.packfile, oid, fd, size,\n \t\t\t\t\t path, flags);\n-\tif (!odb_transaction_nesting)\n-\t\tflush_bulk_checkin_packfile(&bulk_checkin_packfile);\n+\tif (!transaction.nesting)\n+\t\tflush_bulk_checkin_packfile(&transaction.packfile);\n \treturn status;\n }\n \n void begin_odb_transaction(void)\n {\n-\todb_transaction_nesting += 1;\n+\ttransaction.nesting += 1;\n }\n \n void flush_odb_transaction(void)\n {\n \tflush_batch_fsync();\n-\tflush_bulk_checkin_packfile(&bulk_checkin_packfile);\n+\tflush_bulk_checkin_packfile(&transaction.packfile);\n }\n \n void end_odb_transaction(void)\n {\n-\todb_transaction_nesting -= 1;\n-\tif (odb_transaction_nesting < 0)\n+\ttransaction.nesting -= 1;\n+\tif (transaction.nesting < 0)\n \t\tBUG(\"Unbalanced ODB transaction nesting\");\n \n-\tif (odb_transaction_nesting)\n+\tif (transaction.nesting)\n \t\treturn;\n \n \tflush_odb_transaction();\n-- \n2.51.0\n\n"},{"id":"524738","messageId":"20250822213500.1488064-3-jltobler@gmail.com","threadId":"64002","inReplyTo":"20250822213500.1488064-1-jltobler@gmail.com","subject":"[PATCH v3 2/4] bulk-checkin: remove global transaction state","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-22T21:34:58Z","receivedAt":"2025-08-22T21:35:07Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Object database transactions in the bulk-checkin subsystem rely on\nglobal state to track transaction status. Stop relying on global state\nand instead store the transaction in the `struct object_database`.\nFunctions that operate on transactions are updated to now wire\ntransaction state.\n\nSigned-off-by: Justin Tobler \n---\n builtin/add.c | 5 ++-\n builtin/unpack-objects.c | 5 ++-\n builtin/update-index.c | 7 ++--\n bulk-checkin.c | 82 ++++++++++++++++++++++++++--------------\n bulk-checkin.h | 18 +++++----\n cache-tree.c | 5 ++-\n object-file.c | 11 +++---\n odb.h | 8 ++++\n read-cache.c | 5 ++-\n 9 files changed, 94 insertions(+), 52 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 0235854f809..740c7c45817 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -389,6 +389,7 @@ int cmd_add(int argc,\n \tchar *seen = NULL;\n \tchar *ps_matched = NULL;\n \tstruct lock_file lock_file = LOCK_INIT;\n+\tstruct odb_transaction *transaction;\n \n \trepo_config(repo, add_config, NULL);\n \n@@ -574,7 +575,7 @@ int cmd_add(int argc,\n \t\tstring_list_clear(&only_match_skip_worktree, 0);\n \t}\n \n-\tbegin_odb_transaction();\n+\ttransaction = begin_odb_transaction(repo->objects);\n \n \tps_matched = xcalloc(pathspec.nr, 1);\n \tif (add_renormalize)\n@@ -593,7 +594,7 @@ int cmd_add(int argc,\n \n \tif (chmod_arg && pathspec.nr)\n \t\texit_status |= chmod_pathspec(repo, &pathspec, chmod_arg[0], show_only);\n-\tend_odb_transaction();\n+\tend_odb_transaction(transaction);\n \n finish:\n \tif (write_locked_index(repo->index, &lock_file,\ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex 7ae7c82b6c0..28124b324d2 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -584,6 +584,7 @@ static void unpack_all(void)\n {\n \tint i;\n \tunsigned char *hdr = fill(sizeof(struct pack_header));\n+\tstruct odb_transaction *transaction;\n \n \tif (get_be32(hdr) != PACK_SIGNATURE)\n \t\tdie(\"bad pack file\");\n@@ -599,12 +600,12 @@ static void unpack_all(void)\n \t\tprogress = start_progress(the_repository,\n \t\t\t\t\t _(\"Unpacking objects\"), nr_objects);\n \tCALLOC_ARRAY(obj_list, nr_objects);\n-\tbegin_odb_transaction();\n+\ttransaction = begin_odb_transaction(the_repository->objects);\n \tfor (i = 0; i < nr_objects; i++) {\n \t\tunpack_one(i);\n \t\tdisplay_progress(progress, i + 1);\n \t}\n-\tend_odb_transaction();\n+\tend_odb_transaction(transaction);\n \tstop_progress(&progress);\n \n \tif (delta_list)\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 2380f3ccd68..2ba2d29c959 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -77,7 +77,7 @@ static void report(const char *fmt, ...)\n \t * objects invisible while a transaction is active, so flush the\n \t * transaction here before reporting a change made by update-index.\n \t */\n-\tflush_odb_transaction();\n+\tflush_odb_transaction(the_repository->objects->transaction);\n \tva_start(vp, fmt);\n \tvprintf(fmt, vp);\n \tputchar('\\n');\n@@ -940,6 +940,7 @@ int cmd_update_index(int argc,\n \tstrbuf_getline_fn getline_fn;\n \tint parseopt_state = PARSE_OPT_UNKNOWN;\n \tstruct repository *r = the_repository;\n+\tstruct odb_transaction *transaction;\n \tstruct option options[] = {\n \t\tOPT_BIT('q', NULL, &refresh_args.flags,\n \t\t\tN_(\"continue refresh even when index needs update\"),\n@@ -1130,7 +1131,7 @@ int cmd_update_index(int argc,\n \t * Allow the object layer to optimize adding multiple objects in\n \t * a batch.\n \t */\n-\tbegin_odb_transaction();\n+\ttransaction = begin_odb_transaction(the_repository->objects);\n \twhile (ctx.argc) {\n \t\tif (parseopt_state != PARSE_OPT_DONE)\n \t\t\tparseopt_state = parse_options_step(&ctx, options,\n@@ -1213,7 +1214,7 @@ int cmd_update_index(int argc,\n \t/*\n \t * By now we have added all of the new objects\n \t */\n-\tend_odb_transaction();\n+\tend_odb_transaction(transaction);\n \n \tif (split_index > 0) {\n \t\tif (repo_config_get_split_index(the_repository) == 0)\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 82a73da79e8..53a20a2d92f 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -30,11 +30,13 @@ struct bulk_checkin_packfile {\n \tuint32_t nr_written;\n };\n \n-static struct odb_transaction {\n+struct odb_transaction {\n+\tstruct object_database *odb;\n+\n \tint nesting;\n \tstruct tmp_objdir *objdir;\n \tstruct bulk_checkin_packfile packfile;\n-} transaction;\n+};\n \n static void finish_tmp_packfile(struct strbuf *basename,\n \t\t\t\tconst char *pack_tmp_name,\n@@ -98,12 +100,12 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n /*\n * Cleanup after batch-mode fsync_object_files.\n */\n-static void flush_batch_fsync(void)\n+static void flush_batch_fsync(struct odb_transaction *transaction)\n {\n \tstruct strbuf temp_path = STRBUF_INIT;\n \tstruct tempfile *temp;\n \n-\tif (!transaction.objdir)\n+\tif (!transaction->objdir)\n \t\treturn;\n \n \t/*\n@@ -125,8 +127,8 @@ static void flush_batch_fsync(void)\n \t * Make the object files visible in the primary ODB after their data is\n \t * fully durable.\n \t */\n-\ttmp_objdir_migrate(transaction.objdir);\n-\ttransaction.objdir = NULL;\n+\ttmp_objdir_migrate(transaction->objdir);\n+\ttransaction->objdir = NULL;\n }\n \n static int already_written(struct bulk_checkin_packfile *state, struct object_id *oid)\n@@ -325,7 +327,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \treturn 0;\n }\n \n-void prepare_loose_object_bulk_checkin(void)\n+void prepare_loose_object_bulk_checkin(struct odb_transaction *transaction)\n {\n \t/*\n \t * We lazily create the temporary object directory\n@@ -333,15 +335,16 @@ void prepare_loose_object_bulk_checkin(void)\n \t * callers may not know whether any objects will be\n \t * added at the time they call begin_odb_transaction.\n \t */\n-\tif (!transaction.nesting || transaction.objdir)\n+\tif (!transaction || transaction->objdir)\n \t\treturn;\n \n-\ttransaction.objdir = tmp_objdir_create(the_repository, \"bulk-fsync\");\n-\tif (transaction.objdir)\n-\t\ttmp_objdir_replace_primary_odb(transaction.objdir, 0);\n+\ttransaction->objdir = tmp_objdir_create(the_repository, \"bulk-fsync\");\n+\tif (transaction->objdir)\n+\t\ttmp_objdir_replace_primary_odb(transaction->objdir, 0);\n }\n \n-void fsync_loose_object_bulk_checkin(int fd, const char *filename)\n+void fsync_loose_object_bulk_checkin(struct odb_transaction *transaction,\n+\t\t\t\t int fd, const char *filename)\n {\n \t/*\n \t * If we have an active ODB transaction, we issue a call that\n@@ -350,7 +353,7 @@ void fsync_loose_object_bulk_checkin(int fd, const char *filename)\n \t * before renaming the objects to their final names as part of\n \t * flush_batch_fsync.\n \t */\n-\tif (!transaction.objdir ||\n+\tif (!transaction || !transaction->objdir ||\n \t git_fsync(fd, FSYNC_WRITEOUT_ONLY) < 0) {\n \t\tif (errno == ENOSYS)\n \t\t\twarning(_(\"core.fsyncMethod = batch is unsupported on this platform\"));\n@@ -358,36 +361,57 @@ void fsync_loose_object_bulk_checkin(int fd, const char *filename)\n \t}\n }\n \n-int index_blob_bulk_checkin(struct object_id *oid,\n-\t\t\t int fd, size_t size,\n+int index_blob_bulk_checkin(struct odb_transaction *transaction,\n+\t\t\t struct object_id *oid, int fd, size_t size,\n \t\t\t const char *path, unsigned flags)\n {\n-\tint status = deflate_blob_to_pack(&transaction.packfile, oid, fd, size,\n-\t\t\t\t\t path, flags);\n-\tif (!transaction.nesting)\n-\t\tflush_bulk_checkin_packfile(&transaction.packfile);\n+\tint status;\n+\n+\tif (transaction) {\n+\t\tstatus = deflate_blob_to_pack(&transaction->packfile, oid, fd,\n+\t\t\t\t\t size, path, flags);\n+\t} else {\n+\t\tstruct bulk_checkin_packfile state = { 0 };\n+\n+\t\tstatus = deflate_blob_to_pack(&state, oid, fd, size, path, flags);\n+\t\tflush_bulk_checkin_packfile(&state);\n+\t}\n+\n \treturn status;\n }\n \n-void begin_odb_transaction(void)\n+struct odb_transaction *begin_odb_transaction(struct object_database *odb)\n {\n-\ttransaction.nesting += 1;\n+\tif (!odb->transaction) {\n+\t\tCALLOC_ARRAY(odb->transaction, 1);\n+\t\todb->transaction->odb = odb;\n+\t}\n+\n+\todb->transaction->nesting += 1;\n+\n+\treturn odb->transaction;\n }\n \n-void flush_odb_transaction(void)\n+void flush_odb_transaction(struct odb_transaction *transaction)\n {\n-\tflush_batch_fsync();\n-\tflush_bulk_checkin_packfile(&transaction.packfile);\n+\tif (!transaction)\n+\t\treturn;\n+\n+\tflush_batch_fsync(transaction);\n+\tflush_bulk_checkin_packfile(&transaction->packfile);\n }\n \n-void end_odb_transaction(void)\n+void end_odb_transaction(struct odb_transaction *transaction)\n {\n-\ttransaction.nesting -= 1;\n-\tif (transaction.nesting < 0)\n+\tif (!transaction || transaction->nesting == 0)\n \t\tBUG(\"Unbalanced ODB transaction nesting\");\n \n-\tif (transaction.nesting)\n+\ttransaction->nesting -= 1;\n+\n+\tif (transaction->nesting)\n \t\treturn;\n \n-\tflush_odb_transaction();\n+\tflush_odb_transaction(transaction);\n+\ttransaction->odb->transaction = NULL;\n+\tfree(transaction);\n }\ndiff --git a/bulk-checkin.h b/bulk-checkin.h\nindex 7246ea58dcf..16254ce6a70 100644\n--- a/bulk-checkin.h\n+++ b/bulk-checkin.h\n@@ -5,9 +5,13 @@\n #define BULK_CHECKIN_H\n \n #include \"object.h\"\n+#include \"odb.h\"\n \n-void prepare_loose_object_bulk_checkin(void);\n-void fsync_loose_object_bulk_checkin(int fd, const char *filename);\n+struct odb_transaction;\n+\n+void prepare_loose_object_bulk_checkin(struct odb_transaction *transaction);\n+void fsync_loose_object_bulk_checkin(struct odb_transaction *transaction,\n+\t\t\t\t int fd, const char *filename);\n \n /*\n * This creates one packfile per large blob unless bulk-checkin\n@@ -24,8 +28,8 @@ void fsync_loose_object_bulk_checkin(int fd, const char *filename);\n * binary blobs, they generally do not want to get any conversion, and\n * callers should avoid this code path when filters are requested.\n */\n-int index_blob_bulk_checkin(struct object_id *oid,\n-\t\t\t int fd, size_t size,\n+int index_blob_bulk_checkin(struct odb_transaction *transaction,\n+\t\t\t struct object_id *oid, int fd, size_t size,\n \t\t\t const char *path, unsigned flags);\n \n /*\n@@ -35,20 +39,20 @@ int index_blob_bulk_checkin(struct object_id *oid,\n * and objects are only visible after the outermost transaction\n * is complete or the transaction is flushed.\n */\n-void begin_odb_transaction(void);\n+struct odb_transaction *begin_odb_transaction(struct object_database *odb);\n \n /*\n * Make any objects that are currently part of a pending object\n * database transaction visible. It is valid to call this function\n * even if no transaction is active.\n */\n-void flush_odb_transaction(void);\n+void flush_odb_transaction(struct odb_transaction *transaction);\n \n /*\n * Tell the object database to make any objects from the\n * current transaction visible if this is the final nested\n * transaction.\n */\n-void end_odb_transaction(void);\n+void end_odb_transaction(struct odb_transaction *transaction);\n \n #endif\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 66ef2becbe0..d225554eedd 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -474,6 +474,7 @@ static int update_one(struct cache_tree *it,\n \n int cache_tree_update(struct index_state *istate, int flags)\n {\n+\tstruct odb_transaction *transaction;\n \tint skip, i;\n \n \ti = verify_cache(istate, flags);\n@@ -489,10 +490,10 @@ int cache_tree_update(struct index_state *istate, int flags)\n \n \ttrace_performance_enter();\n \ttrace2_region_enter(\"cache_tree\", \"update\", the_repository);\n-\tbegin_odb_transaction();\n+\ttransaction = begin_odb_transaction(the_repository->objects);\n \ti = update_one(istate->cache_tree, istate->cache, istate->cache_nr,\n \t\t \"\", 0, &skip, flags);\n-\tend_odb_transaction();\n+\tend_odb_transaction(transaction);\n \ttrace2_region_leave(\"cache_tree\", \"update\", the_repository);\n \ttrace_performance_leave(\"cache_tree_update\");\n \tif (i < 0)\ndiff --git a/object-file.c b/object-file.c\nindex 2bc36ab3ee8..1740aa2b2e3 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -674,7 +674,7 @@ static void close_loose_object(struct odb_source *source,\n \t\tgoto out;\n \n \tif (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))\n-\t\tfsync_loose_object_bulk_checkin(fd, filename);\n+\t\tfsync_loose_object_bulk_checkin(source->odb->transaction, fd, filename);\n \telse if (fsync_object_files > 0)\n \t\tfsync_or_die(fd, filename);\n \telse\n@@ -852,7 +852,7 @@ static int write_loose_object(struct odb_source *source,\n \tstatic struct strbuf filename = STRBUF_INIT;\n \n \tif (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))\n-\t\tprepare_loose_object_bulk_checkin();\n+\t\tprepare_loose_object_bulk_checkin(source->odb->transaction);\n \n \todb_loose_path(source, &filename, oid);\n \n@@ -941,7 +941,7 @@ int stream_loose_object(struct odb_source *source,\n \tint hdrlen;\n \n \tif (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))\n-\t\tprepare_loose_object_bulk_checkin();\n+\t\tprepare_loose_object_bulk_checkin(source->odb->transaction);\n \n \t/* Since oid is not determined, save tmp file to odb path. */\n \tstrbuf_addf(&filename, \"%s/\", source->path);\n@@ -1263,8 +1263,9 @@ int index_fd(struct index_state *istate, struct object_id *oid,\n \t\tret = index_core(istate, oid, fd, xsize_t(st->st_size),\n \t\t\t\t type, path, flags);\n \telse\n-\t\tret = index_blob_bulk_checkin(oid, fd, xsize_t(st->st_size), path,\n-\t\t\t\t\t flags);\n+\t\tret = index_blob_bulk_checkin(the_repository->objects->transaction,\n+\t\t\t\t\t oid, fd, xsize_t(st->st_size),\n+\t\t\t\t\t path, flags);\n \tclose(fd);\n \treturn ret;\n }\ndiff --git a/odb.h b/odb.h\nindex 3dfc66d75a3..a89b2143909 100644\n--- a/odb.h\n+++ b/odb.h\n@@ -84,6 +84,7 @@ struct odb_source {\n \n struct packed_git;\n struct cached_object_entry;\n+struct odb_transaction;\n \n /*\n * The object database encapsulates access to objects in a repository. It\n@@ -94,6 +95,13 @@ struct object_database {\n \t/* Repository that owns this database. */\n \tstruct repository *repo;\n \n+\t/*\n+\t * State of current current object database transaction. Only one\n+\t * transaction may be pending at a time. Is NULL when no transaction is\n+\t * configured.\n+\t */\n+\tstruct odb_transaction *transaction;\n+\n \t/*\n \t * Set of all object directories; the main directory is first (and\n \t * cannot be NULL after initialization). Subsequent directories are\ndiff --git a/read-cache.c b/read-cache.c\nindex 06ad74db228..229b8ef11c9 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -3947,6 +3947,7 @@ int add_files_to_cache(struct repository *repo, const char *prefix,\n \t\t const struct pathspec *pathspec, char *ps_matched,\n \t\t int include_sparse, int flags)\n {\n+\tstruct odb_transaction *transaction;\n \tstruct update_callback_data data;\n \tstruct rev_info rev;\n \n@@ -3972,9 +3973,9 @@ int add_files_to_cache(struct repository *repo, const char *prefix,\n \t * This function is invoked from commands other than 'add', which\n \t * may not have their own transaction active.\n \t */\n-\tbegin_odb_transaction();\n+\ttransaction = begin_odb_transaction(repo->objects);\n \trun_diff_files(&rev, DIFF_RACY_IS_MODIFIED);\n-\tend_odb_transaction();\n+\tend_odb_transaction(transaction);\n \n \trelease_revisions(&rev);\n \treturn !!data.add_errors;\n-- \n2.51.0\n\n"},{"id":"524740","messageId":"20250822213500.1488064-4-jltobler@gmail.com","threadId":"64002","inReplyTo":"20250822213500.1488064-1-jltobler@gmail.com","subject":"[PATCH v3 3/4] bulk-checkin: require transaction for index_blob_bulk_checkin()","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-22T21:34:59Z","receivedAt":"2025-08-22T21:35:07Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"The bulk-checkin subsystem provides a mechanism to write blobs directly\nto a packfile via `index_blob_bulk_checkin()`. If there is an ongoing\ntransaction when invoked, objects written via this function are stored\nin the same packfile. The packfile is not flushed until the transaction\nitself is flushed. If there is no transaction, the single object is\nwritten to a packfile and immediately flushed. This complicates\n`index_blob_bulk_checkin()` as it cannot reliably use the provided\ntransaction to get the associated repository.\n\nUpdate `index_blob_bulk_checkin()` to assume that a valid transaction is\nalways provided. Callers are now expected to ensure a transaction is set\nup beforehand. With this simplification, `deflate_blob_bulk_checkin()`\nis no longer needed as a standalone internal function and is combined\nwith `index_blob_bulk_checkin()`. The single call site in\n`object-file.c:index_fd()` is updated accordingly. Due to how\n`{begin,end}_odb_transaction()` handles nested transactions, a new\ntransaction is only created and committed if there is not already an\nongoing transaction.\n\nSigned-off-by: Justin Tobler \n---\n bulk-checkin.c | 27 ++++-----------------------\n bulk-checkin.h | 7 +++++--\n object-file.c | 21 ++++++++++++++-------\n 3 files changed, 23 insertions(+), 32 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 53a20a2d92f..542d8125a86 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -254,11 +254,11 @@ static void prepare_to_stream(struct bulk_checkin_packfile *state,\n \t\tdie_errno(\"unable to write pack header\");\n }\n \n-static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n-\t\t\t\tstruct object_id *result_oid,\n-\t\t\t\tint fd, size_t size,\n-\t\t\t\tconst char *path, unsigned flags)\n+int index_blob_bulk_checkin(struct odb_transaction *transaction,\n+\t\t\t struct object_id *result_oid, int fd, size_t size,\n+\t\t\t const char *path, unsigned flags)\n {\n+\tstruct bulk_checkin_packfile *state = &transaction->packfile;\n \toff_t seekback, already_hashed_to;\n \tstruct git_hash_ctx ctx;\n \tunsigned char obuf[16384];\n@@ -361,25 +361,6 @@ void fsync_loose_object_bulk_checkin(struct odb_transaction *transaction,\n \t}\n }\n \n-int index_blob_bulk_checkin(struct odb_transaction *transaction,\n-\t\t\t struct object_id *oid, int fd, size_t size,\n-\t\t\t const char *path, unsigned flags)\n-{\n-\tint status;\n-\n-\tif (transaction) {\n-\t\tstatus = deflate_blob_to_pack(&transaction->packfile, oid, fd,\n-\t\t\t\t\t size, path, flags);\n-\t} else {\n-\t\tstruct bulk_checkin_packfile state = { 0 };\n-\n-\t\tstatus = deflate_blob_to_pack(&state, oid, fd, size, path, flags);\n-\t\tflush_bulk_checkin_packfile(&state);\n-\t}\n-\n-\treturn status;\n-}\n-\n struct odb_transaction *begin_odb_transaction(struct object_database *odb)\n {\n \tif (!odb->transaction) {\ndiff --git a/bulk-checkin.h b/bulk-checkin.h\nindex 16254ce6a70..ac8887f476b 100644\n--- a/bulk-checkin.h\n+++ b/bulk-checkin.h\n@@ -14,8 +14,11 @@ void fsync_loose_object_bulk_checkin(struct odb_transaction *transaction,\n \t\t\t\t int fd, const char *filename);\n \n /*\n- * This creates one packfile per large blob unless bulk-checkin\n- * machinery is \"plugged\".\n+ * This writes the specified object to a packfile. Objects written here\n+ * during the same transaction are written to the same packfile. The\n+ * packfile is not flushed until the transaction is flushed. The caller\n+ * is expected to ensure a valid transaction is setup for objects to be\n+ * recorded to.\n *\n * This also bypasses the usual \"convert-to-git\" dance, and that is on\n * purpose. We could write a streaming version of the converting\ndiff --git a/object-file.c b/object-file.c\nindex 1740aa2b2e3..bc15af42450 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1253,19 +1253,26 @@ int index_fd(struct index_state *istate, struct object_id *oid,\n \t * Call xsize_t() only when needed to avoid potentially unnecessary\n \t * die() for large files.\n \t */\n-\tif (type == OBJ_BLOB && path && would_convert_to_git_filter_fd(istate, path))\n+\tif (type == OBJ_BLOB && path && would_convert_to_git_filter_fd(istate, path)) {\n \t\tret = index_stream_convert_blob(istate, oid, fd, path, flags);\n-\telse if (!S_ISREG(st->st_mode))\n+\t} else if (!S_ISREG(st->st_mode)) {\n \t\tret = index_pipe(istate, oid, fd, type, path, flags);\n-\telse if ((st->st_size >= 0 && (size_t) st->st_size <= repo_settings_get_big_file_threshold(istate->repo)) ||\n-\t\t type != OBJ_BLOB ||\n-\t\t (path && would_convert_to_git(istate, path)))\n+\t} else if ((st->st_size >= 0 &&\n+\t\t (size_t)st->st_size <= repo_settings_get_big_file_threshold(istate->repo)) ||\n+\t\t type != OBJ_BLOB ||\n+\t\t (path && would_convert_to_git(istate, path))) {\n \t\tret = index_core(istate, oid, fd, xsize_t(st->st_size),\n \t\t\t\t type, path, flags);\n-\telse\n-\t\tret = index_blob_bulk_checkin(the_repository->objects->transaction,\n+\t} else {\n+\t\tstruct odb_transaction *transaction;\n+\n+\t\ttransaction = begin_odb_transaction(the_repository->objects);\n+\t\tret = index_blob_bulk_checkin(transaction,\n \t\t\t\t\t oid, fd, xsize_t(st->st_size),\n \t\t\t\t\t path, flags);\n+\t\tend_odb_transaction(transaction);\n+\t}\n+\n \tclose(fd);\n \treturn ret;\n }\n-- \n2.51.0\n\n"},{"id":"524739","messageId":"20250822213500.1488064-5-jltobler@gmail.com","threadId":"64002","inReplyTo":"20250822213500.1488064-1-jltobler@gmail.com","subject":"[PATCH v3 4/4] bulk-checkin: use repository variable from transaction","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-22T21:35:00Z","receivedAt":"2025-08-22T21:35:08Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"The bulk-checkin subsystem depends on `the_repository`. Adapt functions\nand call sites to access the repository through `struct odb_transaction`\ninstead. The `USE_THE_REPOSITORY_VARIBALE` is still required as the\n`pack_compression_level` and `pack_size_limit_cfg` globals are still\nused.\n\nAlso adapt functions using packfile state to instead access it through\nthe transaction. This makes some function parameters redundant and go\naway.\n\nSigned-off-by: Justin Tobler \n---\n bulk-checkin.c | 67 +++++++++++++++++++++++++++-----------------------\n 1 file changed, 36 insertions(+), 31 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 542d8125a86..124c4930676 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -38,25 +38,26 @@ struct odb_transaction {\n \tstruct bulk_checkin_packfile packfile;\n };\n \n-static void finish_tmp_packfile(struct strbuf *basename,\n-\t\t\t\tconst char *pack_tmp_name,\n-\t\t\t\tstruct pack_idx_entry **written_list,\n-\t\t\t\tuint32_t nr_written,\n-\t\t\t\tstruct pack_idx_option *pack_idx_opts,\n+static void finish_tmp_packfile(struct odb_transaction *transaction,\n+\t\t\t\tstruct strbuf *basename,\n \t\t\t\tunsigned char hash[])\n {\n+\tstruct bulk_checkin_packfile *state = &transaction->packfile;\n+\tstruct repository *repo = transaction->odb->repo;\n \tchar *idx_tmp_name = NULL;\n \n-\tstage_tmp_packfiles(the_repository, basename, pack_tmp_name,\n-\t\t\t written_list, nr_written, NULL, pack_idx_opts, hash,\n-\t\t\t &idx_tmp_name);\n-\trename_tmp_packfile_idx(the_repository, basename, &idx_tmp_name);\n+\tstage_tmp_packfiles(repo, basename, state->pack_tmp_name,\n+\t\t\t state->written, state->nr_written, NULL,\n+\t\t\t &state->pack_idx_opts, hash, &idx_tmp_name);\n+\trename_tmp_packfile_idx(repo, basename, &idx_tmp_name);\n \n \tfree(idx_tmp_name);\n }\n \n-static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n+static void flush_bulk_checkin_packfile(struct odb_transaction *transaction)\n {\n+\tstruct bulk_checkin_packfile *state = &transaction->packfile;\n+\tstruct repository *repo = transaction->odb->repo;\n \tunsigned char hash[GIT_MAX_RAWSZ];\n \tstruct strbuf packname = STRBUF_INIT;\n \n@@ -73,17 +74,17 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n \t\t\t\t CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);\n \t} else {\n \t\tint fd = finalize_hashfile(state->f, hash, FSYNC_COMPONENT_PACK, 0);\n-\t\tfixup_pack_header_footer(the_hash_algo, fd, hash, state->pack_tmp_name,\n+\t\tfixup_pack_header_footer(repo->hash_algo, fd, hash, state->pack_tmp_name,\n \t\t\t\t\t state->nr_written, hash,\n \t\t\t\t\t state->offset);\n \t\tclose(fd);\n \t}\n \n-\tstrbuf_addf(&packname, \"%s/pack/pack-%s.\", repo_get_object_directory(the_repository),\n-\t\t hash_to_hex(hash));\n-\tfinish_tmp_packfile(&packname, state->pack_tmp_name,\n-\t\t\t state->written, state->nr_written,\n-\t\t\t &state->pack_idx_opts, hash);\n+\tstrbuf_addf(&packname, \"%s/pack/pack-%s.\",\n+\t\t repo_get_object_directory(transaction->odb->repo),\n+\t\t hash_to_hex_algop(hash, repo->hash_algo));\n+\n+\tfinish_tmp_packfile(transaction, &packname, hash);\n \tfor (uint32_t i = 0; i < state->nr_written; i++)\n \t\tfree(state->written[i]);\n \n@@ -94,7 +95,7 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n \n \tstrbuf_release(&packname);\n \t/* Make objects we just wrote available to ourselves */\n-\treprepare_packed_git(the_repository);\n+\treprepare_packed_git(repo);\n }\n \n /*\n@@ -117,7 +118,8 @@ static void flush_batch_fsync(struct odb_transaction *transaction)\n \t * to ensure that the data in each new object file is durable before\n \t * the final name is visible.\n \t */\n-\tstrbuf_addf(&temp_path, \"%s/bulk_fsync_XXXXXX\", repo_get_object_directory(the_repository));\n+\tstrbuf_addf(&temp_path, \"%s/bulk_fsync_XXXXXX\",\n+\t\t repo_get_object_directory(transaction->odb->repo));\n \ttemp = xmks_tempfile(temp_path.buf);\n \tfsync_or_die(get_tempfile_fd(temp), get_tempfile_path(temp));\n \tdelete_tempfile(&temp);\n@@ -131,16 +133,17 @@ static void flush_batch_fsync(struct odb_transaction *transaction)\n \ttransaction->objdir = NULL;\n }\n \n-static int already_written(struct bulk_checkin_packfile *state, struct object_id *oid)\n+static int already_written(struct odb_transaction *transaction,\n+\t\t\t struct object_id *oid)\n {\n \t/* The object may already exist in the repository */\n-\tif (odb_has_object(the_repository->objects, oid,\n+\tif (odb_has_object(transaction->odb, oid,\n \t\t\t HAS_OBJECT_RECHECK_PACKED | HAS_OBJECT_FETCH_PROMISOR))\n \t\treturn 1;\n \n \t/* Might want to keep the list sorted */\n-\tfor (uint32_t i = 0; i < state->nr_written; i++)\n-\t\tif (oideq(&state->written[i]->oid, oid))\n+\tfor (uint32_t i = 0; i < transaction->packfile.nr_written; i++)\n+\t\tif (oideq(&transaction->packfile.written[i]->oid, oid))\n \t\t\treturn 1;\n \n \t/* This is a new object we need to keep */\n@@ -239,13 +242,15 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n }\n \n /* Lazily create backing packfile for the state */\n-static void prepare_to_stream(struct bulk_checkin_packfile *state,\n+static void prepare_to_stream(struct odb_transaction *transaction,\n \t\t\t unsigned flags)\n {\n+\tstruct bulk_checkin_packfile *state = &transaction->packfile;\n \tif (!(flags & INDEX_WRITE_OBJECT) || state->f)\n \t\treturn;\n \n-\tstate->f = create_tmp_packfile(the_repository, &state->pack_tmp_name);\n+\tstate->f = create_tmp_packfile(transaction->odb->repo,\n+\t\t\t\t &state->pack_tmp_name);\n \treset_pack_idx_option(&state->pack_idx_opts);\n \n \t/* Pretend we are going to write only one object */\n@@ -272,21 +277,21 @@ int index_blob_bulk_checkin(struct odb_transaction *transaction,\n \n \theader_len = format_object_header((char *)obuf, sizeof(obuf),\n \t\t\t\t\t OBJ_BLOB, size);\n-\tthe_hash_algo->init_fn(&ctx);\n+\ttransaction->odb->repo->hash_algo->init_fn(&ctx);\n \tgit_hash_update(&ctx, obuf, header_len);\n \n \t/* Note: idx is non-NULL when we are writing */\n \tif ((flags & INDEX_WRITE_OBJECT) != 0) {\n \t\tCALLOC_ARRAY(idx, 1);\n \n-\t\tprepare_to_stream(state, flags);\n+\t\tprepare_to_stream(transaction, flags);\n \t\thashfile_checkpoint_init(state->f, &checkpoint);\n \t}\n \n \talready_hashed_to = 0;\n \n \twhile (1) {\n-\t\tprepare_to_stream(state, flags);\n+\t\tprepare_to_stream(transaction, flags);\n \t\tif (idx) {\n \t\t\thashfile_checkpoint(state->f, &checkpoint);\n \t\t\tidx->offset = state->offset;\n@@ -304,7 +309,7 @@ int index_blob_bulk_checkin(struct odb_transaction *transaction,\n \t\t\tBUG(\"should not happen\");\n \t\thashfile_truncate(state->f, &checkpoint);\n \t\tstate->offset = checkpoint.offset;\n-\t\tflush_bulk_checkin_packfile(state);\n+\t\tflush_bulk_checkin_packfile(transaction);\n \t\tif (lseek(fd, seekback, SEEK_SET) == (off_t) -1)\n \t\t\treturn error(\"cannot seek back\");\n \t}\n@@ -313,7 +318,7 @@ int index_blob_bulk_checkin(struct odb_transaction *transaction,\n \t\treturn 0;\n \n \tidx->crc32 = crc32_end(state->f);\n-\tif (already_written(state, result_oid)) {\n+\tif (already_written(transaction, result_oid)) {\n \t\thashfile_truncate(state->f, &checkpoint);\n \t\tstate->offset = checkpoint.offset;\n \t\tfree(idx);\n@@ -338,7 +343,7 @@ void prepare_loose_object_bulk_checkin(struct odb_transaction *transaction)\n \tif (!transaction || transaction->objdir)\n \t\treturn;\n \n-\ttransaction->objdir = tmp_objdir_create(the_repository, \"bulk-fsync\");\n+\ttransaction->objdir = tmp_objdir_create(transaction->odb->repo, \"bulk-fsync\");\n \tif (transaction->objdir)\n \t\ttmp_objdir_replace_primary_odb(transaction->objdir, 0);\n }\n@@ -379,7 +384,7 @@ void flush_odb_transaction(struct odb_transaction *transaction)\n \t\treturn;\n \n \tflush_batch_fsync(transaction);\n-\tflush_bulk_checkin_packfile(&transaction->packfile);\n+\tflush_bulk_checkin_packfile(transaction);\n }\n \n void end_odb_transaction(struct odb_transaction *transaction)\n-- \n2.51.0\n\n"},{"id":"524891","messageId":"xmqqsehfp2s1.fsf@gitster.g","threadId":"64002","inReplyTo":"20250822213500.1488064-1-jltobler@gmail.com","subject":"Re: [PATCH v3 0/4] bulk-checkin: remove global transaction state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-25T20:25:18Z","receivedAt":"2025-08-25T20:25:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler writes:\n\n> Changes since V2:\n>\n> - `index_blob_bulk_checkin()` is combined with\n> `deflate_blob_bulk_checkin()` in patch 3 instead of 4.\n> - Continue to use `repo_get_object_directory()` instead of open coding.\n>\n> Changes since V1:\n>\n> - `index_blob_bulk_checkin()` now assumes that the caller always\n> provides a setup `struct odb_transaction`. Callers are adjusted to\n> ensure this.\n> - Functions in bulk-checkin.c now consistently access the repository\n> through the provided `odb_transaction`.\n>\n> Thanks,\n> -Justin\n>\n> Justin Tobler (4):\n> bulk-checkin: introduce object database transaction structure\n> bulk-checkin: remove global transaction state\n> bulk-checkin: require transaction for index_blob_bulk_checkin()\n> bulk-checkin: use repository variable from transaction\n>\n> builtin/add.c | 5 +-\n> builtin/unpack-objects.c | 5 +-\n> builtin/update-index.c | 7 +-\n> bulk-checkin.c | 152 +++++++++++++++++++++------------------\n> bulk-checkin.h | 25 ++++---\n> cache-tree.c | 5 +-\n> object-file.c | 30 +++++---\n> odb.h | 8 +++\n> read-cache.c | 5 +-\n> 9 files changed, 141 insertions(+), 101 deletions(-)\n\nLooking good. Will queue. Thanks.\n"}]}