{"thread":{"id":"54753","subject":"[PATCH 00/15] Refactor chunk-format into an API","startedAt":"2020-12-03T16:17:55Z","lastAt":"2020-12-10T18:56:45Z","messageCount":40,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","René Scharfe","Derrick Stolee","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":15},"messages":[{"id":"411238","messageId":"pull.804.git.1607012215.gitgitgadget@gmail.com","threadId":"54753","inReplyTo":null,"subject":"[PATCH 00/15] Refactor chunk-format into an API","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T16:16:39Z","receivedAt":"2020-12-03T16:17:55Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"I was thinking about file formats recently and realized that the \"chunks\"\nthat are common to the commit-graph and multi-pack-index could inform future\nfile formats. To make that process easier, let's combine the process of\nwriting and reading chunks into a common API that both of these existing\nformats use.\n\nThere is some extra benefit immediately: the writing and reading code for\neach gets a bit cleaner. Also, there were different checks in each that made\nthe process more robust. Now, these share a common set of checks.\n\nIn particular, Szeder made some updates to the commit-graph writing process\nthat forms the model for this API.\n\nThanks, -Stolee\n\nDerrick Stolee (15):\n commit-graph: anonymize data in chunk_write_fn\n chunk-format: add API for writing table of contents\n midx: rename pack_info to write_midx_context\n midx: use context in write_midx_pack_names()\n midx: add entries to write_midx_context\n midx: add pack_perm to write_midx_context\n midx: add num_large_offsets to write_midx_context\n midx: convert chunk write methods to return int\n midx: drop chunk progress during write\n midx: use chunk-format API in write_midx_internal()\n midx: use 64-bit multiplication for chunk sizes\n chunk-format: create write_chunks()\n chunk-format: create chunk reading API\n commit-graph: restore duplicate chunk checks\n chunk-format: add technical docs\n\n Documentation/technical/chunk-format.txt | 54 ++\n .../technical/commit-graph-format.txt | 3 +\n Documentation/technical/pack-format.txt | 3 +\n Makefile | 1 +\n chunk-format.c | 105 ++++\n chunk-format.h | 69 +++\n commit-graph.c | 298 ++++++-----\n midx.c | 466 ++++++++----------\n t/t5318-commit-graph.sh | 2 +-\n t/t5319-multi-pack-index.sh | 6 +-\n 10 files changed, 623 insertions(+), 384 deletions(-)\n create mode 100644 Documentation/technical/chunk-format.txt\n create mode 100644 chunk-format.c\n create mode 100644 chunk-format.h\n\n\nbase-commit: 72ffeb997eaf999f6938b2a7e0d9a75dcceaa311\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-804%2Fderrickstolee%2Fchunk-format%2Frefactor-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-804/derrickstolee/chunk-format/refactor-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/804\n-- \ngitgitgadget\n"},{"id":"411239","messageId":"da7933cc59928c7995821dcc72ceae0c690ebaca.1607012215.git.gitgitgadget@gmail.com","threadId":"54753","inReplyTo":"pull.804.git.1607012215.gitgitgadget@gmail.com","subject":"[PATCH 02/15] chunk-format: add API for writing table of contents","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T16:16:41Z","receivedAt":"2020-12-03T16:17:55Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe commit-graph and multi-pack-index formats share a concept of\n\"chunks\" that are described by a table of contents near the beginning of\nthe file.\n\nThe table of contents consists of rows of 12 bytes. Each row starts with\na 4-byte ID that signals the type of data stored in the chunk. The row\nthen continues with an 8-byte offset describing the position in the file\nwhere that data starts. The table of contents lists the chunks in\nposition order so the length of a chunk can be determined by subtracting\nits start position from the start position of the next chunk.\n\nThe table of contents always ends with ID 0x0000 to assist finding the\nend of the last \"real\" chunk. Typically, this points to the trailing\nhash of a file.\n\nConvert the chunk-writing loop in commit-graph.c to use the new\nwrite_table_of_contents() method in chunk-format.c.\n\nThe most subtle part of this conversion is the use of 'cur_offset' to\nallow the caller to specify how many bytes were written in the file's\nheader before the table of contents. This may differ between formats.\n\nSigned-off-by: Derrick Stolee \n---\n Makefile | 1 +\n chunk-format.c | 26 ++++++++++++++++++++++++++\n chunk-format.h | 36 ++++++++++++++++++++++++++++++++++++\n commit-graph.c | 23 ++---------------------\n 4 files changed, 65 insertions(+), 21 deletions(-)\n create mode 100644 chunk-format.c\n create mode 100644 chunk-format.h\n\ndiff --git a/Makefile b/Makefile\nindex d3a531d3c6..cdbcadac14 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -854,6 +854,7 @@ LIB_OBJS += bundle.o\n LIB_OBJS += cache-tree.o\n LIB_OBJS += chdir-notify.o\n LIB_OBJS += checkout.o\n+LIB_OBJS += chunk-format.o\n LIB_OBJS += color.o\n LIB_OBJS += column.o\n LIB_OBJS += combine-diff.o\ndiff --git a/chunk-format.c b/chunk-format.c\nnew file mode 100644\nindex 0000000000..771b6d98d0\n--- /dev/null\n+++ b/chunk-format.c\n@@ -0,0 +1,26 @@\n+#include \"git-compat-util.h\"\n+#include \"chunk-format.h\"\n+#include \"csum-file.h\"\n+#define CHUNK_LOOKUP_WIDTH 12\n+\n+void write_table_of_contents(struct hashfile *f,\n+\t\t\t uint64_t cur_offset,\n+\t\t\t struct chunk_info *chunks,\n+\t\t\t int nr)\n+{\n+\tint i;\n+\n+\t/* Add the table of contents to the current offset */\n+\tcur_offset += (nr + 1) * CHUNK_LOOKUP_WIDTH;\n+\n+\tfor (i = 0; i < nr; i++) {\n+\t\thashwrite_be32(f, chunks[i].id);\n+\t\thashwrite_be64(f, cur_offset);\n+\n+\t\tcur_offset += chunks[i].size;\n+\t}\n+\n+\t/* Trailing entry marks the end of the chunks */\n+\thashwrite_be32(f, 0);\n+\thashwrite_be64(f, cur_offset);\n+}\ndiff --git a/chunk-format.h b/chunk-format.h\nnew file mode 100644\nindex 0000000000..4b9cbeb372\n--- /dev/null\n+++ b/chunk-format.h\n@@ -0,0 +1,36 @@\n+#ifndef CHUNK_FORMAT_H\n+#define CHUNK_FORMAT_H\n+\n+#include \"git-compat-util.h\"\n+\n+struct hashfile;\n+\n+typedef int (*chunk_write_fn)(struct hashfile *f,\n+\t\t\t void *data);\n+\n+/*\n+ * When writing a chunk-based file format, collect the chunks in\n+ * an array of chunk_info structs. The size stores the _expected_\n+ * amount of data that will be written by write_fn.\n+ */\n+struct chunk_info {\n+\tuint32_t id;\n+\tuint64_t size;\n+\tchunk_write_fn write_fn;\n+};\n+\n+/*\n+ * Write the chunk data into the supplied hashfile.\n+ *\n+ * * 'cur_offset' indicates the number of bytes written to the hashfile\n+ * before the table of contents starts.\n+ *\n+ * * 'nr' is the number of chunks with non-zero IDs, so 'nr + 1'\n+ * chunks are written in total.\n+ */\n+void write_table_of_contents(struct hashfile *f,\n+\t\t\t uint64_t cur_offset,\n+\t\t\t struct chunk_info *chunks,\n+\t\t\t int nr);\n+\n+#endif\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 6b5bb8b6b8..5494fda1d3 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -19,6 +19,7 @@\n #include \"shallow.h\"\n #include \"json-writer.h\"\n #include \"trace2.h\"\n+#include \"chunk-format.h\"\n \n void git_test_write_commit_graph_or_die(void)\n {\n@@ -1696,15 +1697,6 @@ static int write_graph_chunk_base(struct hashfile *f,\n \treturn 0;\n }\n \n-typedef int (*chunk_write_fn)(struct hashfile *f,\n-\t\t\t void *data);\n-\n-struct chunk_info {\n-\tuint32_t id;\n-\tuint64_t size;\n-\tchunk_write_fn write_fn;\n-};\n-\n static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n {\n \tuint32_t i;\n@@ -1715,7 +1707,6 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \tconst unsigned hashsz = the_hash_algo->rawsz;\n \tstruct strbuf progress_title = STRBUF_INIT;\n \tint num_chunks = 3;\n-\tuint64_t chunk_offset;\n \tstruct object_id file_hash;\n \n \tif (ctx->split) {\n@@ -1805,17 +1796,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \thashwrite_u8(f, num_chunks);\n \thashwrite_u8(f, ctx->num_commit_graphs_after - 1);\n \n-\tchunk_offset = 8 + (num_chunks + 1) * GRAPH_CHUNKLOOKUP_WIDTH;\n-\tfor (i = 0; i <= num_chunks; i++) {\n-\t\tuint32_t chunk_write[3];\n-\n-\t\tchunk_write[0] = htonl(chunks[i].id);\n-\t\tchunk_write[1] = htonl(chunk_offset >> 32);\n-\t\tchunk_write[2] = htonl(chunk_offset & 0xffffffff);\n-\t\thashwrite(f, chunk_write, 12);\n-\n-\t\tchunk_offset += chunks[i].size;\n-\t}\n+\twrite_table_of_contents(f, /* cur_offset */ 8, chunks, num_chunks);\n \n \tif (ctx->report_progress) {\n \t\tstrbuf_addf(&progress_title,\n-- \ngitgitgadget\n\n"},{"id":"411241","messageId":"191b0afba825fceb2721effeb6783961bf42b59e.1607012215.git.gitgitgadget@gmail.com","threadId":"54753","inReplyTo":"pull.804.git.1607012215.gitgitgadget@gmail.com","subject":"[PATCH 01/15] commit-graph: anonymize data in chunk_write_fn","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T16:16:40Z","receivedAt":"2020-12-03T16:17:55Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nIn preparation for creating an API around file formats using chunks and\ntables of contents, prepare the commit-graph write code to use\nprototypes that will match this new API.\n\nSpecifically, convert chunk_write_fn to take a \"void *data\" parameter\ninstead of the commit-graph-specific \"struct write_commit_graph_context\"\npointer.\n\nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 30 ++++++++++++++++++++++--------\n 1 file changed, 22 insertions(+), 8 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 6f62a07313..6b5bb8b6b8 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -980,8 +980,10 @@ struct write_commit_graph_context {\n };\n \n static int write_graph_chunk_fanout(struct hashfile *f,\n-\t\t\t\t struct write_commit_graph_context *ctx)\n+\t\t\t\t void *data)\n {\n+\tstruct write_commit_graph_context *ctx =\n+\t\t(struct write_commit_graph_context *)data;\n \tint i, count = 0;\n \tstruct commit **list = ctx->commits.list;\n \n@@ -1006,8 +1008,10 @@ static int write_graph_chunk_fanout(struct hashfile *f,\n }\n \n static int write_graph_chunk_oids(struct hashfile *f,\n-\t\t\t\t struct write_commit_graph_context *ctx)\n+\t\t\t\t void *data)\n {\n+\tstruct write_commit_graph_context *ctx =\n+\t\t(struct write_commit_graph_context *)data;\n \tstruct commit **list = ctx->commits.list;\n \tint count;\n \tfor (count = 0; count < ctx->commits.nr; count++, list++) {\n@@ -1025,8 +1029,10 @@ static const unsigned char *commit_to_sha1(size_t index, void *table)\n }\n \n static int write_graph_chunk_data(struct hashfile *f,\n-\t\t\t\t struct write_commit_graph_context *ctx)\n+\t\t\t\t void *data)\n {\n+\tstruct write_commit_graph_context *ctx =\n+\t\t(struct write_commit_graph_context *)data;\n \tstruct commit **list = ctx->commits.list;\n \tstruct commit **last = ctx->commits.list + ctx->commits.nr;\n \tuint32_t num_extra_edges = 0;\n@@ -1127,8 +1133,10 @@ static int write_graph_chunk_data(struct hashfile *f,\n }\n \n static int write_graph_chunk_extra_edges(struct hashfile *f,\n-\t\t\t\t\t struct write_commit_graph_context *ctx)\n+\t\t\t\t\t void *data)\n {\n+\tstruct write_commit_graph_context *ctx =\n+\t\t(struct write_commit_graph_context *)data;\n \tstruct commit **list = ctx->commits.list;\n \tstruct commit **last = ctx->commits.list + ctx->commits.nr;\n \tstruct commit_list *parent;\n@@ -1181,8 +1189,10 @@ static int write_graph_chunk_extra_edges(struct hashfile *f,\n }\n \n static int write_graph_chunk_bloom_indexes(struct hashfile *f,\n-\t\t\t\t\t struct write_commit_graph_context *ctx)\n+\t\t\t\t\t void *data)\n {\n+\tstruct write_commit_graph_context *ctx =\n+\t\t(struct write_commit_graph_context *)data;\n \tstruct commit **list = ctx->commits.list;\n \tstruct commit **last = ctx->commits.list + ctx->commits.nr;\n \tuint32_t cur_pos = 0;\n@@ -1216,8 +1226,10 @@ static void trace2_bloom_filter_settings(struct write_commit_graph_context *ctx)\n }\n \n static int write_graph_chunk_bloom_data(struct hashfile *f,\n-\t\t\t\t\tstruct write_commit_graph_context *ctx)\n+\t\t\t\t\tvoid *data)\n {\n+\tstruct write_commit_graph_context *ctx =\n+\t\t(struct write_commit_graph_context *)data;\n \tstruct commit **list = ctx->commits.list;\n \tstruct commit **last = ctx->commits.list + ctx->commits.nr;\n \n@@ -1670,8 +1682,10 @@ static int write_graph_chunk_base_1(struct hashfile *f,\n }\n \n static int write_graph_chunk_base(struct hashfile *f,\n-\t\t\t\t struct write_commit_graph_context *ctx)\n+\t\t\t\t void *data)\n {\n+\tstruct write_commit_graph_context *ctx =\n+\t\t(struct write_commit_graph_context *)data;\n \tint num = write_graph_chunk_base_1(f, ctx->new_base_graph);\n \n \tif (num != ctx->num_commit_graphs_after - 1) {\n@@ -1683,7 +1697,7 @@ static int write_graph_chunk_base(struct hashfile *f,\n }\n \n typedef int (*chunk_write_fn)(struct hashfile *f,\n-\t\t\t struct write_commit_graph_context *ctx);\n+\t\t\t void *data);\n \n struct chunk_info {\n \tuint32_t id;\n-- \ngitgitgadget\n\n"},{"id":"411240","messageId":"f75c34375acb254de90044dc6dcd7ab04f92c72a.1607012215.git.gitgitgadget@gmail.com","threadId":"54753","inReplyTo":"pull.804.git.1607012215.gitgitgadget@gmail.com","subject":"[PATCH 04/15] midx: use context in write_midx_pack_names()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T16:16:43Z","receivedAt":"2020-12-03T16:17:56Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nIn an effort to align the write_midx_internal() to use the chunk-format\nAPI, start converting chunk writing methods to match chunk_write_fn. The\nfirst case is to convert write_midx_pack_names() to take \"void *data\".\nWe already have the necessary data in \"struct write_midx_context\", so\nthis conversion is rather mechanical.\n\nSigned-off-by: Derrick Stolee \n---\n midx.c | 21 ++++++++++-----------\n 1 file changed, 10 insertions(+), 11 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex ded4d394bb..6ab655ddda 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -643,27 +643,26 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \treturn deduplicated_entries;\n }\n \n-static size_t write_midx_pack_names(struct hashfile *f,\n-\t\t\t\t struct pack_info *info,\n-\t\t\t\t uint32_t num_packs)\n+static size_t write_midx_pack_names(struct hashfile *f, void *data)\n {\n+\tstruct write_midx_context *ctx = (struct write_midx_context *)data;\n \tuint32_t i;\n \tunsigned char padding[MIDX_CHUNK_ALIGNMENT];\n \tsize_t written = 0;\n \n-\tfor (i = 0; i < num_packs; i++) {\n+\tfor (i = 0; i < ctx->nr; i++) {\n \t\tsize_t writelen;\n \n-\t\tif (info[i].expired)\n+\t\tif (ctx->info[i].expired)\n \t\t\tcontinue;\n \n-\t\tif (i && strcmp(info[i].pack_name, info[i - 1].pack_name) <= 0)\n+\t\tif (i && strcmp(ctx->info[i].pack_name, ctx->info[i - 1].pack_name) <= 0)\n \t\t\tBUG(\"incorrect pack-file order: %s before %s\",\n-\t\t\t info[i - 1].pack_name,\n-\t\t\t info[i].pack_name);\n+\t\t\t ctx->info[i - 1].pack_name,\n+\t\t\t ctx->info[i].pack_name);\n \n-\t\twritelen = strlen(info[i].pack_name) + 1;\n-\t\thashwrite(f, info[i].pack_name, writelen);\n+\t\twritelen = strlen(ctx->info[i].pack_name) + 1;\n+\t\thashwrite(f, ctx->info[i].pack_name, writelen);\n \t\twritten += writelen;\n \t}\n \n@@ -990,7 +989,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \n \t\tswitch (chunk_ids[i]) {\n \t\t\tcase MIDX_CHUNKID_PACKNAMES:\n-\t\t\t\twritten += write_midx_pack_names(f, ctx.info, ctx.nr);\n+\t\t\t\twritten += write_midx_pack_names(f, &ctx);\n \t\t\t\tbreak;\n \n \t\t\tcase MIDX_CHUNKID_OIDFANOUT:\n-- \ngitgitgadget\n\n"},{"id":"411242","messageId":"5a882fc773d7ab07e0ee71b5f588cc8c68d8b5cd.1607012215.git.gitgitgadget@gmail.com","threadId":"54753","inReplyTo":"pull.804.git.1607012215.gitgitgadget@gmail.com","subject":"[PATCH 03/15] midx: rename pack_info to write_midx_context","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T16:16:42Z","receivedAt":"2020-12-03T16:17:56Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nIn an effort to streamline our chunk-based file formats, align some of\nthe code structure in write_midx_internal() to be similar to the\npatterns in write_commit_graph_file().\n\nSpecifically, let's create a \"struct write_midx_context\" that can be\nused as a data parameter to abstract function types.\n\nThis change only renames \"struct pack_info\" to \"struct\nwrite_midx_context\" and the names of instances from \"packs\" to \"ctx\". In\nfuture changes, we will expand the data inside \"struct\nwrite_midx_context\" and align our chunk-writing method with the\nchunk-format API.\n\nSigned-off-by: Derrick Stolee \n---\n midx.c | 130 ++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 65 insertions(+), 65 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex da03c1449a..ded4d394bb 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -451,7 +451,7 @@ static int pack_info_compare(const void *_a, const void *_b)\n \treturn strcmp(a->pack_name, b->pack_name);\n }\n \n-struct pack_list {\n+struct write_midx_context {\n \tstruct pack_info *info;\n \tuint32_t nr;\n \tuint32_t alloc;\n@@ -463,37 +463,37 @@ struct pack_list {\n static void add_pack_to_midx(const char *full_path, size_t full_path_len,\n \t\t\t const char *file_name, void *data)\n {\n-\tstruct pack_list *packs = (struct pack_list *)data;\n+\tstruct write_midx_context *ctx = (struct write_midx_context *)data;\n \n \tif (ends_with(file_name, \".idx\")) {\n-\t\tdisplay_progress(packs->progress, ++packs->pack_paths_checked);\n-\t\tif (packs->m && midx_contains_pack(packs->m, file_name))\n+\t\tdisplay_progress(ctx->progress, ++ctx->pack_paths_checked);\n+\t\tif (ctx->m && midx_contains_pack(ctx->m, file_name))\n \t\t\treturn;\n \n-\t\tALLOC_GROW(packs->info, packs->nr + 1, packs->alloc);\n+\t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n \n-\t\tpacks->info[packs->nr].p = add_packed_git(full_path,\n-\t\t\t\t\t\t\t full_path_len,\n-\t\t\t\t\t\t\t 0);\n+\t\tctx->info[ctx->nr].p = add_packed_git(full_path,\n+\t\t\t\t\t\t full_path_len,\n+\t\t\t\t\t\t 0);\n \n-\t\tif (!packs->info[packs->nr].p) {\n+\t\tif (!ctx->info[ctx->nr].p) {\n \t\t\twarning(_(\"failed to add packfile '%s'\"),\n \t\t\t\tfull_path);\n \t\t\treturn;\n \t\t}\n \n-\t\tif (open_pack_index(packs->info[packs->nr].p)) {\n+\t\tif (open_pack_index(ctx->info[ctx->nr].p)) {\n \t\t\twarning(_(\"failed to open pack-index '%s'\"),\n \t\t\t\tfull_path);\n-\t\t\tclose_pack(packs->info[packs->nr].p);\n-\t\t\tFREE_AND_NULL(packs->info[packs->nr].p);\n+\t\t\tclose_pack(ctx->info[ctx->nr].p);\n+\t\t\tFREE_AND_NULL(ctx->info[ctx->nr].p);\n \t\t\treturn;\n \t\t}\n \n-\t\tpacks->info[packs->nr].pack_name = xstrdup(file_name);\n-\t\tpacks->info[packs->nr].orig_pack_int_id = packs->nr;\n-\t\tpacks->info[packs->nr].expired = 0;\n-\t\tpacks->nr++;\n+\t\tctx->info[ctx->nr].pack_name = xstrdup(file_name);\n+\t\tctx->info[ctx->nr].orig_pack_int_id = ctx->nr;\n+\t\tctx->info[ctx->nr].expired = 0;\n+\t\tctx->nr++;\n \t}\n }\n \n@@ -801,7 +801,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \tuint32_t i;\n \tstruct hashfile *f = NULL;\n \tstruct lock_file lk;\n-\tstruct pack_list packs;\n+\tstruct write_midx_context ctx = { 0 };\n \tuint32_t *pack_perm = NULL;\n \tuint64_t written = 0;\n \tuint32_t chunk_ids[MIDX_MAX_CHUNKS + 1];\n@@ -820,40 +820,40 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \t\t\t midx_name);\n \n \tif (m)\n-\t\tpacks.m = m;\n+\t\tctx.m = m;\n \telse\n-\t\tpacks.m = load_multi_pack_index(object_dir, 1);\n-\n-\tpacks.nr = 0;\n-\tpacks.alloc = packs.m ? packs.m->num_packs : 16;\n-\tpacks.info = NULL;\n-\tALLOC_ARRAY(packs.info, packs.alloc);\n-\n-\tif (packs.m) {\n-\t\tfor (i = 0; i < packs.m->num_packs; i++) {\n-\t\t\tALLOC_GROW(packs.info, packs.nr + 1, packs.alloc);\n-\n-\t\t\tpacks.info[packs.nr].orig_pack_int_id = i;\n-\t\t\tpacks.info[packs.nr].pack_name = xstrdup(packs.m->pack_names[i]);\n-\t\t\tpacks.info[packs.nr].p = NULL;\n-\t\t\tpacks.info[packs.nr].expired = 0;\n-\t\t\tpacks.nr++;\n+\t\tctx.m = load_multi_pack_index(object_dir, 1);\n+\n+\tctx.nr = 0;\n+\tctx.alloc = ctx.m ? ctx.m->num_packs : 16;\n+\tctx.info = NULL;\n+\tALLOC_ARRAY(ctx.info, ctx.alloc);\n+\n+\tif (ctx.m) {\n+\t\tfor (i = 0; i < ctx.m->num_packs; i++) {\n+\t\t\tALLOC_GROW(ctx.info, ctx.nr + 1, ctx.alloc);\n+\n+\t\t\tctx.info[ctx.nr].orig_pack_int_id = i;\n+\t\t\tctx.info[ctx.nr].pack_name = xstrdup(ctx.m->pack_names[i]);\n+\t\t\tctx.info[ctx.nr].p = NULL;\n+\t\t\tctx.info[ctx.nr].expired = 0;\n+\t\t\tctx.nr++;\n \t\t}\n \t}\n \n-\tpacks.pack_paths_checked = 0;\n+\tctx.pack_paths_checked = 0;\n \tif (flags & MIDX_PROGRESS)\n-\t\tpacks.progress = start_delayed_progress(_(\"Adding packfiles to multi-pack-index\"), 0);\n+\t\tctx.progress = start_delayed_progress(_(\"Adding packfiles to multi-pack-index\"), 0);\n \telse\n-\t\tpacks.progress = NULL;\n+\t\tctx.progress = NULL;\n \n-\tfor_each_file_in_pack_dir(object_dir, add_pack_to_midx, &packs);\n-\tstop_progress(&packs.progress);\n+\tfor_each_file_in_pack_dir(object_dir, add_pack_to_midx, &ctx);\n+\tstop_progress(&ctx.progress);\n \n-\tif (packs.m && packs.nr == packs.m->num_packs && !packs_to_drop)\n+\tif (ctx.m && ctx.nr == ctx.m->num_packs && !packs_to_drop)\n \t\tgoto cleanup;\n \n-\tentries = get_sorted_entries(packs.m, packs.info, packs.nr, &nr_entries);\n+\tentries = get_sorted_entries(ctx.m, ctx.info, ctx.nr, &nr_entries);\n \n \tfor (i = 0; i < nr_entries; i++) {\n \t\tif (entries[i].offset > 0x7fffffff)\n@@ -862,19 +862,19 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \t\t\tlarge_offsets_needed = 1;\n \t}\n \n-\tQSORT(packs.info, packs.nr, pack_info_compare);\n+\tQSORT(ctx.info, ctx.nr, pack_info_compare);\n \n \tif (packs_to_drop && packs_to_drop->nr) {\n \t\tint drop_index = 0;\n \t\tint missing_drops = 0;\n \n-\t\tfor (i = 0; i < packs.nr && drop_index < packs_to_drop->nr; i++) {\n-\t\t\tint cmp = strcmp(packs.info[i].pack_name,\n+\t\tfor (i = 0; i < ctx.nr && drop_index < packs_to_drop->nr; i++) {\n+\t\t\tint cmp = strcmp(ctx.info[i].pack_name,\n \t\t\t\t\t packs_to_drop->items[drop_index].string);\n \n \t\t\tif (!cmp) {\n \t\t\t\tdrop_index++;\n-\t\t\t\tpacks.info[i].expired = 1;\n+\t\t\t\tctx.info[i].expired = 1;\n \t\t\t} else if (cmp > 0) {\n \t\t\t\terror(_(\"did not see pack-file %s to drop\"),\n \t\t\t\t packs_to_drop->items[drop_index].string);\n@@ -882,7 +882,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \t\t\t\tmissing_drops++;\n \t\t\t\ti--;\n \t\t\t} else {\n-\t\t\t\tpacks.info[i].expired = 0;\n+\t\t\t\tctx.info[i].expired = 0;\n \t\t\t}\n \t\t}\n \n@@ -898,19 +898,19 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \t *\n \t * pack_perm[old_id] = new_id\n \t */\n-\tALLOC_ARRAY(pack_perm, packs.nr);\n-\tfor (i = 0; i < packs.nr; i++) {\n-\t\tif (packs.info[i].expired) {\n+\tALLOC_ARRAY(pack_perm, ctx.nr);\n+\tfor (i = 0; i < ctx.nr; i++) {\n+\t\tif (ctx.info[i].expired) {\n \t\t\tdropped_packs++;\n-\t\t\tpack_perm[packs.info[i].orig_pack_int_id] = PACK_EXPIRED;\n+\t\t\tpack_perm[ctx.info[i].orig_pack_int_id] = PACK_EXPIRED;\n \t\t} else {\n-\t\t\tpack_perm[packs.info[i].orig_pack_int_id] = i - dropped_packs;\n+\t\t\tpack_perm[ctx.info[i].orig_pack_int_id] = i - dropped_packs;\n \t\t}\n \t}\n \n-\tfor (i = 0; i < packs.nr; i++) {\n-\t\tif (!packs.info[i].expired)\n-\t\t\tpack_name_concat_len += strlen(packs.info[i].pack_name) + 1;\n+\tfor (i = 0; i < ctx.nr; i++) {\n+\t\tif (!ctx.info[i].expired)\n+\t\t\tpack_name_concat_len += strlen(ctx.info[i].pack_name) + 1;\n \t}\n \n \tif (pack_name_concat_len % MIDX_CHUNK_ALIGNMENT)\n@@ -921,19 +921,19 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \tf = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);\n \tFREE_AND_NULL(midx_name);\n \n-\tif (packs.m)\n-\t\tclose_midx(packs.m);\n+\tif (ctx.m)\n+\t\tclose_midx(ctx.m);\n \n \tcur_chunk = 0;\n \tnum_chunks = large_offsets_needed ? 5 : 4;\n \n-\tif (packs.nr - dropped_packs == 0) {\n+\tif (ctx.nr - dropped_packs == 0) {\n \t\terror(_(\"no pack files to index.\"));\n \t\tresult = 1;\n \t\tgoto cleanup;\n \t}\n \n-\twritten = write_midx_header(f, num_chunks, packs.nr - dropped_packs);\n+\twritten = write_midx_header(f, num_chunks, ctx.nr - dropped_packs);\n \n \tchunk_ids[cur_chunk] = MIDX_CHUNKID_PACKNAMES;\n \tchunk_offsets[cur_chunk] = written + (num_chunks + 1) * MIDX_CHUNKLOOKUP_WIDTH;\n@@ -990,7 +990,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \n \t\tswitch (chunk_ids[i]) {\n \t\t\tcase MIDX_CHUNKID_PACKNAMES:\n-\t\t\t\twritten += write_midx_pack_names(f, packs.info, packs.nr);\n+\t\t\t\twritten += write_midx_pack_names(f, ctx.info, ctx.nr);\n \t\t\t\tbreak;\n \n \t\t\tcase MIDX_CHUNKID_OIDFANOUT:\n@@ -1027,15 +1027,15 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \tcommit_lock_file(&lk);\n \n cleanup:\n-\tfor (i = 0; i < packs.nr; i++) {\n-\t\tif (packs.info[i].p) {\n-\t\t\tclose_pack(packs.info[i].p);\n-\t\t\tfree(packs.info[i].p);\n+\tfor (i = 0; i < ctx.nr; i++) {\n+\t\tif (ctx.info[i].p) {\n+\t\t\tclose_pack(ctx.info[i].p);\n+\t\t\tfree(ctx.info[i].p);\n \t\t}\n-\t\tfree(packs.info[i].pack_name);\n+\t\tfree(ctx.info[i].pack_name);\n \t}\n \n-\tfree(packs.info);\n+\tfree(ctx.info);\n \tfree(entries);\n \tfree(pack_perm);\n \tfree(midx_name);\n-- \ngitgitgadget\n\n"},{"id":"411243","messageId":"491667de2baef422e801df1e2c7d3173462a96ff.1607012215.git.gitgitgadget@gmail.com","threadId":"54753","inReplyTo":"pull.804.git.1607012215.gitgitgadget@gmail.com","subject":"[PATCH 05/15] midx: add entries to write_midx_context","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T16:16:44Z","receivedAt":"2020-12-03T16:17:56Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nIn an effort to align write_midx_internal() with the chunk-format API,\ncontinue to group necessary data into \"struct write_midx_context\". This\nchange collects the \"struct pack_midx_entry *entries\" list and its count\ninto the context.\n\nUpdate write_midx_oid_fanout() and write_midx_oid_lookup() to take the\ncontext directly, as these are easy conversions with this new data.\n\nOnly the callers of write_midx_object_offsets() and\nwrite_midx_large_offsets() are updated here, since additional data in\nthe context before those methods can match chunk_write_fn.\n\nSigned-off-by: Derrick Stolee \n---\n midx.c | 49 ++++++++++++++++++++++++++-----------------------\n 1 file changed, 26 insertions(+), 23 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 6ab655ddda..2af4452165 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -458,6 +458,9 @@ struct write_midx_context {\n \tstruct multi_pack_index *m;\n \tstruct progress *progress;\n \tunsigned pack_paths_checked;\n+\n+\tstruct pack_midx_entry *entries;\n+\tuint32_t entries_nr;\n };\n \n static void add_pack_to_midx(const char *full_path, size_t full_path_len,\n@@ -678,11 +681,11 @@ static size_t write_midx_pack_names(struct hashfile *f, void *data)\n }\n \n static size_t write_midx_oid_fanout(struct hashfile *f,\n-\t\t\t\t struct pack_midx_entry *objects,\n-\t\t\t\t uint32_t nr_objects)\n+\t\t\t\t void *data)\n {\n-\tstruct pack_midx_entry *list = objects;\n-\tstruct pack_midx_entry *last = objects + nr_objects;\n+\tstruct write_midx_context *ctx = (struct write_midx_context *)data;\n+\tstruct pack_midx_entry *list = ctx->entries;\n+\tstruct pack_midx_entry *last = ctx->entries + ctx->entries_nr;\n \tuint32_t count = 0;\n \tuint32_t i;\n \n@@ -706,18 +709,19 @@ static size_t write_midx_oid_fanout(struct hashfile *f,\n \treturn MIDX_CHUNK_FANOUT_SIZE;\n }\n \n-static size_t write_midx_oid_lookup(struct hashfile *f, unsigned char hash_len,\n-\t\t\t\t struct pack_midx_entry *objects,\n-\t\t\t\t uint32_t nr_objects)\n+static size_t write_midx_oid_lookup(struct hashfile *f,\n+\t\t\t\t void *data)\n {\n-\tstruct pack_midx_entry *list = objects;\n+\tstruct write_midx_context *ctx = (struct write_midx_context *)data;\n+\tunsigned char hash_len = the_hash_algo->rawsz;\n+\tstruct pack_midx_entry *list = ctx->entries;\n \tuint32_t i;\n \tsize_t written = 0;\n \n-\tfor (i = 0; i < nr_objects; i++) {\n+\tfor (i = 0; i < ctx->entries_nr; i++) {\n \t\tstruct pack_midx_entry *obj = list++;\n \n-\t\tif (i < nr_objects - 1) {\n+\t\tif (i < ctx->entries_nr - 1) {\n \t\t\tstruct pack_midx_entry *next = list;\n \t\t\tif (oidcmp(&obj->oid, &next->oid) >= 0)\n \t\t\t\tBUG(\"OIDs not in order: %s >= %s\",\n@@ -805,8 +809,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \tuint64_t written = 0;\n \tuint32_t chunk_ids[MIDX_MAX_CHUNKS + 1];\n \tuint64_t chunk_offsets[MIDX_MAX_CHUNKS + 1];\n-\tuint32_t nr_entries, num_large_offsets = 0;\n-\tstruct pack_midx_entry *entries = NULL;\n+\tuint32_t num_large_offsets = 0;\n \tstruct progress *progress = NULL;\n \tint large_offsets_needed = 0;\n \tint pack_name_concat_len = 0;\n@@ -852,12 +855,12 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \tif (ctx.m && ctx.nr == ctx.m->num_packs && !packs_to_drop)\n \t\tgoto cleanup;\n \n-\tentries = get_sorted_entries(ctx.m, ctx.info, ctx.nr, &nr_entries);\n+\tctx.entries = get_sorted_entries(ctx.m, ctx.info, ctx.nr, &ctx.entries_nr);\n \n-\tfor (i = 0; i < nr_entries; i++) {\n-\t\tif (entries[i].offset > 0x7fffffff)\n+\tfor (i = 0; i < ctx.entries_nr; i++) {\n+\t\tif (ctx.entries[i].offset > 0x7fffffff)\n \t\t\tnum_large_offsets++;\n-\t\tif (entries[i].offset > 0xffffffff)\n+\t\tif (ctx.entries[i].offset > 0xffffffff)\n \t\t\tlarge_offsets_needed = 1;\n \t}\n \n@@ -947,10 +950,10 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \n \tcur_chunk++;\n \tchunk_ids[cur_chunk] = MIDX_CHUNKID_OBJECTOFFSETS;\n-\tchunk_offsets[cur_chunk] = chunk_offsets[cur_chunk - 1] + nr_entries * the_hash_algo->rawsz;\n+\tchunk_offsets[cur_chunk] = chunk_offsets[cur_chunk - 1] + ctx.entries_nr * the_hash_algo->rawsz;\n \n \tcur_chunk++;\n-\tchunk_offsets[cur_chunk] = chunk_offsets[cur_chunk - 1] + nr_entries * MIDX_CHUNK_OFFSET_WIDTH;\n+\tchunk_offsets[cur_chunk] = chunk_offsets[cur_chunk - 1] + ctx.entries_nr * MIDX_CHUNK_OFFSET_WIDTH;\n \tif (large_offsets_needed) {\n \t\tchunk_ids[cur_chunk] = MIDX_CHUNKID_LARGEOFFSETS;\n \n@@ -993,19 +996,19 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \t\t\t\tbreak;\n \n \t\t\tcase MIDX_CHUNKID_OIDFANOUT:\n-\t\t\t\twritten += write_midx_oid_fanout(f, entries, nr_entries);\n+\t\t\t\twritten += write_midx_oid_fanout(f, &ctx);\n \t\t\t\tbreak;\n \n \t\t\tcase MIDX_CHUNKID_OIDLOOKUP:\n-\t\t\t\twritten += write_midx_oid_lookup(f, the_hash_algo->rawsz, entries, nr_entries);\n+\t\t\t\twritten += write_midx_oid_lookup(f, &ctx);\n \t\t\t\tbreak;\n \n \t\t\tcase MIDX_CHUNKID_OBJECTOFFSETS:\n-\t\t\t\twritten += write_midx_object_offsets(f, large_offsets_needed, pack_perm, entries, nr_entries);\n+\t\t\t\twritten += write_midx_object_offsets(f, large_offsets_needed, pack_perm, ctx.entries, ctx.entries_nr);\n \t\t\t\tbreak;\n \n \t\t\tcase MIDX_CHUNKID_LARGEOFFSETS:\n-\t\t\t\twritten += write_midx_large_offsets(f, num_large_offsets, entries, nr_entries);\n+\t\t\t\twritten += write_midx_large_offsets(f, num_large_offsets, ctx.entries, ctx.entries_nr);\n \t\t\t\tbreak;\n \n \t\t\tdefault:\n@@ -1035,7 +1038,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \t}\n \n \tfree(ctx.info);\n-\tfree(entries);\n+\tfree(ctx.entries);\n \tfree(pack_perm);\n \tfree(midx_name);\n \treturn result;\n-- \ngitgitgadget\n\n"},{"id":"411244","messageId":"84f7bc46f93a756da2148ebbc175f0cb02ef3c1a.1607012215.git.gitgitgadget@gmail.com","threadId":"54753","inReplyTo":"pull.804.git.1607012215.gitgitgadget@gmail.com","subject":"[PATCH 07/15] midx: add num_large_offsets to write_midx_context","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T16:16:46Z","receivedAt":"2020-12-03T16:17:56Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nIn an effort to align write_midx_internal() with the chunk-format API,\ncontinue to group necessary data into \"struct write_midx_context\". This\nchange collects the \"uint32_t num_large_offsets\" into the context. With\nthis new data, write_midx_large_offsets() now matches the\nchunk_write_fn type.\n\nSigned-off-by: Derrick Stolee \n---\n midx.c | 17 ++++++++++-------\n 1 file changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex f7c3a54a33..d7da358a3f 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -464,6 +464,7 @@ struct write_midx_context {\n \n \tuint32_t *pack_perm;\n \tunsigned large_offsets_needed:1;\n+\tuint32_t num_large_offsets;\n };\n \n static void add_pack_to_midx(const char *full_path, size_t full_path_len,\n@@ -772,11 +773,14 @@ static size_t write_midx_object_offsets(struct hashfile *f,\n \treturn written;\n }\n \n-static size_t write_midx_large_offsets(struct hashfile *f, uint32_t nr_large_offset,\n-\t\t\t\t struct pack_midx_entry *objects, uint32_t nr_objects)\n+static size_t write_midx_large_offsets(struct hashfile *f,\n+\t\t\t\t void *data)\n {\n-\tstruct pack_midx_entry *list = objects, *end = objects + nr_objects;\n+\tstruct write_midx_context *ctx = (struct write_midx_context *)data;\n+\tstruct pack_midx_entry *list = ctx->entries;\n+\tstruct pack_midx_entry *end = ctx->entries + ctx->entries_nr;\n \tsize_t written = 0;\n+\tuint32_t nr_large_offset = ctx->num_large_offsets;\n \n \twhile (nr_large_offset) {\n \t\tstruct pack_midx_entry *obj;\n@@ -811,7 +815,6 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \tuint64_t written = 0;\n \tuint32_t chunk_ids[MIDX_MAX_CHUNKS + 1];\n \tuint64_t chunk_offsets[MIDX_MAX_CHUNKS + 1];\n-\tuint32_t num_large_offsets = 0;\n \tstruct progress *progress = NULL;\n \tint pack_name_concat_len = 0;\n \tint dropped_packs = 0;\n@@ -861,7 +864,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \tctx.large_offsets_needed = 0;\n \tfor (i = 0; i < ctx.entries_nr; i++) {\n \t\tif (ctx.entries[i].offset > 0x7fffffff)\n-\t\t\tnum_large_offsets++;\n+\t\t\tctx.num_large_offsets++;\n \t\tif (ctx.entries[i].offset > 0xffffffff)\n \t\t\tctx.large_offsets_needed = 1;\n \t}\n@@ -961,7 +964,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \n \t\tcur_chunk++;\n \t\tchunk_offsets[cur_chunk] = chunk_offsets[cur_chunk - 1] +\n-\t\t\t\t\t num_large_offsets * MIDX_CHUNK_LARGE_OFFSET_WIDTH;\n+\t\t\t\t\t ctx.num_large_offsets * MIDX_CHUNK_LARGE_OFFSET_WIDTH;\n \t}\n \n \tchunk_ids[cur_chunk] = 0;\n@@ -1010,7 +1013,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \t\t\t\tbreak;\n \n \t\t\tcase MIDX_CHUNKID_LARGEOFFSETS:\n-\t\t\t\twritten += write_midx_large_offsets(f, num_large_offsets, ctx.entries, ctx.entries_nr);\n+\t\t\t\twritten += write_midx_large_offsets(f, &ctx);\n \t\t\t\tbreak;\n \n \t\t\tdefault:\n-- \ngitgitgadget\n\n"},{"id":"411245","messageId":"0a2f81e7cfd39d69f6388bbfc8b157ba28321a21.1607012215.git.gitgitgadget@gmail.com","threadId":"54753","inReplyTo":"pull.804.git.1607012215.gitgitgadget@gmail.com","subject":"[PATCH 06/15] midx: add pack_perm to write_midx_context","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T16:16:45Z","receivedAt":"2020-12-03T16:17:56Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nIn an effort to align write_midx_internal() with the chunk-format API,\ncontinue to group necessary data into \"struct write_midx_context\". This\nchange collects the \"uint32_t *pack_perm\" and large_offsets_needed bit\ninto the context.\n\nUpdate write_midx_object_offsets() to match chunk_write_fn.\n\nSigned-off-by: Derrick Stolee \n---\n midx.c | 40 +++++++++++++++++++++-------------------\n 1 file changed, 21 insertions(+), 19 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 2af4452165..f7c3a54a33 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -461,6 +461,9 @@ struct write_midx_context {\n \n \tstruct pack_midx_entry *entries;\n \tuint32_t entries_nr;\n+\n+\tuint32_t *pack_perm;\n+\tunsigned large_offsets_needed:1;\n };\n \n static void add_pack_to_midx(const char *full_path, size_t full_path_len,\n@@ -736,27 +739,27 @@ static size_t write_midx_oid_lookup(struct hashfile *f,\n \treturn written;\n }\n \n-static size_t write_midx_object_offsets(struct hashfile *f, int large_offset_needed,\n-\t\t\t\t\tuint32_t *perm,\n-\t\t\t\t\tstruct pack_midx_entry *objects, uint32_t nr_objects)\n+static size_t write_midx_object_offsets(struct hashfile *f,\n+\t\t\t\t\tvoid *data)\n {\n-\tstruct pack_midx_entry *list = objects;\n+\tstruct write_midx_context *ctx = (struct write_midx_context *)data;\n+\tstruct pack_midx_entry *list = ctx->entries;\n \tuint32_t i, nr_large_offset = 0;\n \tsize_t written = 0;\n \n-\tfor (i = 0; i < nr_objects; i++) {\n+\tfor (i = 0; i < ctx->entries_nr; i++) {\n \t\tstruct pack_midx_entry *obj = list++;\n \n-\t\tif (perm[obj->pack_int_id] == PACK_EXPIRED)\n+\t\tif (ctx->pack_perm[obj->pack_int_id] == PACK_EXPIRED)\n \t\t\tBUG(\"object %s is in an expired pack with int-id %d\",\n \t\t\t oid_to_hex(&obj->oid),\n \t\t\t obj->pack_int_id);\n \n-\t\thashwrite_be32(f, perm[obj->pack_int_id]);\n+\t\thashwrite_be32(f, ctx->pack_perm[obj->pack_int_id]);\n \n-\t\tif (large_offset_needed && obj->offset >> 31)\n+\t\tif (ctx->large_offsets_needed && obj->offset >> 31)\n \t\t\thashwrite_be32(f, MIDX_LARGE_OFFSET_NEEDED | nr_large_offset++);\n-\t\telse if (!large_offset_needed && obj->offset >> 32)\n+\t\telse if (!ctx->large_offsets_needed && obj->offset >> 32)\n \t\t\tBUG(\"object %s requires a large offset (%\"PRIx64\") but the MIDX is not writing large offsets!\",\n \t\t\t oid_to_hex(&obj->oid),\n \t\t\t obj->offset);\n@@ -805,13 +808,11 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \tstruct hashfile *f = NULL;\n \tstruct lock_file lk;\n \tstruct write_midx_context ctx = { 0 };\n-\tuint32_t *pack_perm = NULL;\n \tuint64_t written = 0;\n \tuint32_t chunk_ids[MIDX_MAX_CHUNKS + 1];\n \tuint64_t chunk_offsets[MIDX_MAX_CHUNKS + 1];\n \tuint32_t num_large_offsets = 0;\n \tstruct progress *progress = NULL;\n-\tint large_offsets_needed = 0;\n \tint pack_name_concat_len = 0;\n \tint dropped_packs = 0;\n \tint result = 0;\n@@ -857,11 +858,12 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \n \tctx.entries = get_sorted_entries(ctx.m, ctx.info, ctx.nr, &ctx.entries_nr);\n \n+\tctx.large_offsets_needed = 0;\n \tfor (i = 0; i < ctx.entries_nr; i++) {\n \t\tif (ctx.entries[i].offset > 0x7fffffff)\n \t\t\tnum_large_offsets++;\n \t\tif (ctx.entries[i].offset > 0xffffffff)\n-\t\t\tlarge_offsets_needed = 1;\n+\t\t\tctx.large_offsets_needed = 1;\n \t}\n \n \tQSORT(ctx.info, ctx.nr, pack_info_compare);\n@@ -900,13 +902,13 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \t *\n \t * pack_perm[old_id] = new_id\n \t */\n-\tALLOC_ARRAY(pack_perm, ctx.nr);\n+\tALLOC_ARRAY(ctx.pack_perm, ctx.nr);\n \tfor (i = 0; i < ctx.nr; i++) {\n \t\tif (ctx.info[i].expired) {\n \t\t\tdropped_packs++;\n-\t\t\tpack_perm[ctx.info[i].orig_pack_int_id] = PACK_EXPIRED;\n+\t\t\tctx.pack_perm[ctx.info[i].orig_pack_int_id] = PACK_EXPIRED;\n \t\t} else {\n-\t\t\tpack_perm[ctx.info[i].orig_pack_int_id] = i - dropped_packs;\n+\t\t\tctx.pack_perm[ctx.info[i].orig_pack_int_id] = i - dropped_packs;\n \t\t}\n \t}\n \n@@ -927,7 +929,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \t\tclose_midx(ctx.m);\n \n \tcur_chunk = 0;\n-\tnum_chunks = large_offsets_needed ? 5 : 4;\n+\tnum_chunks = ctx.large_offsets_needed ? 5 : 4;\n \n \tif (ctx.nr - dropped_packs == 0) {\n \t\terror(_(\"no pack files to index.\"));\n@@ -954,7 +956,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \n \tcur_chunk++;\n \tchunk_offsets[cur_chunk] = chunk_offsets[cur_chunk - 1] + ctx.entries_nr * MIDX_CHUNK_OFFSET_WIDTH;\n-\tif (large_offsets_needed) {\n+\tif (ctx.large_offsets_needed) {\n \t\tchunk_ids[cur_chunk] = MIDX_CHUNKID_LARGEOFFSETS;\n \n \t\tcur_chunk++;\n@@ -1004,7 +1006,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \t\t\t\tbreak;\n \n \t\t\tcase MIDX_CHUNKID_OBJECTOFFSETS:\n-\t\t\t\twritten += write_midx_object_offsets(f, large_offsets_needed, pack_perm, ctx.entries, ctx.entries_nr);\n+\t\t\t\twritten += write_midx_object_offsets(f, &ctx);\n \t\t\t\tbreak;\n \n \t\t\tcase MIDX_CHUNKID_LARGEOFFSETS:\n@@ -1039,7 +1041,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \n \tfree(ctx.info);\n \tfree(ctx.entries);\n-\tfree(pack_perm);\n+\tfree(ctx.pack_perm);\n \tfree(midx_name);\n \treturn result;\n }\n-- \ngitgitgadget\n\n"},{"id":"411246","messageId":"dd85922eadd1c6eb4b08c9a3241afba8e7faecd9.1607012215.git.gitgitgadget@gmail.com","threadId":"54753","inReplyTo":"pull.804.git.1607012215.gitgitgadget@gmail.com","subject":"[PATCH 09/15] midx: drop chunk progress during write","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T16:16:48Z","receivedAt":"2020-12-03T16:17:56Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nMost expensive operations in write_midx_internal() use the context\nstruct's progress member, and these indicate the process of the\nexpensive operations within the chunk writing methods. However, there is\na competing progress struct that counts the progress over all chunks.\nThis is not very helpful compared to the others, so drop it.\n\nThis also reduces our barriers to combining the chunk writing code with\nchunk-format.c.\n\nSigned-off-by: Derrick Stolee \n---\n midx.c | 7 -------\n 1 file changed, 7 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 5eb1b01946..ce6d4339bd 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -808,7 +808,6 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \tuint64_t header_size = 0;\n \tuint32_t chunk_ids[MIDX_MAX_CHUNKS + 1];\n \tuint64_t chunk_offsets[MIDX_MAX_CHUNKS + 1];\n-\tstruct progress *progress = NULL;\n \tint pack_name_concat_len = 0;\n \tint dropped_packs = 0;\n \tint result = 0;\n@@ -976,9 +975,6 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \t\thashwrite_be64(f, chunk_offsets[i]);\n \t}\n \n-\tif (flags & MIDX_PROGRESS)\n-\t\tprogress = start_delayed_progress(_(\"Writing chunks to multi-pack-index\"),\n-\t\t\t\t\t num_chunks);\n \tfor (i = 0; i < num_chunks; i++) {\n \t\tif (f->total + f->offset != chunk_offsets[i])\n \t\t\tBUG(\"incorrect chunk offset (%\"PRIu64\" != %\"PRIu64\") for chunk id %\"PRIx32,\n@@ -1011,10 +1007,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \t\t\t\tBUG(\"trying to write unknown chunk id %\"PRIx32,\n \t\t\t\t chunk_ids[i]);\n \t\t}\n-\n-\t\tdisplay_progress(progress, i + 1);\n \t}\n-\tstop_progress(&progress);\n \n \tif (f->total + f->offset != chunk_offsets[num_chunks])\n \t\tBUG(\"incorrect final offset %\"PRIu64\" != %\"PRIu64,\n-- \ngitgitgadget\n\n"},{"id":"411247","messageId":"dbb637a7ac6d2cc6bb78428ef7bda67687095f88.1607012215.git.gitgitgadget@gmail.com","threadId":"54753","inReplyTo":"pull.804.git.1607012215.gitgitgadget@gmail.com","subject":"[PATCH 08/15] midx: convert chunk write methods to return int","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T16:16:47Z","receivedAt":"2020-12-03T16:17:59Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nHistorically, the chunk-writing methods in midx.c have returned the\namount of data written so the writer method could compare this with the\ntable of contents. This presents with some interesting issues:\n\n1. If a chunk writing method has a bug that miscalculates the written\n bytes, then we can satisfy the table of contents without actually\n writing the right amount of data to the hashfile. The commit-graph\n writing code checks the hashfile struct directly for a more robust\n verification.\n\n2. There is no way for a chunk writing method to gracefully fail.\n Returning an int presents an opportunity to fail without a die().\n\n3. The current pattern doesn't match chunk_write_fn type exactly, so we\n cannot share code with commit-graph.c\n\nFor these reasons, convert the midx chunk writer methods to return an\n'int'. Since none of them fail at the moment, they all return 0.\n\nSigned-off-by: Derrick Stolee \n---\n midx.c | 63 +++++++++++++++++++++++++---------------------------------\n 1 file changed, 27 insertions(+), 36 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex d7da358a3f..5eb1b01946 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -650,7 +650,7 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \treturn deduplicated_entries;\n }\n \n-static size_t write_midx_pack_names(struct hashfile *f, void *data)\n+static int write_midx_pack_names(struct hashfile *f, void *data)\n {\n \tstruct write_midx_context *ctx = (struct write_midx_context *)data;\n \tuint32_t i;\n@@ -678,14 +678,13 @@ static size_t write_midx_pack_names(struct hashfile *f, void *data)\n \tif (i < MIDX_CHUNK_ALIGNMENT) {\n \t\tmemset(padding, 0, sizeof(padding));\n \t\thashwrite(f, padding, i);\n-\t\twritten += i;\n \t}\n \n-\treturn written;\n+\treturn 0;\n }\n \n-static size_t write_midx_oid_fanout(struct hashfile *f,\n-\t\t\t\t void *data)\n+static int write_midx_oid_fanout(struct hashfile *f,\n+\t\t\t\t void *data)\n {\n \tstruct write_midx_context *ctx = (struct write_midx_context *)data;\n \tstruct pack_midx_entry *list = ctx->entries;\n@@ -710,17 +709,16 @@ static size_t write_midx_oid_fanout(struct hashfile *f,\n \t\tlist = next;\n \t}\n \n-\treturn MIDX_CHUNK_FANOUT_SIZE;\n+\treturn 0;\n }\n \n-static size_t write_midx_oid_lookup(struct hashfile *f,\n-\t\t\t\t void *data)\n+static int write_midx_oid_lookup(struct hashfile *f,\n+\t\t\t\t void *data)\n {\n \tstruct write_midx_context *ctx = (struct write_midx_context *)data;\n \tunsigned char hash_len = the_hash_algo->rawsz;\n \tstruct pack_midx_entry *list = ctx->entries;\n \tuint32_t i;\n-\tsize_t written = 0;\n \n \tfor (i = 0; i < ctx->entries_nr; i++) {\n \t\tstruct pack_midx_entry *obj = list++;\n@@ -734,19 +732,17 @@ static size_t write_midx_oid_lookup(struct hashfile *f,\n \t\t}\n \n \t\thashwrite(f, obj->oid.hash, (int)hash_len);\n-\t\twritten += hash_len;\n \t}\n \n-\treturn written;\n+\treturn 0;\n }\n \n-static size_t write_midx_object_offsets(struct hashfile *f,\n-\t\t\t\t\tvoid *data)\n+static int write_midx_object_offsets(struct hashfile *f,\n+\t\t\t\t void *data)\n {\n \tstruct write_midx_context *ctx = (struct write_midx_context *)data;\n \tstruct pack_midx_entry *list = ctx->entries;\n \tuint32_t i, nr_large_offset = 0;\n-\tsize_t written = 0;\n \n \tfor (i = 0; i < ctx->entries_nr; i++) {\n \t\tstruct pack_midx_entry *obj = list++;\n@@ -766,20 +762,17 @@ static size_t write_midx_object_offsets(struct hashfile *f,\n \t\t\t obj->offset);\n \t\telse\n \t\t\thashwrite_be32(f, (uint32_t)obj->offset);\n-\n-\t\twritten += MIDX_CHUNK_OFFSET_WIDTH;\n \t}\n \n-\treturn written;\n+\treturn 0;\n }\n \n-static size_t write_midx_large_offsets(struct hashfile *f,\n-\t\t\t\t void *data)\n+static int write_midx_large_offsets(struct hashfile *f,\n+\t\t\t\t void *data)\n {\n \tstruct write_midx_context *ctx = (struct write_midx_context *)data;\n \tstruct pack_midx_entry *list = ctx->entries;\n \tstruct pack_midx_entry *end = ctx->entries + ctx->entries_nr;\n-\tsize_t written = 0;\n \tuint32_t nr_large_offset = ctx->num_large_offsets;\n \n \twhile (nr_large_offset) {\n@@ -795,12 +788,12 @@ static size_t write_midx_large_offsets(struct hashfile *f,\n \t\tif (!(offset >> 31))\n \t\t\tcontinue;\n \n-\t\twritten += hashwrite_be64(f, offset);\n+\t\thashwrite_be64(f, offset);\n \n \t\tnr_large_offset--;\n \t}\n \n-\treturn written;\n+\treturn 0;\n }\n \n static int write_midx_internal(const char *object_dir, struct multi_pack_index *m,\n@@ -812,7 +805,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \tstruct hashfile *f = NULL;\n \tstruct lock_file lk;\n \tstruct write_midx_context ctx = { 0 };\n-\tuint64_t written = 0;\n+\tuint64_t header_size = 0;\n \tuint32_t chunk_ids[MIDX_MAX_CHUNKS + 1];\n \tuint64_t chunk_offsets[MIDX_MAX_CHUNKS + 1];\n \tstruct progress *progress = NULL;\n@@ -940,10 +933,10 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \t\tgoto cleanup;\n \t}\n \n-\twritten = write_midx_header(f, num_chunks, ctx.nr - dropped_packs);\n+\theader_size = write_midx_header(f, num_chunks, ctx.nr - dropped_packs);\n \n \tchunk_ids[cur_chunk] = MIDX_CHUNKID_PACKNAMES;\n-\tchunk_offsets[cur_chunk] = written + (num_chunks + 1) * MIDX_CHUNKLOOKUP_WIDTH;\n+\tchunk_offsets[cur_chunk] = header_size + (num_chunks + 1) * MIDX_CHUNKLOOKUP_WIDTH;\n \n \tcur_chunk++;\n \tchunk_ids[cur_chunk] = MIDX_CHUNKID_OIDFANOUT;\n@@ -981,39 +974,37 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \n \t\thashwrite_be32(f, chunk_ids[i]);\n \t\thashwrite_be64(f, chunk_offsets[i]);\n-\n-\t\twritten += MIDX_CHUNKLOOKUP_WIDTH;\n \t}\n \n \tif (flags & MIDX_PROGRESS)\n \t\tprogress = start_delayed_progress(_(\"Writing chunks to multi-pack-index\"),\n \t\t\t\t\t num_chunks);\n \tfor (i = 0; i < num_chunks; i++) {\n-\t\tif (written != chunk_offsets[i])\n+\t\tif (f->total + f->offset != chunk_offsets[i])\n \t\t\tBUG(\"incorrect chunk offset (%\"PRIu64\" != %\"PRIu64\") for chunk id %\"PRIx32,\n \t\t\t chunk_offsets[i],\n-\t\t\t written,\n+\t\t\t f->total + f->offset,\n \t\t\t chunk_ids[i]);\n \n \t\tswitch (chunk_ids[i]) {\n \t\t\tcase MIDX_CHUNKID_PACKNAMES:\n-\t\t\t\twritten += write_midx_pack_names(f, &ctx);\n+\t\t\t\twrite_midx_pack_names(f, &ctx);\n \t\t\t\tbreak;\n \n \t\t\tcase MIDX_CHUNKID_OIDFANOUT:\n-\t\t\t\twritten += write_midx_oid_fanout(f, &ctx);\n+\t\t\t\twrite_midx_oid_fanout(f, &ctx);\n \t\t\t\tbreak;\n \n \t\t\tcase MIDX_CHUNKID_OIDLOOKUP:\n-\t\t\t\twritten += write_midx_oid_lookup(f, &ctx);\n+\t\t\t\twrite_midx_oid_lookup(f, &ctx);\n \t\t\t\tbreak;\n \n \t\t\tcase MIDX_CHUNKID_OBJECTOFFSETS:\n-\t\t\t\twritten += write_midx_object_offsets(f, &ctx);\n+\t\t\t\twrite_midx_object_offsets(f, &ctx);\n \t\t\t\tbreak;\n \n \t\t\tcase MIDX_CHUNKID_LARGEOFFSETS:\n-\t\t\t\twritten += write_midx_large_offsets(f, &ctx);\n+\t\t\t\twrite_midx_large_offsets(f, &ctx);\n \t\t\t\tbreak;\n \n \t\t\tdefault:\n@@ -1025,9 +1016,9 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \t}\n \tstop_progress(&progress);\n \n-\tif (written != chunk_offsets[num_chunks])\n+\tif (f->total + f->offset != chunk_offsets[num_chunks])\n \t\tBUG(\"incorrect final offset %\"PRIu64\" != %\"PRIu64,\n-\t\t written,\n+\t\t f->total + f->offset,\n \t\t chunk_offsets[num_chunks]);\n \n \tfinalize_hashfile(f, NULL, CSUM_FSYNC | CSUM_HASH_IN_STREAM);\n-- \ngitgitgadget\n\n"},{"id":"411248","messageId":"c6a433efba3214e83a7265e53c24cb4001345f14.1607012215.git.gitgitgadget@gmail.com","threadId":"54753","inReplyTo":"pull.804.git.1607012215.gitgitgadget@gmail.com","subject":"[PATCH 11/15] midx: use 64-bit multiplication for chunk sizes","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T16:16:50Z","receivedAt":"2020-12-03T16:18:20Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhen calculating the sizes of certain chunks, we should use 64-bit\nmultiplication always. This allows us to properly predict the chunk\nsizes without risk of overflow.\n\nSigned-off-by: Derrick Stolee \n---\n midx.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 0548266bea..47f5f60fcd 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -946,12 +946,12 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \tchunks[2].write_fn = write_midx_oid_lookup;\n \n \tchunks[3].id = MIDX_CHUNKID_OBJECTOFFSETS;\n-\tchunks[3].size = ctx.entries_nr * MIDX_CHUNK_OFFSET_WIDTH;\n+\tchunks[3].size = (uint64_t)ctx.entries_nr * MIDX_CHUNK_OFFSET_WIDTH;\n \tchunks[3].write_fn = write_midx_object_offsets;\n \n \tif (ctx.large_offsets_needed) {\n \t\tchunks[4].id = MIDX_CHUNKID_LARGEOFFSETS;\n-\t\tchunks[4].size = ctx.num_large_offsets * MIDX_CHUNK_LARGE_OFFSET_WIDTH;\n+\t\tchunks[4].size = (uint64_t)ctx.num_large_offsets * MIDX_CHUNK_LARGE_OFFSET_WIDTH;\n \t\tchunks[4].write_fn = write_midx_large_offsets;\n \t}\n \n-- \ngitgitgadget\n\n"},{"id":"411249","messageId":"03f3255c8f4a953065b2ff8e61816f83534c23ed.1607012215.git.gitgitgadget@gmail.com","threadId":"54753","inReplyTo":"pull.804.git.1607012215.gitgitgadget@gmail.com","subject":"[PATCH 12/15] chunk-format: create write_chunks()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T16:16:51Z","receivedAt":"2020-12-03T16:18:21Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe commit-graph and multi-pack-index files both use a chunk-based file\nformat. They have already unified on using write_table_of_contents(),\nbut we expand upon that by unifying their chunk writing loop.\n\nThis takes the concepts already present in the commit-graph that were\ndropped in the multi-pack-index code during refactoring, including:\n\n* Check the hashfile for how much data was written by each write_fn.\n\n* Allow write_fn() to report an error that results in a failure\n without using die() in the low-level commands.\n\nThis simplifies the code in commit-graph.c and midx.c while laying the\nfoundation for future formats using similar ideas.\n\nSigned-off-by: Derrick Stolee \n---\n chunk-format.c | 23 +++++++++++++++++++++++\n chunk-format.h | 13 +++++++++++++\n commit-graph.c | 13 ++-----------\n midx.c | 3 +--\n 4 files changed, 39 insertions(+), 13 deletions(-)\n\ndiff --git a/chunk-format.c b/chunk-format.c\nindex 771b6d98d0..a6643a4fc8 100644\n--- a/chunk-format.c\n+++ b/chunk-format.c\n@@ -24,3 +24,26 @@ void write_table_of_contents(struct hashfile *f,\n \thashwrite_be32(f, 0);\n \thashwrite_be64(f, cur_offset);\n }\n+\n+int write_chunks(struct hashfile *f,\n+\t\t struct chunk_info *chunks,\n+\t\t int nr,\n+\t\t void *data)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < nr; i++) {\n+\t\tuint64_t start_offset = f->total + f->offset;\n+\t\tint result = chunks[i].write_fn(f, data);\n+\n+\t\tif (result)\n+\t\t\treturn result;\n+\n+\t\tif (f->total + f->offset != start_offset + chunks[i].size)\n+\t\t\tBUG(\"expected to write %\"PRId64\" bytes to chunk %\"PRIx32\", but wrote %\"PRId64\" instead\",\n+\t\t\t chunks[i].size, chunks[i].id,\n+\t\t\t f->total + f->offset - start_offset);\n+\t}\n+\n+\treturn 0;\n+}\ndiff --git a/chunk-format.h b/chunk-format.h\nindex 4b9cbeb372..a2c7ddb23b 100644\n--- a/chunk-format.h\n+++ b/chunk-format.h\n@@ -33,4 +33,17 @@ void write_table_of_contents(struct hashfile *f,\n \t\t\t struct chunk_info *chunks,\n \t\t\t int nr);\n \n+/*\n+ * Write the data for the given chunk list using the provided\n+ * write_fn values. The given 'data' parameter is passed to those\n+ * methods.\n+ *\n+ * The data that is written by each write_fn is checked to be of\n+ * the expected size, and a BUG() is thrown if not specified correctly.\n+ */\n+int write_chunks(struct hashfile *f,\n+\t\t struct chunk_info *chunks,\n+\t\t int nr,\n+\t\t void *data);\n+\n #endif\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 5494fda1d3..10dcef9d6b 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1809,17 +1809,8 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t\t\tnum_chunks * ctx->commits.nr);\n \t}\n \n-\tfor (i = 0; i < num_chunks; i++) {\n-\t\tuint64_t start_offset = f->total + f->offset;\n-\n-\t\tif (chunks[i].write_fn(f, ctx))\n-\t\t\treturn -1;\n-\n-\t\tif (f->total + f->offset != start_offset + chunks[i].size)\n-\t\t\tBUG(\"expected to write %\"PRId64\" bytes to chunk %\"PRIx32\", but wrote %\"PRId64\" instead\",\n-\t\t\t chunks[i].size, chunks[i].id,\n-\t\t\t f->total + f->offset - start_offset);\n-\t}\n+\tif (write_chunks(f, chunks, num_chunks, ctx))\n+\t\treturn -1;\n \n \tstop_progress(&ctx->progress);\n \tstrbuf_release(&progress_title);\ndiff --git a/midx.c b/midx.c\nindex 47f5f60fcd..67ac232a81 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -957,8 +957,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \n \twrite_table_of_contents(f, header_size, chunks, num_chunks);\n \n-\tfor (i = 0; i < num_chunks; i++)\n-\t\tchunks[i].write_fn(f, &ctx);\n+\tresult = write_chunks(f, chunks, num_chunks, &ctx);\n \n \tfinalize_hashfile(f, NULL, CSUM_FSYNC | CSUM_HASH_IN_STREAM);\n \tcommit_lock_file(&lk);\n-- \ngitgitgadget\n\n"},{"id":"411250","messageId":"6801e231f7414444a272f2ea87dcc6f60f29e25a.1607012215.git.gitgitgadget@gmail.com","threadId":"54753","inReplyTo":"pull.804.git.1607012215.gitgitgadget@gmail.com","subject":"[PATCH 13/15] chunk-format: create chunk reading API","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T16:16:52Z","receivedAt":"2020-12-03T16:18:21Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nNow that the chunk-format API has a consistent mechanism for writing\nfile formats based on chunks, let's extend it to also parse chunk-based\nfiles during read.\n\nSimilar to the write scenario, the caller supplies some context\ninformation, such as a memory location, the offset of the table of\ncontents, and some information of what to do with each chunk. The table\nof contents parsing will skip any unspecified chunks and will leave the\nspecifics of each chunk to a function pointer.\n\nThis implementation handles some of the important error cases, such as\nchunk offsets that escape the size of the file. However, we drop the\ncase of duplicate chunks and leave that to the given functions. It may\nbe helpful to allow multiple instances of the same chunk ID for some\nformats. The new location of these error checks change the error strings\nand also the tests that verify for corruption in the table of contents.\n\nSigned-off-by: Derrick Stolee \n---\n chunk-format.c | 56 ++++++++++\n chunk-format.h | 20 ++++\n commit-graph.c | 200 ++++++++++++++++++++----------------\n midx.c | 114 ++++++++++++--------\n t/t5318-commit-graph.sh | 2 +-\n t/t5319-multi-pack-index.sh | 6 +-\n 6 files changed, 261 insertions(+), 137 deletions(-)\n\ndiff --git a/chunk-format.c b/chunk-format.c\nindex a6643a4fc8..d888ef6ec7 100644\n--- a/chunk-format.c\n+++ b/chunk-format.c\n@@ -1,6 +1,7 @@\n #include \"git-compat-util.h\"\n #include \"chunk-format.h\"\n #include \"csum-file.h\"\n+#include \"cache.h\"\n #define CHUNK_LOOKUP_WIDTH 12\n \n void write_table_of_contents(struct hashfile *f,\n@@ -47,3 +48,58 @@ int write_chunks(struct hashfile *f,\n \n \treturn 0;\n }\n+\n+int read_table_of_contents(const unsigned char *mfile,\n+\t\t\t size_t mfile_size,\n+\t\t\t uint64_t toc_offset,\n+\t\t\t int toc_length,\n+\t\t\t struct read_chunk_info *chunks,\n+\t\t\t int nr,\n+\t\t\t void *data)\n+{\n+\tuint32_t chunk_id;\n+\tconst unsigned char *table_of_contents = mfile + toc_offset;\n+\n+\twhile (toc_length--) {\n+\t\tint i;\n+\t\tuint64_t chunk_offset, next_chunk_offset;\n+\n+\t\tchunk_id = get_be32(table_of_contents);\n+\t\tchunk_offset = get_be64(table_of_contents + 4);\n+\n+\t\tif (!chunk_id) {\n+\t\t\terror(_(\"terminating chunk id appears earlier than expected\"));\n+\t\t\treturn 1;\n+\t\t}\n+\n+\t\ttable_of_contents += CHUNK_LOOKUP_WIDTH;\n+\t\tnext_chunk_offset = get_be64(table_of_contents + 4);\n+\n+\t\tif (next_chunk_offset < chunk_offset ||\n+\t\t next_chunk_offset > mfile_size - the_hash_algo->rawsz) {\n+\t\t\terror(_(\"improper chunk offset(s) %\"PRIx64\" and %\"PRIx64\"\"),\n+\t\t\t chunk_offset, next_chunk_offset);\n+\t\t\treturn 1;\n+\t\t}\n+\t\tfor (i = 0; i < nr; i++) {\n+\t\t\tif (chunks[i].id == chunk_id) {\n+\t\t\t\tint result = chunks[i].read_fn(\n+\t\t\t\t\t\tmfile + chunk_offset,\n+\t\t\t\t\t\tnext_chunk_offset - chunk_offset,\n+\t\t\t\t\t\tdata);\n+\n+\t\t\t\tif (result)\n+\t\t\t\t\treturn result;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+\tchunk_id = get_be32(table_of_contents);\n+\tif (chunk_id) {\n+\t\terror(_(\"final chunk has non-zero id %\"PRIx32\"\"), chunk_id);\n+\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\ndiff --git a/chunk-format.h b/chunk-format.h\nindex a2c7ddb23b..7049800f73 100644\n--- a/chunk-format.h\n+++ b/chunk-format.h\n@@ -46,4 +46,24 @@ int write_chunks(struct hashfile *f,\n \t\t int nr,\n \t\t void *data);\n \n+/*\n+ * When reading a table of contents, we find the chunk with matching 'id'\n+ * then call its read_fn to populate the necessary 'data' based on the\n+ * chunk start and size.\n+ */\n+typedef int (*chunk_read_fn)(const unsigned char *chunk_start,\n+\t\t\t size_t chunk_size, void *data);\n+struct read_chunk_info {\n+\tuint32_t id;\n+\tchunk_read_fn read_fn;\n+};\n+\n+int read_table_of_contents(const unsigned char *mfile,\n+\t\t\t size_t mfile_size,\n+\t\t\t uint64_t toc_offset,\n+\t\t\t int toc_length,\n+\t\t\t struct read_chunk_info *chunks,\n+\t\t\t int nr,\n+\t\t\t void *data);\n+\n #endif\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 10dcef9d6b..0a3ba147df 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -289,15 +289,114 @@ static int verify_commit_graph_lite(struct commit_graph *g)\n \treturn 0;\n }\n \n+static int graph_read_oid_fanout(const unsigned char *chunk_start,\n+\t\t\t\t size_t chunk_size, void *data)\n+{\n+\tstruct commit_graph *g = (struct commit_graph *)data;\n+\tg->chunk_oid_fanout = (uint32_t*)chunk_start;\n+\treturn 0;\n+}\n+\n+static int graph_read_oid_lookup(const unsigned char *chunk_start,\n+\t\t\t\t size_t chunk_size, void *data)\n+{\n+\tstruct commit_graph *g = (struct commit_graph *)data;\n+\tg->chunk_oid_lookup = chunk_start;\n+\tg->num_commits = chunk_size / g->hash_len;\n+\treturn 0;\n+}\n+\n+static int graph_read_data(const unsigned char *chunk_start,\n+\t\t\t\t size_t chunk_size, void *data)\n+{\n+\tstruct commit_graph *g = (struct commit_graph *)data;\n+\tg->chunk_commit_data = chunk_start;\n+\treturn 0;\n+}\n+\n+static int graph_read_extra_edges(const unsigned char *chunk_start,\n+\t\t\t\t size_t chunk_size, void *data)\n+{\n+\tstruct commit_graph *g = (struct commit_graph *)data;\n+\tg->chunk_extra_edges = chunk_start;\n+\treturn 0;\n+}\n+\n+static int graph_read_base_graphs(const unsigned char *chunk_start,\n+\t\t\t\t size_t chunk_size, void *data)\n+{\n+\tstruct commit_graph *g = (struct commit_graph *)data;\n+\tg->chunk_base_graphs = chunk_start;\n+\treturn 0;\n+}\n+\n+static int graph_read_bloom_indices(const unsigned char *chunk_start,\n+\t\t\t\t size_t chunk_size, void *data)\n+{\n+\tstruct commit_graph *g = (struct commit_graph *)data;\n+\tg->chunk_bloom_indexes = chunk_start;\n+\treturn 0;\n+}\n+\n+static int graph_read_bloom_data(const unsigned char *chunk_start,\n+\t\t\t\t size_t chunk_size, void *data)\n+{\n+\tstruct commit_graph *g = (struct commit_graph *)data;\n+\tuint32_t hash_version;\n+\tg->chunk_bloom_data = chunk_start;\n+\thash_version = get_be32(chunk_start);\n+\n+\tif (hash_version != 1)\n+\t\treturn 0;\n+\n+\tg->bloom_filter_settings = xmalloc(sizeof(struct bloom_filter_settings));\n+\tg->bloom_filter_settings->hash_version = hash_version;\n+\tg->bloom_filter_settings->num_hashes = get_be32(chunk_start + 4);\n+\tg->bloom_filter_settings->bits_per_entry = get_be32(chunk_start + 8);\n+\tg->bloom_filter_settings->max_changed_paths = DEFAULT_BLOOM_MAX_CHANGES;\n+\n+\treturn 0;\n+}\n+\n+static struct read_chunk_info read_chunks[] = {\n+\t[0] = {\n+\t\tGRAPH_CHUNKID_OIDFANOUT,\n+\t\tgraph_read_oid_fanout\n+\t},\n+\t[1] = {\n+\t\tGRAPH_CHUNKID_OIDLOOKUP,\n+\t\tgraph_read_oid_lookup\n+\t},\n+\t[2] = {\n+\t\tGRAPH_CHUNKID_DATA,\n+\t\tgraph_read_data\n+\t},\n+\t[3] = {\n+\t\tGRAPH_CHUNKID_EXTRAEDGES,\n+\t\tgraph_read_extra_edges\n+\t},\n+\t[4] = {\n+\t\tGRAPH_CHUNKID_BASE,\n+\t\tgraph_read_base_graphs\n+\t},\n+\t[5] = {\n+\t\tGRAPH_CHUNKID_BLOOMINDEXES,\n+\t\tgraph_read_bloom_indices\n+\t},\n+\t[6] = {\n+\t\tGRAPH_CHUNKID_BLOOMDATA,\n+\t\tgraph_read_bloom_data\n+\t}\n+};\n+\n struct commit_graph *parse_commit_graph(struct repository *r,\n \t\t\t\t\tvoid *graph_map, size_t graph_size)\n {\n-\tconst unsigned char *data, *chunk_lookup;\n-\tuint32_t i;\n+\tconst unsigned char *data;\n \tstruct commit_graph *graph;\n-\tuint64_t next_chunk_offset;\n \tuint32_t graph_signature;\n \tunsigned char graph_version, hash_version;\n+\tint chunks_nr = MAX_NUM_CHUNKS;\n \n \tif (!graph_map)\n \t\treturn NULL;\n@@ -346,95 +445,14 @@ struct commit_graph *parse_commit_graph(struct repository *r,\n \t\treturn NULL;\n \t}\n \n-\tchunk_lookup = data + 8;\n-\tnext_chunk_offset = get_be64(chunk_lookup + 4);\n-\tfor (i = 0; i < graph->num_chunks; i++) {\n-\t\tuint32_t chunk_id;\n-\t\tuint64_t chunk_offset = next_chunk_offset;\n-\t\tint chunk_repeated = 0;\n-\n-\t\tchunk_id = get_be32(chunk_lookup + 0);\n-\n-\t\tchunk_lookup += GRAPH_CHUNKLOOKUP_WIDTH;\n-\t\tnext_chunk_offset = get_be64(chunk_lookup + 4);\n-\n-\t\tif (chunk_offset > graph_size - the_hash_algo->rawsz) {\n-\t\t\terror(_(\"commit-graph improper chunk offset %08x%08x\"), (uint32_t)(chunk_offset >> 32),\n-\t\t\t (uint32_t)chunk_offset);\n-\t\t\tgoto free_and_return;\n-\t\t}\n-\n-\t\tswitch (chunk_id) {\n-\t\tcase GRAPH_CHUNKID_OIDFANOUT:\n-\t\t\tif (graph->chunk_oid_fanout)\n-\t\t\t\tchunk_repeated = 1;\n-\t\t\telse\n-\t\t\t\tgraph->chunk_oid_fanout = (uint32_t*)(data + chunk_offset);\n-\t\t\tbreak;\n-\n-\t\tcase GRAPH_CHUNKID_OIDLOOKUP:\n-\t\t\tif (graph->chunk_oid_lookup)\n-\t\t\t\tchunk_repeated = 1;\n-\t\t\telse {\n-\t\t\t\tgraph->chunk_oid_lookup = data + chunk_offset;\n-\t\t\t\tgraph->num_commits = (next_chunk_offset - chunk_offset)\n-\t\t\t\t\t\t / graph->hash_len;\n-\t\t\t}\n-\t\t\tbreak;\n-\n-\t\tcase GRAPH_CHUNKID_DATA:\n-\t\t\tif (graph->chunk_commit_data)\n-\t\t\t\tchunk_repeated = 1;\n-\t\t\telse\n-\t\t\t\tgraph->chunk_commit_data = data + chunk_offset;\n-\t\t\tbreak;\n+\t/* limit the chunk-format list if we are ignoring Bloom filters */\n+\tif (!r->settings.commit_graph_read_changed_paths)\n+\t\tchunks_nr = 5;\n \n-\t\tcase GRAPH_CHUNKID_EXTRAEDGES:\n-\t\t\tif (graph->chunk_extra_edges)\n-\t\t\t\tchunk_repeated = 1;\n-\t\t\telse\n-\t\t\t\tgraph->chunk_extra_edges = data + chunk_offset;\n-\t\t\tbreak;\n-\n-\t\tcase GRAPH_CHUNKID_BASE:\n-\t\t\tif (graph->chunk_base_graphs)\n-\t\t\t\tchunk_repeated = 1;\n-\t\t\telse\n-\t\t\t\tgraph->chunk_base_graphs = data + chunk_offset;\n-\t\t\tbreak;\n-\n-\t\tcase GRAPH_CHUNKID_BLOOMINDEXES:\n-\t\t\tif (graph->chunk_bloom_indexes)\n-\t\t\t\tchunk_repeated = 1;\n-\t\t\telse if (r->settings.commit_graph_read_changed_paths)\n-\t\t\t\tgraph->chunk_bloom_indexes = data + chunk_offset;\n-\t\t\tbreak;\n-\n-\t\tcase GRAPH_CHUNKID_BLOOMDATA:\n-\t\t\tif (graph->chunk_bloom_data)\n-\t\t\t\tchunk_repeated = 1;\n-\t\t\telse if (r->settings.commit_graph_read_changed_paths) {\n-\t\t\t\tuint32_t hash_version;\n-\t\t\t\tgraph->chunk_bloom_data = data + chunk_offset;\n-\t\t\t\thash_version = get_be32(data + chunk_offset);\n-\n-\t\t\t\tif (hash_version != 1)\n-\t\t\t\t\tbreak;\n-\n-\t\t\t\tgraph->bloom_filter_settings = xmalloc(sizeof(struct bloom_filter_settings));\n-\t\t\t\tgraph->bloom_filter_settings->hash_version = hash_version;\n-\t\t\t\tgraph->bloom_filter_settings->num_hashes = get_be32(data + chunk_offset + 4);\n-\t\t\t\tgraph->bloom_filter_settings->bits_per_entry = get_be32(data + chunk_offset + 8);\n-\t\t\t\tgraph->bloom_filter_settings->max_changed_paths = DEFAULT_BLOOM_MAX_CHANGES;\n-\t\t\t}\n-\t\t\tbreak;\n-\t\t}\n-\n-\t\tif (chunk_repeated) {\n-\t\t\terror(_(\"commit-graph chunk id %08x appears multiple times\"), chunk_id);\n-\t\t\tgoto free_and_return;\n-\t\t}\n-\t}\n+\tif (read_table_of_contents(\n+\t\tgraph->data, graph_size, GRAPH_HEADER_SIZE, graph->num_chunks,\n+\t\tread_chunks, chunks_nr, graph))\n+\t\tgoto free_and_return;\n \n \tif (graph->chunk_bloom_indexes && graph->chunk_bloom_data) {\n \t\tinit_bloom_filters();\ndiff --git a/midx.c b/midx.c\nindex 67ac232a81..786b3b51c3 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -54,6 +54,74 @@ static char *get_midx_filename(const char *object_dir)\n \treturn xstrfmt(\"%s/pack/multi-pack-index\", object_dir);\n }\n \n+static int midx_read_pack_names(const unsigned char *chunk_start,\n+\t\t\t\tsize_t chunk_size, void *data)\n+{\n+\tstruct multi_pack_index *m = (struct multi_pack_index *)data;\n+\tm->chunk_pack_names = chunk_start;\n+\treturn 0;\n+}\n+\n+static int midx_read_oid_fanout(const unsigned char *chunk_start,\n+\t\t\t\tsize_t chunk_size, void *data)\n+{\n+\tstruct multi_pack_index *m = (struct multi_pack_index *)data;\n+\tm->chunk_oid_fanout = (uint32_t *)chunk_start;\n+\n+\tif (chunk_size != 4 * 256) {\n+\t\terror(_(\"multi-pack-index OID fanout is of the wrong size\"));\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n+static int midx_read_oid_lookup(const unsigned char *chunk_start,\n+\t\t\t\tsize_t chunk_size, void *data)\n+{\n+\tstruct multi_pack_index *m = (struct multi_pack_index *)data;\n+\tm->chunk_oid_lookup = chunk_start;\n+\treturn 0;\n+}\n+\n+static int midx_read_offsets(const unsigned char *chunk_start,\n+\t\t\t size_t chunk_size, void *data)\n+{\n+\tstruct multi_pack_index *m = (struct multi_pack_index *)data;\n+\tm->chunk_object_offsets = chunk_start;\n+\treturn 0;\n+}\n+\n+static int midx_read_large_offsets(const unsigned char *chunk_start,\n+\t\t\t\t size_t chunk_size, void *data)\n+{\n+\tstruct multi_pack_index *m = (struct multi_pack_index *)data;\n+\tm->chunk_large_offsets = chunk_start;\n+\treturn 0;\n+}\n+\n+static struct read_chunk_info read_chunks[] = {\n+\t[0] = {\n+\t\tMIDX_CHUNKID_PACKNAMES,\n+\t\tmidx_read_pack_names\n+\t},\n+\t[1] = {\n+\t\tMIDX_CHUNKID_OIDFANOUT,\n+\t\tmidx_read_oid_fanout\n+\t},\n+\t[2] = {\n+\t\tMIDX_CHUNKID_OIDLOOKUP,\n+\t\tmidx_read_oid_lookup\n+\t},\n+\t[3] = {\n+\t\tMIDX_CHUNKID_OBJECTOFFSETS,\n+\t\tmidx_read_offsets\n+\t},\n+\t[4] = {\n+\t\tMIDX_CHUNKID_LARGEOFFSETS,\n+\t\tmidx_read_large_offsets\n+\t}\n+};\n+\n struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local)\n {\n \tstruct multi_pack_index *m = NULL;\n@@ -114,48 +182,10 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n \n \tm->num_packs = get_be32(m->data + MIDX_BYTE_NUM_PACKS);\n \n-\tfor (i = 0; i < m->num_chunks; i++) {\n-\t\tuint32_t chunk_id = get_be32(m->data + MIDX_HEADER_SIZE +\n-\t\t\t\t\t MIDX_CHUNKLOOKUP_WIDTH * i);\n-\t\tuint64_t chunk_offset = get_be64(m->data + MIDX_HEADER_SIZE + 4 +\n-\t\t\t\t\t\t MIDX_CHUNKLOOKUP_WIDTH * i);\n-\n-\t\tif (chunk_offset >= m->data_len)\n-\t\t\tdie(_(\"invalid chunk offset (too large)\"));\n-\n-\t\tswitch (chunk_id) {\n-\t\t\tcase MIDX_CHUNKID_PACKNAMES:\n-\t\t\t\tm->chunk_pack_names = m->data + chunk_offset;\n-\t\t\t\tbreak;\n-\n-\t\t\tcase MIDX_CHUNKID_OIDFANOUT:\n-\t\t\t\tm->chunk_oid_fanout = (uint32_t *)(m->data + chunk_offset);\n-\t\t\t\tbreak;\n-\n-\t\t\tcase MIDX_CHUNKID_OIDLOOKUP:\n-\t\t\t\tm->chunk_oid_lookup = m->data + chunk_offset;\n-\t\t\t\tbreak;\n-\n-\t\t\tcase MIDX_CHUNKID_OBJECTOFFSETS:\n-\t\t\t\tm->chunk_object_offsets = m->data + chunk_offset;\n-\t\t\t\tbreak;\n-\n-\t\t\tcase MIDX_CHUNKID_LARGEOFFSETS:\n-\t\t\t\tm->chunk_large_offsets = m->data + chunk_offset;\n-\t\t\t\tbreak;\n-\n-\t\t\tcase 0:\n-\t\t\t\tdie(_(\"terminating multi-pack-index chunk id appears earlier than expected\"));\n-\t\t\t\tbreak;\n-\n-\t\t\tdefault:\n-\t\t\t\t/*\n-\t\t\t\t * Do nothing on unrecognized chunks, allowing future\n-\t\t\t\t * extensions to add optional chunks.\n-\t\t\t\t */\n-\t\t\t\tbreak;\n-\t\t}\n-\t}\n+\tif (read_table_of_contents(m->data, midx_size, MIDX_HEADER_SIZE,\n+\t\t\t\t m->num_chunks, read_chunks,\n+\t\t\t\t MIDX_MAX_CHUNKS, m))\n+\t\tgoto cleanup_fail;\n \n \tif (!m->chunk_pack_names)\n \t\tdie(_(\"multi-pack-index missing required pack-name chunk\"));\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 2ed0c1544d..65879af6c0 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -563,7 +563,7 @@ test_expect_success 'detect bad hash version' '\n \n test_expect_success 'detect low chunk count' '\n \tcorrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT \"\\01\" \\\n-\t\t\"missing the .* chunk\"\n+\t\t\"final chunk has non-zero id\"\n '\n \n test_expect_success 'detect missing OID fanout chunk' '\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex ace469c95c..a02d612f4d 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -314,12 +314,12 @@ test_expect_success 'verify bad OID version' '\n \n test_expect_success 'verify truncated chunk count' '\n \tcorrupt_midx_and_verify $MIDX_BYTE_CHUNK_COUNT \"\\01\" $objdir \\\n-\t\t\"missing required\"\n+\t\t\"final chunk has non-zero id\"\n '\n \n test_expect_success 'verify extended chunk count' '\n \tcorrupt_midx_and_verify $MIDX_BYTE_CHUNK_COUNT \"\\07\" $objdir \\\n-\t\t\"terminating multi-pack-index chunk id appears earlier than expected\"\n+\t\t\"terminating chunk id appears earlier than expected\"\n '\n \n test_expect_success 'verify missing required chunk' '\n@@ -329,7 +329,7 @@ test_expect_success 'verify missing required chunk' '\n \n test_expect_success 'verify invalid chunk offset' '\n \tcorrupt_midx_and_verify $MIDX_BYTE_CHUNK_OFFSET \"\\01\" $objdir \\\n-\t\t\"invalid chunk offset (too large)\"\n+\t\t\"improper chunk offset(s)\"\n '\n \n test_expect_success 'verify packnames out of order' '\n-- \ngitgitgadget\n\n"},{"id":"411251","messageId":"2ce1c2a54261494df31808660792fef800dc9665.1607012215.git.gitgitgadget@gmail.com","threadId":"54753","inReplyTo":"pull.804.git.1607012215.gitgitgadget@gmail.com","subject":"[PATCH 15/15] chunk-format: add technical docs","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T16:16:54Z","receivedAt":"2020-12-03T16:18:22Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe chunk-based file format is now an API in the code, but we should\nalso take time to document it as a file format. Specifically, it matches\nthe CHUNK LOOKUP sections of the commit-graph and multi-pack-index\nfiles, but there are some commonalities that should be grouped in this\ndocument.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/technical/chunk-format.txt | 54 +++++++++++++++++++\n .../technical/commit-graph-format.txt | 3 ++\n Documentation/technical/pack-format.txt | 3 ++\n 3 files changed, 60 insertions(+)\n create mode 100644 Documentation/technical/chunk-format.txt\n\ndiff --git a/Documentation/technical/chunk-format.txt b/Documentation/technical/chunk-format.txt\nnew file mode 100644\nindex 0000000000..3db3792dea\n--- /dev/null\n+++ b/Documentation/technical/chunk-format.txt\n@@ -0,0 +1,54 @@\n+Chunk-based file formats\n+========================\n+\n+Some file formats in Git use a common concept of \"chunks\" to describe\n+sections of the file. This allows structured access to a large file by\n+scanning a small \"table of contents\" for the remaining data. This common\n+format is used by the `commit-graph` and `multi-pack-index` files. See\n+link:technical/pack-format.html[the `multi-pack-index` format] and\n+link:technical/commit-graph-format.html[the `commit-graph` format] for\n+how they use the chunks to describe structured data.\n+\n+A chunk-based file format begins with some header information custom to\n+that format. That header should include enough information to identify\n+the file type, format version, and number of chunks in the file. From this\n+information, that file can determine the start of the chunk-based region.\n+\n+The chunk-based region starts with a table of contents describing where\n+each chunk starts and ends. This consists of (C+1) rows of 12 bytes each,\n+where C is the number of chunks. Consider the following table:\n+\n+ | Chunk ID (4 bytes) | Chunk Offset (8 bytes) |\n+ |--------------------|------------------------|\n+ | ID[0] | OFFSET[0] |\n+ | ... | ... |\n+ | ID[C] | OFFSET[C] |\n+ | 0x0000 | OFFSET[C+1] |\n+\n+Each row consists of a 4-byte chunk identifier (ID) and an 8-byte offset.\n+Each integer is stored in network-byte order.\n+\n+The chunk identifier `ID[i]` is a label for the data stored within this\n+fill from `OFFSET[i]` (inclusive) to `OFFSET[i+1]` (exclusive). Thus, the\n+size of the `i`th chunk is equal to the difference between `OFFSET[i+1]`\n+and `OFFSET[i]`. This requires that the chunk data appears contiguously\n+in the same order as the table of contents.\n+\n+The final entry in the table of contents must be four zero bytes. This\n+confirms that the table of contents is ending and provides the offset for\n+the end of the chunk-based data.\n+\n+Note: The chunk-based format expects that the file contains _at least_ a\n+trailing hash after `OFFSET[C+1]`.\n+\n+Functions for working with chunk-based file formats are declared in\n+`chunk-format.h`. Using these methods provide extra checks that assist\n+developers when creating new file formats, including:\n+\n+ 1. Writing and reading the table of contents.\n+\n+ 2. Verifying that the data written in a chunk matches the expected size\n+ that was recorded in the table of contents.\n+\n+ 3. Checking that a table of contents describes offsets properly within\n+ the file boundaries.\ndiff --git a/Documentation/technical/commit-graph-format.txt b/Documentation/technical/commit-graph-format.txt\nindex b3b58880b9..b92442780e 100644\n--- a/Documentation/technical/commit-graph-format.txt\n+++ b/Documentation/technical/commit-graph-format.txt\n@@ -65,6 +65,9 @@ CHUNK LOOKUP:\n the length using the next chunk position if necessary.) Each chunk\n ID appears at most once.\n \n+ The CHUNK LOOKUP matches the table of contents from\n+ link:technical/chunk-format.html[the chunk-based file format].\n+\n The remaining data in the body is described one chunk at a time, and\n these chunks may be given in any order. Chunks are required unless\n otherwise specified.\ndiff --git a/Documentation/technical/pack-format.txt b/Documentation/technical/pack-format.txt\nindex f96b2e605f..2fb1e60d29 100644\n--- a/Documentation/technical/pack-format.txt\n+++ b/Documentation/technical/pack-format.txt\n@@ -301,6 +301,9 @@ CHUNK LOOKUP:\n \t (Chunks are provided in file-order, so you can infer the length\n \t using the next chunk position if necessary.)\n \n+\tThe CHUNK LOOKUP matches the table of contents from\n+\tlink:technical/chunk-format.html[the chunk-based file format].\n+\n \tThe remaining data in the body is described one chunk at a time, and\n \tthese chunks may be given in any order. Chunks are required unless\n \totherwise specified.\n-- \ngitgitgadget\n"},{"id":"411252","messageId":"106dd51f75699fbf4fc1e46687124995f5ef0278.1607012215.git.gitgitgadget@gmail.com","threadId":"54753","inReplyTo":"pull.804.git.1607012215.gitgitgadget@gmail.com","subject":"[PATCH 14/15] commit-graph: restore duplicate chunk checks","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T16:16:53Z","receivedAt":"2020-12-03T16:18:23Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe previous change introduced read_table_of_contents() in the\nchunk-format API, but dropped the duplicate chunk check from the\ncommit-graph parsing logic. This was done to keep flexibility in the\nchunk-format API.\n\nOne way to restore this check is to have each chunk_read_fn method check\nif it has run before. This is somewhat repetitive. If we determine that\nthe chunk-format API would be better off with a hard requirement that\nchunks are never repeated, then this could be replaced with a check in\nchunk-format.c.\n\nFor now, only restore the duplicate checks that previously existed in\nthe commit-graph parsing logic.\n\nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 34 ++++++++++++++++++++++++++++++++++\n 1 file changed, 34 insertions(+)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 0a3ba147df..c0102fceba 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -289,10 +289,20 @@ static int verify_commit_graph_lite(struct commit_graph *g)\n \treturn 0;\n }\n \n+static int report_duplicate(void)\n+{\n+\twarning(_(\"duplicate chunk detected\"));\n+\treturn 1;\n+}\n+\n static int graph_read_oid_fanout(const unsigned char *chunk_start,\n \t\t\t\t size_t chunk_size, void *data)\n {\n \tstruct commit_graph *g = (struct commit_graph *)data;\n+\n+\tif (g->chunk_oid_fanout)\n+\t\treturn report_duplicate();\n+\n \tg->chunk_oid_fanout = (uint32_t*)chunk_start;\n \treturn 0;\n }\n@@ -301,6 +311,10 @@ static int graph_read_oid_lookup(const unsigned char *chunk_start,\n \t\t\t\t size_t chunk_size, void *data)\n {\n \tstruct commit_graph *g = (struct commit_graph *)data;\n+\n+\tif (g->chunk_oid_lookup)\n+\t\treturn report_duplicate();\n+\n \tg->chunk_oid_lookup = chunk_start;\n \tg->num_commits = chunk_size / g->hash_len;\n \treturn 0;\n@@ -310,6 +324,10 @@ static int graph_read_data(const unsigned char *chunk_start,\n \t\t\t\t size_t chunk_size, void *data)\n {\n \tstruct commit_graph *g = (struct commit_graph *)data;\n+\n+\tif (g->chunk_commit_data)\n+\t\treturn report_duplicate();\n+\n \tg->chunk_commit_data = chunk_start;\n \treturn 0;\n }\n@@ -318,6 +336,10 @@ static int graph_read_extra_edges(const unsigned char *chunk_start,\n \t\t\t\t size_t chunk_size, void *data)\n {\n \tstruct commit_graph *g = (struct commit_graph *)data;\n+\n+\tif (g->chunk_extra_edges)\n+\t\treturn report_duplicate();\n+\n \tg->chunk_extra_edges = chunk_start;\n \treturn 0;\n }\n@@ -326,6 +348,10 @@ static int graph_read_base_graphs(const unsigned char *chunk_start,\n \t\t\t\t size_t chunk_size, void *data)\n {\n \tstruct commit_graph *g = (struct commit_graph *)data;\n+\n+\tif (g->chunk_base_graphs)\n+\t\treturn report_duplicate();\n+\n \tg->chunk_base_graphs = chunk_start;\n \treturn 0;\n }\n@@ -334,6 +360,10 @@ static int graph_read_bloom_indices(const unsigned char *chunk_start,\n \t\t\t\t size_t chunk_size, void *data)\n {\n \tstruct commit_graph *g = (struct commit_graph *)data;\n+\n+\tif (g->chunk_bloom_indexes)\n+\t\treturn report_duplicate();\n+\n \tg->chunk_bloom_indexes = chunk_start;\n \treturn 0;\n }\n@@ -343,6 +373,10 @@ static int graph_read_bloom_data(const unsigned char *chunk_start,\n {\n \tstruct commit_graph *g = (struct commit_graph *)data;\n \tuint32_t hash_version;\n+\n+\tif (g->chunk_bloom_data)\n+\t\treturn report_duplicate();\n+\n \tg->chunk_bloom_data = chunk_start;\n \thash_version = get_be32(chunk_start);\n \n-- \ngitgitgadget\n\n"},{"id":"411253","messageId":"f2f78ee1054f64fe767335c4b0c77ba2fbec5fae.1607012215.git.gitgitgadget@gmail.com","threadId":"54753","inReplyTo":"pull.804.git.1607012215.gitgitgadget@gmail.com","subject":"[PATCH 10/15] midx: use chunk-format API in write_midx_internal()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-03T16:16:49Z","receivedAt":"2020-12-03T16:18:24Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe chunk-format API allows automatically writing the table of contents\nfor a chunk-based file format when using an array of \"struct\nchunk_info\"s. Update write_midx_internal() to use this strategy, which\nalso simplifies the chunk writing loop. This loop will be replaced with\na chunk-format API call in an upcoming change.\n\nSigned-off-by: Derrick Stolee \n---\n midx.c | 96 +++++++++++++---------------------------------------------\n 1 file changed, 21 insertions(+), 75 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex ce6d4339bd..0548266bea 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -11,6 +11,7 @@\n #include \"trace2.h\"\n #include \"run-command.h\"\n #include \"repository.h\"\n+#include \"chunk-format.h\"\n \n #define MIDX_SIGNATURE 0x4d494458 /* \"MIDX\" */\n #define MIDX_VERSION 1\n@@ -799,15 +800,14 @@ static int write_midx_large_offsets(struct hashfile *f,\n static int write_midx_internal(const char *object_dir, struct multi_pack_index *m,\n \t\t\t struct string_list *packs_to_drop, unsigned flags)\n {\n-\tunsigned char cur_chunk, num_chunks = 0;\n+\tunsigned char num_chunks = 0;\n \tchar *midx_name;\n \tuint32_t i;\n \tstruct hashfile *f = NULL;\n \tstruct lock_file lk;\n \tstruct write_midx_context ctx = { 0 };\n \tuint64_t header_size = 0;\n-\tuint32_t chunk_ids[MIDX_MAX_CHUNKS + 1];\n-\tuint64_t chunk_offsets[MIDX_MAX_CHUNKS + 1];\n+\tstruct chunk_info chunks[MIDX_MAX_CHUNKS];\n \tint pack_name_concat_len = 0;\n \tint dropped_packs = 0;\n \tint result = 0;\n@@ -923,7 +923,6 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \tif (ctx.m)\n \t\tclose_midx(ctx.m);\n \n-\tcur_chunk = 0;\n \tnum_chunks = ctx.large_offsets_needed ? 5 : 4;\n \n \tif (ctx.nr - dropped_packs == 0) {\n@@ -934,85 +933,32 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \n \theader_size = write_midx_header(f, num_chunks, ctx.nr - dropped_packs);\n \n-\tchunk_ids[cur_chunk] = MIDX_CHUNKID_PACKNAMES;\n-\tchunk_offsets[cur_chunk] = header_size + (num_chunks + 1) * MIDX_CHUNKLOOKUP_WIDTH;\n+\tchunks[0].id = MIDX_CHUNKID_PACKNAMES;\n+\tchunks[0].size = pack_name_concat_len;\n+\tchunks[0].write_fn = write_midx_pack_names;\n \n-\tcur_chunk++;\n-\tchunk_ids[cur_chunk] = MIDX_CHUNKID_OIDFANOUT;\n-\tchunk_offsets[cur_chunk] = chunk_offsets[cur_chunk - 1] + pack_name_concat_len;\n+\tchunks[1].id = MIDX_CHUNKID_OIDFANOUT;\n+\tchunks[1].size = MIDX_CHUNK_FANOUT_SIZE;\n+\tchunks[1].write_fn = write_midx_oid_fanout;\n \n-\tcur_chunk++;\n-\tchunk_ids[cur_chunk] = MIDX_CHUNKID_OIDLOOKUP;\n-\tchunk_offsets[cur_chunk] = chunk_offsets[cur_chunk - 1] + MIDX_CHUNK_FANOUT_SIZE;\n+\tchunks[2].id = MIDX_CHUNKID_OIDLOOKUP;\n+\tchunks[2].size = ctx.entries_nr * the_hash_algo->rawsz;\n+\tchunks[2].write_fn = write_midx_oid_lookup;\n \n-\tcur_chunk++;\n-\tchunk_ids[cur_chunk] = MIDX_CHUNKID_OBJECTOFFSETS;\n-\tchunk_offsets[cur_chunk] = chunk_offsets[cur_chunk - 1] + ctx.entries_nr * the_hash_algo->rawsz;\n+\tchunks[3].id = MIDX_CHUNKID_OBJECTOFFSETS;\n+\tchunks[3].size = ctx.entries_nr * MIDX_CHUNK_OFFSET_WIDTH;\n+\tchunks[3].write_fn = write_midx_object_offsets;\n \n-\tcur_chunk++;\n-\tchunk_offsets[cur_chunk] = chunk_offsets[cur_chunk - 1] + ctx.entries_nr * MIDX_CHUNK_OFFSET_WIDTH;\n \tif (ctx.large_offsets_needed) {\n-\t\tchunk_ids[cur_chunk] = MIDX_CHUNKID_LARGEOFFSETS;\n-\n-\t\tcur_chunk++;\n-\t\tchunk_offsets[cur_chunk] = chunk_offsets[cur_chunk - 1] +\n-\t\t\t\t\t ctx.num_large_offsets * MIDX_CHUNK_LARGE_OFFSET_WIDTH;\n+\t\tchunks[4].id = MIDX_CHUNKID_LARGEOFFSETS;\n+\t\tchunks[4].size = ctx.num_large_offsets * MIDX_CHUNK_LARGE_OFFSET_WIDTH;\n+\t\tchunks[4].write_fn = write_midx_large_offsets;\n \t}\n \n-\tchunk_ids[cur_chunk] = 0;\n-\n-\tfor (i = 0; i <= num_chunks; i++) {\n-\t\tif (i && chunk_offsets[i] < chunk_offsets[i - 1])\n-\t\t\tBUG(\"incorrect chunk offsets: %\"PRIu64\" before %\"PRIu64,\n-\t\t\t chunk_offsets[i - 1],\n-\t\t\t chunk_offsets[i]);\n-\n-\t\tif (chunk_offsets[i] % MIDX_CHUNK_ALIGNMENT)\n-\t\t\tBUG(\"chunk offset %\"PRIu64\" is not properly aligned\",\n-\t\t\t chunk_offsets[i]);\n-\n-\t\thashwrite_be32(f, chunk_ids[i]);\n-\t\thashwrite_be64(f, chunk_offsets[i]);\n-\t}\n-\n-\tfor (i = 0; i < num_chunks; i++) {\n-\t\tif (f->total + f->offset != chunk_offsets[i])\n-\t\t\tBUG(\"incorrect chunk offset (%\"PRIu64\" != %\"PRIu64\") for chunk id %\"PRIx32,\n-\t\t\t chunk_offsets[i],\n-\t\t\t f->total + f->offset,\n-\t\t\t chunk_ids[i]);\n-\n-\t\tswitch (chunk_ids[i]) {\n-\t\t\tcase MIDX_CHUNKID_PACKNAMES:\n-\t\t\t\twrite_midx_pack_names(f, &ctx);\n-\t\t\t\tbreak;\n-\n-\t\t\tcase MIDX_CHUNKID_OIDFANOUT:\n-\t\t\t\twrite_midx_oid_fanout(f, &ctx);\n-\t\t\t\tbreak;\n-\n-\t\t\tcase MIDX_CHUNKID_OIDLOOKUP:\n-\t\t\t\twrite_midx_oid_lookup(f, &ctx);\n-\t\t\t\tbreak;\n-\n-\t\t\tcase MIDX_CHUNKID_OBJECTOFFSETS:\n-\t\t\t\twrite_midx_object_offsets(f, &ctx);\n-\t\t\t\tbreak;\n-\n-\t\t\tcase MIDX_CHUNKID_LARGEOFFSETS:\n-\t\t\t\twrite_midx_large_offsets(f, &ctx);\n-\t\t\t\tbreak;\n-\n-\t\t\tdefault:\n-\t\t\t\tBUG(\"trying to write unknown chunk id %\"PRIx32,\n-\t\t\t\t chunk_ids[i]);\n-\t\t}\n-\t}\n+\twrite_table_of_contents(f, header_size, chunks, num_chunks);\n \n-\tif (f->total + f->offset != chunk_offsets[num_chunks])\n-\t\tBUG(\"incorrect final offset %\"PRIu64\" != %\"PRIu64,\n-\t\t f->total + f->offset,\n-\t\t chunk_offsets[num_chunks]);\n+\tfor (i = 0; i < num_chunks; i++)\n+\t\tchunks[i].write_fn(f, &ctx);\n \n \tfinalize_hashfile(f, NULL, CSUM_FSYNC | CSUM_HASH_IN_STREAM);\n \tcommit_lock_file(&lk);\n-- \ngitgitgadget\n\n"},{"id":"411282","messageId":"xmqqim9ih8hz.fsf@gitster.c.googlers.com","threadId":"54753","inReplyTo":"491667de2baef422e801df1e2c7d3173462a96ff.1607012215.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 05/15] midx: add entries to write_midx_context","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-03T21:42:16Z","receivedAt":"2020-12-03T21:43:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> -static size_t write_midx_oid_lookup(struct hashfile *f, unsigned char hash_len,\n> -\t\t\t\t struct pack_midx_entry *objects,\n> -\t\t\t\t uint32_t nr_objects)\n> +static size_t write_midx_oid_lookup(struct hashfile *f,\n> +\t\t\t\t void *data)\n> {\n> -\tstruct pack_midx_entry *list = objects;\n> +\tstruct write_midx_context *ctx = (struct write_midx_context *)data;\n> +\tunsigned char hash_len = the_hash_algo->rawsz;\n> +\tstruct pack_midx_entry *list = ctx->entries;\n\nI know this is meant to be a faithful rewrite, but can we lose this\n\"length of the hash function output must be smaller than 256\"\nimposed by \"unsigned char\" at some point, perhaps after this series\nsettles? .rawsz field is size_t IIRC.\n\n"},{"id":"411283","messageId":"xmqqeek6h853.fsf@gitster.c.googlers.com","threadId":"54753","inReplyTo":"dbb637a7ac6d2cc6bb78428ef7bda67687095f88.1607012215.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 08/15] midx: convert chunk write methods to return int","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-03T21:50:00Z","receivedAt":"2020-12-03T21:50:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n\n> Subject: Re: [PATCH 08/15] midx: convert chunk write methods to return int\n\nPlease rephrase the title to avoid misleading the readers; it is not\nlike we used to return the number of bytes written in size_t but we\nrealized that we never write mroe than INTMAX and narrowed the\nreturn type to \"int\".\n\nIn other words, returning \"int\" is not the most important thing in\nthis change. What is more important is that the function will\neventually signal success/failure with its return value.\n\n"},{"id":"411284","messageId":"xmqqa6uuh7n1.fsf@gitster.c.googlers.com","threadId":"54753","inReplyTo":"c6a433efba3214e83a7265e53c24cb4001345f14.1607012215.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 11/15] midx: use 64-bit multiplication for chunk sizes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-03T22:00:50Z","receivedAt":"2020-12-03T22:01:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> When calculating the sizes of certain chunks, we should use 64-bit\n> multiplication always. This allows us to properly predict the chunk\n> sizes without risk of overflow.\n\nThat's quite an obvious fix, applicable even before this entire\nseries. I think we saw quite similar bugfixes in different parts of\nthe codebase recently.\n\nSo far, everything looks good.\n\n\n> Signed-off-by: Derrick Stolee \n> ---\n> midx.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/midx.c b/midx.c\n> index 0548266bea..47f5f60fcd 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -946,12 +946,12 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n> \tchunks[2].write_fn = write_midx_oid_lookup;\n> \n> \tchunks[3].id = MIDX_CHUNKID_OBJECTOFFSETS;\n> -\tchunks[3].size = ctx.entries_nr * MIDX_CHUNK_OFFSET_WIDTH;\n> +\tchunks[3].size = (uint64_t)ctx.entries_nr * MIDX_CHUNK_OFFSET_WIDTH;\n> \tchunks[3].write_fn = write_midx_object_offsets;\n> \n> \tif (ctx.large_offsets_needed) {\n> \t\tchunks[4].id = MIDX_CHUNKID_LARGEOFFSETS;\n> -\t\tchunks[4].size = ctx.num_large_offsets * MIDX_CHUNK_LARGE_OFFSET_WIDTH;\n> +\t\tchunks[4].size = (uint64_t)ctx.num_large_offsets * MIDX_CHUNK_LARGE_OFFSET_WIDTH;\n> \t\tchunks[4].write_fn = write_midx_large_offsets;\n> \t}\n"},{"id":"411285","messageId":"xmqq5z5ih6v1.fsf@gitster.c.googlers.com","threadId":"54753","inReplyTo":"6801e231f7414444a272f2ea87dcc6f60f29e25a.1607012215.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 13/15] chunk-format: create chunk reading API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-03T22:17:38Z","receivedAt":"2020-12-03T22:18:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> Now that the chunk-format API has a consistent mechanism for writing\n> file formats based on chunks, let's extend it to also parse chunk-based\n> files during read.\n\nHeads-up. The generation-data work by Abhishek would conflict with\nthe way how this refactors the way the beginning offset of each\nchunk is found.\n"},{"id":"411286","messageId":"xmqq1rg6h5o6.fsf@gitster.c.googlers.com","threadId":"54753","inReplyTo":"6801e231f7414444a272f2ea87dcc6f60f29e25a.1607012215.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 13/15] chunk-format: create chunk reading API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-03T22:43:21Z","receivedAt":"2020-12-03T22:44:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n> ...\n> -\t\tcase GRAPH_CHUNKID_DATA:\n> -\t\t\tif (graph->chunk_commit_data)\n> -\t\t\t\tchunk_repeated = 1;\n> -\t\t\telse\n> -\t\t\t\tgraph->chunk_commit_data = data + chunk_offset;\n> -\t\t\tbreak;\n> +\t/* limit the chunk-format list if we are ignoring Bloom filters */\n> +\tif (!r->settings.commit_graph_read_changed_paths)\n> +\t\tchunks_nr = 5;\n\nI presume that this magic number 5 is directly connected to ...\n\n> +static struct read_chunk_info read_chunks[] = {\n> +\t[0] = {\n> +\t\tGRAPH_CHUNKID_OIDFANOUT,\n> +\t\tgraph_read_oid_fanout\n> +\t},\n> +\t[1] = {\n> +\t\tGRAPH_CHUNKID_OIDLOOKUP,\n> +\t\tgraph_read_oid_lookup\n> +\t},\n> +\t[2] = {\n> +\t\tGRAPH_CHUNKID_DATA,\n> +\t\tgraph_read_data\n> +\t},\n> +\t[3] = {\n> +\t\tGRAPH_CHUNKID_EXTRAEDGES,\n> +\t\tgraph_read_extra_edges\n> +\t},\n> +\t[4] = {\n> +\t\tGRAPH_CHUNKID_BASE,\n> +\t\tgraph_read_base_graphs\n> +\t},\n> +\t[5] = {\n> +\t\tGRAPH_CHUNKID_BLOOMINDEXES,\n> +\t\tgraph_read_bloom_indices\n> +\t},\n> +\t[6] = {\n> +\t\tGRAPH_CHUNKID_BLOOMDATA,\n> +\t\tgraph_read_bloom_data\n> +\t}\n> +};\n\n... the index of these entries in the table.\n\nI find this arrangement brittle. For one thing, it means that new\nchunks cannot be added at an arbitrary place, and BLOOMDATA must be\nat the end for the \"limit the list when ignoring\" logic to work\ncorrectly (even when the developer who is adding a new chunk type\nrealizes that new one must be inserted before [5], and the magic\nnumber 5 above must be updated), and it won't work at all if a\nsimilar \"we can optionally ignore reading the data\" treatment needs\nto be made for new chunk types, since two or more things cannot be\nat the end of the list at the same time X-<.\n\nWould it be a cleaner way to implement this \"when member X in the\nsettings structure is not set, ignore BLOOMINDEXES and BLOOMDATA\"\nfeature to let these graph_read_*() functions have access to the\nsettings structure, so that bloom_indices and bloom_data callback\nfunctions can make decisions for themselves?\n\nOnce we do that, as far as I can tell, there is no longer any reason\nto initialize read_chunks[] array with designated initializer. The\nimplementation of read_table_of_contents() does not tolerate if this\narray is sparsely populated anyway, and except for the \"do we ignore\nbloom?\" hack, nothing really depends on the order of entries in the\narray, right?\n\nThanks.\n"},{"id":"411320","messageId":"ee0b73f7-8f59-a1dc-0a21-bf796bf9f2e2@web.de","threadId":"54753","inReplyTo":"pull.804.git.1607012215.gitgitgadget@gmail.com","subject":"Re: [PATCH 00/15] Refactor chunk-format into an API","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-12-04T12:48:31Z","receivedAt":"2020-12-04T12:50:42Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.12.20 um 17:16 schrieb Derrick Stolee via GitGitGadget:\n> I was thinking about file formats recently and realized that the \"chunks\"\n> that are common to the commit-graph and multi-pack-index could inform future\n> file formats. To make that process easier, let's combine the process of\n> writing and reading chunks into a common API that both of these existing\n> formats use.\n>\n> There is some extra benefit immediately: the writing and reading code for\n> each gets a bit cleaner. Also, there were different checks in each that made\n> the process more robust. Now, these share a common set of checks.\n\n> Documentation/technical/chunk-format.txt | 54 ++\n> .../technical/commit-graph-format.txt | 3 +\n> Documentation/technical/pack-format.txt | 3 +\n> Makefile | 1 +\n> chunk-format.c | 105 ++++\n> chunk-format.h | 69 +++\n> commit-graph.c | 298 ++++++-----\n> midx.c | 466 ++++++++----------\n> t/t5318-commit-graph.sh | 2 +-\n> t/t5319-multi-pack-index.sh | 6 +-\n> 10 files changed, 623 insertions(+), 384 deletions(-)\n\n623-384-54-3-3-1-69-2-6 = 101\n\nSo if we ignore changes to documentation, headers, tests and build\nscript this spends ca. 100 more lines of code than the current version.\nThat's roughly the size of the new file chunk-format.c -- from this\nbird's-eye-view the new API seems to be pure overhead.\n\nIn the new code I see several magic numbers, use of void pointers and\ncasting as well as repetition -- is this really going in the right\ndirection? I get the feeling that YAGNI.\n\nRené\n"},{"id":"411321","messageId":"f9ca5bd8-3a7a-232b-f038-3311087d60f1@gmail.com","threadId":"54753","inReplyTo":"xmqqim9ih8hz.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 05/15] midx: add entries to write_midx_context","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-12-04T13:39:39Z","receivedAt":"2020-12-04T13:40:37Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/3/2020 4:42 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> -static size_t write_midx_oid_lookup(struct hashfile *f, unsigned char hash_len,\n>> -\t\t\t\t struct pack_midx_entry *objects,\n>> -\t\t\t\t uint32_t nr_objects)\n>> +static size_t write_midx_oid_lookup(struct hashfile *f,\n>> +\t\t\t\t void *data)\n>> {\n>> -\tstruct pack_midx_entry *list = objects;\n>> +\tstruct write_midx_context *ctx = (struct write_midx_context *)data;\n>> +\tunsigned char hash_len = the_hash_algo->rawsz;\n>> +\tstruct pack_midx_entry *list = ctx->entries;\n> \n> I know this is meant to be a faithful rewrite, but can we lose this\n> \"length of the hash function output must be smaller than 256\"\n> imposed by \"unsigned char\" at some point, perhaps after this series\n> settles? .rawsz field is size_t IIRC.\n\nThere's no reason why this should be \"unsigned char\" except that\nI was probably thinking about the \"hash version\" byte in the\nheader when I added this line. Clearly there isn't a failure (yet),\nbut it's better to be safe.\n\nThanks,\n-Stolee\n"},{"id":"411322","messageId":"eefba4cd-4989-49ac-4967-264af9f99c68@gmail.com","threadId":"54753","inReplyTo":"xmqqeek6h853.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 08/15] midx: convert chunk write methods to return int","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-12-04T13:40:34Z","receivedAt":"2020-12-04T13:41:17Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/3/2020 4:50 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> From: Derrick Stolee \n> \n>> Subject: Re: [PATCH 08/15] midx: convert chunk write methods to return int\n> \n> Please rephrase the title to avoid misleading the readers; it is not\n> like we used to return the number of bytes written in size_t but we\n> realized that we never write mroe than INTMAX and narrowed the\n> return type to \"int\".\n> \n> In other words, returning \"int\" is not the most important thing in\n> this change. What is more important is that the function will\n> eventually signal success/failure with its return value.\n \nI suppose \"return int\" is what I meant, but this is probably better:\n\n midx: return success/failure in chunk write methods\n\nThanks,\n-Stolee\n"},{"id":"411323","messageId":"1df7dec3-37d1-9ebf-b9c5-66d5bd4cbaa1@gmail.com","threadId":"54753","inReplyTo":"xmqq1rg6h5o6.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 13/15] chunk-format: create chunk reading API","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-12-04T13:45:10Z","receivedAt":"2020-12-04T13:45:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/3/2020 5:43 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> From: Derrick Stolee \n>> ...\n>> -\t\tcase GRAPH_CHUNKID_DATA:\n>> -\t\t\tif (graph->chunk_commit_data)\n>> -\t\t\t\tchunk_repeated = 1;\n>> -\t\t\telse\n>> -\t\t\t\tgraph->chunk_commit_data = data + chunk_offset;\n>> -\t\t\tbreak;\n>> +\t/* limit the chunk-format list if we are ignoring Bloom filters */\n>> +\tif (!r->settings.commit_graph_read_changed_paths)\n>> +\t\tchunks_nr = 5;\n> \n> I presume that this magic number 5 is directly connected to ...\n> \n>> +static struct read_chunk_info read_chunks[] = {\n>> +\t[0] = {\n>> +\t\tGRAPH_CHUNKID_OIDFANOUT,\n>> +\t\tgraph_read_oid_fanout\n>> +\t},\n>> +\t[1] = {\n>> +\t\tGRAPH_CHUNKID_OIDLOOKUP,\n>> +\t\tgraph_read_oid_lookup\n>> +\t},\n>> +\t[2] = {\n>> +\t\tGRAPH_CHUNKID_DATA,\n>> +\t\tgraph_read_data\n>> +\t},\n>> +\t[3] = {\n>> +\t\tGRAPH_CHUNKID_EXTRAEDGES,\n>> +\t\tgraph_read_extra_edges\n>> +\t},\n>> +\t[4] = {\n>> +\t\tGRAPH_CHUNKID_BASE,\n>> +\t\tgraph_read_base_graphs\n>> +\t},\n>> +\t[5] = {\n>> +\t\tGRAPH_CHUNKID_BLOOMINDEXES,\n>> +\t\tgraph_read_bloom_indices\n>> +\t},\n>> +\t[6] = {\n>> +\t\tGRAPH_CHUNKID_BLOOMDATA,\n>> +\t\tgraph_read_bloom_data\n>> +\t}\n>> +};\n> \n> ... the index of these entries in the table.\n> \n> I find this arrangement brittle. For one thing, it means that new\n> chunks cannot be added at an arbitrary place, and BLOOMDATA must be\n> at the end for the \"limit the list when ignoring\" logic to work\n> correctly (even when the developer who is adding a new chunk type\n> realizes that new one must be inserted before [5], and the magic\n> number 5 above must be updated), and it won't work at all if a\n> similar \"we can optionally ignore reading the data\" treatment needs\n> to be made for new chunk types, since two or more things cannot be\n> at the end of the list at the same time X-<.\n> \n> Would it be a cleaner way to implement this \"when member X in the\n> settings structure is not set, ignore BLOOMINDEXES and BLOOMDATA\"\n> feature to let these graph_read_*() functions have access to the\n> settings structure, so that bloom_indices and bloom_data callback\n> functions can make decisions for themselves?\n> \n> Once we do that, as far as I can tell, there is no longer any reason\n> to initialize read_chunks[] array with designated initializer. The\n> implementation of read_table_of_contents() does not tolerate if this\n> array is sparsely populated anyway, and except for the \"do we ignore\n> bloom?\" hack, nothing really depends on the order of entries in the\n> array, right?\n\nThis is a good point, and it's my fault for not noticing this behavior.\nIn an earlier version, I was initializing read_chunks dynamically\ninside parse_commit_graph(), where the change made more sense (the\nBloom chunks were not initialized at all based on this condition). I\nthink the better way to handle this is to check the config within\nthe reading functions graph_read_bloom_(indices|data).\n\nThe added benefit of checking in the read_chunk_fn is that the\nchunk-format API can see that these chunks are \"known\" chunk types,\nin case we were to add logging for \"I don't understand this chunk\ntype\".\n\nThanks,\n-Stolee\n"},{"id":"411324","messageId":"a7be6d74-373e-e18e-b915-487bab28f438@gmail.com","threadId":"54753","inReplyTo":"xmqq5z5ih6v1.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 13/15] chunk-format: create chunk reading API","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-12-04T13:47:03Z","receivedAt":"2020-12-04T13:48:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/3/2020 5:17 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> From: Derrick Stolee \n>>\n>> Now that the chunk-format API has a consistent mechanism for writing\n>> file formats based on chunks, let's extend it to also parse chunk-based\n>> files during read.\n> \n> Heads-up. The generation-data work by Abhishek would conflict with\n> the way how this refactors the way the beginning offset of each\n> chunk is found.\n\nRight! I should have checked against 'seen'. Abhishek's work should\ntake precedence and I can adapt on top of his topic. For now, feel\nfree to keep this topic out of 'seen' while we address the worth\nof the topic (re: René's concerns).\n\nThanks,\n-Stolee\n\n"},{"id":"411325","messageId":"3e3387b6-d2ab-7d86-0535-2ef182fbc469@gmail.com","threadId":"54753","inReplyTo":"ee0b73f7-8f59-a1dc-0a21-bf796bf9f2e2@web.de","subject":"Re: [PATCH 00/15] Refactor chunk-format into an API","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-12-04T13:57:34Z","receivedAt":"2020-12-04T13:58:22Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/4/2020 7:48 AM, René Scharfe wrote:\n> Am 03.12.20 um 17:16 schrieb Derrick Stolee via GitGitGadget:\n...\n>> Documentation/technical/chunk-format.txt | 54 ++\n>> .../technical/commit-graph-format.txt | 3 +\n>> Documentation/technical/pack-format.txt | 3 +\n>> Makefile | 1 +\n>> chunk-format.c | 105 ++++\n>> chunk-format.h | 69 +++\n>> commit-graph.c | 298 ++++++-----\n>> midx.c | 466 ++++++++----------\n>> t/t5318-commit-graph.sh | 2 +-\n>> t/t5319-multi-pack-index.sh | 6 +-\n>> 10 files changed, 623 insertions(+), 384 deletions(-)\n> \n> 623-384-54-3-3-1-69-2-6 = 101\n> \n> So if we ignore changes to documentation, headers, tests and build\n> script this spends ca. 100 more lines of code than the current version.\n> That's roughly the size of the new file chunk-format.c -- from this\n> bird's-eye-view the new API seems to be pure overhead.\n\nOverhead in terms of lines of code, but many of those are function\nprototypes and single lines containing only \"{\" and \"}\". So yes,\nthe code files are a bit longer, but the amount of executed code is\nnot meaningfully different.\n\nExtra lines of code is an expected cost of refactoring. The remaining\nquestion is, \"is it worth the cost?\" I believe it is.\n \n> In the new code I see several magic numbers, use of void pointers and\n> casting as well as repetition -- is this really going in the right\n> direction? I get the feeling that YAGNI.\n\nvoid pointers are a cost of abstraction in C that we use all over the\ncodebase.\n\nYou (and Junio) are right to point out my magic numbers. Those should\nbe replaced with something better when possible.\n\nAs far as YAGNI, I doubt that very much. First, we have already seen\nextensions to the commit-graph that added several new chunks, and\nplugging into this (documented) API should be easier than the previous\nad-hoc mechanism.\n\nI've CC'd Abhishek to get his opinion, since he's recently added chunks\nto the commit-graph file. Outside of the fact that this series conflicts\nwith his series (which I will fix), it would be good to see if he\nappreciates this model.\n\n>> I was thinking about file formats recently and realized that the \"chunks\"\n>> that are common to the commit-graph and multi-pack-index could inform future\n>> file formats. To make that process easier, let's combine the process of\n>> writing and reading chunks into a common API that both of these existing\n>> formats use.\n\nAnd another point on YAGNI: I'm literally prototyping a new file format and\nwant to use this API to build it instead of repeating myself. Specifically,\nI noticed that the commit-graph and multi-pack-index were inconsistent in\nhow they protected the file format in different ways during writes and reads.\nThis leads to...\n\n>> There is some extra benefit immediately: the writing and reading code for\n>> each gets a bit cleaner. Also, there were different checks in each that made\n>> the process more robust. Now, these share a common set of checks.\n\n...my point that combining these checks make both codepaths slightly more\nrobust. I didn't even include the potential extension of storing the size\nof each chunk in \"struct commit_graph\" and \"struct multi_pack_index\" for\nrun-time bound checks during lookups. That seemed like too much new\nbehavior for a series that intends to only refactor.\n\nThanks,\n-Stolee\n"},{"id":"411361","messageId":"xmqq7dpxfjdq.fsf@gitster.c.googlers.com","threadId":"54753","inReplyTo":"ee0b73f7-8f59-a1dc-0a21-bf796bf9f2e2@web.de","subject":"Re: [PATCH 00/15] Refactor chunk-format into an API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-04T19:42:25Z","receivedAt":"2020-12-04T19:43:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> Documentation/technical/chunk-format.txt | 54 ++\n>> .../technical/commit-graph-format.txt | 3 +\n>> Documentation/technical/pack-format.txt | 3 +\n>> Makefile | 1 +\n>> chunk-format.c | 105 ++++\n>> chunk-format.h | 69 +++\n>> commit-graph.c | 298 ++++++-----\n>> midx.c | 466 ++++++++----------\n>> t/t5318-commit-graph.sh | 2 +-\n>> t/t5319-multi-pack-index.sh | 6 +-\n>> 10 files changed, 623 insertions(+), 384 deletions(-)\n>\n> 623-384-54-3-3-1-69-2-6 = 101\n>\n> So if we ignore changes to documentation, headers, tests and build\n> script this spends ca. 100 more lines of code than the current version.\n> That's roughly the size of the new file chunk-format.c -- from this\n> bird's-eye-view the new API seems to be pure overhead.\n>\n> In the new code I see several magic numbers, use of void pointers and\n> casting as well as repetition -- is this really going in the right\n> direction? I get the feeling that YAGNI.\n\nHmph, two existing users consolidated into one and still not losing\nlines is not a very convincing sign. Perhaps a third existing user\nwould purely lose lines when converted to use this (do we have a\nthird or fourth one?) I dunno.\n\n"},{"id":"411374","messageId":"xmqq8sade360.fsf@gitster.c.googlers.com","threadId":"54753","inReplyTo":"a7be6d74-373e-e18e-b915-487bab28f438@gmail.com","subject":"Re: [PATCH 13/15] chunk-format: create chunk reading API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-04T20:17:59Z","receivedAt":"2020-12-04T20:19:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 12/3/2020 5:17 PM, Junio C Hamano wrote:\n>> \"Derrick Stolee via GitGitGadget\" writes:\n>> \n>>> From: Derrick Stolee \n>>>\n>>> Now that the chunk-format API has a consistent mechanism for writing\n>>> file formats based on chunks, let's extend it to also parse chunk-based\n>>> files during read.\n>> \n>> Heads-up. The generation-data work by Abhishek would conflict with\n>> the way how this refactors the way the beginning offset of each\n>> chunk is found.\n>\n> Right! I should have checked against 'seen'. Abhishek's work should\n> take precedence and I can adapt on top of his topic. For now, feel\n> free to keep this topic out of 'seen' while we address the worth\n> of the topic (re: René's concerns).\n\nSure. We may want to do [PATCH 11/15] even if the \"worth of the\ntopic\" discussion ends up negative, though.\n\nThanks.\n"},{"id":"411600","messageId":"86e1d647-2242-8a3b-d5a3-832e37e79ff8@gmail.com","threadId":"54753","inReplyTo":"106dd51f75699fbf4fc1e46687124995f5ef0278.1607012215.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 14/15] commit-graph: restore duplicate chunk checks","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-12-07T13:43:24Z","receivedAt":"2020-12-07T13:44:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/3/2020 11:16 AM, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> The previous change introduced read_table_of_contents() in the\n> chunk-format API, but dropped the duplicate chunk check from the\n> commit-graph parsing logic. This was done to keep flexibility in the\n> chunk-format API.\n\n\"keep flexibility\" is bogus. This is the biggest YAGNI of this\nseries. Instead, consider the patch below instead which restores\nduplicate checks for the commit-graph file AND adds them to the\nmulti-pack-index file due to the shared API.\n\nThis is also roughly half of the added lines from the previous\npatch.\n\nThanks,\n-Stolee\n\n-- >8 --\n\nFrom 0df4959d59d7f9df3e9f6326bb0acb7b84f84980 Mon Sep 17 00:00:00 2001\nFrom: Derrick Stolee \nDate: Mon, 7 Dec 2020 08:36:42 -0500\nSubject: [PATCH] chunk-format: restore duplicate chunk checks\n\nBefore refactoring into the chunk-format API, the commit-graph parsing\nlogic included checks for duplicate chunks. It is unlikely that we would\ndesire a chunk-based file format that allows duplicate chunk IDs in the\ntable of contents, so add duplicate checks into\nread_table_of_contents().\n\nSigned-off-by: Derrick Stolee \n---\n chunk-format.c | 15 ++++++++++++++-\n chunk-format.h | 3 +++\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/chunk-format.c b/chunk-format.c\nindex d888ef6ec73..a4891bbd28a 100644\n--- a/chunk-format.c\n+++ b/chunk-format.c\n@@ -57,9 +57,13 @@ int read_table_of_contents(const unsigned char *mfile,\n \t\t\t int nr,\n \t\t\t void *data)\n {\n+\tint i;\n \tuint32_t chunk_id;\n \tconst unsigned char *table_of_contents = mfile + toc_offset;\n \n+\tfor (i = 0; i < nr; i++)\n+\t\tchunks[i].found = 0;\n+\n \twhile (toc_length--) {\n \t\tint i;\n \t\tuint64_t chunk_offset, next_chunk_offset;\n@@ -83,7 +87,16 @@ int read_table_of_contents(const unsigned char *mfile,\n \t\t}\n \t\tfor (i = 0; i < nr; i++) {\n \t\t\tif (chunks[i].id == chunk_id) {\n-\t\t\t\tint result = chunks[i].read_fn(\n+\t\t\t\tint result;\n+\n+\t\t\t\tif (chunks[i].found) {\n+\t\t\t\t\terror(_(\"duplicate chunk ID %\"PRIx32\" found\"),\n+\t\t\t\t\t chunk_id);\n+\t\t\t\t\treturn 1;\n+\t\t\t\t}\n+\n+\t\t\t\tchunks[i].found = 1;\n+\t\t\t\tresult = chunks[i].read_fn(\n \t\t\t\t\t\tmfile + chunk_offset,\n \t\t\t\t\t\tnext_chunk_offset - chunk_offset,\n \t\t\t\t\t\tdata);\ndiff --git a/chunk-format.h b/chunk-format.h\nindex 7049800f734..de45797223a 100644\n--- a/chunk-format.h\n+++ b/chunk-format.h\n@@ -56,6 +56,9 @@ typedef int (*chunk_read_fn)(const unsigned char *chunk_start,\n struct read_chunk_info {\n \tuint32_t id;\n \tchunk_read_fn read_fn;\n+\n+\t/* used internally */\n+\tunsigned found:1;\n };\n \n int read_table_of_contents(const unsigned char *mfile,\n-- \n2.29.0.vfs.0.0\n\n\n\n"},{"id":"411727","messageId":"X8++THm5K3O9AUrT@nand.local","threadId":"54753","inReplyTo":"da7933cc59928c7995821dcc72ceae0c690ebaca.1607012215.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 02/15] chunk-format: add API for writing table of contents","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-12-08T17:56:28Z","receivedAt":"2020-12-08T17:57:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Dec 03, 2020 at 04:16:41PM +0000, Derrick Stolee via GitGitGadget wrote:\n> @@ -0,0 +1,26 @@\n> +#include \"git-compat-util.h\"\n> +#include \"chunk-format.h\"\n> +#include \"csum-file.h\"\n> +#define CHUNK_LOOKUP_WIDTH 12\n> +\n> +void write_table_of_contents(struct hashfile *f,\n> +\t\t\t uint64_t cur_offset,\n\nHmmph. Could we not use hashfile_total() here instead? I wonder how much\nthe callers would be able to be cleaned up without having to keep track\nof the offset themselves.\n\nIs this to indicate that we might call hashfd() on a file that already\nhas data in it at an offset that doesn't cause us to overwrite that\ndata? I doubt it (since both callers currently write all of their data\ninto a hashfile), but if so that should be documented, too.\n\n(In either case, off_t is a more appropriate type than uint64_t here.)\n\n> @@ -0,0 +1,36 @@\n> +#ifndef CHUNK_FORMAT_H\n> +#define CHUNK_FORMAT_H\n> +\n> +#include \"git-compat-util.h\"\n> +\n> +struct hashfile;\n> +\n> +typedef int (*chunk_write_fn)(struct hashfile *f,\n> +\t\t\t void *data);\n> +\n> +/*\n> + * When writing a chunk-based file format, collect the chunks in\n> + * an array of chunk_info structs. The size stores the _expected_\n\nI'm not clear on what \"_expected_\" means here. Does that mean that the\nactual data written may be over- or under-sized? If not, does violating\nthe expectation cause an error?\n\nI realize that I'm being pedantic, but documenting this would be useful\nto callers.\n\n> + * amount of data that will be written by write_fn.\n> + */\n> +struct chunk_info {\n> +\tuint32_t id;\n> +\tuint64_t size;\n> +\tchunk_write_fn write_fn;\n> +};\n> +\n> +/*\n> + * Write the chunk data into the supplied hashfile.\n> + *\n> + * * 'cur_offset' indicates the number of bytes written to the hashfile\n> + * before the table of contents starts.\n> + *\n> + * * 'nr' is the number of chunks with non-zero IDs, so 'nr + 1'\n> + * chunks are written in total.\n> + */\n> +void write_table_of_contents(struct hashfile *f,\n> +\t\t\t uint64_t cur_offset,\n\nSame goes here about cur_offset.\n\nThanks,\nTaylor\n"},{"id":"411728","messageId":"X8+/WHBTO6GGH6Dt@nand.local","threadId":"54753","inReplyTo":"491667de2baef422e801df1e2c7d3173462a96ff.1607012215.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 05/15] midx: add entries to write_midx_context","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-12-08T18:00:56Z","receivedAt":"2020-12-08T18:01:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Dec 03, 2020 at 04:16:44PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> diff --git a/midx.c b/midx.c\n> index 6ab655ddda..2af4452165 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -458,6 +458,9 @@ struct write_midx_context {\n> \tstruct multi_pack_index *m;\n> \tstruct progress *progress;\n> \tunsigned pack_paths_checked;\n> +\n> +\tstruct pack_midx_entry *entries;\n> +\tuint32_t entries_nr;\n> };\n\nI like that you have added a newline between the members used for each\nof the two (so far) callback functions.\n\nIt may be helpful to add a comment to the effect of, \"these are used to\nwrite the XXX chunk via write_midx_xxx()\", or something.\n\nThanks,\nTaylor\n"},{"id":"411735","messageId":"X8/K1dUgUmwp8ZOv@nand.local","threadId":"54753","inReplyTo":"ee0b73f7-8f59-a1dc-0a21-bf796bf9f2e2@web.de","subject":"Re: [PATCH 00/15] Refactor chunk-format into an API","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-12-08T18:49:57Z","receivedAt":"2020-12-08T20:39:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Dec 04, 2020 at 01:48:31PM +0100, René Scharfe wrote:\n> Am 03.12.20 um 17:16 schrieb Derrick Stolee via GitGitGadget:\n> > I was thinking about file formats recently and realized that the \"chunks\"\n> > that are common to the commit-graph and multi-pack-index could inform future\n> > file formats. To make that process easier, let's combine the process of\n> > writing and reading chunks into a common API that both of these existing\n> > formats use.\n> >\n> > There is some extra benefit immediately: the writing and reading code for\n> > each gets a bit cleaner. Also, there were different checks in each that made\n> > the process more robust. Now, these share a common set of checks.\n>\n> > Documentation/technical/chunk-format.txt | 54 ++\n> > .../technical/commit-graph-format.txt | 3 +\n> > Documentation/technical/pack-format.txt | 3 +\n> > Makefile | 1 +\n> > chunk-format.c | 105 ++++\n> > chunk-format.h | 69 +++\n> > commit-graph.c | 298 ++++++-----\n> > midx.c | 466 ++++++++----------\n> > t/t5318-commit-graph.sh | 2 +-\n> > t/t5319-multi-pack-index.sh | 6 +-\n> > 10 files changed, 623 insertions(+), 384 deletions(-)\n>\n> 623-384-54-3-3-1-69-2-6 = 101\n>\n> So if we ignore changes to documentation, headers, tests and build\n> script this spends ca. 100 more lines of code than the current version.\n> That's roughly the size of the new file chunk-format.c -- from this\n> bird's-eye-view the new API seems to be pure overhead.\n>\n> In the new code I see several magic numbers, use of void pointers and\n> casting as well as repetition -- is this really going in the right\n> direction? I get the feeling that YAGNI.\n\nI think that Stolee is going in the right direction. I suggested earlier\nin the thread making a new \"chunkfile\" type which can handle allocating\nnew chunks, writing their tables of contents, and so on.\n\nSo, I think that we should pursues that direction a little further\nbefore deciding whether or not this is worth continuing. My early\nexperiments showed that it does add a little more code to the\nchunk-format.{c,h} files, but you get negative diffs in midx.c and\ncommit-graph.c, which is more in line with what I would expect from this\nseries.\n\nI do think that the \"overhead\" here is more tolerable than we might\nthink; I'd rather have a well-documented \"chunkfile\" implementation\nwritten once and called twice, than two near-identical implementations\nof _writing_ the chunks / table of contents at each of the call sites.\nSo, even if this does end up being a net-lines-added kind of diff, I'd\nstill say that it's worth it.\n\nWith regards to the \"YAGNI\" comment... I do have thoughts about\nextending the reachability bitmap format to use chunks (of course, this\nwould break compatibility with JGit, and it isn't something that I plan\nto do in the short-term, or even necessarily in the future).\n\nIn any event, I'm sure that this won't be these two won't be the last\nchunk-based formats that we have in Git.\n\n> René\n\nThanks,\nTaylor\n"},{"id":"411737","messageId":"X8/I/RzXZksio+ri@nand.local","threadId":"54753","inReplyTo":"f2f78ee1054f64fe767335c4b0c77ba2fbec5fae.1607012215.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 10/15] midx: use chunk-format API in write_midx_internal()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-12-08T18:42:05Z","receivedAt":"2020-12-08T20:44:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Dec 03, 2020 at 04:16:49PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n>\n> The chunk-format API allows automatically writing the table of contents\n> for a chunk-based file format when using an array of \"struct\n> chunk_info\"s. Update write_midx_internal() to use this strategy, which\n> also simplifies the chunk writing loop. This loop will be replaced with\n> a chunk-format API call in an upcoming change.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> midx.c | 96 +++++++++++++---------------------------------------------\n> 1 file changed, 21 insertions(+), 75 deletions(-)\n>\n> diff --git a/midx.c b/midx.c\n> index ce6d4339bd..0548266bea 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -11,6 +11,7 @@\n> #include \"trace2.h\"\n> #include \"run-command.h\"\n> #include \"repository.h\"\n> +#include \"chunk-format.h\"\n>\n> #define MIDX_SIGNATURE 0x4d494458 /* \"MIDX\" */\n> #define MIDX_VERSION 1\n> @@ -799,15 +800,14 @@ static int write_midx_large_offsets(struct hashfile *f,\n> static int write_midx_internal(const char *object_dir, struct multi_pack_index *m,\n> \t\t\t struct string_list *packs_to_drop, unsigned flags)\n> {\n> -\tunsigned char cur_chunk, num_chunks = 0;\n> +\tunsigned char num_chunks = 0;\n> \tchar *midx_name;\n> \tuint32_t i;\n> \tstruct hashfile *f = NULL;\n> \tstruct lock_file lk;\n> \tstruct write_midx_context ctx = { 0 };\n> \tuint64_t header_size = 0;\n> -\tuint32_t chunk_ids[MIDX_MAX_CHUNKS + 1];\n> -\tuint64_t chunk_offsets[MIDX_MAX_CHUNKS + 1];\n> +\tstruct chunk_info chunks[MIDX_MAX_CHUNKS];\n> \tint pack_name_concat_len = 0;\n> \tint dropped_packs = 0;\n> \tint result = 0;\n> @@ -923,7 +923,6 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n> \tif (ctx.m)\n> \t\tclose_midx(ctx.m);\n>\n> -\tcur_chunk = 0;\n> \tnum_chunks = ctx.large_offsets_needed ? 5 : 4;\n>\n> \tif (ctx.nr - dropped_packs == 0) {\n> @@ -934,85 +933,32 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n>\n> \theader_size = write_midx_header(f, num_chunks, ctx.nr - dropped_packs);\n>\n> -\tchunk_ids[cur_chunk] = MIDX_CHUNKID_PACKNAMES;\n> -\tchunk_offsets[cur_chunk] = header_size + (num_chunks + 1) * MIDX_CHUNKLOOKUP_WIDTH;\n> +\tchunks[0].id = MIDX_CHUNKID_PACKNAMES;\n> +\tchunks[0].size = pack_name_concat_len;\n> +\tchunks[0].write_fn = write_midx_pack_names;\n\n[...]\n\nHmm. The caller has to do quite a lot of work in order to feed chunks\ninto the new chunk-format code.\n\nI wonder what it would look like if we introduced a new 'struct\nchunkfile' type. It would know about the underlying hashfile that it's\nwriting to, as well as the chunks it's supposed to write. It would\nlikely support three operations: add a new chunk, write the TOC, and\nwrite the contents (the latter two could probably be combined into one).\n\nBelow is a patch to do just that. It seems to work OK on t5319, but\nfails some tests in t5318. I haven't looked into the commit-graph test\nfailures, but I'd be happy to do so if you think this is a direction\nworth going in.\n\nBefore I show the patch, though, let's take a look at the stat:\n\n chunk-format.c | 61 ++++++++++++++++++++++++++++++++++++++----------\n chunk-format.h | 25 ++++++++++++--------\n commit-graph.c | 90 ++++++++++++++++++++++-------------------------------------------------\n midx.c | 38 +++++++++++++-----------------\n 4 files changed, 107 insertions(+), 107 deletions(-)\n\nThat's a little bit more code in the chunk-format code, which I can live\nwith, but the diff in commit-graph.c and midx.c is a net-negative, as I\nwould expect it to be. So, I'm happy that this seems to make things\neasier on the callers.\n\nOK, enough rambling. Here's the patch:\n\ndiff --git a/chunk-format.c b/chunk-format.c\nindex 771b6d98d0..14f2fe5c9a 100644\n--- a/chunk-format.c\n+++ b/chunk-format.c\n@@ -1,26 +1,63 @@\n #include \"git-compat-util.h\"\n #include \"chunk-format.h\"\n #include \"csum-file.h\"\n+#include \"cache.h\"\n #define CHUNK_LOOKUP_WIDTH 12\n\n-void write_table_of_contents(struct hashfile *f,\n-\t\t\t uint64_t cur_offset,\n-\t\t\t struct chunk_info *chunks,\n-\t\t\t int nr)\n+void chunkfile_init(struct chunkfile *out, struct hashfile *f)\n {\n-\tint i;\n+\tif (!out)\n+\t\tBUG(\"cannot initialize null chunkfile\");\n+\n+\tmemset(out, 0, sizeof(*out));\n+\tout->f = f;\n+}\n+\n+void chunkfile_push_chunk(struct chunkfile *c,\n+\t\t\t uint32_t id, uint64_t size, chunk_write_fn write_fn)\n+{\n+\tALLOC_GROW(c->chunks, c->chunks_nr + 1, c->chunks_alloc);\n+\tc->chunks[c->chunks_nr].id = id;\n+\tc->chunks[c->chunks_nr].size = size;\n+\tc->chunks[c->chunks_nr].write_fn = write_fn;\n+\tc->chunks_nr++;\n+}\n+\n+void chunkfile_write_toc(struct chunkfile *c)\n+{\n+\tsize_t i;\n+\toff_t cur_offset = hashfile_total(c->f);\n\n \t/* Add the table of contents to the current offset */\n-\tcur_offset += (nr + 1) * CHUNK_LOOKUP_WIDTH;\n+\tcur_offset += (c->chunks_nr + 1) * CHUNK_LOOKUP_WIDTH;\n\n-\tfor (i = 0; i < nr; i++) {\n-\t\thashwrite_be32(f, chunks[i].id);\n-\t\thashwrite_be64(f, cur_offset);\n+\tfor (i = 0; i < c->chunks_nr; i++) {\n+\t\thashwrite_be32(c->f, c->chunks[i].id);\n+\t\thashwrite_be64(c->f, cur_offset);\n\n-\t\tcur_offset += chunks[i].size;\n+\t\tcur_offset += c->chunks[i].size;\n \t}\n\n \t/* Trailing entry marks the end of the chunks */\n-\thashwrite_be32(f, 0);\n-\thashwrite_be64(f, cur_offset);\n+\thashwrite_be32(c->f, 0);\n+\thashwrite_be64(c->f, cur_offset);\n+}\n+\n+void chunkfile_write_chunks(struct chunkfile *c, void *ctx)\n+{\n+\tsize_t i;\n+\n+\tfor (i = 0; i < c->chunks_nr; i++) {\n+\t\toff_t before, after;\n+\t\tstruct chunk_info *chunk = &c->chunks[i];\n+\t\tif (!chunk->write_fn)\n+\t\t\tcontinue;\n+\n+\t\tbefore = hashfile_total(c->f);\n+\t\tchunk->write_fn(c->f, ctx);\n+\t\tafter = hashfile_total(c->f);\n+\n+\t\tif (after - before != chunk->size)\n+\t\t\tBUG(\"unexpected chunk size\");\n+\t}\n }\ndiff --git a/chunk-format.h b/chunk-format.h\nindex 4b9cbeb372..b0fb515425 100644\n--- a/chunk-format.h\n+++ b/chunk-format.h\n@@ -19,18 +19,23 @@ struct chunk_info {\n \tchunk_write_fn write_fn;\n };\n\n+struct chunkfile {\n+\tstruct hashfile *f;\n+\n+\tstruct chunk_info *chunks;\n+\tsize_t chunks_nr;\n+\tsize_t chunks_alloc;\n+};\n+\n+void chunkfile_init(struct chunkfile *out, struct hashfile *f);\n+void chunkfile_push_chunk(struct chunkfile *c,\n+\t\t\t uint32_t id, uint64_t size, chunk_write_fn write_fn);\n+\n /*\n * Write the chunk data into the supplied hashfile.\n- *\n- * * 'cur_offset' indicates the number of bytes written to the hashfile\n- * before the table of contents starts.\n- *\n- * * 'nr' is the number of chunks with non-zero IDs, so 'nr + 1'\n- * chunks are written in total.\n */\n-void write_table_of_contents(struct hashfile *f,\n-\t\t\t uint64_t cur_offset,\n-\t\t\t struct chunk_info *chunks,\n-\t\t\t int nr);\n+void chunkfile_write_toc(struct chunkfile *c);\n+\n+void chunkfile_write_chunks(struct chunkfile *c, void *ctx);\n\n #endif\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 5494fda1d3..abc0c9e46f 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1702,11 +1702,9 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \tuint32_t i;\n \tint fd;\n \tstruct hashfile *f;\n+\tstruct chunkfile cf;\n \tstruct lock_file lk = LOCK_INIT;\n-\tstruct chunk_info chunks[MAX_NUM_CHUNKS + 1];\n \tconst unsigned hashsz = the_hash_algo->rawsz;\n-\tstruct strbuf progress_title = STRBUF_INIT;\n-\tint num_chunks = 3;\n \tstruct object_id file_hash;\n\n \tif (ctx->split) {\n@@ -1753,76 +1751,42 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t\tf = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);\n \t}\n\n-\tchunks[0].id = GRAPH_CHUNKID_OIDFANOUT;\n-\tchunks[0].size = GRAPH_FANOUT_SIZE;\n-\tchunks[0].write_fn = write_graph_chunk_fanout;\n-\tchunks[1].id = GRAPH_CHUNKID_OIDLOOKUP;\n-\tchunks[1].size = hashsz * ctx->commits.nr;\n-\tchunks[1].write_fn = write_graph_chunk_oids;\n-\tchunks[2].id = GRAPH_CHUNKID_DATA;\n-\tchunks[2].size = (hashsz + 16) * ctx->commits.nr;\n-\tchunks[2].write_fn = write_graph_chunk_data;\n-\tif (ctx->num_extra_edges) {\n-\t\tchunks[num_chunks].id = GRAPH_CHUNKID_EXTRAEDGES;\n-\t\tchunks[num_chunks].size = 4 * ctx->num_extra_edges;\n-\t\tchunks[num_chunks].write_fn = write_graph_chunk_extra_edges;\n-\t\tnum_chunks++;\n-\t}\n+\tchunkfile_init(&cf, f);\n+\n+\tchunkfile_push_chunk(&cf, GRAPH_CHUNKID_OIDFANOUT,\n+\t\t\t GRAPH_FANOUT_SIZE, write_graph_chunk_fanout);\n+\tchunkfile_push_chunk(&cf, GRAPH_CHUNKID_OIDLOOKUP,\n+\t\t\t hashsz * ctx->commits.nr, write_graph_chunk_oids);\n+\tchunkfile_push_chunk(&cf, GRAPH_CHUNKID_DATA,\n+\t\t\t (hashsz + 16) * ctx->commits.nr, write_graph_chunk_data);\n+\tif (ctx->num_extra_edges)\n+\t\tchunkfile_push_chunk(&cf, GRAPH_CHUNKID_EXTRAEDGES,\n+\t\t\t\t 4 * ctx->num_extra_edges,\n+\t\t\t\t write_graph_chunk_extra_edges);\n \tif (ctx->changed_paths) {\n-\t\tchunks[num_chunks].id = GRAPH_CHUNKID_BLOOMINDEXES;\n-\t\tchunks[num_chunks].size = sizeof(uint32_t) * ctx->commits.nr;\n-\t\tchunks[num_chunks].write_fn = write_graph_chunk_bloom_indexes;\n-\t\tnum_chunks++;\n-\t\tchunks[num_chunks].id = GRAPH_CHUNKID_BLOOMDATA;\n-\t\tchunks[num_chunks].size = sizeof(uint32_t) * 3\n-\t\t\t\t\t + ctx->total_bloom_filter_data_size;\n-\t\tchunks[num_chunks].write_fn = write_graph_chunk_bloom_data;\n-\t\tnum_chunks++;\n-\t}\n-\tif (ctx->num_commit_graphs_after > 1) {\n-\t\tchunks[num_chunks].id = GRAPH_CHUNKID_BASE;\n-\t\tchunks[num_chunks].size = hashsz * (ctx->num_commit_graphs_after - 1);\n-\t\tchunks[num_chunks].write_fn = write_graph_chunk_base;\n-\t\tnum_chunks++;\n+\t\tchunkfile_push_chunk(&cf, GRAPH_CHUNKID_BLOOMINDEXES,\n+\t\t\t\t sizeof(uint32_t) * ctx->commits.nr,\n+\t\t\t\t write_graph_chunk_bloom_indexes);\n+\t\tchunkfile_push_chunk(&cf, GRAPH_CHUNKID_BLOOMDATA,\n+\t\t\t\t sizeof(uint32_t) * 3 + ctx->total_bloom_filter_data_size,\n+\t\t\t\t write_graph_chunk_bloom_data);\n \t}\n+\tif (ctx->num_commit_graphs_after > 1)\n+\t\tchunkfile_push_chunk(&cf, GRAPH_CHUNKID_BASE,\n+\t\t\t\t hashsz * (ctx->num_commit_graphs_after - 1),\n+\t\t\t\t write_graph_chunk_base);\n\n-\tchunks[num_chunks].id = 0;\n-\tchunks[num_chunks].size = 0;\n+\tchunkfile_push_chunk(&cf, 0, 0, NULL);\n\n \thashwrite_be32(f, GRAPH_SIGNATURE);\n\n \thashwrite_u8(f, GRAPH_VERSION);\n \thashwrite_u8(f, oid_version());\n-\thashwrite_u8(f, num_chunks);\n+\thashwrite_u8(f, cf.chunks_nr - 1);\n \thashwrite_u8(f, ctx->num_commit_graphs_after - 1);\n\n-\twrite_table_of_contents(f, /* cur_offset */ 8, chunks, num_chunks);\n-\n-\tif (ctx->report_progress) {\n-\t\tstrbuf_addf(&progress_title,\n-\t\t\t Q_(\"Writing out commit graph in %d pass\",\n-\t\t\t \"Writing out commit graph in %d passes\",\n-\t\t\t num_chunks),\n-\t\t\t num_chunks);\n-\t\tctx->progress = start_delayed_progress(\n-\t\t\tprogress_title.buf,\n-\t\t\tnum_chunks * ctx->commits.nr);\n-\t}\n-\n-\tfor (i = 0; i < num_chunks; i++) {\n-\t\tuint64_t start_offset = f->total + f->offset;\n-\n-\t\tif (chunks[i].write_fn(f, ctx))\n-\t\t\treturn -1;\n-\n-\t\tif (f->total + f->offset != start_offset + chunks[i].size)\n-\t\t\tBUG(\"expected to write %\"PRId64\" bytes to chunk %\"PRIx32\", but wrote %\"PRId64\" instead\",\n-\t\t\t chunks[i].size, chunks[i].id,\n-\t\t\t f->total + f->offset - start_offset);\n-\t}\n-\n-\tstop_progress(&ctx->progress);\n-\tstrbuf_release(&progress_title);\n+\tchunkfile_write_toc(&cf);\n+\tchunkfile_write_chunks(&cf, ctx);\n\n \tif (ctx->split && ctx->base_graph_name && ctx->num_commit_graphs_after > 1) {\n \t\tchar *new_base_hash = xstrdup(oid_to_hex(&ctx->new_base_graph->oid));\ndiff --git a/midx.c b/midx.c\nindex 0548266bea..6315ea7555 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -807,7 +807,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \tstruct lock_file lk;\n \tstruct write_midx_context ctx = { 0 };\n \tuint64_t header_size = 0;\n-\tstruct chunk_info chunks[MIDX_MAX_CHUNKS];\n+\tstruct chunkfile cf;\n \tint pack_name_concat_len = 0;\n \tint dropped_packs = 0;\n \tint result = 0;\n@@ -933,33 +933,27 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n\n \theader_size = write_midx_header(f, num_chunks, ctx.nr - dropped_packs);\n\n-\tchunks[0].id = MIDX_CHUNKID_PACKNAMES;\n-\tchunks[0].size = pack_name_concat_len;\n-\tchunks[0].write_fn = write_midx_pack_names;\n+\tchunkfile_init(&cf, f);\n\n-\tchunks[1].id = MIDX_CHUNKID_OIDFANOUT;\n-\tchunks[1].size = MIDX_CHUNK_FANOUT_SIZE;\n-\tchunks[1].write_fn = write_midx_oid_fanout;\n-\n-\tchunks[2].id = MIDX_CHUNKID_OIDLOOKUP;\n-\tchunks[2].size = ctx.entries_nr * the_hash_algo->rawsz;\n-\tchunks[2].write_fn = write_midx_oid_lookup;\n-\n-\tchunks[3].id = MIDX_CHUNKID_OBJECTOFFSETS;\n-\tchunks[3].size = ctx.entries_nr * MIDX_CHUNK_OFFSET_WIDTH;\n-\tchunks[3].write_fn = write_midx_object_offsets;\n+\tchunkfile_push_chunk(&cf, MIDX_CHUNKID_PACKNAMES,\n+\t\t\t pack_name_concat_len, write_midx_pack_names);\n+\tchunkfile_push_chunk(&cf, MIDX_CHUNKID_OIDFANOUT,\n+\t\t\t MIDX_CHUNK_FANOUT_SIZE, write_midx_oid_fanout);\n+\tchunkfile_push_chunk(&cf, MIDX_CHUNKID_OIDLOOKUP,\n+\t\t\t ctx.entries_nr * the_hash_algo->rawsz, write_midx_oid_lookup);\n+\tchunkfile_push_chunk(&cf, MIDX_CHUNKID_OBJECTOFFSETS,\n+\t\t\t ctx.entries_nr * MIDX_CHUNK_OFFSET_WIDTH, write_midx_object_offsets);\n\n \tif (ctx.large_offsets_needed) {\n-\t\tchunks[4].id = MIDX_CHUNKID_LARGEOFFSETS;\n-\t\tchunks[4].size = ctx.num_large_offsets * MIDX_CHUNK_LARGE_OFFSET_WIDTH;\n-\t\tchunks[4].write_fn = write_midx_large_offsets;\n+\t\tchunkfile_push_chunk(&cf, MIDX_CHUNKID_LARGEOFFSETS,\n+\t\t\t\t ctx.num_large_offsets * MIDX_CHUNK_LARGE_OFFSET_WIDTH,\n+\t\t\t\t write_midx_large_offsets);\n \t}\n\n-\twrite_table_of_contents(f, header_size, chunks, num_chunks);\n-\n-\tfor (i = 0; i < num_chunks; i++)\n-\t\tchunks[i].write_fn(f, &ctx);\n+\tchunkfile_write_toc(&cf);\n+\tchunkfile_write_chunks(&cf, &ctx);\n\n+\t/* maybe move this into chunkfile??? */\n \tfinalize_hashfile(f, NULL, CSUM_FSYNC | CSUM_HASH_IN_STREAM);\n \tcommit_lock_file(&lk);\n\n"},{"id":"411740","messageId":"X8/JySnLxR2DaO4V@nand.local","threadId":"54753","inReplyTo":"03f3255c8f4a953065b2ff8e61816f83534c23ed.1607012215.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 12/15] chunk-format: create write_chunks()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-12-08T18:45:29Z","receivedAt":"2020-12-08T20:55:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Dec 03, 2020 at 04:16:51PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n>\n> The commit-graph and multi-pack-index files both use a chunk-based file\n> format. They have already unified on using write_table_of_contents(),\n> but we expand upon that by unifying their chunk writing loop.\n>\n> This takes the concepts already present in the commit-graph that were\n> dropped in the multi-pack-index code during refactoring, including:\n>\n> * Check the hashfile for how much data was written by each write_fn.\n>\n> * Allow write_fn() to report an error that results in a failure\n> without using die() in the low-level commands.\n>\n> This simplifies the code in commit-graph.c and midx.c while laying the\n> foundation for future formats using similar ideas.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> chunk-format.c | 23 +++++++++++++++++++++++\n> chunk-format.h | 13 +++++++++++++\n> commit-graph.c | 13 ++-----------\n> midx.c | 3 +--\n> 4 files changed, 39 insertions(+), 13 deletions(-)\n>\n> diff --git a/chunk-format.c b/chunk-format.c\n> index 771b6d98d0..a6643a4fc8 100644\n> --- a/chunk-format.c\n> +++ b/chunk-format.c\n> @@ -24,3 +24,26 @@ void write_table_of_contents(struct hashfile *f,\n> \thashwrite_be32(f, 0);\n> \thashwrite_be64(f, cur_offset);\n> }\n> +\n> +int write_chunks(struct hashfile *f,\n> +\t\t struct chunk_info *chunks,\n> +\t\t int nr,\n> +\t\t void *data)\n> +{\n\nServes me right for thinking that a function like write_chunks() would\nbe a good addition to this series without... actually reading the whole\nseries first ;-).\n\nI'm glad to see that you're adding such a function, but I think that I\nprefer my version which moves these four parameters into the chunkfile\nstruct. That allows for other functions to be created which can, for\ne.g., manage the chunks themselves (like my chunkfile_push_chunk()).\n\nThanks,\nTaylor\n"},{"id":"411741","messageId":"X8/JXfTlbhuhgizF@nand.local","threadId":"54753","inReplyTo":"xmqqa6uuh7n1.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 11/15] midx: use 64-bit multiplication for chunk sizes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-12-08T18:43:41Z","receivedAt":"2020-12-08T20:57:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Dec 03, 2020 at 02:00:50PM -0800, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n>\n> > From: Derrick Stolee \n> >\n> > When calculating the sizes of certain chunks, we should use 64-bit\n> > multiplication always. This allows us to properly predict the chunk\n> > sizes without risk of overflow.\n>\n> That's quite an obvious fix, applicable even before this entire\n> series. I think we saw quite similar bugfixes in different parts of\n> the codebase recently.\n\n:-). Indeed, Stolee and I were probably looking at output from the same\nstatic analysis tool, which seems to be eager to catch these sorts of\nmultiplication-widening errors.\n\nI'd be happy to see this patch get applied down regardless of what\nhappens with the rest of the series.\n\nThanks,\nTaylor\n"},{"id":"411888","messageId":"4696bd93-9406-0abd-25ec-a739665a24d5@web.de","threadId":"54753","inReplyTo":"X8/K1dUgUmwp8ZOv@nand.local","subject":"Re: [PATCH 00/15] Refactor chunk-format into an API","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-12-09T17:13:18Z","receivedAt":"2020-12-09T17:15:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.12.20 um 19:49 schrieb Taylor Blau:\n> So, I think that we should pursues that direction a little further\n> before deciding whether or not this is worth continuing. My early\n> experiments showed that it does add a little more code to the\n> chunk-format.{c,h} files, but you get negative diffs in midx.c and\n> commit-graph.c, which is more in line with what I would expect from this\n> series.\n\nOK.\n\n> I do think that the \"overhead\" here is more tolerable than we might\n> think; I'd rather have a well-documented \"chunkfile\" implementation\n> written once and called twice, than two near-identical implementations\n> of _writing_ the chunks / table of contents at each of the call sites.\n> So, even if this does end up being a net-lines-added kind of diff, I'd\n> still say that it's worth it.\n\nWell, interfaces are hard, and having two similar-but-not-quite-equal\npieces of code instead of a central API implementation trying to\nserve two callers can actually be better.\n\nI'm not too familiar with the chunk producers and consumers, so I can\nonly offer some high-level observations. And I don't have to use the\nAPI, so go wild! ;) I was just triggered by the appearance of two\nworking pieces of code being replaced by two slightly different pieces\nof code plus a third one on top.\n\n> With regards to the \"YAGNI\" comment... I do have thoughts about\n> extending the reachability bitmap format to use chunks (of course, this\n> would break compatibility with JGit, and it isn't something that I plan\n> to do in the short-term, or even necessarily in the future).\n>\n> In any event, I'm sure that this won't be these two won't be the last\n> chunk-based formats that we have in Git.\n\nOK, so perhaps we can do better before this scheme is copied. The write\nside is complicated by the fact that the table of contents (TOC) is\nwritten first, followed by the actual chunks. That requires two passes\nover the data.\n\nThe ZIP format solved a similar issue by placing the TOC at the end,\nwhich allows for one-pass streaming. Another way to achieve that would\nbe to put the TOC in a separate file, like we do for .pack and .idx\nfiles. This way you could have a single write function for chunks, and\nwriters would just be a single sequence of calls for the different\ntypes.\n\nBut seeing that the read side just loads all of the chunks anyway\n(skipping unknown IDs) I wonder why we need a TOC at all. That would\nonly be useful if callers were trying to read just some small subset\nof the whole file. A collection of chunks for easy dumping and loading\ncould be serialized by writing just a small header for each chunk\ncontaining its type and size followed by its payload.\n\nRené\n"},{"id":"411959","messageId":"X9Fw0Pzn5wsy3wt0@nand.local","threadId":"54753","inReplyTo":"4696bd93-9406-0abd-25ec-a739665a24d5@web.de","subject":"Re: [PATCH 00/15] Refactor chunk-format into an API","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-12-10T00:50:24Z","receivedAt":"2020-12-10T00:51:32Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Dec 09, 2020 at 06:13:18PM +0100, René Scharfe wrote:\n> I'm not too familiar with the chunk producers and consumers, so I can\n> only offer some high-level observations. And I don't have to use the\n> API, so go wild! ;) I was just triggered by the appearance of two\n> working pieces of code being replaced by two slightly different pieces\n> of code plus a third one on top.\n\n:-).\n\n> > With regards to the \"YAGNI\" comment... I do have thoughts about\n> > extending the reachability bitmap format to use chunks (of course, this\n> > would break compatibility with JGit, and it isn't something that I plan\n> > to do in the short-term, or even necessarily in the future).\n> >\n> > In any event, I'm sure that this won't be these two won't be the last\n> > chunk-based formats that we have in Git.\n>\n> OK, so perhaps we can do better before this scheme is copied. The write\n> side is complicated by the fact that the table of contents (TOC) is\n> written first, followed by the actual chunks. That requires two passes\n> over the data.\n\n\"Two passes\" meaning that we have to both compute the size of and then\nwrite the data? This is relatively cheap to do, at least so I think.\n\nFor e.g., the OIDLOOKUP commit-graph chunk is just the_hash_algo->hashsz\n* commits->nr bytes wide, so that can be done in constant time. A more\nheavyweight case might be for e.g., the Bloom data section, where Bloom\nfilters have to first be computed, their lengths accounted for, and\n_then_ written when we eventually get to writing that chunk.\n\nThis happens in compute_bloom_filters(); and write_chunk_bloom_indexes()\n+ write_chunk_bloom_data(), respectively. Those Bloom filters are all\nstored in a commit slab until they are written, so these \"two passes\"\nare just paid for in memory.\n\n> The ZIP format solved a similar issue by placing the TOC at the end,\n> which allows for one-pass streaming. Another way to achieve that would\n> be to put the TOC in a separate file, like we do for .pack and .idx\n> files. This way you could have a single write function for chunks, and\n> writers would just be a single sequence of calls for the different\n> types.\n\nInteresting. I'm not opposed to changing any of these formats (and maybe\nthere is some compelling argument for doing so, I am not sure) but I\nthink that unifying the implementation for reading / writing the chunk\nformat _before_ changing it is a postive step.\n\n> But seeing that the read side just loads all of the chunks anyway\n> (skipping unknown IDs) I wonder why we need a TOC at all. That would\n> only be useful if callers were trying to read just some small subset\n> of the whole file. A collection of chunks for easy dumping and loading\n> could be serialized by writing just a small header for each chunk\n> containing its type and size followed by its payload.\n\nAFAIK, we do use the table of contents to locate where the chunks are so\nthat we can for e.g., set up the commit_graph structure's pointers to\npoint at each chunk appropriately.\n\n> René\n\nThanks,\nTaylor\n"},{"id":"412003","messageId":"47900592-2c97-cff1-c70f-594691a8083f@gmail.com","threadId":"54753","inReplyTo":"X8/I/RzXZksio+ri@nand.local","subject":"Re: [PATCH 10/15] midx: use chunk-format API in write_midx_internal()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-12-10T14:36:57Z","receivedAt":"2020-12-10T16:49:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/8/2020 1:42 PM, Taylor Blau wrote:\n> I wonder what it would look like if we introduced a new 'struct\n> chunkfile' type. It would know about the underlying hashfile that it's\n> writing to, as well as the chunks it's supposed to write. It would\n> likely support three operations: add a new chunk, write the TOC, and\n> write the contents (the latter two could probably be combined into one).\n> \n> Below is a patch to do just that. It seems to work OK on t5319, but\n> fails some tests in t5318. I haven't looked into the commit-graph test\n> failures, but I'd be happy to do so if you think this is a direction\n> worth going in.\n\nI like this direction. It makes it easier to adapt future formats or\nadd new chunks. I wonder how this write interface can adapt to the\nreading portion, since that is where some of the magic numbers came\nabout. Likely there is an equivalent that we could do to have the\nchunk-format API construct the \"struct chunkfile\" and then the\ncommit-graph and multi-pack-index parsers can interpret the chunks\n_after_ the TOC parse is complete.\n\nAs discussed, I'm putting this series on hold for a while so\ngeneration number v2 is not interrupted. When I come back to it,\nI'll incorporate these design ideas and build on top of that\ntopic.\n\nThanks,\n-Stolee\n"},{"id":"412007","messageId":"6691b5ab-6975-9aa3-3b72-11299d0889de@gmail.com","threadId":"54753","inReplyTo":"X9Fw0Pzn5wsy3wt0@nand.local","subject":"Re: [PATCH 00/15] Refactor chunk-format into an API","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-12-10T14:30:52Z","receivedAt":"2020-12-10T18:56:45Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/9/2020 7:50 PM, Taylor Blau wrote:\n> On Wed, Dec 09, 2020 at 06:13:18PM +0100, René Scharfe wrote:\n>> I'm not too familiar with the chunk producers and consumers, so I can\n>> only offer some high-level observations. And I don't have to use the\n>> API, so go wild! ;) I was just triggered by the appearance of two\n>> working pieces of code being replaced by two slightly different pieces\n>> of code plus a third one on top.\n> \n> :-).\n> \n>>> With regards to the \"YAGNI\" comment... I do have thoughts about\n>>> extending the reachability bitmap format to use chunks (of course, this\n>>> would break compatibility with JGit, and it isn't something that I plan\n>>> to do in the short-term, or even necessarily in the future).\n>>>\n>>> In any event, I'm sure that this won't be these two won't be the last\n>>> chunk-based formats that we have in Git.\n>>\n>> OK, so perhaps we can do better before this scheme is copied. The write\n>> side is complicated by the fact that the table of contents (TOC) is\n>> written first, followed by the actual chunks. That requires two passes\n>> over the data.\n> \n> \"Two passes\" meaning that we have to both compute the size of and then\n> write the data? This is relatively cheap to do, at least so I think.\n> \n> For e.g., the OIDLOOKUP commit-graph chunk is just the_hash_algo->hashsz\n> * commits->nr bytes wide, so that can be done in constant time. A more\n> heavyweight case might be for e.g., the Bloom data section, where Bloom\n> filters have to first be computed, their lengths accounted for, and\n> _then_ written when we eventually get to writing that chunk.\n> \n> This happens in compute_bloom_filters(); and write_chunk_bloom_indexes()\n> + write_chunk_bloom_data(), respectively. Those Bloom filters are all\n> stored in a commit slab until they are written, so these \"two passes\"\n> are just paid for in memory.\n\nThe current design of the format (TOC first) does require that we can\npredict the chunk sizes before we start writing the file. But also this\nhas _some_ desirable properties for the writer. Specifically, we keep\nthe file write handle for a smaller amount of time. How valuable is that?\n:shrug:\n\n>> The ZIP format solved a similar issue by placing the TOC at the end,\n>> which allows for one-pass streaming. Another way to achieve that would\n>> be to put the TOC in a separate file, like we do for .pack and .idx\n>> files. This way you could have a single write function for chunks, and\n>> writers would just be a single sequence of calls for the different\n>> types.\n> \n> Interesting. I'm not opposed to changing any of these formats (and maybe\n> there is some compelling argument for doing so, I am not sure) but I\n> think that unifying the implementation for reading / writing the chunk\n> format _before_ changing it is a postive step.\n\nChanging the TOC location would require a version increment in the file\nformats, which is a bit painful.\n\nI understand why the ZIP format does this, it is trying to stream data\nthrough a compression algorithm and cannot store the result in memory\nbefore writing. Perhaps we would want to consider that as a way to\nreduce the memory load for things like changed-path Bloom filters, but\nlet's wait for that to be an actually noticeable problem before making\nthe change.\n\nThe TOC location is definitely optimized for readers, who are already\nreading the initial header to discover some info about the file.\n\n>> But seeing that the read side just loads all of the chunks anyway\n>> (skipping unknown IDs) I wonder why we need a TOC at all. That would\n>> only be useful if callers were trying to read just some small subset\n>> of the whole file. A collection of chunks for easy dumping and loading\n>> could be serialized by writing just a small header for each chunk\n>> containing its type and size followed by its payload.\n> \n> AFAIK, we do use the table of contents to locate where the chunks are so\n> that we can for e.g., set up the commit_graph structure's pointers to\n> point at each chunk appropriately.\n\nThe chunk-based format is really optimized for _structured data_ where\nthese sizes are mostly predictable. The chunk sizes that are not\npredictable are things like the \"extra edges\" or changed-path Bloom\nfilter data, but that data is indexed by a structured chunk.\n\nA natural fit for the chunk-based format is the reachability bitmap\nformat, since the current format requires a scan to discover which\ncommits have bitmaps. If we used chunks, then we could quickly\nsearch a lookup table for the commits that have bitmaps, then navigate\nto the binary data chunk for the bitmap itself.\n\nThanks,\n-Stolee\n"}]}