{"thread":{"id":"60403","subject":"[PATCH v4 0/7] merge-ort: implement support for packing objects together","startedAt":"2023-10-19T17:28:44Z","lastAt":"2023-11-14T22:04:37Z","messageCount":63,"participants":["Taylor Blau","Junio C Hamano","Jeff King","Patrick Steinhardt","Eric Sunshine","Johannes Schindelin","Elijah Newren"],"isPatch":true,"patchVersion":4,"patchTotal":7},"messages":[{"id":"483499","messageId":"cover.1697736516.git.me@ttaylorr.com","threadId":"60403","inReplyTo":null,"subject":"[PATCH v4 0/7] merge-ort: implement support for packing objects together","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-19T17:28:38Z","receivedAt":"2023-10-19T17:28:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"(Rebased onto the current tip of 'master', which is 813d9a9188 (The\nnineteenth batch, 2023-10-18) at the time of writing).\n\nThis series implements support for a new merge-tree option,\n`--write-pack`, which causes any newly-written objects to be packed\ntogether instead of being stored individually as loose.\n\nI intentionally broke this off from the existing thread, since I\naccidentally rerolled mine and Jonathan Tan's Bloom v2 series into it,\ncausing some confusion.\n\nThis is a new round that is significantly simplified thanks to\nanother very helpful suggestion[1] from Junio. By factoring out a common\n\"deflate object to pack\" that takes an abstract bulk_checkin_source as a\nparameter, all of the earlier refactorings can be dropped since we\nretain only a single caller instead of multiple.\n\nThis resulted in a rather satisfying range-diff (included below, as\nusual), and a similarly satisfying inter-diff:\n\n $ git diff --stat tb/ort-bulk-checkin.v3..\n bulk-checkin.c | 203 ++++++++++++++++---------------------------------\n 1 file changed, 64 insertions(+), 139 deletions(-)\n\nBeyond that, the changes since last time can be viewed in the range-diff\nbelow. Thanks in advance for any review!\n\n[1]: https://lore.kernel.org/git/xmqq34y7plj4.fsf@gitster.g/\n\nTaylor Blau (7):\n bulk-checkin: extract abstract `bulk_checkin_source`\n bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types\n bulk-checkin: refactor deflate routine to accept a\n `bulk_checkin_source`\n bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`\n bulk-checkin: introduce `index_blob_bulk_checkin_incore()`\n bulk-checkin: introduce `index_tree_bulk_checkin_incore()`\n builtin/merge-tree.c: implement support for `--write-pack`\n\n Documentation/git-merge-tree.txt | 4 +\n builtin/merge-tree.c | 5 +\n bulk-checkin.c | 161 ++++++++++++++++++++++++++-----\n bulk-checkin.h | 8 ++\n merge-ort.c | 42 ++++++--\n merge-recursive.h | 1 +\n t/t4301-merge-tree-write-tree.sh | 93 ++++++++++++++++++\n 7 files changed, 280 insertions(+), 34 deletions(-)\n\nRange-diff against v3:\n 1: 2dffa45183 < -: ---------- bulk-checkin: factor out `format_object_header_hash()`\n 2: 7a10dc794a < -: ---------- bulk-checkin: factor out `prepare_checkpoint()`\n 3: 20c32d2178 < -: ---------- bulk-checkin: factor out `truncate_checkpoint()`\n 4: 893051d0b7 < -: ---------- bulk-checkin: factor out `finalize_checkpoint()`\n 5: da52ec8380 ! 1: 97bb6e9f59 bulk-checkin: extract abstract `bulk_checkin_source`\n @@ bulk-checkin.c: static int stream_blob_to_pack(struct bulk_checkin_packfile *sta\n \t\t\tif (*already_hashed_to < offset) {\n \t\t\t\tsize_t hsize = offset - *already_hashed_to;\n @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n - \tgit_hash_ctx ctx;\n + \tunsigned header_len;\n \tstruct hashfile_checkpoint checkpoint = {0};\n \tstruct pack_idx_entry *idx = NULL;\n +\tstruct bulk_checkin_source source = {\n @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st\n \tseekback = lseek(fd, 0, SEEK_CUR);\n \tif (seekback == (off_t) -1)\n @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n - \twhile (1) {\n - \t\tprepare_checkpoint(state, &checkpoint, idx, flags);\n + \t\t\tcrc32_begin(state->f);\n + \t\t}\n \t\tif (!stream_blob_to_pack(state, &ctx, &already_hashed_to,\n -\t\t\t\t\t fd, size, path, flags))\n +\t\t\t\t\t &source, flags))\n \t\t\tbreak;\n - \t\ttruncate_checkpoint(state, &checkpoint, idx);\n + \t\t/*\n + \t\t * Writing this object to the current pack will make\n +@@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n + \t\thashfile_truncate(state->f, &checkpoint);\n + \t\tstate->offset = checkpoint.offset;\n + \t\tflush_bulk_checkin_packfile(state);\n -\t\tif (lseek(fd, seekback, SEEK_SET) == (off_t) -1)\n +\t\tif (bulk_checkin_source_seek_to(&source, seekback) == (off_t)-1)\n \t\t\treturn error(\"cannot seek back\");\n \t}\n - \tfinalize_checkpoint(state, &ctx, &checkpoint, idx, result_oid);\n + \tthe_hash_algo->final_oid_fn(result_oid, &ctx);\n 7: 04ec74e357 ! 2: 9d633df339 bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types\n @@ bulk-checkin.c: static int stream_blob_to_pack(struct bulk_checkin_packfile *sta\n \ts.avail_out = sizeof(obuf) - hdrlen;\n \n @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n - \n - \twhile (1) {\n - \t\tprepare_checkpoint(state, &checkpoint, idx, flags);\n + \t\t\tidx->offset = state->offset;\n + \t\t\tcrc32_begin(state->f);\n + \t\t}\n -\t\tif (!stream_blob_to_pack(state, &ctx, &already_hashed_to,\n -\t\t\t\t\t &source, flags))\n +\t\tif (!stream_obj_to_pack(state, &ctx, &already_hashed_to,\n +\t\t\t\t\t&source, OBJ_BLOB, flags))\n \t\t\tbreak;\n - \t\ttruncate_checkpoint(state, &checkpoint, idx);\n - \t\tif (bulk_checkin_source_seek_to(&source, seekback) == (off_t)-1)\n + \t\t/*\n + \t\t * Writing this object to the current pack will make\n -: ---------- > 3: d5bbd7810e bulk-checkin: refactor deflate routine to accept a `bulk_checkin_source`\n 6: 4e9bac5bc1 = 4: e427fe6ad3 bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`\n 8: 8667b76365 ! 5: 48095afe80 bulk-checkin: introduce `index_blob_bulk_checkin_incore()`\n @@ Commit message\n objects individually as loose.\n \n Similar to the existing `index_blob_bulk_checkin()` function, the\n - entrypoint delegates to `deflate_blob_to_pack_incore()`, which is\n - responsible for formatting the pack header and then deflating the\n - contents into the pack. The latter is accomplished by calling\n - deflate_obj_contents_to_pack_incore(), which takes advantage of the\n - earlier refactorings and is responsible for writing the object to the\n - pack and handling any overage from pack.packSizeLimit.\n -\n - The bulk of the new functionality is implemented in the function\n - `stream_obj_to_pack()`, which can handle streaming objects from memory\n - to the bulk-checkin pack as a result of the earlier refactoring.\n + entrypoint delegates to `deflate_obj_to_pack_incore()`. That function in\n + turn delegates to deflate_obj_to_pack(), which is responsible for\n + formatting the pack header and then deflating the contents into the\n + pack.\n \n Consistent with the rest of the bulk-checkin mechanism, there are no\n direct tests here. In future commits when we expose this new\n @@ Commit message\n Signed-off-by: Taylor Blau \n \n ## bulk-checkin.c ##\n -@@ bulk-checkin.c: static void finalize_checkpoint(struct bulk_checkin_packfile *state,\n - \t}\n +@@ bulk-checkin.c: static int deflate_obj_to_pack(struct bulk_checkin_packfile *state,\n + \treturn 0;\n }\n \n -+static int deflate_obj_contents_to_pack_incore(struct bulk_checkin_packfile *state,\n -+\t\t\t\t\t git_hash_ctx *ctx,\n -+\t\t\t\t\t struct hashfile_checkpoint *checkpoint,\n -+\t\t\t\t\t struct object_id *result_oid,\n -+\t\t\t\t\t const void *buf, size_t size,\n -+\t\t\t\t\t enum object_type type,\n -+\t\t\t\t\t const char *path, unsigned flags)\n ++static int deflate_obj_to_pack_incore(struct bulk_checkin_packfile *state,\n ++\t\t\t\t struct object_id *result_oid,\n ++\t\t\t\t const void *buf, size_t size,\n ++\t\t\t\t const char *path, enum object_type type,\n ++\t\t\t\t unsigned flags)\n +{\n -+\tstruct pack_idx_entry *idx = NULL;\n -+\toff_t already_hashed_to = 0;\n +\tstruct bulk_checkin_source source = {\n +\t\t.type = SOURCE_INCORE,\n +\t\t.buf = buf,\n @@ bulk-checkin.c: static void finalize_checkpoint(struct bulk_checkin_packfile *st\n +\t\t.path = path,\n +\t};\n +\n -+\t/* Note: idx is non-NULL when we are writing */\n -+\tif (flags & HASH_WRITE_OBJECT)\n -+\t\tCALLOC_ARRAY(idx, 1);\n -+\n -+\twhile (1) {\n -+\t\tprepare_checkpoint(state, checkpoint, idx, flags);\n -+\n -+\t\tif (!stream_obj_to_pack(state, ctx, &already_hashed_to, &source,\n -+\t\t\t\t\ttype, flags))\n -+\t\t\tbreak;\n -+\t\ttruncate_checkpoint(state, checkpoint, idx);\n -+\t\tbulk_checkin_source_seek_to(&source, 0);\n -+\t}\n -+\n -+\tfinalize_checkpoint(state, ctx, checkpoint, idx, result_oid);\n -+\n -+\treturn 0;\n -+}\n -+\n -+static int deflate_blob_to_pack_incore(struct bulk_checkin_packfile *state,\n -+\t\t\t\t struct object_id *result_oid,\n -+\t\t\t\t const void *buf, size_t size,\n -+\t\t\t\t const char *path, unsigned flags)\n -+{\n -+\tgit_hash_ctx ctx;\n -+\tstruct hashfile_checkpoint checkpoint = {0};\n -+\n -+\tformat_object_header_hash(the_hash_algo, &ctx, &checkpoint, OBJ_BLOB,\n -+\t\t\t\t size);\n -+\n -+\treturn deflate_obj_contents_to_pack_incore(state, &ctx, &checkpoint,\n -+\t\t\t\t\t\t result_oid, buf, size,\n -+\t\t\t\t\t\t OBJ_BLOB, path, flags);\n ++\treturn deflate_obj_to_pack(state, result_oid, &source, type, 0, flags);\n +}\n +\n static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n @@ bulk-checkin.c: int index_blob_bulk_checkin(struct object_id *oid,\n +\t\t\t\t const void *buf, size_t size,\n +\t\t\t\t const char *path, unsigned flags)\n +{\n -+\tint status = deflate_blob_to_pack_incore(&bulk_checkin_packfile, oid,\n -+\t\t\t\t\t\t buf, size, path, flags);\n ++\tint status = deflate_obj_to_pack_incore(&bulk_checkin_packfile, oid,\n ++\t\t\t\t\t\tbuf, size, path, OBJ_BLOB,\n ++\t\t\t\t\t\tflags);\n +\tif (!odb_transaction_nesting)\n +\t\tflush_bulk_checkin_packfile(&bulk_checkin_packfile);\n +\treturn status;\n 9: cba043ef14 ! 6: 60568f9281 bulk-checkin: introduce `index_tree_bulk_checkin_incore()`\n @@ Commit message\n machinery will have enough to keep track of the converted object's hash\n in order to update the compatibility mapping.\n \n - Within `deflate_tree_to_pack_incore()`, the changes should be limited\n - to something like:\n + Within some thin wrapper around `deflate_obj_to_pack_incore()` (perhaps\n + `deflate_tree_to_pack_incore()`), the changes should be limited to\n + something like:\n \n struct strbuf converted = STRBUF_INIT;\n if (the_repository->compat_hash_algo) {\n @@ Commit message\n Signed-off-by: Taylor Blau \n \n ## bulk-checkin.c ##\n -@@ bulk-checkin.c: static int deflate_blob_to_pack_incore(struct bulk_checkin_packfile *state,\n - \t\t\t\t\t\t OBJ_BLOB, path, flags);\n - }\n - \n -+static int deflate_tree_to_pack_incore(struct bulk_checkin_packfile *state,\n -+\t\t\t\t struct object_id *result_oid,\n -+\t\t\t\t const void *buf, size_t size,\n -+\t\t\t\t const char *path, unsigned flags)\n -+{\n -+\tgit_hash_ctx ctx;\n -+\tstruct hashfile_checkpoint checkpoint = {0};\n -+\n -+\tformat_object_header_hash(the_hash_algo, &ctx, &checkpoint, OBJ_TREE,\n -+\t\t\t\t size);\n -+\n -+\treturn deflate_obj_contents_to_pack_incore(state, &ctx, &checkpoint,\n -+\t\t\t\t\t\t result_oid, buf, size,\n -+\t\t\t\t\t\t OBJ_TREE, path, flags);\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 @@ bulk-checkin.c: int index_blob_bulk_checkin_incore(struct object_id *oid,\n \treturn status;\n }\n @@ bulk-checkin.c: int index_blob_bulk_checkin_incore(struct object_id *oid,\n +\t\t\t\t const void *buf, size_t size,\n +\t\t\t\t const char *path, unsigned flags)\n +{\n -+\tint status = deflate_tree_to_pack_incore(&bulk_checkin_packfile, oid,\n -+\t\t\t\t\t\t buf, size, path, flags);\n ++\tint status = deflate_obj_to_pack_incore(&bulk_checkin_packfile, oid,\n ++\t\t\t\t\t\tbuf, size, path, OBJ_TREE,\n ++\t\t\t\t\t\tflags);\n +\tif (!odb_transaction_nesting)\n +\t\tflush_bulk_checkin_packfile(&bulk_checkin_packfile);\n +\treturn status;\n10: ae70508037 = 7: b9be9df122 builtin/merge-tree.c: implement support for `--write-pack`\n-- \n2.42.0.405.g86fe3250c2\n"},{"id":"483500","messageId":"97bb6e9f59e5092f0519c7d1141d0673313fdc33.1697736516.git.me@ttaylorr.com","threadId":"60403","inReplyTo":"cover.1697736516.git.me@ttaylorr.com","subject":"[PATCH v4 1/7] bulk-checkin: extract abstract `bulk_checkin_source`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-19T17:28:42Z","receivedAt":"2023-10-19T17:28:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"A future commit will want to implement a very similar routine as in\n`stream_blob_to_pack()` with two notable changes:\n\n - Instead of streaming just OBJ_BLOBs, this new function may want to\n stream objects of arbitrary type.\n\n - Instead of streaming the object's contents from an open\n file-descriptor, this new function may want to \"stream\" its contents\n from memory.\n\nTo avoid duplicating a significant chunk of code between the existing\n`stream_blob_to_pack()`, extract an abstract `bulk_checkin_source`. This\nconcept currently is a thin layer of `lseek()` and `read_in_full()`, but\nwill grow to understand how to perform analogous operations when writing\nout an object's contents from memory.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Taylor Blau \n---\n bulk-checkin.c | 61 +++++++++++++++++++++++++++++++++++++++++++-------\n 1 file changed, 53 insertions(+), 8 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 6ce62999e5..c05d06e1e1 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -140,8 +140,41 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id\n \treturn 0;\n }\n \n+struct bulk_checkin_source {\n+\tenum { SOURCE_FILE } type;\n+\n+\t/* SOURCE_FILE fields */\n+\tint fd;\n+\n+\t/* common fields */\n+\tsize_t size;\n+\tconst char *path;\n+};\n+\n+static off_t bulk_checkin_source_seek_to(struct bulk_checkin_source *source,\n+\t\t\t\t\t off_t offset)\n+{\n+\tswitch (source->type) {\n+\tcase SOURCE_FILE:\n+\t\treturn lseek(source->fd, offset, SEEK_SET);\n+\tdefault:\n+\t\tBUG(\"unknown bulk-checkin source: %d\", source->type);\n+\t}\n+}\n+\n+static ssize_t bulk_checkin_source_read(struct bulk_checkin_source *source,\n+\t\t\t\t\tvoid *buf, size_t nr)\n+{\n+\tswitch (source->type) {\n+\tcase SOURCE_FILE:\n+\t\treturn read_in_full(source->fd, buf, nr);\n+\tdefault:\n+\t\tBUG(\"unknown bulk-checkin source: %d\", source->type);\n+\t}\n+}\n+\n /*\n- * Read the contents from fd for size bytes, streaming it to the\n+ * Read the contents from 'source' for 'size' bytes, streaming it to the\n * packfile in state while updating the hash in ctx. Signal a failure\n * by returning a negative value when the resulting pack would exceed\n * the pack size limit and this is not the first object in the pack,\n@@ -157,7 +190,7 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id\n */\n static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n \t\t\t git_hash_ctx *ctx, off_t *already_hashed_to,\n-\t\t\t int fd, size_t size, const char *path,\n+\t\t\t struct bulk_checkin_source *source,\n \t\t\t unsigned flags)\n {\n \tgit_zstream s;\n@@ -167,22 +200,28 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n \tint status = Z_OK;\n \tint write_object = (flags & HASH_WRITE_OBJECT);\n \toff_t offset = 0;\n+\tsize_t size = source->size;\n \n \tgit_deflate_init(&s, pack_compression_level);\n \n-\thdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB, size);\n+\thdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB,\n+\t\t\t\t\t size);\n \ts.next_out = obuf + hdrlen;\n \ts.avail_out = sizeof(obuf) - hdrlen;\n \n \twhile (status != Z_STREAM_END) {\n \t\tif (size && !s.avail_in) {\n \t\t\tssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);\n-\t\t\tssize_t read_result = read_in_full(fd, ibuf, rsize);\n+\t\t\tssize_t read_result;\n+\n+\t\t\tread_result = bulk_checkin_source_read(source, ibuf,\n+\t\t\t\t\t\t\t rsize);\n \t\t\tif (read_result < 0)\n-\t\t\t\tdie_errno(\"failed to read from '%s'\", path);\n+\t\t\t\tdie_errno(\"failed to read from '%s'\",\n+\t\t\t\t\t source->path);\n \t\t\tif (read_result != rsize)\n \t\t\t\tdie(\"failed to read %d bytes from '%s'\",\n-\t\t\t\t (int)rsize, path);\n+\t\t\t\t (int)rsize, source->path);\n \t\t\toffset += rsize;\n \t\t\tif (*already_hashed_to < offset) {\n \t\t\t\tsize_t hsize = offset - *already_hashed_to;\n@@ -258,6 +297,12 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \tunsigned header_len;\n \tstruct hashfile_checkpoint checkpoint = {0};\n \tstruct pack_idx_entry *idx = NULL;\n+\tstruct bulk_checkin_source source = {\n+\t\t.type = SOURCE_FILE,\n+\t\t.fd = fd,\n+\t\t.size = size,\n+\t\t.path = path,\n+\t};\n \n \tseekback = lseek(fd, 0, SEEK_CUR);\n \tif (seekback == (off_t) -1)\n@@ -283,7 +328,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \t\t\tcrc32_begin(state->f);\n \t\t}\n \t\tif (!stream_blob_to_pack(state, &ctx, &already_hashed_to,\n-\t\t\t\t\t fd, size, path, flags))\n+\t\t\t\t\t &source, flags))\n \t\t\tbreak;\n \t\t/*\n \t\t * Writing this object to the current pack will make\n@@ -295,7 +340,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \t\thashfile_truncate(state->f, &checkpoint);\n \t\tstate->offset = checkpoint.offset;\n \t\tflush_bulk_checkin_packfile(state);\n-\t\tif (lseek(fd, seekback, SEEK_SET) == (off_t) -1)\n+\t\tif (bulk_checkin_source_seek_to(&source, seekback) == (off_t)-1)\n \t\t\treturn error(\"cannot seek back\");\n \t}\n \tthe_hash_algo->final_oid_fn(result_oid, &ctx);\n-- \n2.42.0.405.g86fe3250c2\n\n"},{"id":"483501","messageId":"9d633df339f2a769bcae4d6328ca47915184e4aa.1697736516.git.me@ttaylorr.com","threadId":"60403","inReplyTo":"cover.1697736516.git.me@ttaylorr.com","subject":"[PATCH v4 2/7] bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-19T17:28:45Z","receivedAt":"2023-10-19T17:28:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The existing `stream_blob_to_pack()` function is named based on the fact\nthat it knows only how to stream blobs into a bulk-checkin pack.\n\nBut there is no longer anything in this function which prevents us from\nwriting objects of arbitrary types to the bulk-checkin pack. Prepare to\nwrite OBJ_TREEs by removing this assumption, adding an `enum\nobject_type` parameter to this function's argument list, and renaming it\nto `stream_obj_to_pack()` accordingly.\n\nSigned-off-by: Taylor Blau \n---\n bulk-checkin.c | 15 +++++++--------\n 1 file changed, 7 insertions(+), 8 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex c05d06e1e1..7e6b52112e 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -188,10 +188,10 @@ static ssize_t bulk_checkin_source_read(struct bulk_checkin_source *source,\n * status before calling us just in case we ask it to call us again\n * with a new pack.\n */\n-static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n-\t\t\t git_hash_ctx *ctx, off_t *already_hashed_to,\n-\t\t\t struct bulk_checkin_source *source,\n-\t\t\t unsigned flags)\n+static int stream_obj_to_pack(struct bulk_checkin_packfile *state,\n+\t\t\t git_hash_ctx *ctx, off_t *already_hashed_to,\n+\t\t\t struct bulk_checkin_source *source,\n+\t\t\t enum object_type type, unsigned flags)\n {\n \tgit_zstream s;\n \tunsigned char ibuf[16384];\n@@ -204,8 +204,7 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n \n \tgit_deflate_init(&s, pack_compression_level);\n \n-\thdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB,\n-\t\t\t\t\t size);\n+\thdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), type, size);\n \ts.next_out = obuf + hdrlen;\n \ts.avail_out = sizeof(obuf) - hdrlen;\n \n@@ -327,8 +326,8 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \t\t\tidx->offset = state->offset;\n \t\t\tcrc32_begin(state->f);\n \t\t}\n-\t\tif (!stream_blob_to_pack(state, &ctx, &already_hashed_to,\n-\t\t\t\t\t &source, flags))\n+\t\tif (!stream_obj_to_pack(state, &ctx, &already_hashed_to,\n+\t\t\t\t\t&source, OBJ_BLOB, flags))\n \t\t\tbreak;\n \t\t/*\n \t\t * Writing this object to the current pack will make\n-- \n2.42.0.405.g86fe3250c2\n\n"},{"id":"483502","messageId":"d5bbd7810ee8f90be4cf6028f6420f52d72d24d0.1697736516.git.me@ttaylorr.com","threadId":"60403","inReplyTo":"cover.1697736516.git.me@ttaylorr.com","subject":"[PATCH v4 3/7] bulk-checkin: refactor deflate routine to accept a `bulk_checkin_source`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-19T17:28:48Z","receivedAt":"2023-10-19T17:28:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Prepare for a future change where we will want to use a routine very\nsimilar to the existing `deflate_blob_to_pack()` but over arbitrary\nsources (i.e. either open file-descriptors, or a location in memory).\n\nExtract out a common \"deflate_obj_to_pack()\" routine that acts on a\nbulk_checkin_source, instead of a (int, size_t) pair. Then rewrite\n`deflate_blob_to_pack()` in terms of it.\n\nSigned-off-by: Taylor Blau \n---\n bulk-checkin.c | 52 ++++++++++++++++++++++++++++++--------------------\n 1 file changed, 31 insertions(+), 21 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 7e6b52112e..28bc8d5ab4 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -285,30 +285,23 @@ 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+\n+static int deflate_obj_to_pack(struct bulk_checkin_packfile *state,\n+\t\t\t struct object_id *result_oid,\n+\t\t\t struct bulk_checkin_source *source,\n+\t\t\t enum object_type type,\n+\t\t\t off_t seekback,\n+\t\t\t unsigned flags)\n {\n-\toff_t seekback, already_hashed_to;\n+\toff_t already_hashed_to = 0;\n \tgit_hash_ctx ctx;\n \tunsigned char obuf[16384];\n \tunsigned header_len;\n \tstruct hashfile_checkpoint checkpoint = {0};\n \tstruct pack_idx_entry *idx = NULL;\n-\tstruct bulk_checkin_source source = {\n-\t\t.type = SOURCE_FILE,\n-\t\t.fd = fd,\n-\t\t.size = size,\n-\t\t.path = path,\n-\t};\n \n-\tseekback = lseek(fd, 0, SEEK_CUR);\n-\tif (seekback == (off_t) -1)\n-\t\treturn error(\"cannot find the current offset\");\n-\n-\theader_len = format_object_header((char *)obuf, sizeof(obuf),\n-\t\t\t\t\t OBJ_BLOB, size);\n+\theader_len = format_object_header((char *)obuf, sizeof(obuf), type,\n+\t\t\t\t\t source->size);\n \tthe_hash_algo->init_fn(&ctx);\n \tthe_hash_algo->update_fn(&ctx, obuf, header_len);\n \tthe_hash_algo->init_fn(&checkpoint.ctx);\n@@ -317,8 +310,6 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \tif ((flags & HASH_WRITE_OBJECT) != 0)\n \t\tCALLOC_ARRAY(idx, 1);\n \n-\talready_hashed_to = 0;\n-\n \twhile (1) {\n \t\tprepare_to_stream(state, flags);\n \t\tif (idx) {\n@@ -327,7 +318,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \t\t\tcrc32_begin(state->f);\n \t\t}\n \t\tif (!stream_obj_to_pack(state, &ctx, &already_hashed_to,\n-\t\t\t\t\t&source, OBJ_BLOB, flags))\n+\t\t\t\t\tsource, type, flags))\n \t\t\tbreak;\n \t\t/*\n \t\t * Writing this object to the current pack will make\n@@ -339,7 +330,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \t\thashfile_truncate(state->f, &checkpoint);\n \t\tstate->offset = checkpoint.offset;\n \t\tflush_bulk_checkin_packfile(state);\n-\t\tif (bulk_checkin_source_seek_to(&source, seekback) == (off_t)-1)\n+\t\tif (bulk_checkin_source_seek_to(source, seekback) == (off_t)-1)\n \t\t\treturn error(\"cannot seek back\");\n \t}\n \tthe_hash_algo->final_oid_fn(result_oid, &ctx);\n@@ -361,6 +352,25 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \treturn 0;\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+{\n+\tstruct bulk_checkin_source source = {\n+\t\t.type = SOURCE_FILE,\n+\t\t.fd = fd,\n+\t\t.size = size,\n+\t\t.path = path,\n+\t};\n+\toff_t seekback = lseek(fd, 0, SEEK_CUR);\n+\tif (seekback == (off_t) -1)\n+\t\treturn error(\"cannot find the current offset\");\n+\n+\treturn deflate_obj_to_pack(state, result_oid, &source, OBJ_BLOB,\n+\t\t\t\t seekback, flags);\n+}\n+\n void prepare_loose_object_bulk_checkin(void)\n {\n \t/*\n-- \n2.42.0.405.g86fe3250c2\n\n"},{"id":"483503","messageId":"e427fe6ad383cc238c13f313dc9f11eab37a3840.1697736516.git.me@ttaylorr.com","threadId":"60403","inReplyTo":"cover.1697736516.git.me@ttaylorr.com","subject":"[PATCH v4 4/7] bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-19T17:28:51Z","receivedAt":"2023-10-19T17:28:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Continue to prepare for streaming an object's contents directly from\nmemory by teaching `bulk_checkin_source` how to perform reads and seeks\nbased on an address in memory.\n\nUnlike file descriptors, which manage their own offset internally, we\nhave to keep track of how many bytes we've read out of the buffer, and\nmake sure we don't read past the end of the buffer.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Taylor Blau \n---\n bulk-checkin.c | 18 +++++++++++++++++-\n 1 file changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 28bc8d5ab4..60361b3e2e 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -141,11 +141,15 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id\n }\n \n struct bulk_checkin_source {\n-\tenum { SOURCE_FILE } type;\n+\tenum { SOURCE_FILE, SOURCE_INCORE } type;\n \n \t/* SOURCE_FILE fields */\n \tint fd;\n \n+\t/* SOURCE_INCORE fields */\n+\tconst void *buf;\n+\tsize_t read;\n+\n \t/* common fields */\n \tsize_t size;\n \tconst char *path;\n@@ -157,6 +161,11 @@ static off_t bulk_checkin_source_seek_to(struct bulk_checkin_source *source,\n \tswitch (source->type) {\n \tcase SOURCE_FILE:\n \t\treturn lseek(source->fd, offset, SEEK_SET);\n+\tcase SOURCE_INCORE:\n+\t\tif (!(0 <= offset && offset < source->size))\n+\t\t\treturn (off_t)-1;\n+\t\tsource->read = offset;\n+\t\treturn source->read;\n \tdefault:\n \t\tBUG(\"unknown bulk-checkin source: %d\", source->type);\n \t}\n@@ -168,6 +177,13 @@ static ssize_t bulk_checkin_source_read(struct bulk_checkin_source *source,\n \tswitch (source->type) {\n \tcase SOURCE_FILE:\n \t\treturn read_in_full(source->fd, buf, nr);\n+\tcase SOURCE_INCORE:\n+\t\tassert(source->read <= source->size);\n+\t\tif (nr > source->size - source->read)\n+\t\t\tnr = source->size - source->read;\n+\t\tmemcpy(buf, (unsigned char *)source->buf + source->read, nr);\n+\t\tsource->read += nr;\n+\t\treturn nr;\n \tdefault:\n \t\tBUG(\"unknown bulk-checkin source: %d\", source->type);\n \t}\n-- \n2.42.0.405.g86fe3250c2\n\n"},{"id":"483504","messageId":"48095afe80fa94a4e9b47f95e9e5821e690075c3.1697736516.git.me@ttaylorr.com","threadId":"60403","inReplyTo":"cover.1697736516.git.me@ttaylorr.com","subject":"[PATCH v4 5/7] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-19T17:28:54Z","receivedAt":"2023-10-19T17:28:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Now that we have factored out many of the common routines necessary to\nindex a new object into a pack created by the bulk-checkin machinery, we\ncan introduce a variant of `index_blob_bulk_checkin()` that acts on\nblobs whose contents we can fit in memory.\n\nThis will be useful in a couple of more commits in order to provide the\n`merge-tree` builtin with a mechanism to create a new pack containing\nany objects it created during the merge, instead of storing those\nobjects individually as loose.\n\nSimilar to the existing `index_blob_bulk_checkin()` function, the\nentrypoint delegates to `deflate_obj_to_pack_incore()`. That function in\nturn delegates to deflate_obj_to_pack(), which is responsible for\nformatting the pack header and then deflating the contents into the\npack.\n\nConsistent with the rest of the bulk-checkin mechanism, there are no\ndirect tests here. In future commits when we expose this new\nfunctionality via the `merge-tree` builtin, we will test it indirectly\nthere.\n\nSigned-off-by: Taylor Blau \n---\n bulk-checkin.c | 29 +++++++++++++++++++++++++++++\n bulk-checkin.h | 4 ++++\n 2 files changed, 33 insertions(+)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 60361b3e2e..655a583b06 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -368,6 +368,23 @@ static int deflate_obj_to_pack(struct bulk_checkin_packfile *state,\n \treturn 0;\n }\n \n+static int deflate_obj_to_pack_incore(struct bulk_checkin_packfile *state,\n+\t\t\t\t struct object_id *result_oid,\n+\t\t\t\t const void *buf, size_t size,\n+\t\t\t\t const char *path, enum object_type type,\n+\t\t\t\t unsigned flags)\n+{\n+\tstruct bulk_checkin_source source = {\n+\t\t.type = SOURCE_INCORE,\n+\t\t.buf = buf,\n+\t\t.size = size,\n+\t\t.read = 0,\n+\t\t.path = path,\n+\t};\n+\n+\treturn deflate_obj_to_pack(state, result_oid, &source, type, 0, flags);\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@@ -431,6 +448,18 @@ int index_blob_bulk_checkin(struct object_id *oid,\n \treturn status;\n }\n \n+int index_blob_bulk_checkin_incore(struct object_id *oid,\n+\t\t\t\t const void *buf, size_t size,\n+\t\t\t\t const char *path, unsigned flags)\n+{\n+\tint status = deflate_obj_to_pack_incore(&bulk_checkin_packfile, oid,\n+\t\t\t\t\t\tbuf, size, path, OBJ_BLOB,\n+\t\t\t\t\t\tflags);\n+\tif (!odb_transaction_nesting)\n+\t\tflush_bulk_checkin_packfile(&bulk_checkin_packfile);\n+\treturn status;\n+}\n+\n void begin_odb_transaction(void)\n {\n \todb_transaction_nesting += 1;\ndiff --git a/bulk-checkin.h b/bulk-checkin.h\nindex aa7286a7b3..1b91daeaee 100644\n--- a/bulk-checkin.h\n+++ b/bulk-checkin.h\n@@ -13,6 +13,10 @@ 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+int index_blob_bulk_checkin_incore(struct object_id *oid,\n+\t\t\t\t const void *buf, size_t size,\n+\t\t\t\t const char *path, unsigned flags);\n+\n /*\n * Tell the object database to optimize for adding\n * multiple objects. end_odb_transaction must be called\n-- \n2.42.0.405.g86fe3250c2\n\n"},{"id":"483505","messageId":"60568f9281c2588f777e3886610a1b40730fcc0f.1697736516.git.me@ttaylorr.com","threadId":"60403","inReplyTo":"cover.1697736516.git.me@ttaylorr.com","subject":"[PATCH v4 6/7] bulk-checkin: introduce `index_tree_bulk_checkin_incore()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-19T17:28:57Z","receivedAt":"2023-10-19T17:29:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The remaining missing piece in order to teach the `merge-tree` builtin\nhow to write the contents of a merge into a pack is a function to index\ntree objects into a bulk-checkin pack.\n\nThis patch implements that missing piece, which is a thin wrapper around\nall of the functionality introduced in previous commits.\n\nIf and when Git gains support for a \"compatibility\" hash algorithm, the\nchanges to support that here will be minimal. The bulk-checkin machinery\nwill need to convert the incoming tree to compute its length under the\ncompatibility hash, necessary to reconstruct its header. With that\ninformation (and the converted contents of the tree), the bulk-checkin\nmachinery will have enough to keep track of the converted object's hash\nin order to update the compatibility mapping.\n\nWithin some thin wrapper around `deflate_obj_to_pack_incore()` (perhaps\n`deflate_tree_to_pack_incore()`), the changes should be limited to\nsomething like:\n\n struct strbuf converted = STRBUF_INIT;\n if (the_repository->compat_hash_algo) {\n if (convert_object_file(&compat_obj,\n the_repository->hash_algo,\n the_repository->compat_hash_algo, ...) < 0)\n die(...);\n\n format_object_header_hash(the_repository->compat_hash_algo,\n OBJ_TREE, size);\n }\n /* compute the converted tree's hash using the compat algorithm */\n strbuf_release(&converted);\n\n, assuming related changes throughout the rest of the bulk-checkin\nmachinery necessary to update the hash of the converted object, which\nare likewise minimal in size.\n\nSigned-off-by: Taylor Blau \n---\n bulk-checkin.c | 12 ++++++++++++\n bulk-checkin.h | 4 ++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 655a583b06..c1faf75f5f 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -460,6 +460,18 @@ int index_blob_bulk_checkin_incore(struct object_id *oid,\n \treturn status;\n }\n \n+int index_tree_bulk_checkin_incore(struct object_id *oid,\n+\t\t\t\t const void *buf, size_t size,\n+\t\t\t\t const char *path, unsigned flags)\n+{\n+\tint status = deflate_obj_to_pack_incore(&bulk_checkin_packfile, oid,\n+\t\t\t\t\t\tbuf, size, path, OBJ_TREE,\n+\t\t\t\t\t\tflags);\n+\tif (!odb_transaction_nesting)\n+\t\tflush_bulk_checkin_packfile(&bulk_checkin_packfile);\n+\treturn status;\n+}\n+\n void begin_odb_transaction(void)\n {\n \todb_transaction_nesting += 1;\ndiff --git a/bulk-checkin.h b/bulk-checkin.h\nindex 1b91daeaee..89786b3954 100644\n--- a/bulk-checkin.h\n+++ b/bulk-checkin.h\n@@ -17,6 +17,10 @@ int index_blob_bulk_checkin_incore(struct object_id *oid,\n \t\t\t\t const void *buf, size_t size,\n \t\t\t\t const char *path, unsigned flags);\n \n+int index_tree_bulk_checkin_incore(struct object_id *oid,\n+\t\t\t\t const void *buf, size_t size,\n+\t\t\t\t const char *path, unsigned flags);\n+\n /*\n * Tell the object database to optimize for adding\n * multiple objects. end_odb_transaction must be called\n-- \n2.42.0.405.g86fe3250c2\n\n"},{"id":"483506","messageId":"b9be9df1227c19c5ee1098afcd37387ff81a4cb5.1697736516.git.me@ttaylorr.com","threadId":"60403","inReplyTo":"cover.1697736516.git.me@ttaylorr.com","subject":"[PATCH v4 7/7] builtin/merge-tree.c: implement support for `--write-pack`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-19T17:29:00Z","receivedAt":"2023-10-19T17:29:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When using merge-tree often within a repository[^1], it is possible to\ngenerate a relatively large number of loose objects, which can result in\ndegraded performance, and inode exhaustion in extreme cases.\n\nBuilding on the functionality introduced in previous commits, the\nbulk-checkin machinery now has support to write arbitrary blob and tree\nobjects which are small enough to be held in-core. We can use this to\nwrite any blob/tree objects generated by ORT into a separate pack\ninstead of writing them out individually as loose.\n\nThis functionality is gated behind a new `--write-pack` option to\n`merge-tree` that works with the (non-deprecated) `--write-tree` mode.\n\nThe implementation is relatively straightforward. There are two spots\nwithin the ORT mechanism where we call `write_object_file()`, one for\ncontent differences within blobs, and another to assemble any new trees\nnecessary to construct the merge. In each of those locations,\nconditionally replace calls to `write_object_file()` with\n`index_blob_bulk_checkin_incore()` or `index_tree_bulk_checkin_incore()`\ndepending on which kind of object we are writing.\n\nThe only remaining task is to begin and end the transaction necessary to\ninitialize the bulk-checkin machinery, and move any new pack(s) it\ncreated into the main object store.\n\n[^1]: Such is the case at GitHub, where we run presumptive \"test merges\"\n on open pull requests to see whether or not we can light up the merge\n button green depending on whether or not the presumptive merge was\n conflicted.\n\n This is done in response to a number of user-initiated events,\n including viewing an open pull request whose last test merge is stale\n with respect to the current base and tip of the pull request. As a\n result, merge-tree can be run very frequently on large, active\n repositories.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/git-merge-tree.txt | 4 ++\n builtin/merge-tree.c | 5 ++\n merge-ort.c | 42 +++++++++++----\n merge-recursive.h | 1 +\n t/t4301-merge-tree-write-tree.sh | 93 ++++++++++++++++++++++++++++++++\n 5 files changed, 136 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt\nindex ffc4fbf7e8..9d37609ef1 100644\n--- a/Documentation/git-merge-tree.txt\n+++ b/Documentation/git-merge-tree.txt\n@@ -69,6 +69,10 @@ OPTIONS\n \tspecify a merge-base for the merge, and specifying multiple bases is\n \tcurrently not supported. This option is incompatible with `--stdin`.\n \n+--write-pack::\n+\tWrite any new objects into a separate packfile instead of as\n+\tindividual loose objects.\n+\n [[OUTPUT]]\n OUTPUT\n ------\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex 0de42aecf4..672ebd4c54 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -18,6 +18,7 @@\n #include \"quote.h\"\n #include \"tree.h\"\n #include \"config.h\"\n+#include \"bulk-checkin.h\"\n \n static int line_termination = '\\n';\n \n@@ -414,6 +415,7 @@ struct merge_tree_options {\n \tint show_messages;\n \tint name_only;\n \tint use_stdin;\n+\tint write_pack;\n };\n \n static int real_merge(struct merge_tree_options *o,\n@@ -440,6 +442,7 @@ static int real_merge(struct merge_tree_options *o,\n \tinit_merge_options(&opt, the_repository);\n \n \topt.show_rename_progress = 0;\n+\topt.write_pack = o->write_pack;\n \n \topt.branch1 = branch1;\n \topt.branch2 = branch2;\n@@ -548,6 +551,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)\n \t\t\t &merge_base,\n \t\t\t N_(\"commit\"),\n \t\t\t N_(\"specify a merge-base for the merge\")),\n+\t\tOPT_BOOL(0, \"write-pack\", &o.write_pack,\n+\t\t\t N_(\"write new objects to a pack instead of as loose\")),\n \t\tOPT_END()\n \t};\n \ndiff --git a/merge-ort.c b/merge-ort.c\nindex 3653725661..523577d71e 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -48,6 +48,7 @@\n #include \"tree.h\"\n #include \"unpack-trees.h\"\n #include \"xdiff-interface.h\"\n+#include \"bulk-checkin.h\"\n \n /*\n * We have many arrays of size 3. Whenever we have such an array, the\n@@ -2108,10 +2109,19 @@ static int handle_content_merge(struct merge_options *opt,\n \t\tif ((merge_status < 0) || !result_buf.ptr)\n \t\t\tret = error(_(\"failed to execute internal merge\"));\n \n-\t\tif (!ret &&\n-\t\t write_object_file(result_buf.ptr, result_buf.size,\n-\t\t\t\t OBJ_BLOB, &result->oid))\n-\t\t\tret = error(_(\"unable to add %s to database\"), path);\n+\t\tif (!ret) {\n+\t\t\tret = opt->write_pack\n+\t\t\t\t? index_blob_bulk_checkin_incore(&result->oid,\n+\t\t\t\t\t\t\t\t result_buf.ptr,\n+\t\t\t\t\t\t\t\t result_buf.size,\n+\t\t\t\t\t\t\t\t path, 1)\n+\t\t\t\t: write_object_file(result_buf.ptr,\n+\t\t\t\t\t\t result_buf.size,\n+\t\t\t\t\t\t OBJ_BLOB, &result->oid);\n+\t\t\tif (ret)\n+\t\t\t\tret = error(_(\"unable to add %s to database\"),\n+\t\t\t\t\t path);\n+\t\t}\n \n \t\tfree(result_buf.ptr);\n \t\tif (ret)\n@@ -3597,7 +3607,8 @@ static int tree_entry_order(const void *a_, const void *b_)\n \t\t\t\t b->string, strlen(b->string), bmi->result.mode);\n }\n \n-static int write_tree(struct object_id *result_oid,\n+static int write_tree(struct merge_options *opt,\n+\t\t struct object_id *result_oid,\n \t\t struct string_list *versions,\n \t\t unsigned int offset,\n \t\t size_t hash_size)\n@@ -3631,8 +3642,14 @@ static int write_tree(struct object_id *result_oid,\n \t}\n \n \t/* Write this object file out, and record in result_oid */\n-\tif (write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid))\n+\tret = opt->write_pack\n+\t\t? index_tree_bulk_checkin_incore(result_oid,\n+\t\t\t\t\t\t buf.buf, buf.len, \"\", 1)\n+\t\t: write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);\n+\n+\tif (ret)\n \t\tret = -1;\n+\n \tstrbuf_release(&buf);\n \treturn ret;\n }\n@@ -3797,8 +3814,8 @@ static int write_completed_directory(struct merge_options *opt,\n \t\t */\n \t\tdir_info->is_null = 0;\n \t\tdir_info->result.mode = S_IFDIR;\n-\t\tif (write_tree(&dir_info->result.oid, &info->versions, offset,\n-\t\t\t opt->repo->hash_algo->rawsz) < 0)\n+\t\tif (write_tree(opt, &dir_info->result.oid, &info->versions,\n+\t\t\t offset, opt->repo->hash_algo->rawsz) < 0)\n \t\t\tret = -1;\n \t}\n \n@@ -4332,9 +4349,13 @@ static int process_entries(struct merge_options *opt,\n \t\tfflush(stdout);\n \t\tBUG(\"dir_metadata accounting completely off; shouldn't happen\");\n \t}\n-\tif (write_tree(result_oid, &dir_metadata.versions, 0,\n+\tif (write_tree(opt, result_oid, &dir_metadata.versions, 0,\n \t\t opt->repo->hash_algo->rawsz) < 0)\n \t\tret = -1;\n+\n+\tif (opt->write_pack)\n+\t\tend_odb_transaction();\n+\n cleanup:\n \tstring_list_clear(&plist, 0);\n \tstring_list_clear(&dir_metadata.versions, 0);\n@@ -4878,6 +4899,9 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)\n \t */\n \tstrmap_init(&opt->priv->conflicts);\n \n+\tif (opt->write_pack)\n+\t\tbegin_odb_transaction();\n+\n \ttrace2_region_leave(\"merge\", \"allocate/init\", opt->repo);\n }\n \ndiff --git a/merge-recursive.h b/merge-recursive.h\nindex b88000e3c2..156e160876 100644\n--- a/merge-recursive.h\n+++ b/merge-recursive.h\n@@ -48,6 +48,7 @@ struct merge_options {\n \tunsigned renormalize : 1;\n \tunsigned record_conflict_msgs_as_headers : 1;\n \tconst char *msg_header_prefix;\n+\tunsigned write_pack : 1;\n \n \t/* internal fields used by the implementation */\n \tstruct merge_options_internal *priv;\ndiff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\nindex 250f721795..2d81ff4de5 100755\n--- a/t/t4301-merge-tree-write-tree.sh\n+++ b/t/t4301-merge-tree-write-tree.sh\n@@ -922,4 +922,97 @@ test_expect_success 'check the input format when --stdin is passed' '\n \ttest_cmp expect actual\n '\n \n+packdir=\".git/objects/pack\"\n+\n+test_expect_success 'merge-tree can pack its result with --write-pack' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\n+\t# base has lines [3, 4, 5]\n+\t# - side adds to the beginning, resulting in [1, 2, 3, 4, 5]\n+\t# - other adds to the end, resulting in [3, 4, 5, 6, 7]\n+\t#\n+\t# merging the two should result in a new blob object containing\n+\t# [1, 2, 3, 4, 5, 6, 7], along with a new tree.\n+\ttest_commit -C repo base file \"$(test_seq 3 5)\" &&\n+\tgit -C repo branch -M main &&\n+\tgit -C repo checkout -b side main &&\n+\ttest_commit -C repo side file \"$(test_seq 1 5)\" &&\n+\tgit -C repo checkout -b other main &&\n+\ttest_commit -C repo other file \"$(test_seq 3 7)\" &&\n+\n+\tfind repo/$packdir -type f -name \"pack-*.idx\" >packs.before &&\n+\ttree=\"$(git -C repo merge-tree --write-pack \\\n+\t\trefs/tags/side refs/tags/other)\" &&\n+\tblob=\"$(git -C repo rev-parse $tree:file)\" &&\n+\tfind repo/$packdir -type f -name \"pack-*.idx\" >packs.after &&\n+\n+\ttest_must_be_empty packs.before &&\n+\ttest_line_count = 1 packs.after &&\n+\n+\tgit show-index <$(cat packs.after) >objects &&\n+\ttest_line_count = 2 objects &&\n+\tgrep \"^[1-9][0-9]* $tree\" objects &&\n+\tgrep \"^[1-9][0-9]* $blob\" objects\n+'\n+\n+test_expect_success 'merge-tree can write multiple packs with --write-pack' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\tgit config pack.packSizeLimit 512 &&\n+\n+\t\ttest_seq 512 >f &&\n+\n+\t\t# \"f\" contains roughly ~2,000 bytes.\n+\t\t#\n+\t\t# Each side (\"foo\" and \"bar\") adds a small amount of data at the\n+\t\t# beginning and end of \"base\", respectively.\n+\t\tgit add f &&\n+\t\ttest_tick &&\n+\t\tgit commit -m base &&\n+\t\tgit branch -M main &&\n+\n+\t\tgit checkout -b foo main &&\n+\t\t{\n+\t\t\techo foo && cat f\n+\t\t} >f.tmp &&\n+\t\tmv f.tmp f &&\n+\t\tgit add f &&\n+\t\ttest_tick &&\n+\t\tgit commit -m foo &&\n+\n+\t\tgit checkout -b bar main &&\n+\t\techo bar >>f &&\n+\t\tgit add f &&\n+\t\ttest_tick &&\n+\t\tgit commit -m bar &&\n+\n+\t\tfind $packdir -type f -name \"pack-*.idx\" >packs.before &&\n+\t\t# Merging either side should result in a new object which is\n+\t\t# larger than 1M, thus the result should be split into two\n+\t\t# separate packs.\n+\t\ttree=\"$(git merge-tree --write-pack \\\n+\t\t\trefs/heads/foo refs/heads/bar)\" &&\n+\t\tblob=\"$(git rev-parse $tree:f)\" &&\n+\t\tfind $packdir -type f -name \"pack-*.idx\" >packs.after &&\n+\n+\t\ttest_must_be_empty packs.before &&\n+\t\ttest_line_count = 2 packs.after &&\n+\t\tfor idx in $(cat packs.after)\n+\t\tdo\n+\t\t\tgit show-index <$idx || return 1\n+\t\tdone >objects &&\n+\n+\t\t# The resulting set of packs should contain one copy of both\n+\t\t# objects, each in a separate pack.\n+\t\ttest_line_count = 2 objects &&\n+\t\tgrep \"^[1-9][0-9]* $tree\" objects &&\n+\t\tgrep \"^[1-9][0-9]* $blob\" objects\n+\n+\t)\n+'\n+\n test_done\n-- \n2.42.0.405.g86fe3250c2\n"},{"id":"483530","messageId":"xmqqpm1auvw3.fsf@gitster.g","threadId":"60403","inReplyTo":"cover.1697736516.git.me@ttaylorr.com","subject":"Re: [PATCH v4 0/7] merge-ort: implement support for packing objects together","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-19T21:47:56Z","receivedAt":"2023-10-19T21:48:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> By factoring out a common\n> \"deflate object to pack\" that takes an abstract bulk_checkin_source as a\n> parameter, all of the earlier refactorings can be dropped since we\n> retain only a single caller instead of multiple.\n\nAh, that is how we managed to lose the preparatory split of new\nhelper functions. The names given to these functions that represent\nsteps of checkpointing were well thought out, and they served their\ndocumentation purposes very well, but if they all have a single\ncaller, then that is fine ;-).\n\nI _think_ I carefully followed the code/data flow of the new code to\nsee that we do identical things when streaming from a file to a pack\n(in other words, this does not introduce regression), and nothing\nobjectionable stood out. The order of presentation was a good way\nto let readers to do so, and the actual \"now we have everything\nworking as we want, let's teach the machinery to also read from an\nin-core buffer\" step being very small gives us extra confidence with\nthe result. Very nicely done.\n"},{"id":"483544","messageId":"20231020072941.GD1642714@coredump.intra.peff.net","threadId":"60403","inReplyTo":"cover.1697736516.git.me@ttaylorr.com","subject":"Re: [PATCH v4 0/7] merge-ort: implement support for packing objects together","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-20T07:29:41Z","receivedAt":"2023-10-20T07:29:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 19, 2023 at 01:28:38PM -0400, Taylor Blau wrote:\n\n> This is a new round that is significantly simplified thanks to\n> another very helpful suggestion[1] from Junio. By factoring out a common\n> \"deflate object to pack\" that takes an abstract bulk_checkin_source as a\n> parameter, all of the earlier refactorings can be dropped since we\n> retain only a single caller instead of multiple.\n\nFWIW, I gave this a read-over and did not see anything wrong (on the\ncontrary, I think the new abstractions make it quite easy to follow).\n\nTwo thoughts that occurred to me while reading it (but neither\nactionable for your series):\n\n - Having not worked with the bulk-checkin code much before, I thought\n it only put in one blob per pack, and thus you'd have to teach it to\n handle multiple objects, too. But fortunately I was wrong, and it\n handles this case already. (I mention this mainly because we had\n talked about it off-list a few weeks ago, and some of my confusion\n may have seeped into my comments then).\n\n - I did briefly wonder if we need this SOURCE abstraction at all. The\n file source assumes we can seek() and read(), so it must be a\n regular file. We could probably just mmap() it and treat it as\n in-core, too (this is what index_fd() and index_path() do under the\n hood!). But it would probably be a disservice to any platforms that\n do not support mmap, as the fallback is to read into a full buffer\n (and the whole point of the bulk checkin code is to avoid that).\n\n-Peff\n"},{"id":"483545","messageId":"20231020073500.GE1642714@coredump.intra.peff.net","threadId":"60403","inReplyTo":"97bb6e9f59e5092f0519c7d1141d0673313fdc33.1697736516.git.me@ttaylorr.com","subject":"Re: [PATCH v4 1/7] bulk-checkin: extract abstract `bulk_checkin_source`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-20T07:35:00Z","receivedAt":"2023-10-20T07:35:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 19, 2023 at 01:28:42PM -0400, Taylor Blau wrote:\n\n> +struct bulk_checkin_source {\n> +\tenum { SOURCE_FILE } type;\n> +\n> +\t/* SOURCE_FILE fields */\n> +\tint fd;\n> +\n> +\t/* common fields */\n> +\tsize_t size;\n> +\tconst char *path;\n> +};\n\nThis is a pretty minor nit, but we may find that \"SOURCE_FILE\" is not\nsufficiently name-spaced. Enum tags are in the global namespace, so\nthe compiler will barf if there are any conflicts.\n\nIt might be OK here, since this is local to a single C file (so we at\nleast are not hurting other code), but we may be in trouble if code in a\nheader file is less careful. There is already a near-miss here with\nGREP_SOURCE_FILE, but fortunately grep.h is indeed careful. :)\n\n(I notice that ref-filter.c similarly uses SOURCE_* for an internal\nenum).\n\n-Peff\n"},{"id":"483570","messageId":"xmqqpm19s0a0.fsf@gitster.g","threadId":"60403","inReplyTo":"20231020072941.GD1642714@coredump.intra.peff.net","subject":"Re: [PATCH v4 0/7] merge-ort: implement support for packing objects together","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-20T16:53:43Z","receivedAt":"2023-10-20T16:53:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> - Having not worked with the bulk-checkin code much before, I thought\n> it only put in one blob per pack, and thus you'd have to teach it to\n> handle multiple objects, too. But fortunately I was wrong, and it\n> handles this case already. (I mention this mainly because we had\n> talked about it off-list a few weeks ago, and some of my confusion\n> may have seeped into my comments then).\n\nYou probably had it mixed up with a real limitation, i.e., \"unlike\nfast-import that considers the immediate previous object could be a\ngood delta base, this code does not even attempt any deltification\"\n;-)\n\n> - I did briefly wonder if we need this SOURCE abstraction at all. The\n> file source assumes we can seek() and read(), so it must be a\n> regular file. We could probably just mmap() it and treat it as\n> in-core, too (this is what index_fd() and index_path() do under the\n> hood!).\n\nAhhhh, I've never thought of that one. I actually was hoping that\nwe can add an abstraction layer that can be reused elsewhere and\neverywhere (not limited to the bulk-checkin codepath for its source\nmaterial), which might help folks who want to refactor Git enough to\nallow them plug $CORP specific goodies like virtual filesystem layer\n;-).\n\n> But it would probably be a disservice to any platforms that\n> do not support mmap, as the fallback is to read into a full buffer\n> (and the whole point of the bulk checkin code is to avoid that).\n\nYes. The result was a pleasant read.\n\nThanks, both.\n"},{"id":"483571","messageId":"xmqqlebxs06n.fsf@gitster.g","threadId":"60403","inReplyTo":"20231020073500.GE1642714@coredump.intra.peff.net","subject":"Re: [PATCH v4 1/7] bulk-checkin: extract abstract `bulk_checkin_source`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-20T16:55:44Z","receivedAt":"2023-10-20T16:55:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Oct 19, 2023 at 01:28:42PM -0400, Taylor Blau wrote:\n>\n>> +struct bulk_checkin_source {\n>> +\tenum { SOURCE_FILE } type;\n>> +\n>> +\t/* SOURCE_FILE fields */\n>> +\tint fd;\n>> +\n>> +\t/* common fields */\n>> +\tsize_t size;\n>> +\tconst char *path;\n>> +};\n>\n> This is a pretty minor nit, but we may find that \"SOURCE_FILE\" is not\n> sufficiently name-spaced. Enum tags are in the global namespace, so\n> the compiler will barf if there are any conflicts.\n\nA very good point. This was one of the reasons why I suggested\navoiding the switch() based on a new enum altogether and use a\nvtable based approach instead. But it may do while this one is\nprivate to the file and never exposed to other subsystems.\n\n> It might be OK here, since this is local to a single C file (so we at\n> least are not hurting other code), but we may be in trouble if code in a\n> header file is less careful. There is already a near-miss here with\n> GREP_SOURCE_FILE, but fortunately grep.h is indeed careful. :)\n>\n> (I notice that ref-filter.c similarly uses SOURCE_* for an internal\n> enum).\n\nThanks.\n"},{"id":"483654","messageId":"ZTY6kTZT-ni16usH@tanuki","threadId":"60403","inReplyTo":"e427fe6ad383cc238c13f313dc9f11eab37a3840.1697736516.git.me@ttaylorr.com","subject":"Re: [PATCH v4 4/7] bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-23T09:19:13Z","receivedAt":"2023-10-23T09:19:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Oct 19, 2023 at 01:28:51PM -0400, Taylor Blau wrote:\n> Continue to prepare for streaming an object's contents directly from\n> memory by teaching `bulk_checkin_source` how to perform reads and seeks\n> based on an address in memory.\n> \n> Unlike file descriptors, which manage their own offset internally, we\n> have to keep track of how many bytes we've read out of the buffer, and\n> make sure we don't read past the end of the buffer.\n> \n> Suggested-by: Junio C Hamano \n> Signed-off-by: Taylor Blau \n> ---\n> bulk-checkin.c | 18 +++++++++++++++++-\n> 1 file changed, 17 insertions(+), 1 deletion(-)\n> \n> diff --git a/bulk-checkin.c b/bulk-checkin.c\n> index 28bc8d5ab4..60361b3e2e 100644\n> --- a/bulk-checkin.c\n> +++ b/bulk-checkin.c\n> @@ -141,11 +141,15 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id\n> }\n> \n> struct bulk_checkin_source {\n> -\tenum { SOURCE_FILE } type;\n> +\tenum { SOURCE_FILE, SOURCE_INCORE } type;\n> \n> \t/* SOURCE_FILE fields */\n> \tint fd;\n> \n> +\t/* SOURCE_INCORE fields */\n> +\tconst void *buf;\n> +\tsize_t read;\n> +\n> \t/* common fields */\n> \tsize_t size;\n> \tconst char *path;\n> @@ -157,6 +161,11 @@ static off_t bulk_checkin_source_seek_to(struct bulk_checkin_source *source,\n> \tswitch (source->type) {\n> \tcase SOURCE_FILE:\n> \t\treturn lseek(source->fd, offset, SEEK_SET);\n> +\tcase SOURCE_INCORE:\n> +\t\tif (!(0 <= offset && offset < source->size))\n> +\t\t\treturn (off_t)-1;\n> +\t\tsource->read = offset;\n> +\t\treturn source->read;\n> \tdefault:\n> \t\tBUG(\"unknown bulk-checkin source: %d\", source->type);\n> \t}\n> @@ -168,6 +177,13 @@ static ssize_t bulk_checkin_source_read(struct bulk_checkin_source *source,\n> \tswitch (source->type) {\n> \tcase SOURCE_FILE:\n> \t\treturn read_in_full(source->fd, buf, nr);\n> +\tcase SOURCE_INCORE:\n> +\t\tassert(source->read <= source->size);\n\nIs there any guideline around when to use `assert()` vs `BUG()`? I think\nthat this assertion here is quite critical, because when it does not\nhold we can end up performing out-of-bounds reads and writes. But as\nasserts are typically missing in non-debug builds, this safeguard would\nnot do anything for our end users, right?\n\nPatrick\n\n> +\t\tif (nr > source->size - source->read)\n> +\t\t\tnr = source->size - source->read;\n> +\t\tmemcpy(buf, (unsigned char *)source->buf + source->read, nr);\n> +\t\tsource->read += nr;\n> +\t\treturn nr;\n> \tdefault:\n> \t\tBUG(\"unknown bulk-checkin source: %d\", source->type);\n> \t}\n> -- \n> 2.42.0.405.g86fe3250c2\n> \n"},{"id":"483655","messageId":"ZTY6lwtMgtRwmMrB@tanuki","threadId":"60403","inReplyTo":"cover.1697736516.git.me@ttaylorr.com","subject":"Re: [PATCH v4 0/7] merge-ort: implement support for packing objects together","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-23T09:19:19Z","receivedAt":"2023-10-23T09:19:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Oct 19, 2023 at 01:28:38PM -0400, Taylor Blau wrote:\n> (Rebased onto the current tip of 'master', which is 813d9a9188 (The\n> nineteenth batch, 2023-10-18) at the time of writing).\n> \n> This series implements support for a new merge-tree option,\n> `--write-pack`, which causes any newly-written objects to be packed\n> together instead of being stored individually as loose.\n> \n> I intentionally broke this off from the existing thread, since I\n> accidentally rerolled mine and Jonathan Tan's Bloom v2 series into it,\n> causing some confusion.\n> \n> This is a new round that is significantly simplified thanks to\n> another very helpful suggestion[1] from Junio. By factoring out a common\n> \"deflate object to pack\" that takes an abstract bulk_checkin_source as a\n> parameter, all of the earlier refactorings can be dropped since we\n> retain only a single caller instead of multiple.\n> \n> This resulted in a rather satisfying range-diff (included below, as\n> usual), and a similarly satisfying inter-diff:\n> \n> $ git diff --stat tb/ort-bulk-checkin.v3..\n> bulk-checkin.c | 203 ++++++++++++++++---------------------------------\n> 1 file changed, 64 insertions(+), 139 deletions(-)\n> \n> Beyond that, the changes since last time can be viewed in the range-diff\n> below. Thanks in advance for any review!\n> \n> [1]: https://lore.kernel.org/git/xmqq34y7plj4.fsf@gitster.g/\n\nA single question regarding an `assert()` from my side. Other than that\nthe series looks good to me, thanks.\n\nPatrick\n\n> Taylor Blau (7):\n> bulk-checkin: extract abstract `bulk_checkin_source`\n> bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types\n> bulk-checkin: refactor deflate routine to accept a\n> `bulk_checkin_source`\n> bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`\n> bulk-checkin: introduce `index_blob_bulk_checkin_incore()`\n> bulk-checkin: introduce `index_tree_bulk_checkin_incore()`\n> builtin/merge-tree.c: implement support for `--write-pack`\n> \n> Documentation/git-merge-tree.txt | 4 +\n> builtin/merge-tree.c | 5 +\n> bulk-checkin.c | 161 ++++++++++++++++++++++++++-----\n> bulk-checkin.h | 8 ++\n> merge-ort.c | 42 ++++++--\n> merge-recursive.h | 1 +\n> t/t4301-merge-tree-write-tree.sh | 93 ++++++++++++++++++\n> 7 files changed, 280 insertions(+), 34 deletions(-)\n> \n> Range-diff against v3:\n> 1: 2dffa45183 < -: ---------- bulk-checkin: factor out `format_object_header_hash()`\n> 2: 7a10dc794a < -: ---------- bulk-checkin: factor out `prepare_checkpoint()`\n> 3: 20c32d2178 < -: ---------- bulk-checkin: factor out `truncate_checkpoint()`\n> 4: 893051d0b7 < -: ---------- bulk-checkin: factor out `finalize_checkpoint()`\n> 5: da52ec8380 ! 1: 97bb6e9f59 bulk-checkin: extract abstract `bulk_checkin_source`\n> @@ bulk-checkin.c: static int stream_blob_to_pack(struct bulk_checkin_packfile *sta\n> \t\t\tif (*already_hashed_to < offset) {\n> \t\t\t\tsize_t hsize = offset - *already_hashed_to;\n> @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n> - \tgit_hash_ctx ctx;\n> + \tunsigned header_len;\n> \tstruct hashfile_checkpoint checkpoint = {0};\n> \tstruct pack_idx_entry *idx = NULL;\n> +\tstruct bulk_checkin_source source = {\n> @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st\n> \tseekback = lseek(fd, 0, SEEK_CUR);\n> \tif (seekback == (off_t) -1)\n> @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n> - \twhile (1) {\n> - \t\tprepare_checkpoint(state, &checkpoint, idx, flags);\n> + \t\t\tcrc32_begin(state->f);\n> + \t\t}\n> \t\tif (!stream_blob_to_pack(state, &ctx, &already_hashed_to,\n> -\t\t\t\t\t fd, size, path, flags))\n> +\t\t\t\t\t &source, flags))\n> \t\t\tbreak;\n> - \t\ttruncate_checkpoint(state, &checkpoint, idx);\n> + \t\t/*\n> + \t\t * Writing this object to the current pack will make\n> +@@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n> + \t\thashfile_truncate(state->f, &checkpoint);\n> + \t\tstate->offset = checkpoint.offset;\n> + \t\tflush_bulk_checkin_packfile(state);\n> -\t\tif (lseek(fd, seekback, SEEK_SET) == (off_t) -1)\n> +\t\tif (bulk_checkin_source_seek_to(&source, seekback) == (off_t)-1)\n> \t\t\treturn error(\"cannot seek back\");\n> \t}\n> - \tfinalize_checkpoint(state, &ctx, &checkpoint, idx, result_oid);\n> + \tthe_hash_algo->final_oid_fn(result_oid, &ctx);\n> 7: 04ec74e357 ! 2: 9d633df339 bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types\n> @@ bulk-checkin.c: static int stream_blob_to_pack(struct bulk_checkin_packfile *sta\n> \ts.avail_out = sizeof(obuf) - hdrlen;\n> \n> @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n> - \n> - \twhile (1) {\n> - \t\tprepare_checkpoint(state, &checkpoint, idx, flags);\n> + \t\t\tidx->offset = state->offset;\n> + \t\t\tcrc32_begin(state->f);\n> + \t\t}\n> -\t\tif (!stream_blob_to_pack(state, &ctx, &already_hashed_to,\n> -\t\t\t\t\t &source, flags))\n> +\t\tif (!stream_obj_to_pack(state, &ctx, &already_hashed_to,\n> +\t\t\t\t\t&source, OBJ_BLOB, flags))\n> \t\t\tbreak;\n> - \t\ttruncate_checkpoint(state, &checkpoint, idx);\n> - \t\tif (bulk_checkin_source_seek_to(&source, seekback) == (off_t)-1)\n> + \t\t/*\n> + \t\t * Writing this object to the current pack will make\n> -: ---------- > 3: d5bbd7810e bulk-checkin: refactor deflate routine to accept a `bulk_checkin_source`\n> 6: 4e9bac5bc1 = 4: e427fe6ad3 bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`\n> 8: 8667b76365 ! 5: 48095afe80 bulk-checkin: introduce `index_blob_bulk_checkin_incore()`\n> @@ Commit message\n> objects individually as loose.\n> \n> Similar to the existing `index_blob_bulk_checkin()` function, the\n> - entrypoint delegates to `deflate_blob_to_pack_incore()`, which is\n> - responsible for formatting the pack header and then deflating the\n> - contents into the pack. The latter is accomplished by calling\n> - deflate_obj_contents_to_pack_incore(), which takes advantage of the\n> - earlier refactorings and is responsible for writing the object to the\n> - pack and handling any overage from pack.packSizeLimit.\n> -\n> - The bulk of the new functionality is implemented in the function\n> - `stream_obj_to_pack()`, which can handle streaming objects from memory\n> - to the bulk-checkin pack as a result of the earlier refactoring.\n> + entrypoint delegates to `deflate_obj_to_pack_incore()`. That function in\n> + turn delegates to deflate_obj_to_pack(), which is responsible for\n> + formatting the pack header and then deflating the contents into the\n> + pack.\n> \n> Consistent with the rest of the bulk-checkin mechanism, there are no\n> direct tests here. In future commits when we expose this new\n> @@ Commit message\n> Signed-off-by: Taylor Blau \n> \n> ## bulk-checkin.c ##\n> -@@ bulk-checkin.c: static void finalize_checkpoint(struct bulk_checkin_packfile *state,\n> - \t}\n> +@@ bulk-checkin.c: static int deflate_obj_to_pack(struct bulk_checkin_packfile *state,\n> + \treturn 0;\n> }\n> \n> -+static int deflate_obj_contents_to_pack_incore(struct bulk_checkin_packfile *state,\n> -+\t\t\t\t\t git_hash_ctx *ctx,\n> -+\t\t\t\t\t struct hashfile_checkpoint *checkpoint,\n> -+\t\t\t\t\t struct object_id *result_oid,\n> -+\t\t\t\t\t const void *buf, size_t size,\n> -+\t\t\t\t\t enum object_type type,\n> -+\t\t\t\t\t const char *path, unsigned flags)\n> ++static int deflate_obj_to_pack_incore(struct bulk_checkin_packfile *state,\n> ++\t\t\t\t struct object_id *result_oid,\n> ++\t\t\t\t const void *buf, size_t size,\n> ++\t\t\t\t const char *path, enum object_type type,\n> ++\t\t\t\t unsigned flags)\n> +{\n> -+\tstruct pack_idx_entry *idx = NULL;\n> -+\toff_t already_hashed_to = 0;\n> +\tstruct bulk_checkin_source source = {\n> +\t\t.type = SOURCE_INCORE,\n> +\t\t.buf = buf,\n> @@ bulk-checkin.c: static void finalize_checkpoint(struct bulk_checkin_packfile *st\n> +\t\t.path = path,\n> +\t};\n> +\n> -+\t/* Note: idx is non-NULL when we are writing */\n> -+\tif (flags & HASH_WRITE_OBJECT)\n> -+\t\tCALLOC_ARRAY(idx, 1);\n> -+\n> -+\twhile (1) {\n> -+\t\tprepare_checkpoint(state, checkpoint, idx, flags);\n> -+\n> -+\t\tif (!stream_obj_to_pack(state, ctx, &already_hashed_to, &source,\n> -+\t\t\t\t\ttype, flags))\n> -+\t\t\tbreak;\n> -+\t\ttruncate_checkpoint(state, checkpoint, idx);\n> -+\t\tbulk_checkin_source_seek_to(&source, 0);\n> -+\t}\n> -+\n> -+\tfinalize_checkpoint(state, ctx, checkpoint, idx, result_oid);\n> -+\n> -+\treturn 0;\n> -+}\n> -+\n> -+static int deflate_blob_to_pack_incore(struct bulk_checkin_packfile *state,\n> -+\t\t\t\t struct object_id *result_oid,\n> -+\t\t\t\t const void *buf, size_t size,\n> -+\t\t\t\t const char *path, unsigned flags)\n> -+{\n> -+\tgit_hash_ctx ctx;\n> -+\tstruct hashfile_checkpoint checkpoint = {0};\n> -+\n> -+\tformat_object_header_hash(the_hash_algo, &ctx, &checkpoint, OBJ_BLOB,\n> -+\t\t\t\t size);\n> -+\n> -+\treturn deflate_obj_contents_to_pack_incore(state, &ctx, &checkpoint,\n> -+\t\t\t\t\t\t result_oid, buf, size,\n> -+\t\t\t\t\t\t OBJ_BLOB, path, flags);\n> ++\treturn deflate_obj_to_pack(state, result_oid, &source, type, 0, flags);\n> +}\n> +\n> static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n> @@ bulk-checkin.c: int index_blob_bulk_checkin(struct object_id *oid,\n> +\t\t\t\t const void *buf, size_t size,\n> +\t\t\t\t const char *path, unsigned flags)\n> +{\n> -+\tint status = deflate_blob_to_pack_incore(&bulk_checkin_packfile, oid,\n> -+\t\t\t\t\t\t buf, size, path, flags);\n> ++\tint status = deflate_obj_to_pack_incore(&bulk_checkin_packfile, oid,\n> ++\t\t\t\t\t\tbuf, size, path, OBJ_BLOB,\n> ++\t\t\t\t\t\tflags);\n> +\tif (!odb_transaction_nesting)\n> +\t\tflush_bulk_checkin_packfile(&bulk_checkin_packfile);\n> +\treturn status;\n> 9: cba043ef14 ! 6: 60568f9281 bulk-checkin: introduce `index_tree_bulk_checkin_incore()`\n> @@ Commit message\n> machinery will have enough to keep track of the converted object's hash\n> in order to update the compatibility mapping.\n> \n> - Within `deflate_tree_to_pack_incore()`, the changes should be limited\n> - to something like:\n> + Within some thin wrapper around `deflate_obj_to_pack_incore()` (perhaps\n> + `deflate_tree_to_pack_incore()`), the changes should be limited to\n> + something like:\n> \n> struct strbuf converted = STRBUF_INIT;\n> if (the_repository->compat_hash_algo) {\n> @@ Commit message\n> Signed-off-by: Taylor Blau \n> \n> ## bulk-checkin.c ##\n> -@@ bulk-checkin.c: static int deflate_blob_to_pack_incore(struct bulk_checkin_packfile *state,\n> - \t\t\t\t\t\t OBJ_BLOB, path, flags);\n> - }\n> - \n> -+static int deflate_tree_to_pack_incore(struct bulk_checkin_packfile *state,\n> -+\t\t\t\t struct object_id *result_oid,\n> -+\t\t\t\t const void *buf, size_t size,\n> -+\t\t\t\t const char *path, unsigned flags)\n> -+{\n> -+\tgit_hash_ctx ctx;\n> -+\tstruct hashfile_checkpoint checkpoint = {0};\n> -+\n> -+\tformat_object_header_hash(the_hash_algo, &ctx, &checkpoint, OBJ_TREE,\n> -+\t\t\t\t size);\n> -+\n> -+\treturn deflate_obj_contents_to_pack_incore(state, &ctx, &checkpoint,\n> -+\t\t\t\t\t\t result_oid, buf, size,\n> -+\t\t\t\t\t\t OBJ_TREE, path, flags);\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> @@ bulk-checkin.c: int index_blob_bulk_checkin_incore(struct object_id *oid,\n> \treturn status;\n> }\n> @@ bulk-checkin.c: int index_blob_bulk_checkin_incore(struct object_id *oid,\n> +\t\t\t\t const void *buf, size_t size,\n> +\t\t\t\t const char *path, unsigned flags)\n> +{\n> -+\tint status = deflate_tree_to_pack_incore(&bulk_checkin_packfile, oid,\n> -+\t\t\t\t\t\t buf, size, path, flags);\n> ++\tint status = deflate_obj_to_pack_incore(&bulk_checkin_packfile, oid,\n> ++\t\t\t\t\t\tbuf, size, path, OBJ_TREE,\n> ++\t\t\t\t\t\tflags);\n> +\tif (!odb_transaction_nesting)\n> +\t\tflush_bulk_checkin_packfile(&bulk_checkin_packfile);\n> +\treturn status;\n> 10: ae70508037 = 7: b9be9df122 builtin/merge-tree.c: implement support for `--write-pack`\n> -- \n> 2.42.0.405.g86fe3250c2\n"},{"id":"483705","messageId":"20231023185842.GE1537181@coredump.intra.peff.net","threadId":"60403","inReplyTo":"ZTY6kTZT-ni16usH@tanuki","subject":"Re: [PATCH v4 4/7] bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-23T18:58:42Z","receivedAt":"2023-10-23T18:58:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 23, 2023 at 11:19:13AM +0200, Patrick Steinhardt wrote:\n\n> > +\tcase SOURCE_INCORE:\n> > +\t\tassert(source->read <= source->size);\n> \n> Is there any guideline around when to use `assert()` vs `BUG()`? I think\n> that this assertion here is quite critical, because when it does not\n> hold we can end up performing out-of-bounds reads and writes. But as\n> asserts are typically missing in non-debug builds, this safeguard would\n> not do anything for our end users, right?\n\nI don't think we have a written guideline. My philosophy is: always use\nBUG(), because you will never be surprised that the assertion was not\ncompiled in (and I think compiling without assertions is almost\ncertainly premature optimization, especially given the way we tend to\nuse them).\n\n-Peff\n"},{"id":"483741","messageId":"cover.1698101088.git.me@ttaylorr.com","threadId":"60403","inReplyTo":"cover.1697736516.git.me@ttaylorr.com","subject":"[PATCH v5 0/5] merge-ort: implement support for packing objects together","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-23T22:44:49Z","receivedAt":"2023-10-23T22:44:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"(Rebased onto the current tip of 'master', which is ceadf0f3cf (The\ntwentieth batch, 2023-10-20)).\n\nThis series implements support for a new merge-tree option,\n`--write-pack`, which causes any newly-written objects to be packed\ntogether instead of being stored individually as loose.\n\nThis is a minor follow-up that could be taken instead of v4 (though the\nchanges between these two most recent rounds are stylistic and a matter\nof subjective opinion).\n\nThis moves us the bulk_checkin_source structure introduced in response\nto Junio's suggestion during the last round further in the OOP\ndirection. Instead of switching on the enum type of the source, have\nfunction pointers for read() and seek() respectively.\n\nThe functionality at the end is the same, but this avoids some of the\nnamespacing issues that Peff pointed out while looking at v4. But I\nthink that this approach ended up being less heavy-weight than I had\noriginally imagined, so I think that this version is a worthwhile\nimprovement over v4.\n\nBeyond that, the changes since last time can be viewed in the range-diff\nbelow. Thanks in advance for any review!\n\n[1]: https://lore.kernel.org/git/xmqq34y7plj4.fsf@gitster.g/\n\nTaylor Blau (5):\n bulk-checkin: extract abstract `bulk_checkin_source`\n bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types\n bulk-checkin: introduce `index_blob_bulk_checkin_incore()`\n bulk-checkin: introduce `index_tree_bulk_checkin_incore()`\n builtin/merge-tree.c: implement support for `--write-pack`\n\n Documentation/git-merge-tree.txt | 4 +\n builtin/merge-tree.c | 5 +\n bulk-checkin.c | 197 +++++++++++++++++++++++++++----\n bulk-checkin.h | 8 ++\n merge-ort.c | 42 +++++--\n merge-recursive.h | 1 +\n t/t4301-merge-tree-write-tree.sh | 93 +++++++++++++++\n 7 files changed, 316 insertions(+), 34 deletions(-)\n\nRange-diff against v4:\n1: 97bb6e9f59 ! 1: 696aa027e4 bulk-checkin: extract abstract `bulk_checkin_source`\n @@ bulk-checkin.c: static int already_written(struct bulk_checkin_packfile *state,\n }\n \n +struct bulk_checkin_source {\n -+\tenum { SOURCE_FILE } type;\n ++\toff_t (*read)(struct bulk_checkin_source *, void *, size_t);\n ++\toff_t (*seek)(struct bulk_checkin_source *, off_t);\n +\n -+\t/* SOURCE_FILE fields */\n -+\tint fd;\n ++\tunion {\n ++\t\tstruct {\n ++\t\t\tint fd;\n ++\t\t} from_fd;\n ++\t} data;\n +\n -+\t/* common fields */\n +\tsize_t size;\n +\tconst char *path;\n +};\n +\n -+static off_t bulk_checkin_source_seek_to(struct bulk_checkin_source *source,\n -+\t\t\t\t\t off_t offset)\n ++static off_t bulk_checkin_source_read_from_fd(struct bulk_checkin_source *source,\n ++\t\t\t\t\t void *buf, size_t nr)\n +{\n -+\tswitch (source->type) {\n -+\tcase SOURCE_FILE:\n -+\t\treturn lseek(source->fd, offset, SEEK_SET);\n -+\tdefault:\n -+\t\tBUG(\"unknown bulk-checkin source: %d\", source->type);\n -+\t}\n ++\treturn read_in_full(source->data.from_fd.fd, buf, nr);\n +}\n +\n -+static ssize_t bulk_checkin_source_read(struct bulk_checkin_source *source,\n -+\t\t\t\t\tvoid *buf, size_t nr)\n ++static off_t bulk_checkin_source_seek_from_fd(struct bulk_checkin_source *source,\n ++\t\t\t\t\t off_t offset)\n +{\n -+\tswitch (source->type) {\n -+\tcase SOURCE_FILE:\n -+\t\treturn read_in_full(source->fd, buf, nr);\n -+\tdefault:\n -+\t\tBUG(\"unknown bulk-checkin source: %d\", source->type);\n -+\t}\n ++\treturn lseek(source->data.from_fd.fd, offset, SEEK_SET);\n ++}\n ++\n ++static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,\n ++\t\t\t\t\t int fd, size_t size,\n ++\t\t\t\t\t const char *path)\n ++{\n ++\tmemset(source, 0, sizeof(struct bulk_checkin_source));\n ++\n ++\tsource->read = bulk_checkin_source_read_from_fd;\n ++\tsource->seek = bulk_checkin_source_seek_from_fd;\n ++\n ++\tsource->data.from_fd.fd = fd;\n ++\n ++\tsource->size = size;\n ++\tsource->path = path;\n +}\n +\n /*\n @@ bulk-checkin.c: static int stream_blob_to_pack(struct bulk_checkin_packfile *sta\n -\t\t\tssize_t read_result = read_in_full(fd, ibuf, rsize);\n +\t\t\tssize_t read_result;\n +\n -+\t\t\tread_result = bulk_checkin_source_read(source, ibuf,\n -+\t\t\t\t\t\t\t rsize);\n ++\t\t\tread_result = source->read(source, ibuf, rsize);\n \t\t\tif (read_result < 0)\n -\t\t\t\tdie_errno(\"failed to read from '%s'\", path);\n +\t\t\t\tdie_errno(\"failed to read from '%s'\",\n @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st\n \tunsigned header_len;\n \tstruct hashfile_checkpoint checkpoint = {0};\n \tstruct pack_idx_entry *idx = NULL;\n -+\tstruct bulk_checkin_source source = {\n -+\t\t.type = SOURCE_FILE,\n -+\t\t.fd = fd,\n -+\t\t.size = size,\n -+\t\t.path = path,\n -+\t};\n ++\tstruct bulk_checkin_source source;\n ++\n ++\tinit_bulk_checkin_source_from_fd(&source, fd, size, path);\n \n \tseekback = lseek(fd, 0, SEEK_CUR);\n \tif (seekback == (off_t) -1)\n @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st\n \t\tstate->offset = checkpoint.offset;\n \t\tflush_bulk_checkin_packfile(state);\n -\t\tif (lseek(fd, seekback, SEEK_SET) == (off_t) -1)\n -+\t\tif (bulk_checkin_source_seek_to(&source, seekback) == (off_t)-1)\n ++\t\tif (source.seek(&source, seekback) == (off_t)-1)\n \t\t\treturn error(\"cannot seek back\");\n \t}\n \tthe_hash_algo->final_oid_fn(result_oid, &ctx);\n2: 9d633df339 < -: ---------- bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types\n3: d5bbd7810e ! 2: 596bd028a7 bulk-checkin: refactor deflate routine to accept a `bulk_checkin_source`\n @@ Metadata\n Author: Taylor Blau \n \n ## Commit message ##\n - bulk-checkin: refactor deflate routine to accept a `bulk_checkin_source`\n + bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types\n \n - Prepare for a future change where we will want to use a routine very\n - similar to the existing `deflate_blob_to_pack()` but over arbitrary\n - sources (i.e. either open file-descriptors, or a location in memory).\n + The existing `stream_blob_to_pack()` function is named based on the fact\n + that it knows only how to stream blobs into a bulk-checkin pack.\n \n - Extract out a common \"deflate_obj_to_pack()\" routine that acts on a\n - bulk_checkin_source, instead of a (int, size_t) pair. Then rewrite\n - `deflate_blob_to_pack()` in terms of it.\n + But there is no longer anything in this function which prevents us from\n + writing objects of arbitrary types to the bulk-checkin pack. Prepare to\n + write OBJ_TREEs by removing this assumption, adding an `enum\n + object_type` parameter to this function's argument list, and renaming it\n + to `stream_obj_to_pack()` accordingly.\n \n Signed-off-by: Taylor Blau \n \n ## bulk-checkin.c ##\n +@@ bulk-checkin.c: static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,\n + * status before calling us just in case we ask it to call us again\n + * with a new pack.\n + */\n +-static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n +-\t\t\t git_hash_ctx *ctx, off_t *already_hashed_to,\n +-\t\t\t struct bulk_checkin_source *source,\n +-\t\t\t unsigned flags)\n ++static int stream_obj_to_pack(struct bulk_checkin_packfile *state,\n ++\t\t\t git_hash_ctx *ctx, off_t *already_hashed_to,\n ++\t\t\t struct bulk_checkin_source *source,\n ++\t\t\t enum object_type type, unsigned flags)\n + {\n + \tgit_zstream s;\n + \tunsigned char ibuf[16384];\n +@@ bulk-checkin.c: static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n + \n + \tgit_deflate_init(&s, pack_compression_level);\n + \n +-\thdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB,\n +-\t\t\t\t\t size);\n ++\thdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), type, size);\n + \ts.next_out = obuf + hdrlen;\n + \ts.avail_out = sizeof(obuf) - hdrlen;\n + \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 @@ bulk-checkin.c: static void prepare_to_stream(struct bulk_checkin_packfile *stat\n \tunsigned header_len;\n \tstruct hashfile_checkpoint checkpoint = {0};\n \tstruct pack_idx_entry *idx = NULL;\n --\tstruct bulk_checkin_source source = {\n --\t\t.type = SOURCE_FILE,\n --\t\t.fd = fd,\n --\t\t.size = size,\n --\t\t.path = path,\n --\t};\n +-\tstruct bulk_checkin_source source;\n \n +-\tinit_bulk_checkin_source_from_fd(&source, fd, size, path);\n +-\n -\tseekback = lseek(fd, 0, SEEK_CUR);\n -\tif (seekback == (off_t) -1)\n -\t\treturn error(\"cannot find the current offset\");\n @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st\n \t\tprepare_to_stream(state, flags);\n \t\tif (idx) {\n @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n + \t\t\tidx->offset = state->offset;\n \t\t\tcrc32_begin(state->f);\n \t\t}\n - \t\tif (!stream_obj_to_pack(state, &ctx, &already_hashed_to,\n --\t\t\t\t\t&source, OBJ_BLOB, flags))\n +-\t\tif (!stream_blob_to_pack(state, &ctx, &already_hashed_to,\n +-\t\t\t\t\t &source, flags))\n ++\t\tif (!stream_obj_to_pack(state, &ctx, &already_hashed_to,\n +\t\t\t\t\tsource, type, flags))\n \t\t\tbreak;\n \t\t/*\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\tif (bulk_checkin_source_seek_to(&source, seekback) == (off_t)-1)\n -+\t\tif (bulk_checkin_source_seek_to(source, seekback) == (off_t)-1)\n +-\t\tif (source.seek(&source, seekback) == (off_t)-1)\n ++\t\tif (source->seek(source, seekback) == (off_t)-1)\n \t\t\treturn error(\"cannot seek back\");\n \t}\n \tthe_hash_algo->final_oid_fn(result_oid, &ctx);\n @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st\n +\t\t\t\tint fd, size_t size,\n +\t\t\t\tconst char *path, unsigned flags)\n +{\n -+\tstruct bulk_checkin_source source = {\n -+\t\t.type = SOURCE_FILE,\n -+\t\t.fd = fd,\n -+\t\t.size = size,\n -+\t\t.path = path,\n -+\t};\n -+\toff_t seekback = lseek(fd, 0, SEEK_CUR);\n ++\tstruct bulk_checkin_source source;\n ++\toff_t seekback;\n ++\n ++\tinit_bulk_checkin_source_from_fd(&source, fd, size, path);\n ++\n ++\tseekback = lseek(fd, 0, SEEK_CUR);\n +\tif (seekback == (off_t) -1)\n +\t\treturn error(\"cannot find the current offset\");\n +\n4: e427fe6ad3 < -: ---------- bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`\n5: 48095afe80 ! 3: d8cf8e4395 bulk-checkin: introduce `index_blob_bulk_checkin_incore()`\n @@ Metadata\n ## Commit message ##\n bulk-checkin: introduce `index_blob_bulk_checkin_incore()`\n \n - Now that we have factored out many of the common routines necessary to\n - index a new object into a pack created by the bulk-checkin machinery, we\n - can introduce a variant of `index_blob_bulk_checkin()` that acts on\n - blobs whose contents we can fit in memory.\n + Introduce `index_blob_bulk_checkin_incore()` which allows streaming\n + arbitrary blob contents from memory into the bulk-checkin pack.\n +\n + In order to support streaming from a location in memory, we must\n + implement a new kind of bulk_checkin_source that does just that. These\n + implementation in spread out across:\n +\n + - init_bulk_checkin_source_incore()\n + - bulk_checkin_source_read_incore()\n + - bulk_checkin_source_seek_incore()\n +\n + Note that, unlike file descriptors, which manage their own offset\n + internally, we have to keep track of how many bytes we've read out of\n + the buffer, and make sure we don't read past the end of the buffer.\n \n This will be useful in a couple of more commits in order to provide the\n `merge-tree` builtin with a mechanism to create a new pack containing\n @@ Commit message\n Signed-off-by: Taylor Blau \n \n ## bulk-checkin.c ##\n +@@ bulk-checkin.c: struct bulk_checkin_source {\n + \t\tstruct {\n + \t\t\tint fd;\n + \t\t} from_fd;\n ++\t\tstruct {\n ++\t\t\tconst void *buf;\n ++\t\t\tsize_t nr_read;\n ++\t\t} incore;\n + \t} data;\n + \n + \tsize_t size;\n +@@ bulk-checkin.c: static off_t bulk_checkin_source_seek_from_fd(struct bulk_checkin_source *source\n + \treturn lseek(source->data.from_fd.fd, offset, SEEK_SET);\n + }\n + \n ++static off_t bulk_checkin_source_read_incore(struct bulk_checkin_source *source,\n ++\t\t\t\t\t void *buf, size_t nr)\n ++{\n ++\tconst unsigned char *src = source->data.incore.buf;\n ++\n ++\tif (source->data.incore.nr_read > source->size)\n ++\t\tBUG(\"read beyond bulk-checkin source buffer end \"\n ++\t\t \"(%\"PRIuMAX\" > %\"PRIuMAX\")\",\n ++\t\t (uintmax_t)source->data.incore.nr_read,\n ++\t\t (uintmax_t)source->size);\n ++\n ++\tif (nr > source->size - source->data.incore.nr_read)\n ++\t\tnr = source->size - source->data.incore.nr_read;\n ++\n ++\tsrc += source->data.incore.nr_read;\n ++\n ++\tmemcpy(buf, src, nr);\n ++\tsource->data.incore.nr_read += nr;\n ++\treturn nr;\n ++}\n ++\n ++static off_t bulk_checkin_source_seek_incore(struct bulk_checkin_source *source,\n ++\t\t\t\t\t off_t offset)\n ++{\n ++\tif (!(0 <= offset && offset < source->size))\n ++\t\treturn (off_t)-1;\n ++\tsource->data.incore.nr_read = offset;\n ++\treturn source->data.incore.nr_read;\n ++}\n ++\n + static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,\n + \t\t\t\t\t int fd, size_t size,\n + \t\t\t\t\t const char *path)\n +@@ bulk-checkin.c: static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,\n + \tsource->path = path;\n + }\n + \n ++static void init_bulk_checkin_source_incore(struct bulk_checkin_source *source,\n ++\t\t\t\t\t const void *buf, size_t size,\n ++\t\t\t\t\t const char *path)\n ++{\n ++\tmemset(source, 0, sizeof(struct bulk_checkin_source));\n ++\n ++\tsource->read = bulk_checkin_source_read_incore;\n ++\tsource->seek = bulk_checkin_source_seek_incore;\n ++\n ++\tsource->data.incore.buf = buf;\n ++\tsource->data.incore.nr_read = 0;\n ++\n ++\tsource->size = size;\n ++\tsource->path = path;\n ++}\n ++\n + /*\n + * Read the contents from 'source' for 'size' bytes, streaming it to the\n + * packfile in state while updating the hash in ctx. Signal a failure\n @@ bulk-checkin.c: static int deflate_obj_to_pack(struct bulk_checkin_packfile *state,\n \treturn 0;\n }\n @@ bulk-checkin.c: static int deflate_obj_to_pack(struct bulk_checkin_packfile *sta\n +\t\t\t\t const char *path, enum object_type type,\n +\t\t\t\t unsigned flags)\n +{\n -+\tstruct bulk_checkin_source source = {\n -+\t\t.type = SOURCE_INCORE,\n -+\t\t.buf = buf,\n -+\t\t.size = size,\n -+\t\t.read = 0,\n -+\t\t.path = path,\n -+\t};\n ++\tstruct bulk_checkin_source source;\n ++\n ++\tinit_bulk_checkin_source_incore(&source, buf, size, path);\n +\n +\treturn deflate_obj_to_pack(state, result_oid, &source, type, 0, flags);\n +}\n6: 60568f9281 = 4: 2670192802 bulk-checkin: introduce `index_tree_bulk_checkin_incore()`\n7: b9be9df122 ! 5: 3595db76a5 builtin/merge-tree.c: implement support for `--write-pack`\n @@ Documentation/git-merge-tree.txt: OPTIONS\n \n ## builtin/merge-tree.c ##\n @@\n - #include \"quote.h\"\n #include \"tree.h\"\n #include \"config.h\"\n + #include \"strvec.h\"\n +#include \"bulk-checkin.h\"\n \n static int line_termination = '\\n';\n \n @@ builtin/merge-tree.c: struct merge_tree_options {\n - \tint show_messages;\n \tint name_only;\n \tint use_stdin;\n + \tstruct merge_options merge_options;\n +\tint write_pack;\n };\n \n static int real_merge(struct merge_tree_options *o,\n @@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,\n - \tinit_merge_options(&opt, the_repository);\n + \t\t\t\t _(\"not something we can merge\"));\n \n \topt.show_rename_progress = 0;\n +\topt.write_pack = o->write_pack;\n @@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,\n \topt.branch1 = branch1;\n \topt.branch2 = branch2;\n @@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char *prefix)\n - \t\t\t &merge_base,\n - \t\t\t N_(\"commit\"),\n \t\t\t N_(\"specify a merge-base for the merge\")),\n + \t\tOPT_STRVEC('X', \"strategy-option\", &xopts, N_(\"option=value\"),\n + \t\t\tN_(\"option for selected merge strategy\")),\n +\t\tOPT_BOOL(0, \"write-pack\", &o.write_pack,\n +\t\t\t N_(\"write new objects to a pack instead of as loose\")),\n \t\tOPT_END()\n\nbase-commit: ceadf0f3cf51550166a387ec8508bb55e7883057\n-- \n2.42.0.425.g963d08ddb3.dirty\n"},{"id":"483742","messageId":"696aa027e46ddec310812fad2d4b12082447d925.1698101088.git.me@ttaylorr.com","threadId":"60403","inReplyTo":"cover.1698101088.git.me@ttaylorr.com","subject":"[PATCH v5 1/5] bulk-checkin: extract abstract `bulk_checkin_source`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-23T22:44:56Z","receivedAt":"2023-10-23T22:44:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"A future commit will want to implement a very similar routine as in\n`stream_blob_to_pack()` with two notable changes:\n\n - Instead of streaming just OBJ_BLOBs, this new function may want to\n stream objects of arbitrary type.\n\n - Instead of streaming the object's contents from an open\n file-descriptor, this new function may want to \"stream\" its contents\n from memory.\n\nTo avoid duplicating a significant chunk of code between the existing\n`stream_blob_to_pack()`, extract an abstract `bulk_checkin_source`. This\nconcept currently is a thin layer of `lseek()` and `read_in_full()`, but\nwill grow to understand how to perform analogous operations when writing\nout an object's contents from memory.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Taylor Blau \n---\n bulk-checkin.c | 65 +++++++++++++++++++++++++++++++++++++++++++-------\n 1 file changed, 57 insertions(+), 8 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 6ce62999e5..174a6c24e4 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -140,8 +140,49 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id\n \treturn 0;\n }\n \n+struct bulk_checkin_source {\n+\toff_t (*read)(struct bulk_checkin_source *, void *, size_t);\n+\toff_t (*seek)(struct bulk_checkin_source *, off_t);\n+\n+\tunion {\n+\t\tstruct {\n+\t\t\tint fd;\n+\t\t} from_fd;\n+\t} data;\n+\n+\tsize_t size;\n+\tconst char *path;\n+};\n+\n+static off_t bulk_checkin_source_read_from_fd(struct bulk_checkin_source *source,\n+\t\t\t\t\t void *buf, size_t nr)\n+{\n+\treturn read_in_full(source->data.from_fd.fd, buf, nr);\n+}\n+\n+static off_t bulk_checkin_source_seek_from_fd(struct bulk_checkin_source *source,\n+\t\t\t\t\t off_t offset)\n+{\n+\treturn lseek(source->data.from_fd.fd, offset, SEEK_SET);\n+}\n+\n+static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,\n+\t\t\t\t\t int fd, size_t size,\n+\t\t\t\t\t const char *path)\n+{\n+\tmemset(source, 0, sizeof(struct bulk_checkin_source));\n+\n+\tsource->read = bulk_checkin_source_read_from_fd;\n+\tsource->seek = bulk_checkin_source_seek_from_fd;\n+\n+\tsource->data.from_fd.fd = fd;\n+\n+\tsource->size = size;\n+\tsource->path = path;\n+}\n+\n /*\n- * Read the contents from fd for size bytes, streaming it to the\n+ * Read the contents from 'source' for 'size' bytes, streaming it to the\n * packfile in state while updating the hash in ctx. Signal a failure\n * by returning a negative value when the resulting pack would exceed\n * the pack size limit and this is not the first object in the pack,\n@@ -157,7 +198,7 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id\n */\n static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n \t\t\t git_hash_ctx *ctx, off_t *already_hashed_to,\n-\t\t\t int fd, size_t size, const char *path,\n+\t\t\t struct bulk_checkin_source *source,\n \t\t\t unsigned flags)\n {\n \tgit_zstream s;\n@@ -167,22 +208,27 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n \tint status = Z_OK;\n \tint write_object = (flags & HASH_WRITE_OBJECT);\n \toff_t offset = 0;\n+\tsize_t size = source->size;\n \n \tgit_deflate_init(&s, pack_compression_level);\n \n-\thdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB, size);\n+\thdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB,\n+\t\t\t\t\t size);\n \ts.next_out = obuf + hdrlen;\n \ts.avail_out = sizeof(obuf) - hdrlen;\n \n \twhile (status != Z_STREAM_END) {\n \t\tif (size && !s.avail_in) {\n \t\t\tssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);\n-\t\t\tssize_t read_result = read_in_full(fd, ibuf, rsize);\n+\t\t\tssize_t read_result;\n+\n+\t\t\tread_result = source->read(source, ibuf, rsize);\n \t\t\tif (read_result < 0)\n-\t\t\t\tdie_errno(\"failed to read from '%s'\", path);\n+\t\t\t\tdie_errno(\"failed to read from '%s'\",\n+\t\t\t\t\t source->path);\n \t\t\tif (read_result != rsize)\n \t\t\t\tdie(\"failed to read %d bytes from '%s'\",\n-\t\t\t\t (int)rsize, path);\n+\t\t\t\t (int)rsize, source->path);\n \t\t\toffset += rsize;\n \t\t\tif (*already_hashed_to < offset) {\n \t\t\t\tsize_t hsize = offset - *already_hashed_to;\n@@ -258,6 +304,9 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \tunsigned header_len;\n \tstruct hashfile_checkpoint checkpoint = {0};\n \tstruct pack_idx_entry *idx = NULL;\n+\tstruct bulk_checkin_source source;\n+\n+\tinit_bulk_checkin_source_from_fd(&source, fd, size, path);\n \n \tseekback = lseek(fd, 0, SEEK_CUR);\n \tif (seekback == (off_t) -1)\n@@ -283,7 +332,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \t\t\tcrc32_begin(state->f);\n \t\t}\n \t\tif (!stream_blob_to_pack(state, &ctx, &already_hashed_to,\n-\t\t\t\t\t fd, size, path, flags))\n+\t\t\t\t\t &source, flags))\n \t\t\tbreak;\n \t\t/*\n \t\t * Writing this object to the current pack will make\n@@ -295,7 +344,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \t\thashfile_truncate(state->f, &checkpoint);\n \t\tstate->offset = checkpoint.offset;\n \t\tflush_bulk_checkin_packfile(state);\n-\t\tif (lseek(fd, seekback, SEEK_SET) == (off_t) -1)\n+\t\tif (source.seek(&source, seekback) == (off_t)-1)\n \t\t\treturn error(\"cannot seek back\");\n \t}\n \tthe_hash_algo->final_oid_fn(result_oid, &ctx);\n-- \n2.42.0.425.g963d08ddb3.dirty\n\n"},{"id":"483743","messageId":"596bd028a74f45c8f7ecf46dc5eb25f45ff5f523.1698101088.git.me@ttaylorr.com","threadId":"60403","inReplyTo":"cover.1698101088.git.me@ttaylorr.com","subject":"[PATCH v5 2/5] bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-23T22:44:59Z","receivedAt":"2023-10-23T22:45:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The existing `stream_blob_to_pack()` function is named based on the fact\nthat it knows only how to stream blobs into a bulk-checkin pack.\n\nBut there is no longer anything in this function which prevents us from\nwriting objects of arbitrary types to the bulk-checkin pack. Prepare to\nwrite OBJ_TREEs by removing this assumption, adding an `enum\nobject_type` parameter to this function's argument list, and renaming it\nto `stream_obj_to_pack()` accordingly.\n\nSigned-off-by: Taylor Blau \n---\n bulk-checkin.c | 61 +++++++++++++++++++++++++++++---------------------\n 1 file changed, 36 insertions(+), 25 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 174a6c24e4..79776e679e 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -196,10 +196,10 @@ static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,\n * status before calling us just in case we ask it to call us again\n * with a new pack.\n */\n-static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n-\t\t\t git_hash_ctx *ctx, off_t *already_hashed_to,\n-\t\t\t struct bulk_checkin_source *source,\n-\t\t\t unsigned flags)\n+static int stream_obj_to_pack(struct bulk_checkin_packfile *state,\n+\t\t\t git_hash_ctx *ctx, off_t *already_hashed_to,\n+\t\t\t struct bulk_checkin_source *source,\n+\t\t\t enum object_type type, unsigned flags)\n {\n \tgit_zstream s;\n \tunsigned char ibuf[16384];\n@@ -212,8 +212,7 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n \n \tgit_deflate_init(&s, pack_compression_level);\n \n-\thdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB,\n-\t\t\t\t\t size);\n+\thdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), type, size);\n \ts.next_out = obuf + hdrlen;\n \ts.avail_out = sizeof(obuf) - hdrlen;\n \n@@ -293,27 +292,23 @@ 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+\n+static int deflate_obj_to_pack(struct bulk_checkin_packfile *state,\n+\t\t\t struct object_id *result_oid,\n+\t\t\t struct bulk_checkin_source *source,\n+\t\t\t enum object_type type,\n+\t\t\t off_t seekback,\n+\t\t\t unsigned flags)\n {\n-\toff_t seekback, already_hashed_to;\n+\toff_t already_hashed_to = 0;\n \tgit_hash_ctx ctx;\n \tunsigned char obuf[16384];\n \tunsigned header_len;\n \tstruct hashfile_checkpoint checkpoint = {0};\n \tstruct pack_idx_entry *idx = NULL;\n-\tstruct bulk_checkin_source source;\n \n-\tinit_bulk_checkin_source_from_fd(&source, fd, size, path);\n-\n-\tseekback = lseek(fd, 0, SEEK_CUR);\n-\tif (seekback == (off_t) -1)\n-\t\treturn error(\"cannot find the current offset\");\n-\n-\theader_len = format_object_header((char *)obuf, sizeof(obuf),\n-\t\t\t\t\t OBJ_BLOB, size);\n+\theader_len = format_object_header((char *)obuf, sizeof(obuf), type,\n+\t\t\t\t\t source->size);\n \tthe_hash_algo->init_fn(&ctx);\n \tthe_hash_algo->update_fn(&ctx, obuf, header_len);\n \tthe_hash_algo->init_fn(&checkpoint.ctx);\n@@ -322,8 +317,6 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \tif ((flags & HASH_WRITE_OBJECT) != 0)\n \t\tCALLOC_ARRAY(idx, 1);\n \n-\talready_hashed_to = 0;\n-\n \twhile (1) {\n \t\tprepare_to_stream(state, flags);\n \t\tif (idx) {\n@@ -331,8 +324,8 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \t\t\tidx->offset = state->offset;\n \t\t\tcrc32_begin(state->f);\n \t\t}\n-\t\tif (!stream_blob_to_pack(state, &ctx, &already_hashed_to,\n-\t\t\t\t\t &source, flags))\n+\t\tif (!stream_obj_to_pack(state, &ctx, &already_hashed_to,\n+\t\t\t\t\tsource, type, flags))\n \t\t\tbreak;\n \t\t/*\n \t\t * Writing this object to the current pack will make\n@@ -344,7 +337,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \t\thashfile_truncate(state->f, &checkpoint);\n \t\tstate->offset = checkpoint.offset;\n \t\tflush_bulk_checkin_packfile(state);\n-\t\tif (source.seek(&source, seekback) == (off_t)-1)\n+\t\tif (source->seek(source, seekback) == (off_t)-1)\n \t\t\treturn error(\"cannot seek back\");\n \t}\n \tthe_hash_algo->final_oid_fn(result_oid, &ctx);\n@@ -366,6 +359,24 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n \treturn 0;\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+{\n+\tstruct bulk_checkin_source source;\n+\toff_t seekback;\n+\n+\tinit_bulk_checkin_source_from_fd(&source, fd, size, path);\n+\n+\tseekback = lseek(fd, 0, SEEK_CUR);\n+\tif (seekback == (off_t) -1)\n+\t\treturn error(\"cannot find the current offset\");\n+\n+\treturn deflate_obj_to_pack(state, result_oid, &source, OBJ_BLOB,\n+\t\t\t\t seekback, flags);\n+}\n+\n void prepare_loose_object_bulk_checkin(void)\n {\n \t/*\n-- \n2.42.0.425.g963d08ddb3.dirty\n\n"},{"id":"483744","messageId":"d8cf8e4395375f88fe4e1ade2b79a3be6ce5fb12.1698101088.git.me@ttaylorr.com","threadId":"60403","inReplyTo":"cover.1698101088.git.me@ttaylorr.com","subject":"[PATCH v5 3/5] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-23T22:45:01Z","receivedAt":"2023-10-23T22:45:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Introduce `index_blob_bulk_checkin_incore()` which allows streaming\narbitrary blob contents from memory into the bulk-checkin pack.\n\nIn order to support streaming from a location in memory, we must\nimplement a new kind of bulk_checkin_source that does just that. These\nimplementation in spread out across:\n\n - init_bulk_checkin_source_incore()\n - bulk_checkin_source_read_incore()\n - bulk_checkin_source_seek_incore()\n\nNote that, unlike file descriptors, which manage their own offset\ninternally, we have to keep track of how many bytes we've read out of\nthe buffer, and make sure we don't read past the end of the buffer.\n\nThis will be useful in a couple of more commits in order to provide the\n`merge-tree` builtin with a mechanism to create a new pack containing\nany objects it created during the merge, instead of storing those\nobjects individually as loose.\n\nSimilar to the existing `index_blob_bulk_checkin()` function, the\nentrypoint delegates to `deflate_obj_to_pack_incore()`. That function in\nturn delegates to deflate_obj_to_pack(), which is responsible for\nformatting the pack header and then deflating the contents into the\npack.\n\nConsistent with the rest of the bulk-checkin mechanism, there are no\ndirect tests here. In future commits when we expose this new\nfunctionality via the `merge-tree` builtin, we will test it indirectly\nthere.\n\nSigned-off-by: Taylor Blau \n---\n bulk-checkin.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++\n bulk-checkin.h | 4 +++\n 2 files changed, 79 insertions(+)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 79776e679e..b728210bc7 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -148,6 +148,10 @@ struct bulk_checkin_source {\n \t\tstruct {\n \t\t\tint fd;\n \t\t} from_fd;\n+\t\tstruct {\n+\t\t\tconst void *buf;\n+\t\t\tsize_t nr_read;\n+\t\t} incore;\n \t} data;\n \n \tsize_t size;\n@@ -166,6 +170,36 @@ static off_t bulk_checkin_source_seek_from_fd(struct bulk_checkin_source *source\n \treturn lseek(source->data.from_fd.fd, offset, SEEK_SET);\n }\n \n+static off_t bulk_checkin_source_read_incore(struct bulk_checkin_source *source,\n+\t\t\t\t\t void *buf, size_t nr)\n+{\n+\tconst unsigned char *src = source->data.incore.buf;\n+\n+\tif (source->data.incore.nr_read > source->size)\n+\t\tBUG(\"read beyond bulk-checkin source buffer end \"\n+\t\t \"(%\"PRIuMAX\" > %\"PRIuMAX\")\",\n+\t\t (uintmax_t)source->data.incore.nr_read,\n+\t\t (uintmax_t)source->size);\n+\n+\tif (nr > source->size - source->data.incore.nr_read)\n+\t\tnr = source->size - source->data.incore.nr_read;\n+\n+\tsrc += source->data.incore.nr_read;\n+\n+\tmemcpy(buf, src, nr);\n+\tsource->data.incore.nr_read += nr;\n+\treturn nr;\n+}\n+\n+static off_t bulk_checkin_source_seek_incore(struct bulk_checkin_source *source,\n+\t\t\t\t\t off_t offset)\n+{\n+\tif (!(0 <= offset && offset < source->size))\n+\t\treturn (off_t)-1;\n+\tsource->data.incore.nr_read = offset;\n+\treturn source->data.incore.nr_read;\n+}\n+\n static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,\n \t\t\t\t\t int fd, size_t size,\n \t\t\t\t\t const char *path)\n@@ -181,6 +215,22 @@ static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,\n \tsource->path = path;\n }\n \n+static void init_bulk_checkin_source_incore(struct bulk_checkin_source *source,\n+\t\t\t\t\t const void *buf, size_t size,\n+\t\t\t\t\t const char *path)\n+{\n+\tmemset(source, 0, sizeof(struct bulk_checkin_source));\n+\n+\tsource->read = bulk_checkin_source_read_incore;\n+\tsource->seek = bulk_checkin_source_seek_incore;\n+\n+\tsource->data.incore.buf = buf;\n+\tsource->data.incore.nr_read = 0;\n+\n+\tsource->size = size;\n+\tsource->path = path;\n+}\n+\n /*\n * Read the contents from 'source' for 'size' bytes, streaming it to the\n * packfile in state while updating the hash in ctx. Signal a failure\n@@ -359,6 +409,19 @@ static int deflate_obj_to_pack(struct bulk_checkin_packfile *state,\n \treturn 0;\n }\n \n+static int deflate_obj_to_pack_incore(struct bulk_checkin_packfile *state,\n+\t\t\t\t struct object_id *result_oid,\n+\t\t\t\t const void *buf, size_t size,\n+\t\t\t\t const char *path, enum object_type type,\n+\t\t\t\t unsigned flags)\n+{\n+\tstruct bulk_checkin_source source;\n+\n+\tinit_bulk_checkin_source_incore(&source, buf, size, path);\n+\n+\treturn deflate_obj_to_pack(state, result_oid, &source, type, 0, flags);\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@@ -421,6 +484,18 @@ int index_blob_bulk_checkin(struct object_id *oid,\n \treturn status;\n }\n \n+int index_blob_bulk_checkin_incore(struct object_id *oid,\n+\t\t\t\t const void *buf, size_t size,\n+\t\t\t\t const char *path, unsigned flags)\n+{\n+\tint status = deflate_obj_to_pack_incore(&bulk_checkin_packfile, oid,\n+\t\t\t\t\t\tbuf, size, path, OBJ_BLOB,\n+\t\t\t\t\t\tflags);\n+\tif (!odb_transaction_nesting)\n+\t\tflush_bulk_checkin_packfile(&bulk_checkin_packfile);\n+\treturn status;\n+}\n+\n void begin_odb_transaction(void)\n {\n \todb_transaction_nesting += 1;\ndiff --git a/bulk-checkin.h b/bulk-checkin.h\nindex aa7286a7b3..1b91daeaee 100644\n--- a/bulk-checkin.h\n+++ b/bulk-checkin.h\n@@ -13,6 +13,10 @@ 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+int index_blob_bulk_checkin_incore(struct object_id *oid,\n+\t\t\t\t const void *buf, size_t size,\n+\t\t\t\t const char *path, unsigned flags);\n+\n /*\n * Tell the object database to optimize for adding\n * multiple objects. end_odb_transaction must be called\n-- \n2.42.0.425.g963d08ddb3.dirty\n\n"},{"id":"483745","messageId":"2670192802a904b42fb0c11c26c9f7311aa8dd90.1698101088.git.me@ttaylorr.com","threadId":"60403","inReplyTo":"cover.1698101088.git.me@ttaylorr.com","subject":"[PATCH v5 4/5] bulk-checkin: introduce `index_tree_bulk_checkin_incore()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-23T22:45:04Z","receivedAt":"2023-10-23T22:45:07Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The remaining missing piece in order to teach the `merge-tree` builtin\nhow to write the contents of a merge into a pack is a function to index\ntree objects into a bulk-checkin pack.\n\nThis patch implements that missing piece, which is a thin wrapper around\nall of the functionality introduced in previous commits.\n\nIf and when Git gains support for a \"compatibility\" hash algorithm, the\nchanges to support that here will be minimal. The bulk-checkin machinery\nwill need to convert the incoming tree to compute its length under the\ncompatibility hash, necessary to reconstruct its header. With that\ninformation (and the converted contents of the tree), the bulk-checkin\nmachinery will have enough to keep track of the converted object's hash\nin order to update the compatibility mapping.\n\nWithin some thin wrapper around `deflate_obj_to_pack_incore()` (perhaps\n`deflate_tree_to_pack_incore()`), the changes should be limited to\nsomething like:\n\n struct strbuf converted = STRBUF_INIT;\n if (the_repository->compat_hash_algo) {\n if (convert_object_file(&compat_obj,\n the_repository->hash_algo,\n the_repository->compat_hash_algo, ...) < 0)\n die(...);\n\n format_object_header_hash(the_repository->compat_hash_algo,\n OBJ_TREE, size);\n }\n /* compute the converted tree's hash using the compat algorithm */\n strbuf_release(&converted);\n\n, assuming related changes throughout the rest of the bulk-checkin\nmachinery necessary to update the hash of the converted object, which\nare likewise minimal in size.\n\nSigned-off-by: Taylor Blau \n---\n bulk-checkin.c | 12 ++++++++++++\n bulk-checkin.h | 4 ++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex b728210bc7..bd6151ba3c 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -496,6 +496,18 @@ int index_blob_bulk_checkin_incore(struct object_id *oid,\n \treturn status;\n }\n \n+int index_tree_bulk_checkin_incore(struct object_id *oid,\n+\t\t\t\t const void *buf, size_t size,\n+\t\t\t\t const char *path, unsigned flags)\n+{\n+\tint status = deflate_obj_to_pack_incore(&bulk_checkin_packfile, oid,\n+\t\t\t\t\t\tbuf, size, path, OBJ_TREE,\n+\t\t\t\t\t\tflags);\n+\tif (!odb_transaction_nesting)\n+\t\tflush_bulk_checkin_packfile(&bulk_checkin_packfile);\n+\treturn status;\n+}\n+\n void begin_odb_transaction(void)\n {\n \todb_transaction_nesting += 1;\ndiff --git a/bulk-checkin.h b/bulk-checkin.h\nindex 1b91daeaee..89786b3954 100644\n--- a/bulk-checkin.h\n+++ b/bulk-checkin.h\n@@ -17,6 +17,10 @@ int index_blob_bulk_checkin_incore(struct object_id *oid,\n \t\t\t\t const void *buf, size_t size,\n \t\t\t\t const char *path, unsigned flags);\n \n+int index_tree_bulk_checkin_incore(struct object_id *oid,\n+\t\t\t\t const void *buf, size_t size,\n+\t\t\t\t const char *path, unsigned flags);\n+\n /*\n * Tell the object database to optimize for adding\n * multiple objects. end_odb_transaction must be called\n-- \n2.42.0.425.g963d08ddb3.dirty\n\n"},{"id":"483746","messageId":"3595db76a525fcebc3c896e231246704b044310c.1698101088.git.me@ttaylorr.com","threadId":"60403","inReplyTo":"cover.1698101088.git.me@ttaylorr.com","subject":"[PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-23T22:45:06Z","receivedAt":"2023-10-23T22:45:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When using merge-tree often within a repository[^1], it is possible to\ngenerate a relatively large number of loose objects, which can result in\ndegraded performance, and inode exhaustion in extreme cases.\n\nBuilding on the functionality introduced in previous commits, the\nbulk-checkin machinery now has support to write arbitrary blob and tree\nobjects which are small enough to be held in-core. We can use this to\nwrite any blob/tree objects generated by ORT into a separate pack\ninstead of writing them out individually as loose.\n\nThis functionality is gated behind a new `--write-pack` option to\n`merge-tree` that works with the (non-deprecated) `--write-tree` mode.\n\nThe implementation is relatively straightforward. There are two spots\nwithin the ORT mechanism where we call `write_object_file()`, one for\ncontent differences within blobs, and another to assemble any new trees\nnecessary to construct the merge. In each of those locations,\nconditionally replace calls to `write_object_file()` with\n`index_blob_bulk_checkin_incore()` or `index_tree_bulk_checkin_incore()`\ndepending on which kind of object we are writing.\n\nThe only remaining task is to begin and end the transaction necessary to\ninitialize the bulk-checkin machinery, and move any new pack(s) it\ncreated into the main object store.\n\n[^1]: Such is the case at GitHub, where we run presumptive \"test merges\"\n on open pull requests to see whether or not we can light up the merge\n button green depending on whether or not the presumptive merge was\n conflicted.\n\n This is done in response to a number of user-initiated events,\n including viewing an open pull request whose last test merge is stale\n with respect to the current base and tip of the pull request. As a\n result, merge-tree can be run very frequently on large, active\n repositories.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/git-merge-tree.txt | 4 ++\n builtin/merge-tree.c | 5 ++\n merge-ort.c | 42 +++++++++++----\n merge-recursive.h | 1 +\n t/t4301-merge-tree-write-tree.sh | 93 ++++++++++++++++++++++++++++++++\n 5 files changed, 136 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt\nindex ffc4fbf7e8..9d37609ef1 100644\n--- a/Documentation/git-merge-tree.txt\n+++ b/Documentation/git-merge-tree.txt\n@@ -69,6 +69,10 @@ OPTIONS\n \tspecify a merge-base for the merge, and specifying multiple bases is\n \tcurrently not supported. This option is incompatible with `--stdin`.\n \n+--write-pack::\n+\tWrite any new objects into a separate packfile instead of as\n+\tindividual loose objects.\n+\n [[OUTPUT]]\n OUTPUT\n ------\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex a35e0452d6..218442ac9b 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -19,6 +19,7 @@\n #include \"tree.h\"\n #include \"config.h\"\n #include \"strvec.h\"\n+#include \"bulk-checkin.h\"\n \n static int line_termination = '\\n';\n \n@@ -416,6 +417,7 @@ struct merge_tree_options {\n \tint name_only;\n \tint use_stdin;\n \tstruct merge_options merge_options;\n+\tint write_pack;\n };\n \n static int real_merge(struct merge_tree_options *o,\n@@ -441,6 +443,7 @@ static int real_merge(struct merge_tree_options *o,\n \t\t\t\t _(\"not something we can merge\"));\n \n \topt.show_rename_progress = 0;\n+\topt.write_pack = o->write_pack;\n \n \topt.branch1 = branch1;\n \topt.branch2 = branch2;\n@@ -553,6 +556,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"specify a merge-base for the merge\")),\n \t\tOPT_STRVEC('X', \"strategy-option\", &xopts, N_(\"option=value\"),\n \t\t\tN_(\"option for selected merge strategy\")),\n+\t\tOPT_BOOL(0, \"write-pack\", &o.write_pack,\n+\t\t\t N_(\"write new objects to a pack instead of as loose\")),\n \t\tOPT_END()\n \t};\n \ndiff --git a/merge-ort.c b/merge-ort.c\nindex 3653725661..523577d71e 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -48,6 +48,7 @@\n #include \"tree.h\"\n #include \"unpack-trees.h\"\n #include \"xdiff-interface.h\"\n+#include \"bulk-checkin.h\"\n \n /*\n * We have many arrays of size 3. Whenever we have such an array, the\n@@ -2108,10 +2109,19 @@ static int handle_content_merge(struct merge_options *opt,\n \t\tif ((merge_status < 0) || !result_buf.ptr)\n \t\t\tret = error(_(\"failed to execute internal merge\"));\n \n-\t\tif (!ret &&\n-\t\t write_object_file(result_buf.ptr, result_buf.size,\n-\t\t\t\t OBJ_BLOB, &result->oid))\n-\t\t\tret = error(_(\"unable to add %s to database\"), path);\n+\t\tif (!ret) {\n+\t\t\tret = opt->write_pack\n+\t\t\t\t? index_blob_bulk_checkin_incore(&result->oid,\n+\t\t\t\t\t\t\t\t result_buf.ptr,\n+\t\t\t\t\t\t\t\t result_buf.size,\n+\t\t\t\t\t\t\t\t path, 1)\n+\t\t\t\t: write_object_file(result_buf.ptr,\n+\t\t\t\t\t\t result_buf.size,\n+\t\t\t\t\t\t OBJ_BLOB, &result->oid);\n+\t\t\tif (ret)\n+\t\t\t\tret = error(_(\"unable to add %s to database\"),\n+\t\t\t\t\t path);\n+\t\t}\n \n \t\tfree(result_buf.ptr);\n \t\tif (ret)\n@@ -3597,7 +3607,8 @@ static int tree_entry_order(const void *a_, const void *b_)\n \t\t\t\t b->string, strlen(b->string), bmi->result.mode);\n }\n \n-static int write_tree(struct object_id *result_oid,\n+static int write_tree(struct merge_options *opt,\n+\t\t struct object_id *result_oid,\n \t\t struct string_list *versions,\n \t\t unsigned int offset,\n \t\t size_t hash_size)\n@@ -3631,8 +3642,14 @@ static int write_tree(struct object_id *result_oid,\n \t}\n \n \t/* Write this object file out, and record in result_oid */\n-\tif (write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid))\n+\tret = opt->write_pack\n+\t\t? index_tree_bulk_checkin_incore(result_oid,\n+\t\t\t\t\t\t buf.buf, buf.len, \"\", 1)\n+\t\t: write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);\n+\n+\tif (ret)\n \t\tret = -1;\n+\n \tstrbuf_release(&buf);\n \treturn ret;\n }\n@@ -3797,8 +3814,8 @@ static int write_completed_directory(struct merge_options *opt,\n \t\t */\n \t\tdir_info->is_null = 0;\n \t\tdir_info->result.mode = S_IFDIR;\n-\t\tif (write_tree(&dir_info->result.oid, &info->versions, offset,\n-\t\t\t opt->repo->hash_algo->rawsz) < 0)\n+\t\tif (write_tree(opt, &dir_info->result.oid, &info->versions,\n+\t\t\t offset, opt->repo->hash_algo->rawsz) < 0)\n \t\t\tret = -1;\n \t}\n \n@@ -4332,9 +4349,13 @@ static int process_entries(struct merge_options *opt,\n \t\tfflush(stdout);\n \t\tBUG(\"dir_metadata accounting completely off; shouldn't happen\");\n \t}\n-\tif (write_tree(result_oid, &dir_metadata.versions, 0,\n+\tif (write_tree(opt, result_oid, &dir_metadata.versions, 0,\n \t\t opt->repo->hash_algo->rawsz) < 0)\n \t\tret = -1;\n+\n+\tif (opt->write_pack)\n+\t\tend_odb_transaction();\n+\n cleanup:\n \tstring_list_clear(&plist, 0);\n \tstring_list_clear(&dir_metadata.versions, 0);\n@@ -4878,6 +4899,9 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)\n \t */\n \tstrmap_init(&opt->priv->conflicts);\n \n+\tif (opt->write_pack)\n+\t\tbegin_odb_transaction();\n+\n \ttrace2_region_leave(\"merge\", \"allocate/init\", opt->repo);\n }\n \ndiff --git a/merge-recursive.h b/merge-recursive.h\nindex 3d3b3e3c29..5c5ff380a8 100644\n--- a/merge-recursive.h\n+++ b/merge-recursive.h\n@@ -48,6 +48,7 @@ struct merge_options {\n \tunsigned renormalize : 1;\n \tunsigned record_conflict_msgs_as_headers : 1;\n \tconst char *msg_header_prefix;\n+\tunsigned write_pack : 1;\n \n \t/* internal fields used by the implementation */\n \tstruct merge_options_internal *priv;\ndiff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\nindex b2c8a43fce..d2a8634523 100755\n--- a/t/t4301-merge-tree-write-tree.sh\n+++ b/t/t4301-merge-tree-write-tree.sh\n@@ -945,4 +945,97 @@ test_expect_success 'check the input format when --stdin is passed' '\n \ttest_cmp expect actual\n '\n \n+packdir=\".git/objects/pack\"\n+\n+test_expect_success 'merge-tree can pack its result with --write-pack' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\n+\t# base has lines [3, 4, 5]\n+\t# - side adds to the beginning, resulting in [1, 2, 3, 4, 5]\n+\t# - other adds to the end, resulting in [3, 4, 5, 6, 7]\n+\t#\n+\t# merging the two should result in a new blob object containing\n+\t# [1, 2, 3, 4, 5, 6, 7], along with a new tree.\n+\ttest_commit -C repo base file \"$(test_seq 3 5)\" &&\n+\tgit -C repo branch -M main &&\n+\tgit -C repo checkout -b side main &&\n+\ttest_commit -C repo side file \"$(test_seq 1 5)\" &&\n+\tgit -C repo checkout -b other main &&\n+\ttest_commit -C repo other file \"$(test_seq 3 7)\" &&\n+\n+\tfind repo/$packdir -type f -name \"pack-*.idx\" >packs.before &&\n+\ttree=\"$(git -C repo merge-tree --write-pack \\\n+\t\trefs/tags/side refs/tags/other)\" &&\n+\tblob=\"$(git -C repo rev-parse $tree:file)\" &&\n+\tfind repo/$packdir -type f -name \"pack-*.idx\" >packs.after &&\n+\n+\ttest_must_be_empty packs.before &&\n+\ttest_line_count = 1 packs.after &&\n+\n+\tgit show-index <$(cat packs.after) >objects &&\n+\ttest_line_count = 2 objects &&\n+\tgrep \"^[1-9][0-9]* $tree\" objects &&\n+\tgrep \"^[1-9][0-9]* $blob\" objects\n+'\n+\n+test_expect_success 'merge-tree can write multiple packs with --write-pack' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\tgit config pack.packSizeLimit 512 &&\n+\n+\t\ttest_seq 512 >f &&\n+\n+\t\t# \"f\" contains roughly ~2,000 bytes.\n+\t\t#\n+\t\t# Each side (\"foo\" and \"bar\") adds a small amount of data at the\n+\t\t# beginning and end of \"base\", respectively.\n+\t\tgit add f &&\n+\t\ttest_tick &&\n+\t\tgit commit -m base &&\n+\t\tgit branch -M main &&\n+\n+\t\tgit checkout -b foo main &&\n+\t\t{\n+\t\t\techo foo && cat f\n+\t\t} >f.tmp &&\n+\t\tmv f.tmp f &&\n+\t\tgit add f &&\n+\t\ttest_tick &&\n+\t\tgit commit -m foo &&\n+\n+\t\tgit checkout -b bar main &&\n+\t\techo bar >>f &&\n+\t\tgit add f &&\n+\t\ttest_tick &&\n+\t\tgit commit -m bar &&\n+\n+\t\tfind $packdir -type f -name \"pack-*.idx\" >packs.before &&\n+\t\t# Merging either side should result in a new object which is\n+\t\t# larger than 1M, thus the result should be split into two\n+\t\t# separate packs.\n+\t\ttree=\"$(git merge-tree --write-pack \\\n+\t\t\trefs/heads/foo refs/heads/bar)\" &&\n+\t\tblob=\"$(git rev-parse $tree:f)\" &&\n+\t\tfind $packdir -type f -name \"pack-*.idx\" >packs.after &&\n+\n+\t\ttest_must_be_empty packs.before &&\n+\t\ttest_line_count = 2 packs.after &&\n+\t\tfor idx in $(cat packs.after)\n+\t\tdo\n+\t\t\tgit show-index <$idx || return 1\n+\t\tdone >objects &&\n+\n+\t\t# The resulting set of packs should contain one copy of both\n+\t\t# objects, each in a separate pack.\n+\t\ttest_line_count = 2 objects &&\n+\t\tgrep \"^[1-9][0-9]* $tree\" objects &&\n+\t\tgrep \"^[1-9][0-9]* $blob\" objects\n+\n+\t)\n+'\n+\n test_done\n-- \n2.42.0.425.g963d08ddb3.dirty\n"},{"id":"483751","messageId":"xmqqo7go7w63.fsf@gitster.g","threadId":"60403","inReplyTo":"cover.1698101088.git.me@ttaylorr.com","subject":"Re: [PATCH v5 0/5] merge-ort: implement support for packing objects together","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-23T23:31:48Z","receivedAt":"2023-10-23T23:31:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> But I\n> think that this approach ended up being less heavy-weight than I had\n> originally imagined, so I think that this version is a worthwhile\n> improvement over v4.\n\n;-).\n\nThis version is a good place to stop, a bit short of going full OO.\nNicely done.\n\n"},{"id":"483757","messageId":"ZTdlgEaRLpdSjB1n@tanuki","threadId":"60403","inReplyTo":"20231023185842.GE1537181@coredump.intra.peff.net","subject":"Re: [PATCH v4 4/7] bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-24T06:34:40Z","receivedAt":"2023-10-24T06:34:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Oct 23, 2023 at 02:58:42PM -0400, Jeff King wrote:\n> On Mon, Oct 23, 2023 at 11:19:13AM +0200, Patrick Steinhardt wrote:\n> \n> > > +\tcase SOURCE_INCORE:\n> > > +\t\tassert(source->read <= source->size);\n> > \n> > Is there any guideline around when to use `assert()` vs `BUG()`? I think\n> > that this assertion here is quite critical, because when it does not\n> > hold we can end up performing out-of-bounds reads and writes. But as\n> > asserts are typically missing in non-debug builds, this safeguard would\n> > not do anything for our end users, right?\n> \n> I don't think we have a written guideline. My philosophy is: always use\n> BUG(), because you will never be surprised that the assertion was not\n> compiled in (and I think compiling without assertions is almost\n> certainly premature optimization, especially given the way we tend to\n> use them).\n> \n> -Peff\n\nI'm inclined to agree with your philosophy. Makes me wonder whether we\nshould write a Coccinelle rule to catch this. But a quick-and-dirty grep\nin our codebase shows that such a rule would cause quite a lot of churn:\n\n$ git grep BUG\\( | wc -l\n677\n$ git grep assert\\( | wc -l\n549\n\nProbably not worth it.\n\nPatrick\n"},{"id":"483797","messageId":"xmqqedhk54pb.fsf@gitster.g","threadId":"60403","inReplyTo":"ZTdlgEaRLpdSjB1n@tanuki","subject":"Re: [PATCH v4 4/7] bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-24T17:08:00Z","receivedAt":"2023-10-24T17:08:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> I'm inclined to agree with your philosophy. Makes me wonder whether we\n> should write a Coccinelle rule to catch this. But a quick-and-dirty grep\n> in our codebase shows that such a rule would cause quite a lot of churn:\n>\n> $ git grep BUG\\( | wc -l\n> 677\n> $ git grep assert\\( | wc -l\n> 549\n>\n> Probably not worth it.\n\nYeah, we can stick to our usual \"do not add new instances, fix them\nwhile touching near-by code\" pattern for this one, I would say.\n\nThanks.\n"},{"id":"483836","messageId":"20231025073736.GB2145145@coredump.intra.peff.net","threadId":"60403","inReplyTo":"696aa027e46ddec310812fad2d4b12082447d925.1698101088.git.me@ttaylorr.com","subject":"Re: [PATCH v5 1/5] bulk-checkin: extract abstract `bulk_checkin_source`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-25T07:37:36Z","receivedAt":"2023-10-25T07:42:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 23, 2023 at 06:44:56PM -0400, Taylor Blau wrote:\n\n> +struct bulk_checkin_source {\n> +\toff_t (*read)(struct bulk_checkin_source *, void *, size_t);\n> +\toff_t (*seek)(struct bulk_checkin_source *, off_t);\n> +\n> +\tunion {\n> +\t\tstruct {\n> +\t\t\tint fd;\n> +\t\t} from_fd;\n> +\t} data;\n> +\n> +\tsize_t size;\n> +\tconst char *path;\n> +};\n\nThe virtual functions combined with the union are a funny mix of\nobject-oriented and procedural code. The bulk_checkin_source has\ntotally virtualized functions, but knows about all of the ancillary data\neach set of virtualized functions might want. ;)\n\nI think the more pure OO version would embed the parent, and have each\nconcrete type define its own struct type, like:\n\n struct bulk_checkin_source_fd {\n\tstruct bulk_checkin_source src;\n\tint fd;\n };\n\nThat works great if the code which constructs it knows which concrete\ntype it wants, and can just do:\n\n struct bulk_checkin_source_fd src;\n init_bulk_checkin_source_from_fd(&src, ...);\n\nIf even the construction is somewhat virtualized, then you are stuck\nwith heap constructors like:\n\n struct bulk_checkin_source *bulk_checkin_source_from_fd(...);\n\nNot too bad, but you have to remember to free now.\n\nAlternatively, I think some of our other OO code just leaves room for\na type-specific void pointer, like:\n\n struct bulk_checkin_source {\n\t...the usual stuff...\n\n\tvoid *magic_type_data;\n };\n\nand then the init_bulk_checkin_source_from_fd() function allocates its\nown heap struct for the magic_type_data field and sticks the int in\nthere.\n\nThat said, both of those are a lot more annoying to use in C (more\nboilerplate, more casting, and more opportunities to get something\nwrong, including leaks). So I don't mind this in-between state. It is a\nfunny layering violating from an OO standpoint, but it's not like we\nexpect an unbounded set of concrete types to \"inherit\" from the source\nstruct.\n\n-Peff\n"},{"id":"483838","messageId":"ZTjKjkRMkmCuxDU1@tanuki","threadId":"60403","inReplyTo":"d8cf8e4395375f88fe4e1ade2b79a3be6ce5fb12.1698101088.git.me@ttaylorr.com","subject":"Re: [PATCH v5 3/5] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-25T07:58:06Z","receivedAt":"2023-10-25T07:58:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Oct 23, 2023 at 06:45:01PM -0400, Taylor Blau wrote:\n> Introduce `index_blob_bulk_checkin_incore()` which allows streaming\n> arbitrary blob contents from memory into the bulk-checkin pack.\n> \n> In order to support streaming from a location in memory, we must\n> implement a new kind of bulk_checkin_source that does just that. These\n> implementation in spread out across:\n\nNit: the commit message is a bit off here. Probably not worth a reroll\nthough.\n\n> - init_bulk_checkin_source_incore()\n> - bulk_checkin_source_read_incore()\n> - bulk_checkin_source_seek_incore()\n> \n> Note that, unlike file descriptors, which manage their own offset\n> internally, we have to keep track of how many bytes we've read out of\n> the buffer, and make sure we don't read past the end of the buffer.\n> \n> This will be useful in a couple of more commits in order to provide the\n> `merge-tree` builtin with a mechanism to create a new pack containing\n> any objects it created during the merge, instead of storing those\n> objects individually as loose.\n> \n> Similar to the existing `index_blob_bulk_checkin()` function, the\n> entrypoint delegates to `deflate_obj_to_pack_incore()`. That function in\n> turn delegates to deflate_obj_to_pack(), which is responsible for\n> formatting the pack header and then deflating the contents into the\n> pack.\n> \n> Consistent with the rest of the bulk-checkin mechanism, there are no\n> direct tests here. In future commits when we expose this new\n> functionality via the `merge-tree` builtin, we will test it indirectly\n> there.\n> \n> Signed-off-by: Taylor Blau \n> ---\n> bulk-checkin.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++\n> bulk-checkin.h | 4 +++\n> 2 files changed, 79 insertions(+)\n> \n> diff --git a/bulk-checkin.c b/bulk-checkin.c\n> index 79776e679e..b728210bc7 100644\n> --- a/bulk-checkin.c\n> +++ b/bulk-checkin.c\n> @@ -148,6 +148,10 @@ struct bulk_checkin_source {\n> \t\tstruct {\n> \t\t\tint fd;\n> \t\t} from_fd;\n> +\t\tstruct {\n> +\t\t\tconst void *buf;\n> +\t\t\tsize_t nr_read;\n> +\t\t} incore;\n> \t} data;\n> \n> \tsize_t size;\n> @@ -166,6 +170,36 @@ static off_t bulk_checkin_source_seek_from_fd(struct bulk_checkin_source *source\n> \treturn lseek(source->data.from_fd.fd, offset, SEEK_SET);\n> }\n> \n> +static off_t bulk_checkin_source_read_incore(struct bulk_checkin_source *source,\n> +\t\t\t\t\t void *buf, size_t nr)\n> +{\n> +\tconst unsigned char *src = source->data.incore.buf;\n> +\n> +\tif (source->data.incore.nr_read > source->size)\n> +\t\tBUG(\"read beyond bulk-checkin source buffer end \"\n> +\t\t \"(%\"PRIuMAX\" > %\"PRIuMAX\")\",\n> +\t\t (uintmax_t)source->data.incore.nr_read,\n> +\t\t (uintmax_t)source->size);\n> +\n> +\tif (nr > source->size - source->data.incore.nr_read)\n> +\t\tnr = source->size - source->data.incore.nr_read;\n> +\n> +\tsrc += source->data.incore.nr_read;\n> +\n> +\tmemcpy(buf, src, nr);\n> +\tsource->data.incore.nr_read += nr;\n> +\treturn nr;\n> +}\n> +\n> +static off_t bulk_checkin_source_seek_incore(struct bulk_checkin_source *source,\n> +\t\t\t\t\t off_t offset)\n> +{\n> +\tif (!(0 <= offset && offset < source->size))\n> +\t\treturn (off_t)-1;\n\nAt the risk of showing my own ignorance, but why is the cast here\nnecessary?\n\nPatrick\n\n> +\tsource->data.incore.nr_read = offset;\n> +\treturn source->data.incore.nr_read;\n> +}\n> +\n> static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,\n> \t\t\t\t\t int fd, size_t size,\n> \t\t\t\t\t const char *path)\n> @@ -181,6 +215,22 @@ static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,\n> \tsource->path = path;\n> }\n> \n> +static void init_bulk_checkin_source_incore(struct bulk_checkin_source *source,\n> +\t\t\t\t\t const void *buf, size_t size,\n> +\t\t\t\t\t const char *path)\n> +{\n> +\tmemset(source, 0, sizeof(struct bulk_checkin_source));\n> +\n> +\tsource->read = bulk_checkin_source_read_incore;\n> +\tsource->seek = bulk_checkin_source_seek_incore;\n> +\n> +\tsource->data.incore.buf = buf;\n> +\tsource->data.incore.nr_read = 0;\n> +\n> +\tsource->size = size;\n> +\tsource->path = path;\n> +}\n> +\n> /*\n> * Read the contents from 'source' for 'size' bytes, streaming it to the\n> * packfile in state while updating the hash in ctx. Signal a failure\n> @@ -359,6 +409,19 @@ static int deflate_obj_to_pack(struct bulk_checkin_packfile *state,\n> \treturn 0;\n> }\n> \n> +static int deflate_obj_to_pack_incore(struct bulk_checkin_packfile *state,\n> +\t\t\t\t struct object_id *result_oid,\n> +\t\t\t\t const void *buf, size_t size,\n> +\t\t\t\t const char *path, enum object_type type,\n> +\t\t\t\t unsigned flags)\n> +{\n> +\tstruct bulk_checkin_source source;\n> +\n> +\tinit_bulk_checkin_source_incore(&source, buf, size, path);\n> +\n> +\treturn deflate_obj_to_pack(state, result_oid, &source, type, 0, flags);\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> @@ -421,6 +484,18 @@ int index_blob_bulk_checkin(struct object_id *oid,\n> \treturn status;\n> }\n> \n> +int index_blob_bulk_checkin_incore(struct object_id *oid,\n> +\t\t\t\t const void *buf, size_t size,\n> +\t\t\t\t const char *path, unsigned flags)\n> +{\n> +\tint status = deflate_obj_to_pack_incore(&bulk_checkin_packfile, oid,\n> +\t\t\t\t\t\tbuf, size, path, OBJ_BLOB,\n> +\t\t\t\t\t\tflags);\n> +\tif (!odb_transaction_nesting)\n> +\t\tflush_bulk_checkin_packfile(&bulk_checkin_packfile);\n> +\treturn status;\n> +}\n> +\n> void begin_odb_transaction(void)\n> {\n> \todb_transaction_nesting += 1;\n> diff --git a/bulk-checkin.h b/bulk-checkin.h\n> index aa7286a7b3..1b91daeaee 100644\n> --- a/bulk-checkin.h\n> +++ b/bulk-checkin.h\n> @@ -13,6 +13,10 @@ 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> +int index_blob_bulk_checkin_incore(struct object_id *oid,\n> +\t\t\t\t const void *buf, size_t size,\n> +\t\t\t\t const char *path, unsigned flags);\n> +\n> /*\n> * Tell the object database to optimize for adding\n> * multiple objects. end_odb_transaction must be called\n> -- \n> 2.42.0.425.g963d08ddb3.dirty\n> \n"},{"id":"483839","messageId":"ZTjKk8E55M7lQN1m@tanuki","threadId":"60403","inReplyTo":"3595db76a525fcebc3c896e231246704b044310c.1698101088.git.me@ttaylorr.com","subject":"Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-25T07:58:11Z","receivedAt":"2023-10-25T07:58:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Oct 23, 2023 at 06:45:06PM -0400, Taylor Blau wrote:\n> When using merge-tree often within a repository[^1], it is possible to\n> generate a relatively large number of loose objects, which can result in\n> degraded performance, and inode exhaustion in extreme cases.\n> \n> Building on the functionality introduced in previous commits, the\n> bulk-checkin machinery now has support to write arbitrary blob and tree\n> objects which are small enough to be held in-core. We can use this to\n> write any blob/tree objects generated by ORT into a separate pack\n> instead of writing them out individually as loose.\n> \n> This functionality is gated behind a new `--write-pack` option to\n> `merge-tree` that works with the (non-deprecated) `--write-tree` mode.\n> \n> The implementation is relatively straightforward. There are two spots\n> within the ORT mechanism where we call `write_object_file()`, one for\n> content differences within blobs, and another to assemble any new trees\n> necessary to construct the merge. In each of those locations,\n> conditionally replace calls to `write_object_file()` with\n> `index_blob_bulk_checkin_incore()` or `index_tree_bulk_checkin_incore()`\n> depending on which kind of object we are writing.\n> \n> The only remaining task is to begin and end the transaction necessary to\n> initialize the bulk-checkin machinery, and move any new pack(s) it\n> created into the main object store.\n> \n> [^1]: Such is the case at GitHub, where we run presumptive \"test merges\"\n> on open pull requests to see whether or not we can light up the merge\n> button green depending on whether or not the presumptive merge was\n> conflicted.\n> \n> This is done in response to a number of user-initiated events,\n> including viewing an open pull request whose last test merge is stale\n> with respect to the current base and tip of the pull request. As a\n> result, merge-tree can be run very frequently on large, active\n> repositories.\n> \n> Signed-off-by: Taylor Blau \n> ---\n> Documentation/git-merge-tree.txt | 4 ++\n> builtin/merge-tree.c | 5 ++\n> merge-ort.c | 42 +++++++++++----\n> merge-recursive.h | 1 +\n> t/t4301-merge-tree-write-tree.sh | 93 ++++++++++++++++++++++++++++++++\n> 5 files changed, 136 insertions(+), 9 deletions(-)\n> \n> diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt\n> index ffc4fbf7e8..9d37609ef1 100644\n> --- a/Documentation/git-merge-tree.txt\n> +++ b/Documentation/git-merge-tree.txt\n> @@ -69,6 +69,10 @@ OPTIONS\n> \tspecify a merge-base for the merge, and specifying multiple bases is\n> \tcurrently not supported. This option is incompatible with `--stdin`.\n> \n> +--write-pack::\n> +\tWrite any new objects into a separate packfile instead of as\n> +\tindividual loose objects.\n> +\n> [[OUTPUT]]\n> OUTPUT\n> ------\n> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\n> index a35e0452d6..218442ac9b 100644\n> --- a/builtin/merge-tree.c\n> +++ b/builtin/merge-tree.c\n> @@ -19,6 +19,7 @@\n> #include \"tree.h\"\n> #include \"config.h\"\n> #include \"strvec.h\"\n> +#include \"bulk-checkin.h\"\n> \n> static int line_termination = '\\n';\n> \n> @@ -416,6 +417,7 @@ struct merge_tree_options {\n> \tint name_only;\n> \tint use_stdin;\n> \tstruct merge_options merge_options;\n> +\tint write_pack;\n> };\n> \n> static int real_merge(struct merge_tree_options *o,\n> @@ -441,6 +443,7 @@ static int real_merge(struct merge_tree_options *o,\n> \t\t\t\t _(\"not something we can merge\"));\n> \n> \topt.show_rename_progress = 0;\n> +\topt.write_pack = o->write_pack;\n> \n> \topt.branch1 = branch1;\n> \topt.branch2 = branch2;\n> @@ -553,6 +556,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)\n> \t\t\t N_(\"specify a merge-base for the merge\")),\n> \t\tOPT_STRVEC('X', \"strategy-option\", &xopts, N_(\"option=value\"),\n> \t\t\tN_(\"option for selected merge strategy\")),\n> +\t\tOPT_BOOL(0, \"write-pack\", &o.write_pack,\n> +\t\t\t N_(\"write new objects to a pack instead of as loose\")),\n> \t\tOPT_END()\n> \t};\n> \n> diff --git a/merge-ort.c b/merge-ort.c\n> index 3653725661..523577d71e 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -48,6 +48,7 @@\n> #include \"tree.h\"\n> #include \"unpack-trees.h\"\n> #include \"xdiff-interface.h\"\n> +#include \"bulk-checkin.h\"\n> \n> /*\n> * We have many arrays of size 3. Whenever we have such an array, the\n> @@ -2108,10 +2109,19 @@ static int handle_content_merge(struct merge_options *opt,\n> \t\tif ((merge_status < 0) || !result_buf.ptr)\n> \t\t\tret = error(_(\"failed to execute internal merge\"));\n> \n> -\t\tif (!ret &&\n> -\t\t write_object_file(result_buf.ptr, result_buf.size,\n> -\t\t\t\t OBJ_BLOB, &result->oid))\n> -\t\t\tret = error(_(\"unable to add %s to database\"), path);\n> +\t\tif (!ret) {\n> +\t\t\tret = opt->write_pack\n> +\t\t\t\t? index_blob_bulk_checkin_incore(&result->oid,\n> +\t\t\t\t\t\t\t\t result_buf.ptr,\n> +\t\t\t\t\t\t\t\t result_buf.size,\n> +\t\t\t\t\t\t\t\t path, 1)\n> +\t\t\t\t: write_object_file(result_buf.ptr,\n> +\t\t\t\t\t\t result_buf.size,\n> +\t\t\t\t\t\t OBJ_BLOB, &result->oid);\n> +\t\t\tif (ret)\n> +\t\t\t\tret = error(_(\"unable to add %s to database\"),\n> +\t\t\t\t\t path);\n> +\t\t}\n> \n> \t\tfree(result_buf.ptr);\n> \t\tif (ret)\n> @@ -3597,7 +3607,8 @@ static int tree_entry_order(const void *a_, const void *b_)\n> \t\t\t\t b->string, strlen(b->string), bmi->result.mode);\n> }\n> \n> -static int write_tree(struct object_id *result_oid,\n> +static int write_tree(struct merge_options *opt,\n> +\t\t struct object_id *result_oid,\n> \t\t struct string_list *versions,\n> \t\t unsigned int offset,\n> \t\t size_t hash_size)\n> @@ -3631,8 +3642,14 @@ static int write_tree(struct object_id *result_oid,\n> \t}\n> \n> \t/* Write this object file out, and record in result_oid */\n> -\tif (write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid))\n> +\tret = opt->write_pack\n> +\t\t? index_tree_bulk_checkin_incore(result_oid,\n> +\t\t\t\t\t\t buf.buf, buf.len, \"\", 1)\n> +\t\t: write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);\n> +\n> +\tif (ret)\n> \t\tret = -1;\n> +\n> \tstrbuf_release(&buf);\n> \treturn ret;\n> }\n> @@ -3797,8 +3814,8 @@ static int write_completed_directory(struct merge_options *opt,\n> \t\t */\n> \t\tdir_info->is_null = 0;\n> \t\tdir_info->result.mode = S_IFDIR;\n> -\t\tif (write_tree(&dir_info->result.oid, &info->versions, offset,\n> -\t\t\t opt->repo->hash_algo->rawsz) < 0)\n> +\t\tif (write_tree(opt, &dir_info->result.oid, &info->versions,\n> +\t\t\t offset, opt->repo->hash_algo->rawsz) < 0)\n> \t\t\tret = -1;\n> \t}\n> \n> @@ -4332,9 +4349,13 @@ static int process_entries(struct merge_options *opt,\n> \t\tfflush(stdout);\n> \t\tBUG(\"dir_metadata accounting completely off; shouldn't happen\");\n> \t}\n> -\tif (write_tree(result_oid, &dir_metadata.versions, 0,\n> +\tif (write_tree(opt, result_oid, &dir_metadata.versions, 0,\n> \t\t opt->repo->hash_algo->rawsz) < 0)\n> \t\tret = -1;\n> +\n> +\tif (opt->write_pack)\n> +\t\tend_odb_transaction();\n> +\n> cleanup:\n> \tstring_list_clear(&plist, 0);\n> \tstring_list_clear(&dir_metadata.versions, 0);\n> @@ -4878,6 +4899,9 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)\n> \t */\n> \tstrmap_init(&opt->priv->conflicts);\n> \n> +\tif (opt->write_pack)\n> +\t\tbegin_odb_transaction();\n> +\n> \ttrace2_region_leave(\"merge\", \"allocate/init\", opt->repo);\n> }\n> \n> diff --git a/merge-recursive.h b/merge-recursive.h\n> index 3d3b3e3c29..5c5ff380a8 100644\n> --- a/merge-recursive.h\n> +++ b/merge-recursive.h\n> @@ -48,6 +48,7 @@ struct merge_options {\n> \tunsigned renormalize : 1;\n> \tunsigned record_conflict_msgs_as_headers : 1;\n> \tconst char *msg_header_prefix;\n> +\tunsigned write_pack : 1;\n> \n> \t/* internal fields used by the implementation */\n> \tstruct merge_options_internal *priv;\n> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh\n> index b2c8a43fce..d2a8634523 100755\n> --- a/t/t4301-merge-tree-write-tree.sh\n> +++ b/t/t4301-merge-tree-write-tree.sh\n> @@ -945,4 +945,97 @@ test_expect_success 'check the input format when --stdin is passed' '\n> \ttest_cmp expect actual\n> '\n> \n> +packdir=\".git/objects/pack\"\n> +\n> +test_expect_success 'merge-tree can pack its result with --write-pack' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\n> +\t# base has lines [3, 4, 5]\n> +\t# - side adds to the beginning, resulting in [1, 2, 3, 4, 5]\n> +\t# - other adds to the end, resulting in [3, 4, 5, 6, 7]\n> +\t#\n> +\t# merging the two should result in a new blob object containing\n> +\t# [1, 2, 3, 4, 5, 6, 7], along with a new tree.\n> +\ttest_commit -C repo base file \"$(test_seq 3 5)\" &&\n> +\tgit -C repo branch -M main &&\n> +\tgit -C repo checkout -b side main &&\n> +\ttest_commit -C repo side file \"$(test_seq 1 5)\" &&\n> +\tgit -C repo checkout -b other main &&\n> +\ttest_commit -C repo other file \"$(test_seq 3 7)\" &&\n> +\n> +\tfind repo/$packdir -type f -name \"pack-*.idx\" >packs.before &&\n> +\ttree=\"$(git -C repo merge-tree --write-pack \\\n> +\t\trefs/tags/side refs/tags/other)\" &&\n> +\tblob=\"$(git -C repo rev-parse $tree:file)\" &&\n> +\tfind repo/$packdir -type f -name \"pack-*.idx\" >packs.after &&\n\nWhile we do assert that we write a new packfile, we don't assert whether\nparts of the written object may have been written as loose objects. Do\nwe want to tighten the checks to verify that?\n\nPatrick\n\n> +\ttest_must_be_empty packs.before &&\n> +\ttest_line_count = 1 packs.after &&\n> +\n> +\tgit show-index <$(cat packs.after) >objects &&\n> +\ttest_line_count = 2 objects &&\n> +\tgrep \"^[1-9][0-9]* $tree\" objects &&\n> +\tgrep \"^[1-9][0-9]* $blob\" objects\n> +'\n> +\n> +test_expect_success 'merge-tree can write multiple packs with --write-pack' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\n> +\t\tgit config pack.packSizeLimit 512 &&\n> +\n> +\t\ttest_seq 512 >f &&\n> +\n> +\t\t# \"f\" contains roughly ~2,000 bytes.\n> +\t\t#\n> +\t\t# Each side (\"foo\" and \"bar\") adds a small amount of data at the\n> +\t\t# beginning and end of \"base\", respectively.\n> +\t\tgit add f &&\n> +\t\ttest_tick &&\n> +\t\tgit commit -m base &&\n> +\t\tgit branch -M main &&\n> +\n> +\t\tgit checkout -b foo main &&\n> +\t\t{\n> +\t\t\techo foo && cat f\n> +\t\t} >f.tmp &&\n> +\t\tmv f.tmp f &&\n> +\t\tgit add f &&\n> +\t\ttest_tick &&\n> +\t\tgit commit -m foo &&\n> +\n> +\t\tgit checkout -b bar main &&\n> +\t\techo bar >>f &&\n> +\t\tgit add f &&\n> +\t\ttest_tick &&\n> +\t\tgit commit -m bar &&\n> +\n> +\t\tfind $packdir -type f -name \"pack-*.idx\" >packs.before &&\n> +\t\t# Merging either side should result in a new object which is\n> +\t\t# larger than 1M, thus the result should be split into two\n> +\t\t# separate packs.\n> +\t\ttree=\"$(git merge-tree --write-pack \\\n> +\t\t\trefs/heads/foo refs/heads/bar)\" &&\n> +\t\tblob=\"$(git rev-parse $tree:f)\" &&\n> +\t\tfind $packdir -type f -name \"pack-*.idx\" >packs.after &&\n> +\n> +\t\ttest_must_be_empty packs.before &&\n> +\t\ttest_line_count = 2 packs.after &&\n> +\t\tfor idx in $(cat packs.after)\n> +\t\tdo\n> +\t\t\tgit show-index <$idx || return 1\n> +\t\tdone >objects &&\n> +\n> +\t\t# The resulting set of packs should contain one copy of both\n> +\t\t# objects, each in a separate pack.\n> +\t\ttest_line_count = 2 objects &&\n> +\t\tgrep \"^[1-9][0-9]* $tree\" objects &&\n> +\t\tgrep \"^[1-9][0-9]* $blob\" objects\n> +\n> +\t)\n> +'\n> +\n> test_done\n> -- \n> 2.42.0.425.g963d08ddb3.dirty\n"},{"id":"483840","messageId":"ZTjKmcV5c_EFuoGo@tanuki","threadId":"60403","inReplyTo":"cover.1698101088.git.me@ttaylorr.com","subject":"Re: [PATCH v5 0/5] merge-ort: implement support for packing objects together","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-25T07:58:17Z","receivedAt":"2023-10-25T07:58:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Oct 23, 2023 at 06:44:49PM -0400, Taylor Blau wrote:\n> (Rebased onto the current tip of 'master', which is ceadf0f3cf (The\n> twentieth batch, 2023-10-20)).\n> \n> This series implements support for a new merge-tree option,\n> `--write-pack`, which causes any newly-written objects to be packed\n> together instead of being stored individually as loose.\n> \n> This is a minor follow-up that could be taken instead of v4 (though the\n> changes between these two most recent rounds are stylistic and a matter\n> of subjective opinion).\n> \n> This moves us the bulk_checkin_source structure introduced in response\n> to Junio's suggestion during the last round further in the OOP\n> direction. Instead of switching on the enum type of the source, have\n> function pointers for read() and seek() respectively.\n> \n> The functionality at the end is the same, but this avoids some of the\n> namespacing issues that Peff pointed out while looking at v4. But I\n> think that this approach ended up being less heavy-weight than I had\n> originally imagined, so I think that this version is a worthwhile\n> improvement over v4.\n> \n> Beyond that, the changes since last time can be viewed in the range-diff\n> below. Thanks in advance for any review!\n\nOverall this version looks good to me. I've only got two smallish nits\nand one question regarding the tests.\n\nThanks!\n\nPatrick\n\n> [1]: https://lore.kernel.org/git/xmqq34y7plj4.fsf@gitster.g/\n> \n> Taylor Blau (5):\n> bulk-checkin: extract abstract `bulk_checkin_source`\n> bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types\n> bulk-checkin: introduce `index_blob_bulk_checkin_incore()`\n> bulk-checkin: introduce `index_tree_bulk_checkin_incore()`\n> builtin/merge-tree.c: implement support for `--write-pack`\n> \n> Documentation/git-merge-tree.txt | 4 +\n> builtin/merge-tree.c | 5 +\n> bulk-checkin.c | 197 +++++++++++++++++++++++++++----\n> bulk-checkin.h | 8 ++\n> merge-ort.c | 42 +++++--\n> merge-recursive.h | 1 +\n> t/t4301-merge-tree-write-tree.sh | 93 +++++++++++++++\n> 7 files changed, 316 insertions(+), 34 deletions(-)\n> \n> Range-diff against v4:\n> 1: 97bb6e9f59 ! 1: 696aa027e4 bulk-checkin: extract abstract `bulk_checkin_source`\n> @@ bulk-checkin.c: static int already_written(struct bulk_checkin_packfile *state,\n> }\n> \n> +struct bulk_checkin_source {\n> -+\tenum { SOURCE_FILE } type;\n> ++\toff_t (*read)(struct bulk_checkin_source *, void *, size_t);\n> ++\toff_t (*seek)(struct bulk_checkin_source *, off_t);\n> +\n> -+\t/* SOURCE_FILE fields */\n> -+\tint fd;\n> ++\tunion {\n> ++\t\tstruct {\n> ++\t\t\tint fd;\n> ++\t\t} from_fd;\n> ++\t} data;\n> +\n> -+\t/* common fields */\n> +\tsize_t size;\n> +\tconst char *path;\n> +};\n> +\n> -+static off_t bulk_checkin_source_seek_to(struct bulk_checkin_source *source,\n> -+\t\t\t\t\t off_t offset)\n> ++static off_t bulk_checkin_source_read_from_fd(struct bulk_checkin_source *source,\n> ++\t\t\t\t\t void *buf, size_t nr)\n> +{\n> -+\tswitch (source->type) {\n> -+\tcase SOURCE_FILE:\n> -+\t\treturn lseek(source->fd, offset, SEEK_SET);\n> -+\tdefault:\n> -+\t\tBUG(\"unknown bulk-checkin source: %d\", source->type);\n> -+\t}\n> ++\treturn read_in_full(source->data.from_fd.fd, buf, nr);\n> +}\n> +\n> -+static ssize_t bulk_checkin_source_read(struct bulk_checkin_source *source,\n> -+\t\t\t\t\tvoid *buf, size_t nr)\n> ++static off_t bulk_checkin_source_seek_from_fd(struct bulk_checkin_source *source,\n> ++\t\t\t\t\t off_t offset)\n> +{\n> -+\tswitch (source->type) {\n> -+\tcase SOURCE_FILE:\n> -+\t\treturn read_in_full(source->fd, buf, nr);\n> -+\tdefault:\n> -+\t\tBUG(\"unknown bulk-checkin source: %d\", source->type);\n> -+\t}\n> ++\treturn lseek(source->data.from_fd.fd, offset, SEEK_SET);\n> ++}\n> ++\n> ++static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,\n> ++\t\t\t\t\t int fd, size_t size,\n> ++\t\t\t\t\t const char *path)\n> ++{\n> ++\tmemset(source, 0, sizeof(struct bulk_checkin_source));\n> ++\n> ++\tsource->read = bulk_checkin_source_read_from_fd;\n> ++\tsource->seek = bulk_checkin_source_seek_from_fd;\n> ++\n> ++\tsource->data.from_fd.fd = fd;\n> ++\n> ++\tsource->size = size;\n> ++\tsource->path = path;\n> +}\n> +\n> /*\n> @@ bulk-checkin.c: static int stream_blob_to_pack(struct bulk_checkin_packfile *sta\n> -\t\t\tssize_t read_result = read_in_full(fd, ibuf, rsize);\n> +\t\t\tssize_t read_result;\n> +\n> -+\t\t\tread_result = bulk_checkin_source_read(source, ibuf,\n> -+\t\t\t\t\t\t\t rsize);\n> ++\t\t\tread_result = source->read(source, ibuf, rsize);\n> \t\t\tif (read_result < 0)\n> -\t\t\t\tdie_errno(\"failed to read from '%s'\", path);\n> +\t\t\t\tdie_errno(\"failed to read from '%s'\",\n> @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st\n> \tunsigned header_len;\n> \tstruct hashfile_checkpoint checkpoint = {0};\n> \tstruct pack_idx_entry *idx = NULL;\n> -+\tstruct bulk_checkin_source source = {\n> -+\t\t.type = SOURCE_FILE,\n> -+\t\t.fd = fd,\n> -+\t\t.size = size,\n> -+\t\t.path = path,\n> -+\t};\n> ++\tstruct bulk_checkin_source source;\n> ++\n> ++\tinit_bulk_checkin_source_from_fd(&source, fd, size, path);\n> \n> \tseekback = lseek(fd, 0, SEEK_CUR);\n> \tif (seekback == (off_t) -1)\n> @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st\n> \t\tstate->offset = checkpoint.offset;\n> \t\tflush_bulk_checkin_packfile(state);\n> -\t\tif (lseek(fd, seekback, SEEK_SET) == (off_t) -1)\n> -+\t\tif (bulk_checkin_source_seek_to(&source, seekback) == (off_t)-1)\n> ++\t\tif (source.seek(&source, seekback) == (off_t)-1)\n> \t\t\treturn error(\"cannot seek back\");\n> \t}\n> \tthe_hash_algo->final_oid_fn(result_oid, &ctx);\n> 2: 9d633df339 < -: ---------- bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types\n> 3: d5bbd7810e ! 2: 596bd028a7 bulk-checkin: refactor deflate routine to accept a `bulk_checkin_source`\n> @@ Metadata\n> Author: Taylor Blau \n> \n> ## Commit message ##\n> - bulk-checkin: refactor deflate routine to accept a `bulk_checkin_source`\n> + bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types\n> \n> - Prepare for a future change where we will want to use a routine very\n> - similar to the existing `deflate_blob_to_pack()` but over arbitrary\n> - sources (i.e. either open file-descriptors, or a location in memory).\n> + The existing `stream_blob_to_pack()` function is named based on the fact\n> + that it knows only how to stream blobs into a bulk-checkin pack.\n> \n> - Extract out a common \"deflate_obj_to_pack()\" routine that acts on a\n> - bulk_checkin_source, instead of a (int, size_t) pair. Then rewrite\n> - `deflate_blob_to_pack()` in terms of it.\n> + But there is no longer anything in this function which prevents us from\n> + writing objects of arbitrary types to the bulk-checkin pack. Prepare to\n> + write OBJ_TREEs by removing this assumption, adding an `enum\n> + object_type` parameter to this function's argument list, and renaming it\n> + to `stream_obj_to_pack()` accordingly.\n> \n> Signed-off-by: Taylor Blau \n> \n> ## bulk-checkin.c ##\n> +@@ bulk-checkin.c: static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,\n> + * status before calling us just in case we ask it to call us again\n> + * with a new pack.\n> + */\n> +-static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n> +-\t\t\t git_hash_ctx *ctx, off_t *already_hashed_to,\n> +-\t\t\t struct bulk_checkin_source *source,\n> +-\t\t\t unsigned flags)\n> ++static int stream_obj_to_pack(struct bulk_checkin_packfile *state,\n> ++\t\t\t git_hash_ctx *ctx, off_t *already_hashed_to,\n> ++\t\t\t struct bulk_checkin_source *source,\n> ++\t\t\t enum object_type type, unsigned flags)\n> + {\n> + \tgit_zstream s;\n> + \tunsigned char ibuf[16384];\n> +@@ bulk-checkin.c: static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n> + \n> + \tgit_deflate_init(&s, pack_compression_level);\n> + \n> +-\thdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB,\n> +-\t\t\t\t\t size);\n> ++\thdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), type, size);\n> + \ts.next_out = obuf + hdrlen;\n> + \ts.avail_out = sizeof(obuf) - hdrlen;\n> + \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> @@ bulk-checkin.c: static void prepare_to_stream(struct bulk_checkin_packfile *stat\n> \tunsigned header_len;\n> \tstruct hashfile_checkpoint checkpoint = {0};\n> \tstruct pack_idx_entry *idx = NULL;\n> --\tstruct bulk_checkin_source source = {\n> --\t\t.type = SOURCE_FILE,\n> --\t\t.fd = fd,\n> --\t\t.size = size,\n> --\t\t.path = path,\n> --\t};\n> +-\tstruct bulk_checkin_source source;\n> \n> +-\tinit_bulk_checkin_source_from_fd(&source, fd, size, path);\n> +-\n> -\tseekback = lseek(fd, 0, SEEK_CUR);\n> -\tif (seekback == (off_t) -1)\n> -\t\treturn error(\"cannot find the current offset\");\n> @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st\n> \t\tprepare_to_stream(state, flags);\n> \t\tif (idx) {\n> @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n> + \t\t\tidx->offset = state->offset;\n> \t\t\tcrc32_begin(state->f);\n> \t\t}\n> - \t\tif (!stream_obj_to_pack(state, &ctx, &already_hashed_to,\n> --\t\t\t\t\t&source, OBJ_BLOB, flags))\n> +-\t\tif (!stream_blob_to_pack(state, &ctx, &already_hashed_to,\n> +-\t\t\t\t\t &source, flags))\n> ++\t\tif (!stream_obj_to_pack(state, &ctx, &already_hashed_to,\n> +\t\t\t\t\tsource, type, flags))\n> \t\t\tbreak;\n> \t\t/*\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\tif (bulk_checkin_source_seek_to(&source, seekback) == (off_t)-1)\n> -+\t\tif (bulk_checkin_source_seek_to(source, seekback) == (off_t)-1)\n> +-\t\tif (source.seek(&source, seekback) == (off_t)-1)\n> ++\t\tif (source->seek(source, seekback) == (off_t)-1)\n> \t\t\treturn error(\"cannot seek back\");\n> \t}\n> \tthe_hash_algo->final_oid_fn(result_oid, &ctx);\n> @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st\n> +\t\t\t\tint fd, size_t size,\n> +\t\t\t\tconst char *path, unsigned flags)\n> +{\n> -+\tstruct bulk_checkin_source source = {\n> -+\t\t.type = SOURCE_FILE,\n> -+\t\t.fd = fd,\n> -+\t\t.size = size,\n> -+\t\t.path = path,\n> -+\t};\n> -+\toff_t seekback = lseek(fd, 0, SEEK_CUR);\n> ++\tstruct bulk_checkin_source source;\n> ++\toff_t seekback;\n> ++\n> ++\tinit_bulk_checkin_source_from_fd(&source, fd, size, path);\n> ++\n> ++\tseekback = lseek(fd, 0, SEEK_CUR);\n> +\tif (seekback == (off_t) -1)\n> +\t\treturn error(\"cannot find the current offset\");\n> +\n> 4: e427fe6ad3 < -: ---------- bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`\n> 5: 48095afe80 ! 3: d8cf8e4395 bulk-checkin: introduce `index_blob_bulk_checkin_incore()`\n> @@ Metadata\n> ## Commit message ##\n> bulk-checkin: introduce `index_blob_bulk_checkin_incore()`\n> \n> - Now that we have factored out many of the common routines necessary to\n> - index a new object into a pack created by the bulk-checkin machinery, we\n> - can introduce a variant of `index_blob_bulk_checkin()` that acts on\n> - blobs whose contents we can fit in memory.\n> + Introduce `index_blob_bulk_checkin_incore()` which allows streaming\n> + arbitrary blob contents from memory into the bulk-checkin pack.\n> +\n> + In order to support streaming from a location in memory, we must\n> + implement a new kind of bulk_checkin_source that does just that. These\n> + implementation in spread out across:\n> +\n> + - init_bulk_checkin_source_incore()\n> + - bulk_checkin_source_read_incore()\n> + - bulk_checkin_source_seek_incore()\n> +\n> + Note that, unlike file descriptors, which manage their own offset\n> + internally, we have to keep track of how many bytes we've read out of\n> + the buffer, and make sure we don't read past the end of the buffer.\n> \n> This will be useful in a couple of more commits in order to provide the\n> `merge-tree` builtin with a mechanism to create a new pack containing\n> @@ Commit message\n> Signed-off-by: Taylor Blau \n> \n> ## bulk-checkin.c ##\n> +@@ bulk-checkin.c: struct bulk_checkin_source {\n> + \t\tstruct {\n> + \t\t\tint fd;\n> + \t\t} from_fd;\n> ++\t\tstruct {\n> ++\t\t\tconst void *buf;\n> ++\t\t\tsize_t nr_read;\n> ++\t\t} incore;\n> + \t} data;\n> + \n> + \tsize_t size;\n> +@@ bulk-checkin.c: static off_t bulk_checkin_source_seek_from_fd(struct bulk_checkin_source *source\n> + \treturn lseek(source->data.from_fd.fd, offset, SEEK_SET);\n> + }\n> + \n> ++static off_t bulk_checkin_source_read_incore(struct bulk_checkin_source *source,\n> ++\t\t\t\t\t void *buf, size_t nr)\n> ++{\n> ++\tconst unsigned char *src = source->data.incore.buf;\n> ++\n> ++\tif (source->data.incore.nr_read > source->size)\n> ++\t\tBUG(\"read beyond bulk-checkin source buffer end \"\n> ++\t\t \"(%\"PRIuMAX\" > %\"PRIuMAX\")\",\n> ++\t\t (uintmax_t)source->data.incore.nr_read,\n> ++\t\t (uintmax_t)source->size);\n> ++\n> ++\tif (nr > source->size - source->data.incore.nr_read)\n> ++\t\tnr = source->size - source->data.incore.nr_read;\n> ++\n> ++\tsrc += source->data.incore.nr_read;\n> ++\n> ++\tmemcpy(buf, src, nr);\n> ++\tsource->data.incore.nr_read += nr;\n> ++\treturn nr;\n> ++}\n> ++\n> ++static off_t bulk_checkin_source_seek_incore(struct bulk_checkin_source *source,\n> ++\t\t\t\t\t off_t offset)\n> ++{\n> ++\tif (!(0 <= offset && offset < source->size))\n> ++\t\treturn (off_t)-1;\n> ++\tsource->data.incore.nr_read = offset;\n> ++\treturn source->data.incore.nr_read;\n> ++}\n> ++\n> + static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,\n> + \t\t\t\t\t int fd, size_t size,\n> + \t\t\t\t\t const char *path)\n> +@@ bulk-checkin.c: static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,\n> + \tsource->path = path;\n> + }\n> + \n> ++static void init_bulk_checkin_source_incore(struct bulk_checkin_source *source,\n> ++\t\t\t\t\t const void *buf, size_t size,\n> ++\t\t\t\t\t const char *path)\n> ++{\n> ++\tmemset(source, 0, sizeof(struct bulk_checkin_source));\n> ++\n> ++\tsource->read = bulk_checkin_source_read_incore;\n> ++\tsource->seek = bulk_checkin_source_seek_incore;\n> ++\n> ++\tsource->data.incore.buf = buf;\n> ++\tsource->data.incore.nr_read = 0;\n> ++\n> ++\tsource->size = size;\n> ++\tsource->path = path;\n> ++}\n> ++\n> + /*\n> + * Read the contents from 'source' for 'size' bytes, streaming it to the\n> + * packfile in state while updating the hash in ctx. Signal a failure\n> @@ bulk-checkin.c: static int deflate_obj_to_pack(struct bulk_checkin_packfile *state,\n> \treturn 0;\n> }\n> @@ bulk-checkin.c: static int deflate_obj_to_pack(struct bulk_checkin_packfile *sta\n> +\t\t\t\t const char *path, enum object_type type,\n> +\t\t\t\t unsigned flags)\n> +{\n> -+\tstruct bulk_checkin_source source = {\n> -+\t\t.type = SOURCE_INCORE,\n> -+\t\t.buf = buf,\n> -+\t\t.size = size,\n> -+\t\t.read = 0,\n> -+\t\t.path = path,\n> -+\t};\n> ++\tstruct bulk_checkin_source source;\n> ++\n> ++\tinit_bulk_checkin_source_incore(&source, buf, size, path);\n> +\n> +\treturn deflate_obj_to_pack(state, result_oid, &source, type, 0, flags);\n> +}\n> 6: 60568f9281 = 4: 2670192802 bulk-checkin: introduce `index_tree_bulk_checkin_incore()`\n> 7: b9be9df122 ! 5: 3595db76a5 builtin/merge-tree.c: implement support for `--write-pack`\n> @@ Documentation/git-merge-tree.txt: OPTIONS\n> \n> ## builtin/merge-tree.c ##\n> @@\n> - #include \"quote.h\"\n> #include \"tree.h\"\n> #include \"config.h\"\n> + #include \"strvec.h\"\n> +#include \"bulk-checkin.h\"\n> \n> static int line_termination = '\\n';\n> \n> @@ builtin/merge-tree.c: struct merge_tree_options {\n> - \tint show_messages;\n> \tint name_only;\n> \tint use_stdin;\n> + \tstruct merge_options merge_options;\n> +\tint write_pack;\n> };\n> \n> static int real_merge(struct merge_tree_options *o,\n> @@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,\n> - \tinit_merge_options(&opt, the_repository);\n> + \t\t\t\t _(\"not something we can merge\"));\n> \n> \topt.show_rename_progress = 0;\n> +\topt.write_pack = o->write_pack;\n> @@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,\n> \topt.branch1 = branch1;\n> \topt.branch2 = branch2;\n> @@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char *prefix)\n> - \t\t\t &merge_base,\n> - \t\t\t N_(\"commit\"),\n> \t\t\t N_(\"specify a merge-base for the merge\")),\n> + \t\tOPT_STRVEC('X', \"strategy-option\", &xopts, N_(\"option=value\"),\n> + \t\t\tN_(\"option for selected merge strategy\")),\n> +\t\tOPT_BOOL(0, \"write-pack\", &o.write_pack,\n> +\t\t\t N_(\"write new objects to a pack instead of as loose\")),\n> \t\tOPT_END()\n> \n> base-commit: ceadf0f3cf51550166a387ec8508bb55e7883057\n> -- \n> 2.42.0.425.g963d08ddb3.dirty\n"},{"id":"483857","messageId":"ZTk2w6rV2y7ZKfQh@nand.local","threadId":"60403","inReplyTo":"20231025073736.GB2145145@coredump.intra.peff.net","subject":"Re: [PATCH v5 1/5] bulk-checkin: extract abstract `bulk_checkin_source`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-25T15:39:47Z","receivedAt":"2023-10-25T15:39:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 25, 2023 at 03:37:36AM -0400, Jeff King wrote:\n> I don't mind this in-between state. It is a funny layering violating\n> from an OO standpoint, but it's not like we expect an unbounded set of\n> concrete types to \"inherit\" from the source struct.\n\nYeah, this was exactly my thinking when writing up the changes for this\nround. Since all of the \"sub-classes\" are local to the bulk-checkin.o\ncompilation unit, I don't have grave concerns about one implementation\npeering into the details of another's.\n\nGotta stop somewhere ;-).\n\nThanks,\nTaylor\n"},{"id":"483858","messageId":"ZTk3zoncT6nvV3aQ@nand.local","threadId":"60403","inReplyTo":"ZTjKjkRMkmCuxDU1@tanuki","subject":"Re: [PATCH v5 3/5] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-25T15:44:14Z","receivedAt":"2023-10-25T15:44:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 25, 2023 at 09:58:06AM +0200, Patrick Steinhardt wrote:\n> On Mon, Oct 23, 2023 at 06:45:01PM -0400, Taylor Blau wrote:\n> > Introduce `index_blob_bulk_checkin_incore()` which allows streaming\n> > arbitrary blob contents from memory into the bulk-checkin pack.\n> >\n> > In order to support streaming from a location in memory, we must\n> > implement a new kind of bulk_checkin_source that does just that. These\n> > implementation in spread out across:\n>\n> Nit: the commit message is a bit off here. Probably not worth a reroll\n> though.\n\nYour eyes are definitely mine, because I'm not seeing where the commit\nmessage is off! But hopefully since you already don't think it's worth a\nreroll, and I'm not even sure what the issue is that we can just leave\nit ;-).\n\n> > +static off_t bulk_checkin_source_seek_incore(struct bulk_checkin_source *source,\n> > +\t\t\t\t\t off_t offset)\n> > +{\n> > +\tif (!(0 <= offset && offset < source->size))\n> > +\t\treturn (off_t)-1;\n>\n> At the risk of showing my own ignorance, but why is the cast here\n> necessary?\n\nIt's not necessary, see this godbolt example that shows two functions\ndoing the same thing (one with the explicit cast, one without):\n\n https://godbolt.org/z/f737EcGfG\n\nBut there are a couple of other spots within the bulk-checkin code\n(specifically within the deflate_blob_to_pack() routine) that explicitly\ncast -1 to an off_t, so I was more trying to imitate the local style.\n\nThanks,\nTaylor\n"},{"id":"483859","messageId":"ZTk4Oz0ETcEMSgan@nand.local","threadId":"60403","inReplyTo":"ZTjKk8E55M7lQN1m@tanuki","subject":"Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-25T15:46:03Z","receivedAt":"2023-10-25T15:46:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 25, 2023 at 09:58:11AM +0200, Patrick Steinhardt wrote:\n> > +test_expect_success 'merge-tree can pack its result with --write-pack' '\n> > +\ttest_when_finished \"rm -rf repo\" &&\n> > +\tgit init repo &&\n> > +\n> > +\t# base has lines [3, 4, 5]\n> > +\t# - side adds to the beginning, resulting in [1, 2, 3, 4, 5]\n> > +\t# - other adds to the end, resulting in [3, 4, 5, 6, 7]\n> > +\t#\n> > +\t# merging the two should result in a new blob object containing\n> > +\t# [1, 2, 3, 4, 5, 6, 7], along with a new tree.\n> > +\ttest_commit -C repo base file \"$(test_seq 3 5)\" &&\n> > +\tgit -C repo branch -M main &&\n> > +\tgit -C repo checkout -b side main &&\n> > +\ttest_commit -C repo side file \"$(test_seq 1 5)\" &&\n> > +\tgit -C repo checkout -b other main &&\n> > +\ttest_commit -C repo other file \"$(test_seq 3 7)\" &&\n> > +\n> > +\tfind repo/$packdir -type f -name \"pack-*.idx\" >packs.before &&\n> > +\ttree=\"$(git -C repo merge-tree --write-pack \\\n> > +\t\trefs/tags/side refs/tags/other)\" &&\n> > +\tblob=\"$(git -C repo rev-parse $tree:file)\" &&\n> > +\tfind repo/$packdir -type f -name \"pack-*.idx\" >packs.after &&\n>\n> While we do assert that we write a new packfile, we don't assert whether\n> parts of the written object may have been written as loose objects. Do\n> we want to tighten the checks to verify that?\n\nWe could, but the tests in t1050 already verify this, so I'm not sure\nthat we would be significantly hardening our test coverage here. If you\nfeel strongly about it, though, I'm happy to change it up.\n\nThanks,\nTaylor\n"},{"id":"483861","messageId":"CAPig+cTjQe6FWo98LxvDS=s3dOs33SUUJa=x-bkyWHNBMx+XFw@mail.gmail.com","threadId":"60403","inReplyTo":"ZTk3zoncT6nvV3aQ@nand.local","subject":"Re: [PATCH v5 3/5] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-10-25T17:21:50Z","receivedAt":"2023-10-25T17:22:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Oct 25, 2023 at 11:44 AM Taylor Blau wrote:\n> On Wed, Oct 25, 2023 at 09:58:06AM +0200, Patrick Steinhardt wrote:\n> > On Mon, Oct 23, 2023 at 06:45:01PM -0400, Taylor Blau wrote:\n> > > In order to support streaming from a location in memory, we must\n> > > implement a new kind of bulk_checkin_source that does just that. These\n> > > implementation in spread out across:\n> >\n> > Nit: the commit message is a bit off here. Probably not worth a reroll\n> > though.\n>\n> Your eyes are definitely mine, because I'm not seeing where the commit\n> message is off! But hopefully since you already don't think it's worth a\n> reroll, and I'm not even sure what the issue is that we can just leave\n> it ;-).\n\nPerhaps:\n\n s/implementation in/implementations are/\n"},{"id":"483889","messageId":"ZTogPThYhQbSQ-NI@tanuki","threadId":"60403","inReplyTo":"CAPig+cTjQe6FWo98LxvDS=s3dOs33SUUJa=x-bkyWHNBMx+XFw@mail.gmail.com","subject":"Re: [PATCH v5 3/5] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-26T08:16:07Z","receivedAt":"2023-10-26T08:16:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Oct 25, 2023 at 01:21:50PM -0400, Eric Sunshine wrote:\n> On Wed, Oct 25, 2023 at 11:44 AM Taylor Blau wrote:\n> > On Wed, Oct 25, 2023 at 09:58:06AM +0200, Patrick Steinhardt wrote:\n> > > On Mon, Oct 23, 2023 at 06:45:01PM -0400, Taylor Blau wrote:\n> > > > In order to support streaming from a location in memory, we must\n> > > > implement a new kind of bulk_checkin_source that does just that. These\n> > > > implementation in spread out across:\n> > >\n> > > Nit: the commit message is a bit off here. Probably not worth a reroll\n> > > though.\n> >\n> > Your eyes are definitely mine, because I'm not seeing where the commit\n> > message is off! But hopefully since you already don't think it's worth a\n> > reroll, and I'm not even sure what the issue is that we can just leave\n> > it ;-).\n> \n> Perhaps:\n> \n> s/implementation in/implementations are/\n\nYeah, that's what I referred to. Sorry for not pointing it out\nexplicitly :)\n\nIn any case, as stated before: I don't think any of my comments warrant\na reroll, and overall this patch series looks good to me.\n\nPatrick\n"},{"id":"484007","messageId":"xmqqpm0zoe23.fsf@gitster.g","threadId":"60403","inReplyTo":"20231025073736.GB2145145@coredump.intra.peff.net","subject":"Re: [PATCH v5 1/5] bulk-checkin: extract abstract `bulk_checkin_source`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-27T23:12:20Z","receivedAt":"2023-10-27T23:12:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Alternatively, I think some of our other OO code just leaves room for\n> a type-specific void pointer, like:\n>\n> struct bulk_checkin_source {\n> \t...the usual stuff...\n>\n> \tvoid *magic_type_data;\n> };\n>\n> and then the init_bulk_checkin_source_from_fd() function allocates its\n> own heap struct for the magic_type_data field and sticks the int in\n> there.\n\nYup. All the pros-and-cons makes sense. I earlier said I found\nthis a good place to stop, exactly because the full OO with \n\n struct specific_subclass {\n\tstruct vtbl *vtbl;\n\tstruct base_class common_data_specific_to_instance;\n\t... other instance specific data members here ...;\n }\n\nwould require us to add too many supporting infrastructure struct\ntypes; with only a few subclasses in use, it is a bit too much to\njustify.\n"},{"id":"484463","messageId":"0ac32374-7d52-8f0c-8583-110de678291e@gmx.de","threadId":"60403","inReplyTo":"xmqqo7go7w63.fsf@gitster.g","subject":"Re: [PATCH v5 0/5] merge-ort: implement support for packing objects together","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2023-11-06T15:46:32Z","receivedAt":"2023-11-06T15:46:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 23 Oct 2023, Junio C Hamano wrote:\n\n> Taylor Blau writes:\n>\n> > But I think that this approach ended up being less heavy-weight than I\n> > had originally imagined, so I think that this version is a worthwhile\n> > improvement over v4.\n>\n> ;-).\n>\n> This version is a good place to stop, a bit short of going full OO.\n> Nicely done.\n\nI wonder whether a more generic approach would be more desirable, an\napproach that would work for `git replay`, too, for example (where\nstreaming objects does not work because they need to be made available\nimmediately because subsequent `merge_incore_nonrecursive()` might expect\nthe created objects to be present)?\n\nWhat I have in mind is more along Elijah's suggestion at the Contributor\nSummit to use the `tmp_objdir*()` machinery. But instead of discarding the\ntemporary object database, the contained objects would be repacked and the\n`.pack`, (maybe `.rev`) and the `.idx` file would then be moved (in that\norder) before discarding the temporary object database.\n\nThis would probably need to be implemented as a new\n`tmp_objdir_pack_and_migrate()` function that basically spawns\n`pack-objects` and feeds it the list of generated objects, writing\ndirectly into the non-temporary object directory, then discarding the\n`tmp_objdir`.\n\nThis approach would not only more easily extend to other commands, but it\nwould also be less intrusive (a `tmp_objdir_*()` call to begin the\ntransaction before the objects are written, another one to commit the\ntransaction after the objects are written), and it would potentially allow\nfor more efficient packs to be generated.\n\nCiao,\nJohannes\n"},{"id":"484480","messageId":"xmqqr0l2cvx5.fsf@gitster.g","threadId":"60403","inReplyTo":"0ac32374-7d52-8f0c-8583-110de678291e@gmx.de","subject":"Re: [PATCH v5 0/5] merge-ort: implement support for packing objects together","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-06T23:19:02Z","receivedAt":"2023-11-06T23:19:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> What I have in mind is more along Elijah's suggestion at the Contributor\n> Summit to use the `tmp_objdir*()` machinery. But instead of discarding the\n> temporary object database, the contained objects would be repacked and the\n> `.pack`, (maybe `.rev`) and the `.idx` file would then be moved (in that\n> order) before discarding the temporary object database.\n\nThat may be more involved but does indeed sound like an approach\nmore generally applicable. Back when the bulk-checkin machinery was\ninvented, I envisioned that we would be adding annotations to\nvarious codepaths so that object creation machinery can say \"now we\nare plugged, in anticipation for creating many objects at once\" and\n\"now the flood of new object creation is done, time to wrap up\" for\nthat kind of optimization. \n\nThe callsites to {begin,end}_odb_transaction() functions haven't\ngrown beyond the original \"add\" and \"update-index\" (because the user\ncan add the entire working tree worth of files to the object\ndatabase), \"unpack-objects\" (because a fetch can bring in many\nobjects), and \"cache-tree\" (because a tree creation can cascade up\nto create many objects), but I agree \"merge\" and \"replay\" are prime\ncandidates to benefit from the optimization of the same kind (so is\n\"fast-import\"). They are about creating many objects at once, and\ngive us an opportunity for such an optimization.\n"},{"id":"484507","messageId":"20231107034224.GA874199@coredump.intra.peff.net","threadId":"60403","inReplyTo":"0ac32374-7d52-8f0c-8583-110de678291e@gmx.de","subject":"Re: [PATCH v5 0/5] merge-ort: implement support for packing objects together","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-07T03:42:24Z","receivedAt":"2023-11-07T03:42:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 06, 2023 at 04:46:32PM +0100, Johannes Schindelin wrote:\n\n> I wonder whether a more generic approach would be more desirable, an\n> approach that would work for `git replay`, too, for example (where\n> streaming objects does not work because they need to be made available\n> immediately because subsequent `merge_incore_nonrecursive()` might expect\n> the created objects to be present)?\n> \n> What I have in mind is more along Elijah's suggestion at the Contributor\n> Summit to use the `tmp_objdir*()` machinery. But instead of discarding the\n> temporary object database, the contained objects would be repacked and the\n> `.pack`, (maybe `.rev`) and the `.idx` file would then be moved (in that\n> order) before discarding the temporary object database.\n> \n> This would probably need to be implemented as a new\n> `tmp_objdir_pack_and_migrate()` function that basically spawns\n> `pack-objects` and feeds it the list of generated objects, writing\n> directly into the non-temporary object directory, then discarding the\n> `tmp_objdir`.\n\nPerhaps I'm missing some piece of the puzzle, but I'm not sure what\nyou're trying to accomplish with that approach.\n\nIf the goal is to increase performance by avoiding the loose object\nwrites, then we haven't really helped much. We're still writing them,\nand then writing them again for the repack.\n\nIf the goal is just to end up with a single nice pack for the long term,\nthen why do we need to use tmp_objdir at all? That point of that API is\nto avoid letting other simultaneous processes see the intermediate state\nbefore we're committed to keeping the objects around. That makes sense\nfor receiving a fetch or push, since we want to do some quality checks\non the objects before agreeing to keep them. But does it make sense for\na merge? Sure, in some workflows (like GitHub's test merges) we might\nend up throwing away the merge result if it's not clean. But there is no\nreal downside to other processes seeing those objects. They can be\ncleaned up at the next pruning repack.\n\nI guess if your scenario requirements include \"and we are never allowed\nto run a pruning repack\", then that could make sense. And I know that\nhas been a historical issue for GitHub. But I'm not sure it's\nnecessarily a good driver for an upstream feature.\n\nAs an alternative, though, I wonder if you need to have access to the\nobjects outside of the merge process. If not, then rather than an\nalternate object store, what if that single process wrote to a streaming\npack _and_ used its running in-core index of the objects to allow access\nvia the usual object-retrieval. Then you'd get a single, clean pack as\nthe outcome _and_ you'd get the performance boost over just \"write loose\nobjects, repack, and prune\".\n\n-Peff\n"},{"id":"484530","messageId":"ZUpepnSCSxL8i96b@nand.local","threadId":"60403","inReplyTo":"0ac32374-7d52-8f0c-8583-110de678291e@gmx.de","subject":"Re: [PATCH v5 0/5] merge-ort: implement support for packing objects together","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-07T15:58:30Z","receivedAt":"2023-11-07T15:58:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 06, 2023 at 04:46:32PM +0100, Johannes Schindelin wrote:\n> Hi,\n>\n> On Mon, 23 Oct 2023, Junio C Hamano wrote:\n>\n> > Taylor Blau writes:\n> >\n> > > But I think that this approach ended up being less heavy-weight than I\n> > > had originally imagined, so I think that this version is a worthwhile\n> > > improvement over v4.\n> >\n> > ;-).\n> >\n> > This version is a good place to stop, a bit short of going full OO.\n> > Nicely done.\n>\n> I wonder whether a more generic approach would be more desirable, an\n> approach that would work for `git replay`, too, for example (where\n> streaming objects does not work because they need to be made available\n> immediately because subsequent `merge_incore_nonrecursive()` might expect\n> the created objects to be present)?\n\nThe goal of this series is to bound the number of inodes consumed by a\nsingle merge-tree invocation down from arbitrarily many (in the case of\nstoring each new object loose) to a constant (by storing everything in a\nsingle pack).\n\nI'd think that we would want a similar approach for 'replay', but as\nyou note we have some additional requirements, too:\n\n - each replayed commit is computed in a single step, which will result\n in a new pack\n - we must be able to see objects from previous steps\n\nI think one feasible approach here for replay is to combine the two\nideas and have a separate objdir that stores N packs (one for each step\nof the replay), but then repacks them down into a single pack before\nmigrating back to the main object store.\n\nThat would ensure that we have some isolation between replay-created\nobjects and the rest of the repository in the intermediate state. Even\nthough we'd have as many packs as there are commits, we'd consume far\nfewer inodes in the process, since each commit can introduce arbitrarily\nmany new objects, each requiring at least a single inode (potentially\nmore with sharding).\n\nWe'd have to be mindful of having a large number of packs, but I think\nthat this should mostly be a non-issue, since we'd only be living with N\npacks for the lifetime of the replay command (before repacking them down\nto a single pack and migrating them back to the main object store).\n\nThanks,\nTaylor\n"},{"id":"484537","messageId":"cover.1699381371.git.me@ttaylorr.com","threadId":"60403","inReplyTo":"ZUpepnSCSxL8i96b@nand.local","subject":"[RFC PATCH 0/3] replay: implement support for writing new objects to a pack","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-07T18:22:52Z","receivedAt":"2023-11-07T18:22:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"(Based on a combination of Christian's cc/git-replay and my\ntb/merge-tree-write-pack branches).\n\nThis RFC demonstrates extending the new `--write-pack` option that\n`merge-tree` recently learned to the `replay` builtin as well.\n\nThe approach is as follows:\n\n - write a pack out after each step in the replay operation, so that\n subsequent steps may see any new object(s) created during previous\n steps\n\n - combine those packs into one before migrating them back into the\n main object store\n\nThis is accomplished with a combination of the bulk-checkin and\ntmp-objdir APIs, with some minor modifications made to when we flush out\nand finalize bulk-checkin transactions.\n\nThe benefit to this approach is that we bound the number of inodes\nrequired per replayed commit to a constant (by default, 3: one for the\n.pack, one for the .idx, and another for the .rev file), instead of\nhaving each operation take an unbounded number of inodes proportional to\nthe number of new objects created during that step. We also only migrate\na single pack back to the main object store.\n\nIn other words, this makes the maximum number of inodes required by\n'replay' grow proportional to the number of commits being replayed,\ninstead of the number of new *objects* created as a result of the replay\noperation.\n\nTaylor Blau (3):\n merge-ort.c: finalize ODB transactions after each step\n tmp-objdir: introduce `tmp_objdir_repack()`\n builtin/replay.c: introduce `--write-pack`\n\n Documentation/git-replay.txt | 4 ++++\n builtin/replay.c | 18 ++++++++++++++++++\n merge-ort.c | 5 ++++-\n t/t3650-replay-basics.sh | 37 ++++++++++++++++++++++++++++++++++++\n tmp-objdir.c | 13 +++++++++++++\n tmp-objdir.h | 6 ++++++\n 6 files changed, 82 insertions(+), 1 deletion(-)\n\n-- \n2.42.0.446.g0b9ef90488\n"},{"id":"484538","messageId":"c615a61d32644b64ef8f47feb47ec909286c56b3.1699381371.git.me@ttaylorr.com","threadId":"60403","inReplyTo":"cover.1699381371.git.me@ttaylorr.com","subject":"[RFC PATCH 1/3] merge-ort.c: finalize ODB transactions after each step","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-07T18:22:55Z","receivedAt":"2023-11-07T18:22:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a previous commit, the ORT merge backend learned how to use the\nbulk-checkin mechanism to emit a single pack containing any new objects\ncreated during the merge. This functionality was implemented by setting\nup a new ODB transaction, and finalizing it at the end of the merge via\n`process_entries()`.\n\nIn a future commit, we will extend this functionality to the new `git\nreplay` command, which needs to see objects from previous steps in order\nto replay each commit.\n\nAs a step towards implementing this, teach the ORT backend to flush the\nODB transaction at the end of each step in `process_entries()`, and then\nfinalize the result with `end_odb_transaction()` when calling\n`merge_finalize()`.\n\nFor normal `merge-tree --write-pack` invocations, this produces no\nfunctional change: the pack is written out at the end of\n`process_entries()`, and then the `end_odb_transaction()` call is a\nnoop.\n\nSigned-off-by: Taylor Blau \n---\n merge-ort.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 523577d71e..7b352451cc 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -4354,7 +4354,7 @@ static int process_entries(struct merge_options *opt,\n \t\tret = -1;\n \n \tif (opt->write_pack)\n-\t\tend_odb_transaction();\n+\t\tflush_odb_transaction();\n \n cleanup:\n \tstring_list_clear(&plist, 0);\n@@ -4726,6 +4726,9 @@ void merge_switch_to_result(struct merge_options *opt,\n void merge_finalize(struct merge_options *opt,\n \t\t struct merge_result *result)\n {\n+\tif (opt->write_pack)\n+\t\tend_odb_transaction();\n+\n \tif (opt->renormalize)\n \t\tgit_attr_set_direction(GIT_ATTR_CHECKIN);\n \tassert(opt->priv == NULL);\n-- \n2.42.0.446.g0b9ef90488\n\n"},{"id":"484539","messageId":"0f19c139ba9bb5105747f545038825d0c89f2e42.1699381371.git.me@ttaylorr.com","threadId":"60403","inReplyTo":"cover.1699381371.git.me@ttaylorr.com","subject":"[RFC PATCH 2/3] tmp-objdir: introduce `tmp_objdir_repack()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-07T18:22:58Z","receivedAt":"2023-11-07T18:23:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the following commit, we will teach `git replay` how to write a pack\ncontaining the set of new objects created as a result of the `replay`\noperation.\n\nSince `replay` needs to be able to see the object(s) written\nfrom previous steps in order to replay each commit, the ODB transaction\nmay have multiple pending packs. Migrating multiple packs back into the\nmain object store has a couple of downsides:\n\n - It is error-prone to do so: each pack must be migrated in the\n correct order (with the \".idx\" file staged last), and the set of\n packs themselves must be moved over in the correct order to avoid\n racy behavior.\n\n - It is a (potentially significant) performance degradation to migrate\n a large number of packs back into the main object store.\n\nIntroduce a new function that combines the set of all packs in the\ntemporary object store to produce a single pack which is the logical\nconcatenation of all packs created during that level of the ODB\ntransaction.\n\nSigned-off-by: Taylor Blau \n---\n tmp-objdir.c | 13 +++++++++++++\n tmp-objdir.h | 6 ++++++\n 2 files changed, 19 insertions(+)\n\ndiff --git a/tmp-objdir.c b/tmp-objdir.c\nindex 5f9074ad1c..ef53180b47 100644\n--- a/tmp-objdir.c\n+++ b/tmp-objdir.c\n@@ -12,6 +12,7 @@\n #include \"strvec.h\"\n #include \"quote.h\"\n #include \"object-store-ll.h\"\n+#include \"run-command.h\"\n \n struct tmp_objdir {\n \tstruct strbuf path;\n@@ -277,6 +278,18 @@ int tmp_objdir_migrate(struct tmp_objdir *t)\n \treturn ret;\n }\n \n+int tmp_objdir_repack(struct tmp_objdir *t)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\tcmd.git_cmd = 1;\n+\n+\tstrvec_pushl(&cmd.args, \"repack\", \"-a\", \"-d\", \"-k\", \"-l\", NULL);\n+\tstrvec_pushv(&cmd.env, tmp_objdir_env(t));\n+\n+\treturn run_command(&cmd);\n+}\n+\n const char **tmp_objdir_env(const struct tmp_objdir *t)\n {\n \tif (!t)\ndiff --git a/tmp-objdir.h b/tmp-objdir.h\nindex 237d96b660..d00e3b3e27 100644\n--- a/tmp-objdir.h\n+++ b/tmp-objdir.h\n@@ -36,6 +36,12 @@ struct tmp_objdir *tmp_objdir_create(const char *prefix);\n */\n const char **tmp_objdir_env(const struct tmp_objdir *);\n \n+/*\n+ * Combines all packs in the tmp_objdir into a single pack before migrating.\n+ * Removes original pack(s) after installing the combined pack into place.\n+ */\n+int tmp_objdir_repack(struct tmp_objdir *);\n+\n /*\n * Finalize a temporary object directory by migrating its objects into the main\n * object database, removing the temporary directory, and freeing any\n-- \n2.42.0.446.g0b9ef90488\n\n"},{"id":"484540","messageId":"0b9ef904888d45289445638ae59817feab1dc813.1699381371.git.me@ttaylorr.com","threadId":"60403","inReplyTo":"cover.1699381371.git.me@ttaylorr.com","subject":"[RFC PATCH 3/3] builtin/replay.c: introduce `--write-pack`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-07T18:23:02Z","receivedAt":"2023-11-07T18:23:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Now that the prerequisites are in place, we can implement a\n`--write-pack` option for `git replay`, corresponding to the existing\none in `git merge-tree`.\n\nThe changes are mostly limited to:\n\n - introducing a new option in the builtin\n - replacing the main object store with the temporary one\n - then repacking and migrating the temporary object store back into\n the main object store after the replay has completed\n\nAlong with tests and documentation to ensure that the new behavior\nmatches our expectations.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/git-replay.txt | 4 ++++\n builtin/replay.c | 18 ++++++++++++++++++\n t/t3650-replay-basics.sh | 37 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 59 insertions(+)\n\ndiff --git a/Documentation/git-replay.txt b/Documentation/git-replay.txt\nindex e7551aec54..f424c1a676 100644\n--- a/Documentation/git-replay.txt\n+++ b/Documentation/git-replay.txt\n@@ -42,6 +42,10 @@ When `--advance` is specified, the update-ref command(s) in the output\n will update the branch passed as an argument to `--advance` to point at\n the new commits (in other words, this mimics a cherry-pick operation).\n \n+--write-pack::\n+\tWrite any new objects into a separate packfile instead of as\n+\tindividual loose objects.\n+\n ::\n \tRange of commits to replay. More than one can\n \tbe passed, but in `--advance ` mode, they should have\ndiff --git a/builtin/replay.c b/builtin/replay.c\nindex c3d53ff0cd..72b7b7f43a 100644\n--- a/builtin/replay.c\n+++ b/builtin/replay.c\n@@ -17,6 +17,7 @@\n #include \"strmap.h\"\n #include \n #include \n+#include \"tmp-objdir.h\"\n \n static const char *short_commit_name(struct commit *commit)\n {\n@@ -272,6 +273,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix)\n \tstruct commit *onto = NULL;\n \tconst char *onto_name = NULL;\n \tint contained = 0;\n+\tint write_pack = 0;\n \n \tstruct rev_info revs;\n \tstruct commit *last_commit = NULL;\n@@ -279,6 +281,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix)\n \tstruct merge_options merge_opt;\n \tstruct merge_result result;\n \tstruct strset *update_refs = NULL;\n+\tstruct tmp_objdir *tmp_objdir = NULL;\n \tkh_oid_map_t *replayed_commits;\n \tint i, ret = 0;\n \n@@ -296,6 +299,8 @@ int cmd_replay(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"replay onto given commit\")),\n \t\tOPT_BOOL(0, \"contained\", &contained,\n \t\t\t N_(\"advance all branches contained in revision-range\")),\n+\t\tOPT_BOOL(0, \"write-pack\", &write_pack,\n+\t\t\t N_(\"write new objects to a pack instead of as loose\")),\n \t\tOPT_END()\n \t};\n \n@@ -352,8 +357,15 @@ int cmd_replay(int argc, const char **argv, const char *prefix)\n \tinit_merge_options(&merge_opt, the_repository);\n \tmemset(&result, 0, sizeof(result));\n \tmerge_opt.show_rename_progress = 0;\n+\tmerge_opt.write_pack = write_pack;\n \tlast_commit = onto;\n \treplayed_commits = kh_init_oid_map();\n+\n+\tif (merge_opt.write_pack) {\n+\t\ttmp_objdir = tmp_objdir_create(\"replay\");\n+\t\ttmp_objdir_replace_primary_odb(tmp_objdir, 0);\n+\t}\n+\n \twhile ((commit = get_revision(&revs))) {\n \t\tconst struct name_decoration *decoration;\n \t\tkhint_t pos;\n@@ -417,5 +429,11 @@ int cmd_replay(int argc, const char **argv, const char *prefix)\n \t/* Return */\n \tif (ret < 0)\n \t\texit(128);\n+\tif (ret && tmp_objdir) {\n+\t\tif (tmp_objdir_repack(tmp_objdir) < 0)\n+\t\t\tret = 0;\n+\t\telse if (tmp_objdir_migrate(tmp_objdir) < 0)\n+\t\t\tret = 0;\n+\t}\n \treturn ret ? 0 : 1;\n }\ndiff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh\nindex 389670262e..e7048748c2 100755\n--- a/t/t3650-replay-basics.sh\n+++ b/t/t3650-replay-basics.sh\n@@ -67,6 +67,43 @@ test_expect_success 'using replay to rebase two branches, one on top of other' '\n \ttest_cmp expect result\n '\n \n+packdir=.git/objects/pack\n+\n+test_expect_success 'using replay to rebase two branches, with --write-pack' '\n+\t# Remove the results of the previous rebase, ensuring that they\n+\t# are pruned from the object store.\n+\tgit gc --prune=now &&\n+\ttest_must_fail git cat-file -t \"$(cut -d \" \" -f 3 expect)\" &&\n+\n+\t# Create an extra packfile to ensure that the tmp-objdir repack\n+\t# takes place outside of the main object store.\n+\tgit checkout --detach &&\n+\ttest_commit unreachable &&\n+\tgit repack -d &&\n+\tgit checkout main &&\n+\n+\tfind $packdir -type f -name \"*.idx\" | sort >packs.before &&\n+\tgit replay --write-pack --onto main topic1..topic2 >result &&\n+\tfind $packdir -type f -name \"*.idx\" | sort >packs.after &&\n+\n+\tcomm -13 packs.before packs.after >packs.new &&\n+\n+\t# Ensure that we got a single new pack.\n+\ttest_line_count = 1 result &&\n+\ttest_line_count = 1 packs.new &&\n+\n+\t# ... and that the rest of the results match our expeectations.\n+\tgit log --format=%s $(cut -f 3 -d \" \" result) >actual &&\n+\ttest_write_lines E D M L B A >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tprintf \"update refs/heads/topic2 \" >expect &&\n+\tprintf \"%s \" $(cut -f 3 -d \" \" result) >>expect &&\n+\tgit rev-parse topic2 >>expect &&\n+\n+\ttest_cmp expect result\n+'\n+\n test_expect_success 'using replay on bare repo to rebase two branches, one on top of other' '\n \tgit -C bare replay --onto main topic1..topic2 >result-bare &&\n \ttest_cmp expect result-bare\n-- \n2.42.0.446.g0b9ef90488\n"},{"id":"484560","messageId":"ZUszSs0CYoFV9YJ0@tanuki","threadId":"60403","inReplyTo":"0f19c139ba9bb5105747f545038825d0c89f2e42.1699381371.git.me@ttaylorr.com","subject":"Re: [RFC PATCH 2/3] tmp-objdir: introduce `tmp_objdir_repack()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-08T07:05:46Z","receivedAt":"2023-11-08T07:05:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Nov 07, 2023 at 01:22:58PM -0500, Taylor Blau wrote:\n> In the following commit, we will teach `git replay` how to write a pack\n> containing the set of new objects created as a result of the `replay`\n> operation.\n> \n> Since `replay` needs to be able to see the object(s) written\n> from previous steps in order to replay each commit, the ODB transaction\n> may have multiple pending packs. Migrating multiple packs back into the\n> main object store has a couple of downsides:\n> \n> - It is error-prone to do so: each pack must be migrated in the\n> correct order (with the \".idx\" file staged last), and the set of\n> packs themselves must be moved over in the correct order to avoid\n> racy behavior.\n> \n> - It is a (potentially significant) performance degradation to migrate\n> a large number of packs back into the main object store.\n> \n> Introduce a new function that combines the set of all packs in the\n> temporary object store to produce a single pack which is the logical\n> concatenation of all packs created during that level of the ODB\n> transaction.\n> \n> Signed-off-by: Taylor Blau \n> ---\n> tmp-objdir.c | 13 +++++++++++++\n> tmp-objdir.h | 6 ++++++\n> 2 files changed, 19 insertions(+)\n> \n> diff --git a/tmp-objdir.c b/tmp-objdir.c\n> index 5f9074ad1c..ef53180b47 100644\n> --- a/tmp-objdir.c\n> +++ b/tmp-objdir.c\n> @@ -12,6 +12,7 @@\n> #include \"strvec.h\"\n> #include \"quote.h\"\n> #include \"object-store-ll.h\"\n> +#include \"run-command.h\"\n> \n> struct tmp_objdir {\n> \tstruct strbuf path;\n> @@ -277,6 +278,18 @@ int tmp_objdir_migrate(struct tmp_objdir *t)\n> \treturn ret;\n> }\n> \n> +int tmp_objdir_repack(struct tmp_objdir *t)\n> +{\n> +\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> +\n> +\tcmd.git_cmd = 1;\n> +\n> +\tstrvec_pushl(&cmd.args, \"repack\", \"-a\", \"-d\", \"-k\", \"-l\", NULL);\n> +\tstrvec_pushv(&cmd.env, tmp_objdir_env(t));\n\nI wonder what performance of this repack would be like in a large\nrepository with many refs. Ideally, I would expect that the repacking\nperformance should scale with the number of objects we have written into\nthe temporary object directory. But in practice, the repack will need to\ncompute reachability and thus also scales with the size of the repo\nitself, doesn't it?\n\nPatrick\n\n> +\treturn run_command(&cmd);\n> +}\n> +\n> const char **tmp_objdir_env(const struct tmp_objdir *t)\n> {\n> \tif (!t)\n> diff --git a/tmp-objdir.h b/tmp-objdir.h\n> index 237d96b660..d00e3b3e27 100644\n> --- a/tmp-objdir.h\n> +++ b/tmp-objdir.h\n> @@ -36,6 +36,12 @@ struct tmp_objdir *tmp_objdir_create(const char *prefix);\n> */\n> const char **tmp_objdir_env(const struct tmp_objdir *);\n> \n> +/*\n> + * Combines all packs in the tmp_objdir into a single pack before migrating.\n> + * Removes original pack(s) after installing the combined pack into place.\n> + */\n> +int tmp_objdir_repack(struct tmp_objdir *);\n> +\n> /*\n> * Finalize a temporary object directory by migrating its objects into the main\n> * object database, removing the temporary directory, and freeing any\n> -- \n> 2.42.0.446.g0b9ef90488\n> \n"},{"id":"484661","messageId":"ZU0yT9karPxHrGuM@nand.local","threadId":"60403","inReplyTo":"ZUszSs0CYoFV9YJ0@tanuki","subject":"Re: [RFC PATCH 2/3] tmp-objdir: introduce `tmp_objdir_repack()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-09T19:26:07Z","receivedAt":"2023-11-09T19:26:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 08, 2023 at 08:05:46AM +0100, Patrick Steinhardt wrote:\n> > @@ -277,6 +278,18 @@ int tmp_objdir_migrate(struct tmp_objdir *t)\n> > \treturn ret;\n> > }\n> >\n> > +int tmp_objdir_repack(struct tmp_objdir *t)\n> > +{\n> > +\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> > +\n> > +\tcmd.git_cmd = 1;\n> > +\n> > +\tstrvec_pushl(&cmd.args, \"repack\", \"-a\", \"-d\", \"-k\", \"-l\", NULL);\n> > +\tstrvec_pushv(&cmd.env, tmp_objdir_env(t));\n>\n> I wonder what performance of this repack would be like in a large\n> repository with many refs. Ideally, I would expect that the repacking\n> performance should scale with the number of objects we have written into\n> the temporary object directory. But in practice, the repack will need to\n> compute reachability and thus also scales with the size of the repo\n> itself, doesn't it?\n\nGood question. We definitely do not want to be doing an all-into-one\nrepack as a consequence of running 'git replay' in a large repository\nwith lots of refs, objects, or both.\n\nBut since we push the result of calling `tmp_objdir_env(t)` into the\nenvironment of the child process, we are only repacking the objects in\nthe temporary directory, not the main object store.\n\nI have a test that verifies this is the case by making sure that in a\nrepository with some arbitrary set of pre-existing packs, that only one\npack is added to that set after running 'replay', and that the\npre-existing packs remain in place.\n\nThanks,\nTaylor\n"},{"id":"484730","messageId":"CABPp-BEfy9VOvimP9==ry_rZXu=metOQ8s=_-XiG_Pdx9c06Ww@mail.gmail.com","threadId":"60403","inReplyTo":"3595db76a525fcebc3c896e231246704b044310c.1698101088.git.me@ttaylorr.com","subject":"Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-11-10T23:51:18Z","receivedAt":"2023-11-10T23:51:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nSorry for taking so long to find some time to review. And sorry for\nthe bad news, but...\n\nOn Mon, Oct 23, 2023 at 3:45 PM Taylor Blau wrote:\n>\n> When using merge-tree often within a repository[^1], it is possible to\n> generate a relatively large number of loose objects, which can result in\n> degraded performance, and inode exhaustion in extreme cases.\n>\n> Building on the functionality introduced in previous commits, the\n> bulk-checkin machinery now has support to write arbitrary blob and tree\n> objects which are small enough to be held in-core. We can use this to\n> write any blob/tree objects generated by ORT into a separate pack\n> instead of writing them out individually as loose.\n>\n> This functionality is gated behind a new `--write-pack` option to\n> `merge-tree` that works with the (non-deprecated) `--write-tree` mode.\n>\n> The implementation is relatively straightforward. There are two spots\n> within the ORT mechanism where we call `write_object_file()`, one for\n> content differences within blobs, and another to assemble any new trees\n> necessary to construct the merge. In each of those locations,\n> conditionally replace calls to `write_object_file()` with\n> `index_blob_bulk_checkin_incore()` or `index_tree_bulk_checkin_incore()`\n> depending on which kind of object we are writing.\n>\n> The only remaining task is to begin and end the transaction necessary to\n> initialize the bulk-checkin machinery, and move any new pack(s) it\n> created into the main object store.\n\nI believe the above is built on an assumption that any objects written\nwill not need to be read until after the merge is completed. And we\nmight have a nesting issue too...\n\n> @@ -2108,10 +2109,19 @@ static int handle_content_merge(struct merge_options *opt,\n> if ((merge_status < 0) || !result_buf.ptr)\n> ret = error(_(\"failed to execute internal merge\"));\n>\n> - if (!ret &&\n> - write_object_file(result_buf.ptr, result_buf.size,\n> - OBJ_BLOB, &result->oid))\n> - ret = error(_(\"unable to add %s to database\"), path);\n> + if (!ret) {\n> + ret = opt->write_pack\n> + ? index_blob_bulk_checkin_incore(&result->oid,\n> + result_buf.ptr,\n> + result_buf.size,\n> + path, 1)\n> + : write_object_file(result_buf.ptr,\n> + result_buf.size,\n> + OBJ_BLOB, &result->oid);\n> + if (ret)\n> + ret = error(_(\"unable to add %s to database\"),\n> + path);\n> + }\n>\n> free(result_buf.ptr);\n> if (ret)\n\nThis is unsafe; the object may need to be read later within the same\nmerge. Perhaps the simplest example related to your testcase is\nmodifying the middle section of that testcase (I'll highlight where\nwhen I comment on the testcase) to read:\n\n test_commit -C repo base file \"$(test_seq 3 5)\" &&\n git -C repo branch -M main &&\n git -C repo checkout -b side main &&\n test_commit -C repo side-file file \"$(test_seq 1 5)\" &&\n test_commit -C repo side-file2 file2 \"$(test_seq 1 6)\" &&\n git -C repo checkout -b other main &&\n test_commit -C repo other-file file \"$(test_seq 3 7)\" &&\n git -C repo mv file file2 &&\n git -C repo commit -m other-file2 &&\n\n find repo/$packdir -type f -name \"pack-*.idx\" >packs.before &&\n git -C repo merge-tree --write-pack side other &&\n\nIn words, what I'm doing here is having both sides modify \"file\" (the\nsame as you did) but also having one side rename \"file\"->\"file2\". The\nside that doesn't rename \"file\" also introduces a new \"file2\". ort\nneeds to merge the three versions of \"file\" to get a new blob object,\nand then merge that with the content from the brand new \"file2\". More\ncomplicated cases are possible, of course. Anyway, with the modified\ntestcase above, merge-tree will fail with:\n\n fatal: unable to read blob object 06e567b11dfdafeaf7d3edcc89864149383aeab6\n\nI think (untested) that you could fix this by creating two packs\ninstead of just one. In particular, add a call to\nflush_odb_transcation() after the \"redo_after_renames\" block in\nmerge_ort_nonrecursive_internal(). (It's probably easier to see that\nyou could place the flush_odb_transaction() call inside\ndetect_and_process_renames() just after the process_renames() call,\nbut when redo_after_renames is true you'd end up with three packs\ninstead of just two).\n\nWhat happens with the odb transaction stuff if no new objects are\nwritten before the call to flush_odb_transaction? Will that be a\nproblem?\n\n(Since any tree written will not be re-read within the same merge, the\nother write_object_file() call you changed does not have the same\nproblem as the one here.)\n\n>@@ -4332,9 +4349,13 @@ static int process_entries(struct merge_options *opt,\n> fflush(stdout);\n> BUG(\"dir_metadata accounting completely off; shouldn't happen\");\n> }\n>- if (write_tree(result_oid, &dir_metadata.versions, 0,\n>+ if (write_tree(opt, result_oid, &dir_metadata.versions, 0,\n> opt->repo->hash_algo->rawsz) < 0)\n> ret = -1;\n>\n> +\n> + if (opt->write_pack)\n> + end_odb_transaction();\n> +\n\nIs the end_odb_transaction() here going to fail with an \"Unbalanced\nODB transaction nesting\" when faced with a recursive merge?\n\nPerhaps flushing here, and then calling end_odb_transaction() in\nmerge_finalize(), much as you do in your replay-and-write-pack series,\nshould actually be moved to this commit and included here?\n\nThis does mean that for a recursive merge, that you'll get up to 2*N\npackfiles, where N is the depth of the recursive merge.\n\n> + test_commit -C repo base file \"$(test_seq 3 5)\" &&\n> + git -C repo branch -M main &&\n> + git -C repo checkout -b side main &&\n> + test_commit -C repo side file \"$(test_seq 1 5)\" &&\n> + git -C repo checkout -b other main &&\n> + test_commit -C repo other file \"$(test_seq 3 7)\" &&\n> +\n> + find repo/$packdir -type f -name \"pack-*.idx\" >packs.before &&\n> + tree=\"$(git -C repo merge-tree --write-pack \\\n> + refs/tags/side refs/tags/other)\" &&\n\nThese were the lines from your testcase that I replaced to show the bug.\n"},{"id":"484736","messageId":"CABPp-BH8+VopU45GOC6+BS3fLWbhZM0sKB2K7G_koobZwY_8rw@mail.gmail.com","threadId":"60403","inReplyTo":"CAPig+cTjQe6FWo98LxvDS=s3dOs33SUUJa=x-bkyWHNBMx+XFw@mail.gmail.com","subject":"Re: [PATCH v5 3/5] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-11-11T00:17:32Z","receivedAt":"2023-11-11T00:17:48Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Oct 25, 2023 at 10:22 AM Eric Sunshine wrote:\n>\n> On Wed, Oct 25, 2023 at 11:44 AM Taylor Blau wrote:\n> > On Wed, Oct 25, 2023 at 09:58:06AM +0200, Patrick Steinhardt wrote:\n> > > On Mon, Oct 23, 2023 at 06:45:01PM -0400, Taylor Blau wrote:\n> > > > In order to support streaming from a location in memory, we must\n> > > > implement a new kind of bulk_checkin_source that does just that. These\n> > > > implementation in spread out across:\n> > >\n>\n> Perhaps:\n>\n> s/implementation in/implementations are/\n\nWas just about to comment with the same suggestion; I had a hard time\nparsing the commit message as well because of this.\n"},{"id":"484738","messageId":"xmqqmsvlrv5t.fsf@gitster.g","threadId":"60403","inReplyTo":"CABPp-BEfy9VOvimP9==ry_rZXu=metOQ8s=_-XiG_Pdx9c06Ww@mail.gmail.com","subject":"Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-11T00:27:42Z","receivedAt":"2023-11-11T00:27:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> I believe the above is built on an assumption that any objects written\n> will not need to be read until after the merge is completed. And we\n> might have a nesting issue too...\n> ...\n> This is unsafe; the object may need to be read later within the same\n> merge.\n\nThanks for a good analysis. I agree.\n\n"},{"id":"484740","messageId":"ZU7X3N/rqCK/Y9cj@nand.local","threadId":"60403","inReplyTo":"CABPp-BEfy9VOvimP9==ry_rZXu=metOQ8s=_-XiG_Pdx9c06Ww@mail.gmail.com","subject":"Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-11T01:24:44Z","receivedAt":"2023-11-11T01:24:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Nov 10, 2023 at 03:51:18PM -0800, Elijah Newren wrote:\n> > @@ -2108,10 +2109,19 @@ static int handle_content_merge(struct merge_options *opt,\n> > if ((merge_status < 0) || !result_buf.ptr)\n> > ret = error(_(\"failed to execute internal merge\"));\n> >\n> > - if (!ret &&\n> > - write_object_file(result_buf.ptr, result_buf.size,\n> > - OBJ_BLOB, &result->oid))\n> > - ret = error(_(\"unable to add %s to database\"), path);\n> > + if (!ret) {\n> > + ret = opt->write_pack\n> > + ? index_blob_bulk_checkin_incore(&result->oid,\n> > + result_buf.ptr,\n> > + result_buf.size,\n> > + path, 1)\n> > + : write_object_file(result_buf.ptr,\n> > + result_buf.size,\n> > + OBJ_BLOB, &result->oid);\n> > + if (ret)\n> > + ret = error(_(\"unable to add %s to database\"),\n> > + path);\n> > + }\n> >\n> > free(result_buf.ptr);\n> > if (ret)\n>\n> This is unsafe; the object may need to be read later within the same\n> merge. Perhaps the simplest example related to your testcase is\n> modifying the middle section of that testcase (I'll highlight where\n> when I comment on the testcase) to read:\n>\n> test_commit -C repo base file \"$(test_seq 3 5)\" &&\n> git -C repo branch -M main &&\n> git -C repo checkout -b side main &&\n> test_commit -C repo side-file file \"$(test_seq 1 5)\" &&\n> test_commit -C repo side-file2 file2 \"$(test_seq 1 6)\" &&\n> git -C repo checkout -b other main &&\n> test_commit -C repo other-file file \"$(test_seq 3 7)\" &&\n> git -C repo mv file file2 &&\n> git -C repo commit -m other-file2 &&\n>\n> find repo/$packdir -type f -name \"pack-*.idx\" >packs.before &&\n> git -C repo merge-tree --write-pack side other &&\n>\n> In words, what I'm doing here is having both sides modify \"file\" (the\n> same as you did) but also having one side rename \"file\"->\"file2\". The\n> side that doesn't rename \"file\" also introduces a new \"file2\". ort\n> needs to merge the three versions of \"file\" to get a new blob object,\n> and then merge that with the content from the brand new \"file2\". More\n> complicated cases are possible, of course. Anyway, with the modified\n> testcase above, merge-tree will fail with:\n>\n> fatal: unable to read blob object 06e567b11dfdafeaf7d3edcc89864149383aeab6\n>\n> I think (untested) that you could fix this by creating two packs\n> instead of just one. In particular, add a call to\n> flush_odb_transcation() after the \"redo_after_renames\" block in\n> merge_ort_nonrecursive_internal(). (It's probably easier to see that\n> you could place the flush_odb_transaction() call inside\n> detect_and_process_renames() just after the process_renames() call,\n> but when redo_after_renames is true you'd end up with three packs\n> instead of just two).\n\nGreat analysis, thanks for catching this error. I tested your approach,\nand indeed a flush_odb_transaction() call after the process_renames()\ncall in detect_and_process_renames() does do the trick.\n\n> What happens with the odb transaction stuff if no new objects are\n> written before the call to flush_odb_transaction? Will that be a\n> problem?\n\nI think that the bulk-checkin code is flexible enough to understand that\nwe shouldn't do anything when there aren't any objects to pack.\n\n> (Since any tree written will not be re-read within the same merge, the\n> other write_object_file() call you changed does not have the same\n> problem as the one here.)\n>\n> >@@ -4332,9 +4349,13 @@ static int process_entries(struct merge_options *opt,\n> > fflush(stdout);\n> > BUG(\"dir_metadata accounting completely off; shouldn't happen\");\n> > }\n> >- if (write_tree(result_oid, &dir_metadata.versions, 0,\n> >+ if (write_tree(opt, result_oid, &dir_metadata.versions, 0,\n> > opt->repo->hash_algo->rawsz) < 0)\n> > ret = -1;\n> >\n> > +\n> > + if (opt->write_pack)\n> > + end_odb_transaction();\n> > +\n>\n> Is the end_odb_transaction() here going to fail with an \"Unbalanced\n> ODB transaction nesting\" when faced with a recursive merge?\n\nI think so, and we should have a test-case demonstrating that. In the\nremaining three patches that I posted to extend this approach to 'git\nreplay', I moved this call elsewhere in such a way that I think\n\n> Perhaps flushing here, and then calling end_odb_transaction() in\n> merge_finalize(), much as you do in your replay-and-write-pack series,\n> should actually be moved to this commit and included here?\n\nYep, exactly.\n\n> This does mean that for a recursive merge, that you'll get up to 2*N\n> packfiles, where N is the depth of the recursive merge.\n\nWe definitely want to avoid that ;-). I think there are a couple of\npotential directions forward here, but the most promising one I think is\ndue to Johannes who suggests that we write loose objects into a\ntemporary directory with a replace_tmp_objdir() call, and then repack\nthat side directory before migrating a single pack back into the main\nobject store.\n\nThanks,\neaylor\n"},{"id":"484742","messageId":"ZU7aClZCAtzkt55w@nand.local","threadId":"60403","inReplyTo":"xmqqmsvlrv5t.fsf@gitster.g","subject":"Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-11T01:34:02Z","receivedAt":"2023-11-11T01:34:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Nov 11, 2023 at 09:27:42AM +0900, Junio C Hamano wrote:\n> Elijah Newren writes:\n>\n> > I believe the above is built on an assumption that any objects written\n> > will not need to be read until after the merge is completed. And we\n> > might have a nesting issue too...\n> > ...\n> > This is unsafe; the object may need to be read later within the same\n> > merge.\n>\n> Thanks for a good analysis. I agree.\n\nDitto. I responded to Elijah more in-depth elsewhere in the thread, but\nI think for your purposes it is OK to discard this series.\n\nThanks,\nTaylor\n"},{"id":"484743","messageId":"CABPp-BESNRXdhzR0MVCNi-F+02BsW3ukGmAgzEk_5xoErs+Fzg@mail.gmail.com","threadId":"60403","inReplyTo":"cover.1699381371.git.me@ttaylorr.com","subject":"Re: [RFC PATCH 0/3] replay: implement support for writing new objects to a pack","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-11-11T03:42:01Z","receivedAt":"2023-11-11T03:42:17Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Nov 7, 2023 at 10:22 AM Taylor Blau wrote:\n>\n> (Based on a combination of Christian's cc/git-replay and my\n> tb/merge-tree-write-pack branches).\n>\n> This RFC demonstrates extending the new `--write-pack` option that\n> `merge-tree` recently learned to the `replay` builtin as well.\n>\n> The approach is as follows:\n>\n> - write a pack out after each step in the replay operation, so that\n> subsequent steps may see any new object(s) created during previous\n> steps\n>\n> - combine those packs into one before migrating them back into the\n> main object store\n>\n> This is accomplished with a combination of the bulk-checkin and\n> tmp-objdir APIs, with some minor modifications made to when we flush out\n> and finalize bulk-checkin transactions.\n>\n> The benefit to this approach is that we bound the number of inodes\n> required per replayed commit to a constant (by default, 3: one for the\n> .pack, one for the .idx, and another for the .rev file), instead of\n> having each operation take an unbounded number of inodes proportional to\n> the number of new objects created during that step. We also only migrate\n> a single pack back to the main object store.\n\nIsn't it actually 4? Since you only put blobs and trees into the bulk\ncheckin packfiles, the commit object will still be loose.\n\n> In other words, this makes the maximum number of inodes required by\n> 'replay' grow proportional to the number of commits being replayed,\n> instead of the number of new *objects* created as a result of the replay\n> operation.\n\nAs per comments on the other series, we actually need 2 packs per\ncommit (when replaying non-merge commits, we don't have to worry about\nrecursive merges, so 2*depth is just 2), so this would be 7 inodes per\ncommit (3 files per pack * 2 packs + 1 loose commit object).\n\nI was curious what the comparative number of loose objects might be if\nwe didn't use the bulk checking, so:\n$ cd linux.git\n$ git rev-list --no-merges --count HEAD\n1141436\n$ time git log --oneline --no-merges --name-only | wc -l\n3781628\n$ python -c 'print(3781628/1141436)'\n3.3130442705504293\n\nSo, if repositories are similar to linux, that's a bit over 3 files\nmodified per commit. It's a bit harder to count trees, but let's take\na wild guess and say that each file is 7 directories (because I'm too\nlazy to do real research and 7 happens to give me nice round numbers\nlater), so that's up to 7*3 tree objects. So 3 file objects + 21 tree\nobjects + 1 commit object = 25 loose objects.\n\nSo, your scheme certainly seems to reduce number of inodes, but does\nit introduce a different scaling issue? git-gc repacks when there are\n>= 50 packs, or when there are >= 6700 loose objects, suggesting that\nif we exceed those numbers, we might start seeing performance suffer.\nWe would hit 50 packs with a mere 25 commits being replayed, and\nwouldn't expect to get to 6700 loose objects until we replayed about\n268 commits (6700/25). Does this mean we are risking worse\nperformance degradation with this scheme than with just using loose\nobjects until the end of the operation?\n"},{"id":"484744","messageId":"CABPp-BGzWciTAyyFyBwfjti+EPoNB=KdO3bUcS-=FOgT-X_NiQ@mail.gmail.com","threadId":"60403","inReplyTo":"c615a61d32644b64ef8f47feb47ec909286c56b3.1699381371.git.me@ttaylorr.com","subject":"Re: [RFC PATCH 1/3] merge-ort.c: finalize ODB transactions after each step","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-11-11T03:45:27Z","receivedAt":"2023-11-11T03:45:43Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Nov 7, 2023 at 10:22 AM Taylor Blau wrote:\n>\n> In a previous commit, the ORT merge backend learned how to use the\n> bulk-checkin mechanism to emit a single pack containing any new objects\n> created during the merge. This functionality was implemented by setting\n> up a new ODB transaction, and finalizing it at the end of the merge via\n> `process_entries()`.\n>\n> In a future commit, we will extend this functionality to the new `git\n> replay` command, which needs to see objects from previous steps in order\n> to replay each commit.\n>\n> As a step towards implementing this, teach the ORT backend to flush the\n> ODB transaction at the end of each step in `process_entries()`, and then\n> finalize the result with `end_odb_transaction()` when calling\n> `merge_finalize()`.\n\nprocess_entries() contains a for loop inside it, so \"end of each step\nin `process_entries()`\" sounds like you are flushing after entry, i.e.\nseveral times per commit being replayed.\n\nPerhaps \"at the end of `process_entries()` (thus creating one pack per\ncommit)\" ?\n\n\n(Of course, the fact that we need this change earlier, in the other\nseries, kinda makes this point moot. Just thought I'd mention it\nanyway in case it comes back up in your restructuring.)\n"},{"id":"484747","messageId":"CABPp-BGy8tkDAfgdhP9+V1dNk8_J3fM6ykb18jO5O4MC0VwTQQ@mail.gmail.com","threadId":"60403","inReplyTo":"cover.1699381371.git.me@ttaylorr.com","subject":"Re: [RFC PATCH 0/3] replay: implement support for writing new objects to a pack","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-11-11T04:04:49Z","receivedAt":"2023-11-11T04:05:04Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Nov 7, 2023 at 10:22 AM Taylor Blau wrote:\n>\n> (Based on a combination of Christian's cc/git-replay and my\n> tb/merge-tree-write-pack branches).\n>\n> This RFC demonstrates extending the new `--write-pack` option that\n> `merge-tree` recently learned to the `replay` builtin as well.\n>\n> The approach is as follows:\n>\n> - write a pack out after each step in the replay operation, so that\n> subsequent steps may see any new object(s) created during previous\n> steps\n>\n> - combine those packs into one before migrating them back into the\n> main object store\n>\n> This is accomplished with a combination of the bulk-checkin and\n> tmp-objdir APIs, with some minor modifications made to when we flush out\n> and finalize bulk-checkin transactions.\n\nJust thought I'd note that I finished reading all of this series as\nwell as the five-commit series it's based on. Just wanted to note\nthat any commits I didn't comment on from either series looked good to\nme.\n"},{"id":"484818","messageId":"20231113220254.GA2065691@coredump.intra.peff.net","threadId":"60403","inReplyTo":"CABPp-BEfy9VOvimP9==ry_rZXu=metOQ8s=_-XiG_Pdx9c06Ww@mail.gmail.com","subject":"Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-13T22:02:54Z","receivedAt":"2023-11-13T22:02:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 10, 2023 at 03:51:18PM -0800, Elijah Newren wrote:\n\n> This is unsafe; the object may need to be read later within the same\n> merge. [...]\n>\n> I think (untested) that you could fix this by creating two packs\n> instead of just one. In particular, add a call to\n> flush_odb_transcation() after the \"redo_after_renames\" block in\n> merge_ort_nonrecursive_internal().\n\nIt might not be too hard to just let in-process callers access the\nobjects we've written. A quick and dirty patch is below, which works\nwith the test you suggested (the test still fails because it finds a\nconflict, but it gets past the \"woah, I can't find that sha1\" part).\n\nI don't know if that is sufficient, though. Would other spawned\nprocesses (hooks, external merge drivers, and so on) need to be able to\nsee these objects, too?\n\nThe patch teaches the packfile code about the special bulk checkin pack.\nIt might be cleaner to invert it, and just have the bulk checkin code\nregister a magic packed_git (it would need to fake the .idx somehow).\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex bd6151ba3c..566fc36e68 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -30,6 +30,8 @@ static struct bulk_checkin_packfile {\n \tstruct pack_idx_entry **written;\n \tuint32_t alloc_written;\n \tuint32_t nr_written;\n+\n+\tstruct packed_git *fake_packed_git;\n } bulk_checkin_packfile;\n \n static void finish_tmp_packfile(struct strbuf *basename,\n@@ -82,6 +84,7 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n \n clear_exit:\n \tfree(state->written);\n+\tfree(state->fake_packed_git);\n \tmemset(state, 0, sizeof(*state));\n \n \tstrbuf_release(&packname);\n@@ -530,3 +533,37 @@ void end_odb_transaction(void)\n \n \tflush_odb_transaction();\n }\n+\n+static struct packed_git *fake_packed_git(struct bulk_checkin_packfile *state)\n+{\n+\tstruct packed_git *p = state->fake_packed_git;\n+\tif (!p) {\n+\t\tFLEX_ALLOC_STR(p, pack_name, \"/fake/in-progress.pack\");\n+\t\tstate->fake_packed_git = p;\n+\t\tp->pack_fd = state->f->fd;\n+\t\tp->do_not_close = 1;\n+\t}\n+\n+\thashflush(state->f);\n+\tp->pack_size = state->f->total; /* maybe add 20 to simulate trailer? */\n+\n+\treturn p;\n+}\n+\n+int bulk_checkin_pack_entry(const struct object_id *oid, struct pack_entry *e)\n+{\n+\tsize_t i;\n+\t/*\n+\t * this really ought to have some more efficient data structure for\n+\t * lookup; ditto for the existing already_written()\n+\t */\n+\tfor (i = 0; i < bulk_checkin_packfile.nr_written; i++) {\n+\t\tstruct pack_idx_entry *p = bulk_checkin_packfile.written[i];\n+\t\tif (oideq(&p->oid, oid)) {\n+\t\t\te->p = fake_packed_git(&bulk_checkin_packfile);\n+\t\t\te->offset = p->offset;\n+\t\t\treturn 0;\n+\t\t}\n+\t}\n+\treturn -1;\n+}\ndiff --git a/bulk-checkin.h b/bulk-checkin.h\nindex 89786b3954..153fe87c06 100644\n--- a/bulk-checkin.h\n+++ b/bulk-checkin.h\n@@ -44,4 +44,7 @@ void flush_odb_transaction(void);\n */\n void end_odb_transaction(void);\n \n+struct pack_entry;\n+int bulk_checkin_pack_entry(const struct object_id *oid, struct pack_entry *e);\n+\n #endif\ndiff --git a/packfile.c b/packfile.c\nindex 9cc0a2e37a..05194b1d9b 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -23,6 +23,7 @@\n #include \"commit-graph.h\"\n #include \"pack-revindex.h\"\n #include \"promisor-remote.h\"\n+#include \"bulk-checkin.h\"\n \n char *odb_pack_name(struct strbuf *buf,\n \t\t const unsigned char *hash,\n@@ -2045,6 +2046,9 @@ int find_pack_entry(struct repository *r, const struct object_id *oid, struct pa\n \tstruct list_head *pos;\n \tstruct multi_pack_index *m;\n \n+\tif (!bulk_checkin_pack_entry(oid, e))\n+\t\treturn 1;\n+\n \tprepare_packed_git(r);\n \tif (!r->objects->packed_git && !r->objects->multi_pack_index)\n \t\treturn 0;\n"},{"id":"484819","messageId":"20231113220546.GB2065691@coredump.intra.peff.net","threadId":"60403","inReplyTo":"ZU7X3N/rqCK/Y9cj@nand.local","subject":"Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-13T22:05:46Z","receivedAt":"2023-11-13T22:05:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 10, 2023 at 08:24:44PM -0500, Taylor Blau wrote:\n\n> > This does mean that for a recursive merge, that you'll get up to 2*N\n> > packfiles, where N is the depth of the recursive merge.\n> \n> We definitely want to avoid that ;-). I think there are a couple of\n> potential directions forward here, but the most promising one I think is\n> due to Johannes who suggests that we write loose objects into a\n> temporary directory with a replace_tmp_objdir() call, and then repack\n> that side directory before migrating a single pack back into the main\n> object store.\n\nI posted an alternative in response to Elijah; the general idea being to\nallow the usual object-lookup code to access the in-progress pack. That\nwould keep us limited to a single pack.\n\nIt _might_ be a terrible idea. E.g., if you write a non-bulk object that\nreferences a bulk one, then that non-bulk one is broken from the\nperspective of other processes (until the bulk checkin is flushed). But\nI think we'd always be writing to one or the other here, never\ninterleaving?\n\n-Peff\n"},{"id":"484822","messageId":"ZVKkgpiFaOwwDcdw@nand.local","threadId":"60403","inReplyTo":"20231113220254.GA2065691@coredump.intra.peff.net","subject":"Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-13T22:34:42Z","receivedAt":"2023-11-13T22:34:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 13, 2023 at 05:02:54PM -0500, Jeff King wrote:\n> On Fri, Nov 10, 2023 at 03:51:18PM -0800, Elijah Newren wrote:\n>\n> > This is unsafe; the object may need to be read later within the same\n> > merge. [...]\n> >\n> > I think (untested) that you could fix this by creating two packs\n> > instead of just one. In particular, add a call to\n> > flush_odb_transcation() after the \"redo_after_renames\" block in\n> > merge_ort_nonrecursive_internal().\n>\n> It might not be too hard to just let in-process callers access the\n> objects we've written. A quick and dirty patch is below, which works\n> with the test you suggested (the test still fails because it finds a\n> conflict, but it gets past the \"woah, I can't find that sha1\" part).\n\nThat's a very slick idea, and I think that this series has some legs to\nstand on now as a result.\n\nThere are a few of interesting conclusions we can draw here:\n\n 1. This solves the recursive merge problem that Elijah mentioned\n earlier where we could generate up to 2^N packs (where N is the\n maximum depth of the recursive merge).\n\n 2. This also solves the case where the merge-ort code needs to read an\n object that it wrote earlier on in the same process without having\n to flush out intermediate packs. So in the best case we need only a\n single pack (instead of two).\n\n 3. This also solves the 'replay' issue, *and* allows us to avoid the\n tmp-objdir thing there entirely, since we can simulate object reads\n in the bulk-checkin pack.\n\nSo I think that this is a direction worth pursuing.\n\nIn terms of making those lookups faster, I think that what you'd want is\nan oidmap. The overhead is slightly unfortunate, but I think that any\nother solution which requires keeping the written array in sorted order\nwould in practice be more expensive as you have to constantly reallocate\nand copy portions of the array around to maintain its invariant.\n\n> I don't know if that is sufficient, though. Would other spawned\n> processes (hooks, external merge drivers, and so on) need to be able to\n> see these objects, too?\n\nInteresting point. In theory those processes could ask about newly\ncreated objects, and if they were spawned before the bulk-checkin pack\nwas flushed, those lookups would fail.\n\nBut that requires that, e.g. for hooks, that we know a-priori the object\nID of some newly-written objects. If you wanted to make those lookups\nsucceed, I think there are a couple of options:\n\n - teach child-processes about the bulk-checkin pack, and let them\n perform the same fake lookup\n\n - flush (but do not close) the bulk-checkin transaction\n\nIn any event, I think that this is a sufficiently rare and niche case\nthat we'd be OK to declare that you should not expect the above\nscenarios to work when using `--write-pack`. If someone does ask for\nthat feature in the future, we could implement it relatively painlessly\nusing one of the above options.\n\nThanks,\nTaylor\n"},{"id":"484836","messageId":"xmqqpm0d1591.fsf@gitster.g","threadId":"60403","inReplyTo":"20231113220546.GB2065691@coredump.intra.peff.net","subject":"Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-14T01:40:58Z","receivedAt":"2023-11-14T01:41:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I posted an alternative in response to Elijah; the general idea being to\n> allow the usual object-lookup code to access the in-progress pack. That\n> would keep us limited to a single pack.\n\nIf such a mechanism is done in a generic way, would we be able to\nsimplify fast-import a lot, I wonder? IIRC, it had quite a lot of\ncode to remember what it has written to its output to work around\nthe exact issue your alternative tries to solve. In fact, maybe we\ncould make fast-import a thin wrapper around the bulk checkin\ninfrastructure?\n\n"},{"id":"484840","messageId":"CABPp-BEV8Mxu=4=TFN=0o9n+o69kKQfNZd_Rhr1swxqgKwd90g@mail.gmail.com","threadId":"60403","inReplyTo":"ZVKkgpiFaOwwDcdw@nand.local","subject":"Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-11-14T02:50:08Z","receivedAt":"2023-11-14T02:50:25Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Nov 13, 2023 at 2:34 PM Taylor Blau wrote:\n>\n> On Mon, Nov 13, 2023 at 05:02:54PM -0500, Jeff King wrote:\n> > On Fri, Nov 10, 2023 at 03:51:18PM -0800, Elijah Newren wrote:\n> >\n> > > This is unsafe; the object may need to be read later within the same\n> > > merge. [...]\n> > >\n> > > I think (untested) that you could fix this by creating two packs\n> > > instead of just one. In particular, add a call to\n> > > flush_odb_transcation() after the \"redo_after_renames\" block in\n> > > merge_ort_nonrecursive_internal().\n> >\n> > It might not be too hard to just let in-process callers access the\n> > objects we've written. A quick and dirty patch is below, which works\n> > with the test you suggested (the test still fails because it finds a\n> > conflict, but it gets past the \"woah, I can't find that sha1\" part).\n>\n> That's a very slick idea, and I think that this series has some legs to\n> stand on now as a result.\n>\n> There are a few of interesting conclusions we can draw here:\n>\n> 1. This solves the recursive merge problem that Elijah mentioned\n> earlier where we could generate up to 2^N packs (where N is the\n> maximum depth of the recursive merge).\n>\n> 2. This also solves the case where the merge-ort code needs to read an\n> object that it wrote earlier on in the same process without having\n> to flush out intermediate packs. So in the best case we need only a\n> single pack (instead of two).\n>\n> 3. This also solves the 'replay' issue, *and* allows us to avoid the\n> tmp-objdir thing there entirely, since we can simulate object reads\n> in the bulk-checkin pack.\n>\n> So I think that this is a direction worth pursuing.\n\nAgreed; this looks great. It's basically bringing fast-import-like\nfunctionality -- writing objects to a single new packfile while making\nprevious objects accessible to subsequent ones -- to additional\ncallers.\n\n> In terms of making those lookups faster, I think that what you'd want is\n> an oidmap. The overhead is slightly unfortunate, but I think that any\n> other solution which requires keeping the written array in sorted order\n> would in practice be more expensive as you have to constantly reallocate\n> and copy portions of the array around to maintain its invariant.\n\nWhen comparing the overhead of an oidmap to a bunch of inodes,\nhowever, it seems relatively cheap. :-)\n\n> > I don't know if that is sufficient, though. Would other spawned\n> > processes (hooks, external merge drivers, and so on) need to be able to\n> > see these objects, too?\n>\n> Interesting point. In theory those processes could ask about newly\n> created objects, and if they were spawned before the bulk-checkin pack\n> was flushed, those lookups would fail.\n\nOne of the big design differences that I was pushing really hard with\ngit-replay was performance and things that came with it -- no\nworktree, no per-commit hooks (which are nearly ruled out by no\nworktree, but it's still worth calling out separately), etc. A\npost-operation hook could be fine, but would also not get to assume a\nworktree.\n\nmerge-tree is the same as far as hooks; I'd rather just not have them,\nbut if we did, they'd be a post-operation hook.\n\nIn both cases, that makes hooks not much of a sticking point.\n\nExternal merge drivers, however...\n\n> But that requires that, e.g. for hooks, that we know a-priori the object\n> ID of some newly-written objects. If you wanted to make those lookups\n> succeed, I think there are a couple of options:\n>\n> - teach child-processes about the bulk-checkin pack, and let them\n> perform the same fake lookup\n>\n> - flush (but do not close) the bulk-checkin transaction\n>\n> In any event, I think that this is a sufficiently rare and niche case\n> that we'd be OK to declare that you should not expect the above\n> scenarios to work when using `--write-pack`. If someone does ask for\n> that feature in the future, we could implement it relatively painlessly\n> using one of the above options.\n\nSeems reasonable to me.\n"},{"id":"484842","messageId":"CABPp-BGukcQVkzOOzuGLgcGSctrdzydMV5FSXn=gmUe9RQE=Hw@mail.gmail.com","threadId":"60403","inReplyTo":"xmqqpm0d1591.fsf@gitster.g","subject":"Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-11-14T02:54:33Z","receivedAt":"2023-11-14T02:54:50Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Nov 13, 2023 at 5:41 PM Junio C Hamano wrote:\n>\n> Jeff King writes:\n>\n> > I posted an alternative in response to Elijah; the general idea being to\n> > allow the usual object-lookup code to access the in-progress pack. That\n> > would keep us limited to a single pack.\n>\n> If such a mechanism is done in a generic way, would we be able to\n> simplify fast-import a lot, I wonder? IIRC, it had quite a lot of\n> code to remember what it has written to its output to work around\n> the exact issue your alternative tries to solve. In fact, maybe we\n> could make fast-import a thin wrapper around the bulk checkin\n> infrastructure?\n\nfast-import also attempts to delta objects against previous ones as it\nwrites them to the pack. That's one thing still lacking from this\nsolution, but aside from that, it also sounds to me like the bulk\ncheckin infrastructure is getting closer to becoming a replacement for\nmuch of the fast-import code.\n"},{"id":"484844","messageId":"CABPp-BEuqpVUmQZ=zD=EYwBYLKY19pLix04eKsMtZkGu=8OZUA@mail.gmail.com","threadId":"60403","inReplyTo":"20231113220546.GB2065691@coredump.intra.peff.net","subject":"Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-11-14T03:08:52Z","receivedAt":"2023-11-14T03:09:09Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Nov 13, 2023 at 2:05 PM Jeff King wrote:\n>\n> On Fri, Nov 10, 2023 at 08:24:44PM -0500, Taylor Blau wrote:\n>\n> > > This does mean that for a recursive merge, that you'll get up to 2*N\n> > > packfiles, where N is the depth of the recursive merge.\n> >\n> > We definitely want to avoid that ;-). I think there are a couple of\n> > potential directions forward here, but the most promising one I think is\n> > due to Johannes who suggests that we write loose objects into a\n> > temporary directory with a replace_tmp_objdir() call, and then repack\n> > that side directory before migrating a single pack back into the main\n> > object store.\n>\n> I posted an alternative in response to Elijah; the general idea being to\n> allow the usual object-lookup code to access the in-progress pack. That\n> would keep us limited to a single pack.\n>\n> It _might_ be a terrible idea. E.g., if you write a non-bulk object that\n> references a bulk one, then that non-bulk one is broken from the\n> perspective of other processes (until the bulk checkin is flushed). But\n> I think we'd always be writing to one or the other here, never\n> interleaving?\n\nI think it sounds like a really cool idea, personally. I also don't\nsee why any current uses would result in interleaving.\n\nfast-import's created pack has the same kind of restriction...and has\neven grown some extensions to work around the need for early access.\nIn particular, it has a `checkpoint` feature for flushing the current\npack and starting a new one, and also gained the `cat-blob` command\nfor when folks really wanted to access an object without writing out\nthe whole packfile. So, _if_ the need for early access arises, we\nhave some prior art to look to for ways to provide it.\n"},{"id":"484895","messageId":"20231114215331.GA12791@coredump.intra.peff.net","threadId":"60403","inReplyTo":"CABPp-BEV8Mxu=4=TFN=0o9n+o69kKQfNZd_Rhr1swxqgKwd90g@mail.gmail.com","subject":"Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-14T21:53:31Z","receivedAt":"2023-11-14T21:53:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 13, 2023 at 06:50:08PM -0800, Elijah Newren wrote:\n\n> merge-tree is the same as far as hooks; I'd rather just not have them,\n> but if we did, they'd be a post-operation hook.\n> \n> In both cases, that makes hooks not much of a sticking point.\n> \n> External merge drivers, however...\n\nI suspect external merge drivers are OK, in the sense that they should\nnot be looking up arbitrary objects anyway. We should hand the driver\nwhat it needs via tempfiles, etc.\n\nThat said, I'm also OK with the notion that \"--write-pack\" is optional,\nand if some features don't work with it, that's OK. It's nice if we can\nflag that explicitly (rather than having things break in weird and\nsubtle ways), but I think we might not even know about an implicit\nassumption until somebody runs across it in practice.\n\n-Peff\n"},{"id":"484896","messageId":"20231114215524.GB12791@coredump.intra.peff.net","threadId":"60403","inReplyTo":"xmqqpm0d1591.fsf@gitster.g","subject":"Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-14T21:55:24Z","receivedAt":"2023-11-14T21:55:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 14, 2023 at 10:40:58AM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I posted an alternative in response to Elijah; the general idea being to\n> > allow the usual object-lookup code to access the in-progress pack. That\n> > would keep us limited to a single pack.\n> \n> If such a mechanism is done in a generic way, would we be able to\n> simplify fast-import a lot, I wonder? IIRC, it had quite a lot of\n> code to remember what it has written to its output to work around\n> the exact issue your alternative tries to solve. In fact, maybe we\n> could make fast-import a thin wrapper around the bulk checkin\n> infrastructure?\n\nI suspect that the implementation could be shared with fast-import. I'm\nnot sure it would save all that much code, though. There's a lot going\non in fast-import besides keeping track of which objects we wrote into a\npack. ;)\n\nThe bigger issue, though, is that fast-import does generate some deltas\nand the bulk checkin code does not. And I'm not sure how the bulk\ncheckin interface would expose that API (you need the caller to say\n\"...and I suspect this object might be a good delta base\").\n\n-Peff\n"},{"id":"484897","messageId":"20231114220434.GC12791@coredump.intra.peff.net","threadId":"60403","inReplyTo":"ZVKkgpiFaOwwDcdw@nand.local","subject":"Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-14T22:04:34Z","receivedAt":"2023-11-14T22:04:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 13, 2023 at 05:34:42PM -0500, Taylor Blau wrote:\n\n> > It might not be too hard to just let in-process callers access the\n> > objects we've written. A quick and dirty patch is below, which works\n> > with the test you suggested (the test still fails because it finds a\n> > conflict, but it gets past the \"woah, I can't find that sha1\" part).\n> \n> That's a very slick idea, and I think that this series has some legs to\n> stand on now as a result.\n\nI'm glad people seem to like it. ;)\n\nI was mostly throwing it out there as an illustration, and I don't plan\non polishing it up further. But in case somebody else wants to work on\nit, here are random extra thoughts on the topic:\n\n - rather than teach packfile.c about bulk checkin, I think it might be\n cleaner to let packed_git structs have a function pointer for\n accessing the index (and if NULL, naturally we'd open the .idx file\n in the usual way). And then bulk checkin could just register the\n \"fake\" packed_git\n\n - the flip side, though, is: would it be weird for other parts of the\n code to ever see the fake bulk packed_git in the list? It doesn't\n represent a real packfile the way the other ones do. So maybe it is\n better to keep it separate\n\n - I suspect this scheme may violate some bounds-checking of the\n packfiles. For example, we haven't written the hashfile trailer yet in\n the still-open packfile. But I think use_pack() assumes we always\n have an extra the_hash_algo->rawsz bytes of padding at the end. I'm\n not sure if that would ever cause us to accidentally read past the\n end of the file.\n\n - as you mentioned, an oidmap would be an obvious replacement for the\n existing unsorted list\n\n - the existing already_written() function faces the same problem. I\n don't think anybody cared because we'd usually only bulk checkin a\n few huge objects. But with --write-pack, we might write a lot of\n such objects, and the linear scan in already_written() makes it\n accidentally quadratic.\n\n - speaking of already_written(): it calls repo_has_object_file() to\n see if we can elide the write. It probably should be using\n freshen_*_object() in the usual way. Again, this is an existing bug\n but I suspect nobody noticed because the bulk checkin code path\n hardly ever kicks in.\n\n-Peff\n"}]}