{"thread":{"id":"53617","subject":"[PATCH 00/10] Szeder's commit-graph cleanups","startedAt":"2020-06-05T13:00:40Z","lastAt":"2020-06-27T15:53:54Z","messageCount":16,"participants":["Derrick Stolee via GitGitGadget","SZEDER Gábor via GitGitGadget","Junio C Hamano","Derrick Stolee","René Scharfe","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"399172","messageId":"pull.650.git.1591362032.gitgitgadget@gmail.com","threadId":"53617","inReplyTo":null,"subject":"[PATCH 00/10] Szeder's commit-graph cleanups","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-05T13:00:22Z","receivedAt":"2020-06-05T13:00:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"This is based on ds/line-log-on-bloom.\n\nSince Szeder so kindly shared his alternate Bloom filter implementation [1],\nI thought it worth my time to start the process of updating the patches to\napply to more recent code in Git. Here is the effort to update the almost\nobviously-good commit-graph cleanups that he presented in that series.\n\n[1] https://lore.kernel.org/git/20200529085038.26008-1-szeder.dev@gmail.com/\n\nThe range-diff below was created by applying his entire series onto v2.25.0\nand then doing cherry-picks as appropriate onto ds/line-log-on-bloom and\ncorrecting conflicts. I have Szeder's original series available as\n\"szeder-bloom\" on my fork [2].\n\n[2] https://github.com/derrickstolee/git/tree/szeder-bloom\n\nAs expected, the write_commit_graph_file() cleanups were the most difficult,\nin part because we added extra chunks with the changed-path Bloom filters.\n\nI did not include this commit since we already handle it mostly with the\nMAX_NUM_CHUNKS macro from 08fd81c9 (commit-graph: implement\nwrite_commit_graph(), 2018-04-02):\n\n11: 56e3c4f57b3 < -: ----------- commit-graph: allocate the 'struct chunk_info' array dinamically\n\nThere were a few cleanups that I did not apply because they are more\ninvolved to handle the conflicts with the changed-path Bloom filters,\nespecially because we pass a \"struct bloom_filter_settings\" in the method\nsignatures for the write_graph_chunk_bloom_*() functions:\n\n12: 28fb1b5bdfe < -: ----------- commit-graph: unify the signatures of all write_graph_chunk_*() functions\n13: 1e1e59e2592 < -: ----------- commit-graph: simplify write_commit_graph_file() #3\n14: 6f0d912e4b8 < -: ----------- commit-graph: check chunk sizes after writing\n\nI'm not saying that we shouldn't do these changes. I'm just saying that they\nare more involved and can wait for a second series. No need to rush things.\n\nThe rest will need to be completely re-implemented to keep the other things\nin mind, like split commit-graphs and the existing changed-path Bloom\nfilters. However, the following commits would be particularly interesting to\nhave equivalents on top of our existing Bloom filter implementation. That\nwould allow a more fair comparison between the two options:\n\n26: 3951fdedf6a < -: ----------- commit-graph: deduplicate modified path Bloom filters\n27: 5aba19a2766 < -: ----------- commit-graph: load modified path Bloom filters for merge commits\n29: f87b37bf08e < -: ----------- commit-graph: extract init and free write_commit_graph_context\n34: 8b40ec4cd30 < -: ----------- commit-graph: use modified path Bloom filters with wildcards, if possible\n\nFull range diff:\n\n 1: 7a8dbfba53a ! 1: af84c253b24 tree-walk.c: don't match submodule entries for 'submod/anything'\n @@ Commit message\n Fix this by rejecting submodules as partial pathnames when their\n trailing slash is followed by anything.\n\n Signed-off-by: SZEDER Gábor \n + Signed-off-by: Derrick Stolee \n\n ## t/t4010-diff-pathspec.sh ##\n @@ t/t4010-diff-pathspec.sh: test_expect_success 'setup submodules' '\n 2: df25e984c58 ! 2: 1e1671e7c69 commit-graph: fix parsing the Chunk Lookup table\n @@ Commit message\n but that is a more invasive change, less suitable for 'maint', so that\n will be done in later patches.\n\n + This additional flexibility of scanning more chunks breaks a test for\n + \"git commit-graph verify\" so alter that test to mutate the commit-graph\n + to have an even lower chunk count.\n +\n Signed-off-by: SZEDER Gábor \n + Signed-off-by: Derrick Stolee \n\n ## commit-graph.c ##\n @@ commit-graph.c: struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n @@ commit-graph.c: struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n uint32_t chunk_id;\n uint64_t chunk_offset;\n int chunk_repeated = 0;\n +\n + ## t/t5318-commit-graph.sh ##\n +@@ t/t5318-commit-graph.sh: test_expect_success 'detect bad hash version' '\n + '\n + \n + test_expect_success 'detect low chunk count' '\n +- corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT \"\\02\" \\\n ++ corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT \"\\01\" \\\n + \"missing the .* chunk\"\n + '\n + \n 3: 598f7f9a978 ! 3: a09d0bd5b7a commit-graph-format.txt: all multi-byte numbers are in network byte order\n @@ Commit message\n\n Clarify that all multi-byte integers are in network byte order.\n\n Signed-off-by: SZEDER Gábor \n + Signed-off-by: Derrick Stolee \n\n ## Documentation/technical/commit-graph-format.txt ##\n @@ Documentation/technical/commit-graph-format.txt: the body into \"chunks\" and provide a binary lookup table at the beginning\n 4: b29e5d39ed6 ! 4: 32a6f11cc47 commit-slab: add a function to deep free entries on the slab\n @@ Commit message\n Use it in get_shallow_commits() in 'shallow.c' to replace an\n open-coded iteration over a commit slab's entries.\n\n Signed-off-by: SZEDER Gábor \n + Signed-off-by: Derrick Stolee \n\n ## commit-slab-decl.h ##\n @@ commit-slab-decl.h: struct slabname { \\\n @@ shallow.c: int is_repository_shallow(struct repository *r)\n * supports a \"valid\" flag.\n */\n define_commit_slab(commit_depth, int *);\n -+void free_depth_in_slab(int **ptr)\n ++static void free_depth_in_slab(int **ptr)\n +{\n + FREE_AND_NULL(*ptr);\n +}\n 5: 18f4db7bfb9 ! 5: 1a70ff05aea diff.h: drop diff_tree_oid() & friends' return value\n @@ Commit message\n [2] diff_tree_oid() traces back to diff-tree.c:main() in 9174026cfe as\n well.\n\n Signed-off-by: SZEDER Gábor \n + Signed-off-by: Derrick Stolee \n\n ## diff.h ##\n @@ diff.h: struct combine_diff_path *diff_tree_paths(\n @@ revision.c: static int rev_compare_tree(struct rev_info *revs,\n - &revs->pruning) < 0)\n - return REV_TREE_DIFFERENT;\n + diff_tree_oid(&t1->object.oid, &t2->object.oid, \"\", &revs->pruning);\n - return tree_difference;\n - }\n + \n + if (!nth_parent)\n + if (bloom_ret == 1 && tree_difference == REV_TREE_SAME)\n +@@ revision.c: static int rev_compare_tree(struct rev_info *revs,\n\n static int rev_same_tree_as_empty(struct rev_info *revs, struct commit *commit)\n {\n @@ tree-diff.c: static struct combine_diff_path *ll_diff_tree_paths(\n const struct object_id **parents_oid, int nparent,\n struct strbuf *base, struct diff_options *opt);\n -static int ll_diff_tree_oid(const struct object_id *old_oid,\n +- const struct object_id *new_oid,\n +- struct strbuf *base, struct diff_options *opt);\n +static void ll_diff_tree_oid(const struct object_id *old_oid,\n - const struct object_id *new_oid,\n - struct strbuf *base, struct diff_options *opt);\n ++ const struct object_id *new_oid,\n ++ struct strbuf *base, struct diff_options *opt);\n\n + /*\n + * Compare two tree entries, taking into account only path/S_ISDIR(mode),\n @@ tree-diff.c: static void try_to_follow_renames(const struct object_id *old_oid,\n q->nr = 1;\n }\n\n -static int ll_diff_tree_oid(const struct object_id *old_oid,\n +- const struct object_id *new_oid,\n +- struct strbuf *base, struct diff_options *opt)\n +static void ll_diff_tree_oid(const struct object_id *old_oid,\n - const struct object_id *new_oid,\n - struct strbuf *base, struct diff_options *opt)\n ++ const struct object_id *new_oid,\n ++ struct strbuf *base, struct diff_options *opt)\n {\n + struct combine_diff_path phead, *p;\n + pathchange_fn_t pathchange_old = opt->pathchange;\n @@ tree-diff.c: static int ll_diff_tree_oid(const struct object_id *old_oid,\n }\n\n @@ tree-diff.c: static int ll_diff_tree_oid(const struct object_id *old_oid,\n }\n\n -int diff_root_tree_oid(const struct object_id *new_oid, const char *base, struct diff_options *opt)\n -+void diff_root_tree_oid(const struct object_id *new_oid, const char *base,\n ++void diff_root_tree_oid(const struct object_id *new_oid,\n ++ const char *base,\n + struct diff_options *opt)\n {\n - return diff_tree_oid(NULL, new_oid, base, opt);\n 6: bf336f109e6 ! 6: 636c2069659 commit-graph: clean up #includes\n @@ Commit message\n 'commit-graph.c' includes 'dir.h', but doesn't actually use anything\n from there, so let's drop that #include as well.\n\n Signed-off-by: SZEDER Gábor \n + Signed-off-by: Derrick Stolee \n\n ## commit-graph.c ##\n @@\n @@ commit-graph.h\n -#include \"repository.h\"\n -#include \"string-list.h\"\n -#include \"cache.h\"\n -+#include \"hash.h\"\n + #include \"object-store.h\"\n\n #define GIT_TEST_COMMIT_GRAPH \"GIT_TEST_COMMIT_GRAPH\"\n - #define GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD \"GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD\"\n +@@ commit-graph.h: void git_test_write_commit_graph_or_die(void);\n\n struct commit;\n + struct bloom_filter_settings;\n +struct repository;\n +struct raw_object_store;\n +struct string_list;\n\n - char *get_commit_graph_filename(const char *obj_dir);\n + char *get_commit_graph_filename(struct object_directory *odb);\n int open_commit_graph(const char *graph_file, int *fd, struct stat *st);\n 7: b7f0f831bcf ! 7: cd9e033d1b1 commit-graph: simplify parse_commit_graph() #1\n @@ Commit message\n and, consequently, have to update the 'detect incorrect chunk count'\n test in 't5318-commit-graph.sh' as well.\n\n Signed-off-by: SZEDER Gábor \n + Signed-off-by: Derrick Stolee \n\n ## commit-graph.c ##\n @@ commit-graph.c: struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n 8: f2752000052 ! 8: 83641b5e49e commit-graph: simplify parse_commit_graph() #2\n @@ Commit message\n iteration, so we can calculate the size of each chunk right away,\n right where we store its starting offset.\n\n Signed-off-by: SZEDER Gábor \n + Signed-off-by: Derrick Stolee \n\n ## commit-graph.c ##\n @@ commit-graph.c: struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n @@ commit-graph.c: struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n + next_chunk_offset = get_be64(chunk_lookup + 4);\n + for (i = 0; i < graph->num_chunks; i++) {\n uint32_t chunk_id;\n - uint64_t chunk_offset;\n +- uint64_t chunk_offset;\n ++ uint64_t chunk_offset = next_chunk_offset;\n int chunk_repeated = 0;\n\n chunk_id = get_be32(chunk_lookup + 0);\n - chunk_offset = get_be64(chunk_lookup + 4);\n -+ chunk_offset = next_chunk_offset;\n -+ next_chunk_offset = get_be64(chunk_lookup + 4 +\n -+ GRAPH_CHUNKLOOKUP_WIDTH);\n\n chunk_lookup += GRAPH_CHUNKLOOKUP_WIDTH;\n ++ next_chunk_offset = get_be64(chunk_lookup + 4);\n\n + if (chunk_offset > graph_size - the_hash_algo->rawsz) {\n + error(_(\"commit-graph improper chunk offset %08x%08x\"), (uint32_t)(chunk_offset >> 32),\n @@ commit-graph.c: struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n case GRAPH_CHUNKID_OIDLOOKUP:\n if (graph->chunk_oid_lookup)\n @@ commit-graph.c: struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n - last_chunk_offset = chunk_offset;\n }\n\n - hashcpy(graph->oid.hash, graph->data + graph->data_len - graph->hash_len);\n + if (graph->chunk_bloom_indexes && graph->chunk_bloom_data) {\n\nI'm stepping out of the range-diff to point out that this change is mostly\nstylistic.\n\n 9: 4e184b8743c ! 9: 9b818b9cb91 commit-graph: simplify write_commit_graph_file() #1\n @@ Commit message\n fill the arrays of chunk IDs and sizes in one go, eliminating one set\n of repeated conditions.\n\n Signed-off-by: SZEDER Gábor \n + Signed-off-by: Derrick Stolee \n\n ## commit-graph.c ##\n @@ commit-graph.c: static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n struct hashfile *f;\n struct lock_file lk = LOCK_INIT;\n - uint32_t chunk_ids[6];\n -- uint64_t chunk_offsets[6];\n -+ uint64_t chunk_sizes[6];\n + uint32_t chunk_ids[MAX_NUM_CHUNKS + 1];\n +- uint64_t chunk_offsets[MAX_NUM_CHUNKS + 1];\n ++ uint64_t chunk_sizes[MAX_NUM_CHUNKS + 1];\n const unsigned hashsz = the_hash_algo->rawsz;\n struct strbuf progress_title = STRBUF_INIT;\n int num_chunks = 3;\n + uint64_t chunk_offset;\n struct object_id file_hash;\n + const struct bloom_filter_settings bloom_settings = DEFAULT_BLOOM_FILTER_SETTINGS;\n\n - if (ctx->split) {\n @@ commit-graph.c: static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n }\n\n @@ commit-graph.c: static int write_commit_graph_file(struct write_commit_graph_con\n + chunk_sizes[1] = hashsz * ctx->commits.nr;\n chunk_ids[2] = GRAPH_CHUNKID_DATA;\n + chunk_sizes[2] = (hashsz + 16) * ctx->commits.nr;\n ++\n if (ctx->num_extra_edges) {\n chunk_ids[num_chunks] = GRAPH_CHUNKID_EXTRAEDGES;\n -+ chunk_sizes[3] = 4 * ctx->num_extra_edges;\n ++ chunk_sizes[num_chunks] = 4 * ctx->num_extra_edges;\n + num_chunks++;\n + }\n + if (ctx->changed_paths) {\n + chunk_ids[num_chunks] = GRAPH_CHUNKID_BLOOMINDEXES;\n ++ chunk_sizes[num_chunks] = sizeof(uint32_t) * ctx->commits.nr;\n + num_chunks++;\n + chunk_ids[num_chunks] = GRAPH_CHUNKID_BLOOMDATA;\n ++ chunk_sizes[num_chunks] = sizeof(uint32_t) * 3\n ++ + ctx->total_bloom_filter_data_size;\n num_chunks++;\n }\n if (ctx->num_commit_graphs_after > 1) {\n chunk_ids[num_chunks] = GRAPH_CHUNKID_BASE;\n -+ chunk_sizes[4] = hashsz * (ctx->num_commit_graphs_after - 1);\n ++ chunk_sizes[num_chunks] = hashsz * (ctx->num_commit_graphs_after - 1);\n num_chunks++;\n }\n\n @@ commit-graph.c: static int write_commit_graph_file(struct write_commit_graph_con\n - 4 * ctx->num_extra_edges;\n - num_chunks++;\n - }\n +- if (ctx->changed_paths) {\n +- chunk_offsets[num_chunks + 1] = chunk_offsets[num_chunks] +\n +- sizeof(uint32_t) * ctx->commits.nr;\n +- num_chunks++;\n +-\n +- chunk_offsets[num_chunks + 1] = chunk_offsets[num_chunks] +\n +- sizeof(uint32_t) * 3 + ctx->total_bloom_filter_data_size;\n +- num_chunks++;\n +- }\n - if (ctx->num_commit_graphs_after > 1) {\n - chunk_offsets[num_chunks + 1] = chunk_offsets[num_chunks] +\n - hashsz * (ctx->num_commit_graphs_after - 1);\n10: 344dd337da5 ! 10: 5984fb01ebc commit-graph: simplify write_commit_graph_file() #2\n @@ Commit message\n 'struct chunk_info'. This will allow more cleanups in the following\n patches.\n\n Signed-off-by: SZEDER Gábor \n + Signed-off-by: Derrick Stolee \n\n ## commit-graph.c ##\n @@ commit-graph.c: static int write_graph_chunk_base(struct hashfile *f,\n @@ commit-graph.c: static int write_graph_chunk_base(struct hashfile *f,\n int fd;\n struct hashfile *f;\n struct lock_file lk = LOCK_INIT;\n -- uint32_t chunk_ids[6];\n -- uint64_t chunk_sizes[6];\n -+ struct chunk_info chunks[6];\n +- uint32_t chunk_ids[MAX_NUM_CHUNKS + 1];\n +- uint64_t chunk_sizes[MAX_NUM_CHUNKS + 1];\n ++ struct chunk_info chunks[MAX_NUM_CHUNKS + 1];\n const unsigned hashsz = the_hash_algo->rawsz;\n struct strbuf progress_title = STRBUF_INIT;\n int num_chunks = 3;\n @@ commit-graph.c: static int write_commit_graph_file(struct write_commit_graph_con\n - chunk_sizes[1] = hashsz * ctx->commits.nr;\n - chunk_ids[2] = GRAPH_CHUNKID_DATA;\n - chunk_sizes[2] = (hashsz + 16) * ctx->commits.nr;\n +-\n + chunks[0].id = GRAPH_CHUNKID_OIDFANOUT;\n + chunks[0].size = GRAPH_FANOUT_SIZE;\n + chunks[1].id = GRAPH_CHUNKID_OIDLOOKUP;\n @@ commit-graph.c: static int write_commit_graph_file(struct write_commit_graph_con\n + chunks[2].size = (hashsz + 16) * ctx->commits.nr;\n if (ctx->num_extra_edges) {\n - chunk_ids[num_chunks] = GRAPH_CHUNKID_EXTRAEDGES;\n -- chunk_sizes[3] = 4 * ctx->num_extra_edges;\n +- chunk_sizes[num_chunks] = 4 * ctx->num_extra_edges;\n + chunks[num_chunks].id = GRAPH_CHUNKID_EXTRAEDGES;\n + chunks[num_chunks].size = 4 * ctx->num_extra_edges;\n num_chunks++;\n }\n + if (ctx->changed_paths) {\n +- chunk_ids[num_chunks] = GRAPH_CHUNKID_BLOOMINDEXES;\n +- chunk_sizes[num_chunks] = sizeof(uint32_t) * ctx->commits.nr;\n ++ chunks[num_chunks].id = GRAPH_CHUNKID_BLOOMINDEXES;\n ++ chunks[num_chunks].size = sizeof(uint32_t) * ctx->commits.nr;\n + num_chunks++;\n +- chunk_ids[num_chunks] = GRAPH_CHUNKID_BLOOMDATA;\n +- chunk_sizes[num_chunks] = sizeof(uint32_t) * 3\n ++ chunks[num_chunks].id = GRAPH_CHUNKID_BLOOMDATA;\n ++ chunks[num_chunks].size = sizeof(uint32_t) * 3\n + + ctx->total_bloom_filter_data_size;\n + num_chunks++;\n + }\n if (ctx->num_commit_graphs_after > 1) {\n - chunk_ids[num_chunks] = GRAPH_CHUNKID_BASE;\n -- chunk_sizes[4] = hashsz * (ctx->num_commit_graphs_after - 1);\n +- chunk_sizes[num_chunks] = hashsz * (ctx->num_commit_graphs_after - 1);\n + chunks[num_chunks].id = GRAPH_CHUNKID_BASE;\n + chunks[num_chunks].size = hashsz * (ctx->num_commit_graphs_after - 1);\n num_chunks++;\n11: 56e3c4f57b3 < -: ----------- commit-graph: allocate the 'struct chunk_info' array dinamically\n12: 28fb1b5bdfe < -: ----------- commit-graph: unify the signatures of all write_graph_chunk_*() functions\n13: 1e1e59e2592 < -: ----------- commit-graph: simplify write_commit_graph_file() #3\n14: 6f0d912e4b8 < -: ----------- commit-graph: check chunk sizes after writing\n15: 0ab955aac32 < -: ----------- commit-graph-format.txt: document the modified path Bloom filter chunks\n16: 4c128d51dfe < -: ----------- Add a generic and minimal Bloom filter implementation\n17: 41f02bc38f7 < -: ----------- Import a streaming-capable Murmur3 hash function implementation\n18: e5fd1da48d4 < -: ----------- commit-graph: write \"empty\" Modified Path Bloom Filter Index chunk\n19: 2dd882ec601 < -: ----------- commit-graph: add commit slab for modified path Bloom filters\n20: f30e495c2b0 < -: ----------- commit-graph: fill the Modified Path Bloom Filter Index chunk\n21: e904cb58301 < -: ----------- commit-graph: load and use the Modified Path Bloom Filter Index chunk\n22: c71647ca374 < -: ----------- commit-graph: write the Modified Path Bloom Filters chunk\n23: 50898d42291 < -: ----------- commit-graph: load and use the Modified Path Bloom Filters chunk\n24: dc96f0d9822 < -: ----------- commit-graph: check all leading directories in modified path Bloom filters\n25: 7cbf1bc6b66 < -: ----------- commit-graph: check embedded modified path Bloom filters with a mask\n26: 3951fdedf6a < -: ----------- commit-graph: deduplicate modified path Bloom filters\n27: 5aba19a2766 < -: ----------- commit-graph: load modified path Bloom filters for merge commits\n28: 93fc6af1d2f < -: ----------- commit-graph: write Modified Path Bloom Filter Merge Index chunk\n29: f87b37bf08e < -: ----------- commit-graph: extract init and free write_commit_graph_context\n30: 943b0d9554c < -: ----------- commit-graph: move write_commit_graph_reachable below write_commit_graph\n31: 47b26ea61aa < -: ----------- t7007-show: make the first test compatible with the next patch\n32: 9201b71071c < -: ----------- PoC commit-graph: use revision walk machinery for '--reachable'\n33: 5c72d97e5e9 < -: ----------- commit-graph: write modified path Bloom filters in \"history order\"\n34: 8b40ec4cd30 < -: ----------- commit-graph: use modified path Bloom filters with wildcards, if possible\n\nThanks, -Stolee\n\nSZEDER Gábor (10):\n tree-walk.c: don't match submodule entries for 'submod/anything'\n commit-graph: fix parsing the Chunk Lookup table\n commit-graph-format.txt: all multi-byte numbers are in network byte\n order\n commit-slab: add a function to deep free entries on the slab\n diff.h: drop diff_tree_oid() & friends' return value\n commit-graph: clean up #includes\n commit-graph: simplify parse_commit_graph() #1\n commit-graph: simplify parse_commit_graph() #2\n commit-graph: simplify write_commit_graph_file() #1\n commit-graph: simplify write_commit_graph_file() #2\n\n .../technical/commit-graph-format.txt | 2 +-\n commit-graph.c | 113 ++++++++----------\n commit-graph.h | 6 +-\n commit-slab-decl.h | 1 +\n commit-slab-impl.h | 13 ++\n commit-slab.h | 10 ++\n diff.h | 10 +-\n revision.c | 9 +-\n shallow.c | 14 +--\n t/t4010-diff-pathspec.sh | 4 +-\n t/t5318-commit-graph.sh | 5 +-\n tree-diff.c | 30 +++--\n tree-walk.c | 9 +-\n 13 files changed, 117 insertions(+), 109 deletions(-)\n\n\nbase-commit: f32dde8c12d941065be848a9f66239df96bde216\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-650%2Fderrickstolee%2Fbloom-improvements-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-650/derrickstolee/bloom-improvements-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/650\n-- \ngitgitgadget\n"},{"id":"399173","messageId":"1e1671e7c699adf72fcf4ba8f03a7427ff30c9e7.1591362033.git.gitgitgadget@gmail.com","threadId":"53617","inReplyTo":"pull.650.git.1591362032.gitgitgadget@gmail.com","subject":"[PATCH 02/10] commit-graph: fix parsing the Chunk Lookup table","fromName":"SZEDER Gábor via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-05T13:00:24Z","receivedAt":"2020-06-05T13:00:42Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= \n\nThe commit-graph file format specifies that the chunks may be in any\norder. However, if the OID Lookup chunk happens to be the last one in\nthe file, then any command attempting to access the commit-graph data\nwill fail with:\n\n fatal: invalid commit position. commit-graph is likely corrupt\n\nIn this case the error is wrong, the commit-graph file does conform to\nthe specification, but the parsing of the Chunk Lookup table is a bit\nbuggy, and leaves the field holding the number of commits in the\ncommit-graph zero-initialized.\n\nThe number of commits in the commit-graph is determined while parsing\nthe Chunk Lookup table, by dividing the size of the OID Lookup chunk\nwith the hash size. However, the Chunk Lookup table doesn't actually\nstore the size of the chunks, but it stores their starting offset.\nConsequently, the size of a chunk can only be calculated by\nsubtracting the starting offsets of that chunk from the offset of the\nsubsequent chunk, or in case of the last chunk from the offset\nrecorded in the terminating label. This is currenly implemented in a\nbit complicated way: as we iterate over the entries of the Chunk\nLookup table, we check the ID of each chunk and store its starting\noffset, then we check the ID of the last seen chunk and calculate its\nsize using its previously saved offset if necessary (at the moment\nit's only necessary for the OID Lookup chunk). Alas, while parsing\nthe Chunk Lookup table we only interate through the \"real\" chunks, but\nnever look at the terminating label, thus don't even check whether\nit's necessary to calulate the size of the last chunk. Consequently,\nif the OID Lookup chunk is the last one, then we don't calculate its\nsize and turn don't run the piece of code determining the number of\ncommits in the commit graph, leaving the field holding that number\nunchanged (i.e. zero-initialized), eventually triggering the sanity\ncheck in load_oid_from_graph().\n\nFix this by iterating through all entries in the Chunk Lookup table,\nincluding the terminating label.\n\nNote that this is the minimal fix, suitable for the maintenance track.\nA better fix would be to simplify how the chunk sizes are calculated,\nbut that is a more invasive change, less suitable for 'maint', so that\nwill be done in later patches.\n\nThis additional flexibility of scanning more chunks breaks a test for\n\"git commit-graph verify\" so alter that test to mutate the commit-graph\nto have an even lower chunk count.\n\nSigned-off-by: SZEDER Gábor \nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 2 +-\n t/t5318-commit-graph.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 196e817a84c..7807d945626 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -277,7 +277,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \tlast_chunk_id = 0;\n \tlast_chunk_offset = 8;\n \tchunk_lookup = data + 8;\n-\tfor (i = 0; i < graph->num_chunks; i++) {\n+\tfor (i = 0; i <= graph->num_chunks; i++) {\n \t\tuint32_t chunk_id;\n \t\tuint64_t chunk_offset;\n \t\tint chunk_repeated = 0;\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 18304a65e4d..79e7fbcd40e 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -488,7 +488,7 @@ test_expect_success 'detect bad hash version' '\n '\n \n test_expect_success 'detect low chunk count' '\n-\tcorrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT \"\\02\" \\\n+\tcorrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT \"\\01\" \\\n \t\t\"missing the .* chunk\"\n '\n \n-- \ngitgitgadget\n\n"},{"id":"399174","messageId":"af84c253b243656f74707e4ea44d13b1d9a464d1.1591362032.git.gitgitgadget@gmail.com","threadId":"53617","inReplyTo":"pull.650.git.1591362032.gitgitgadget@gmail.com","subject":"[PATCH 01/10] tree-walk.c: don't match submodule entries for 'submod/anything'","fromName":"SZEDER Gábor via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-05T13:00:23Z","receivedAt":"2020-06-05T13:00:43Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= \n\nSubmodules should be handled the same as regular directories with\nrespect to the presence of a trailing slash, i.e. commands like:\n\n git diff rev1 rev2 -- $path\n git rev-list HEAD -- $path\n\nshould produce the same output whether $path is 'submod' or 'submod/'.\nThis has been fixed in commit 74b4f7f277 (tree-walk.c: ignore trailing\nslash on submodule in tree_entry_interesting(), 2014-01-23).\n\nUnfortunately, that commit had the unintended side effect to handle\n'submod/anything' the same as 'submod' and 'submod/' as well, e.g.:\n\n $ git log --oneline --name-only -- sha1collisiondetection/whatever\n 4125f78222 sha1dc: update from upstream\n sha1collisiondetection\n 07a20f569b Makefile: fix unaligned loads in sha1dc with UBSan\n sha1collisiondetection\n 23e37f8e9d sha1dc: update from upstream\n sha1collisiondetection\n 86cfd61e6b sha1dc: optionally use sha1collisiondetection as a submodule\n sha1collisiondetection\n\nFix this by rejecting submodules as partial pathnames when their\ntrailing slash is followed by anything.\n\nSigned-off-by: SZEDER Gábor \nSigned-off-by: Derrick Stolee \n---\n t/t4010-diff-pathspec.sh | 4 +++-\n tree-walk.c | 9 ++++++++-\n 2 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh\nindex e5ca359edfa..65cc703c659 100755\n--- a/t/t4010-diff-pathspec.sh\n+++ b/t/t4010-diff-pathspec.sh\n@@ -125,7 +125,9 @@ test_expect_success 'setup submodules' '\n test_expect_success 'diff-tree ignores trailing slash on submodule path' '\n \tgit diff --name-only HEAD^ HEAD submod >expect &&\n \tgit diff --name-only HEAD^ HEAD submod/ >actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\tgit diff --name-only HEAD^ HEAD -- submod/whatever >actual &&\n+\ttest_must_be_empty actual\n '\n \n test_expect_success 'diff multiple wildcard pathspecs' '\ndiff --git a/tree-walk.c b/tree-walk.c\nindex bb0ad34c545..0160294712b 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -851,7 +851,14 @@ static int match_entry(const struct pathspec_item *item,\n \tif (matchlen > pathlen) {\n \t\tif (match[pathlen] != '/')\n \t\t\treturn 0;\n-\t\tif (!S_ISDIR(entry->mode) && !S_ISGITLINK(entry->mode))\n+\t\t/*\n+\t\t * Reject non-directories as partial pathnames, except\n+\t\t * when match is a submodule with a trailing slash and\n+\t\t * nothing else (to handle 'submod/' and 'submod'\n+\t\t * uniformly).\n+\t\t */\n+\t\tif (!S_ISDIR(entry->mode) &&\n+\t\t (!S_ISGITLINK(entry->mode) || matchlen > pathlen + 1))\n \t\t\treturn 0;\n \t}\n \n-- \ngitgitgadget\n\n"},{"id":"399175","messageId":"a09d0bd5b7a9869430f4633df7f01651b4cfbdda.1591362033.git.gitgitgadget@gmail.com","threadId":"53617","inReplyTo":"pull.650.git.1591362032.gitgitgadget@gmail.com","subject":"[PATCH 03/10] commit-graph-format.txt: all multi-byte numbers are in network byte order","fromName":"SZEDER Gábor via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-05T13:00:25Z","receivedAt":"2020-06-05T13:00:45Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= \n\nThe commit-graph format specifies that \"All 4-byte numbers are in\nnetwork order\", but the commit-graph contains 8-byte integers as well\n(file offsets in the Chunk Lookup table), and their byte order is\nunspecified.\n\nClarify that all multi-byte integers are in network byte order.\n\nSigned-off-by: SZEDER Gábor \nSigned-off-by: Derrick Stolee \n---\n Documentation/technical/commit-graph-format.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/technical/commit-graph-format.txt b/Documentation/technical/commit-graph-format.txt\nindex 1beef171822..440541045d4 100644\n--- a/Documentation/technical/commit-graph-format.txt\n+++ b/Documentation/technical/commit-graph-format.txt\n@@ -32,7 +32,7 @@ the body into \"chunks\" and provide a binary lookup table at the beginning\n of the body. The header includes certain values, such as number of chunks\n and hash type.\n \n-All 4-byte numbers are in network order.\n+All multi-byte numbers are in network byte order.\n \n HEADER:\n \n-- \ngitgitgadget\n\n"},{"id":"399176","messageId":"32a6f11cc475f5292075093dcdc3b2a5c6d02ed0.1591362033.git.gitgitgadget@gmail.com","threadId":"53617","inReplyTo":"pull.650.git.1591362032.gitgitgadget@gmail.com","subject":"[PATCH 04/10] commit-slab: add a function to deep free entries on the slab","fromName":"SZEDER Gábor via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-05T13:00:26Z","receivedAt":"2020-06-05T13:00:50Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= \n\nclear_##slabname() frees only the memory allocated for a commit slab\nitself, but entries in the commit slab might own additional memory\noutside the slab that should be freed as well. We already have (at\nleast) one such commit slab, and this patch series is about to add one\nmore.\n\nTo free all additional memory owned by entries on the commit slab the\nuser of such a slab could iterate over all commits it knows about,\npeek whether there is a valid entry associated with each commit, and\nfree the additional memory, if any. Or it could rely on intimate\nknowledge about the internals of the commit slab implementation, and\ncould itself iterate directly through all entries in the slab, and\nfree the additional memory. Or it could just leak the additional\nmemory...\n\nIntroduce deep_clear_##slabname() to allow releasing memory owned by\ncommit slab entries by invoking the 'void free_fn(elemtype *ptr)'\nfunction specified as parameter for each entry in the slab.\n\nUse it in get_shallow_commits() in 'shallow.c' to replace an\nopen-coded iteration over a commit slab's entries.\n\nSigned-off-by: SZEDER Gábor \nSigned-off-by: Derrick Stolee \n---\n commit-slab-decl.h | 1 +\n commit-slab-impl.h | 13 +++++++++++++\n commit-slab.h | 10 ++++++++++\n shallow.c | 14 +++++---------\n 4 files changed, 29 insertions(+), 9 deletions(-)\n\ndiff --git a/commit-slab-decl.h b/commit-slab-decl.h\nindex adc7b46c83b..286164b7e27 100644\n--- a/commit-slab-decl.h\n+++ b/commit-slab-decl.h\n@@ -32,6 +32,7 @@ struct slabname {\t\t\t\t\t\t\t\\\n void init_ ##slabname## _with_stride(struct slabname *s, unsigned stride); \\\n void init_ ##slabname(struct slabname *s);\t\t\t\t\\\n void clear_ ##slabname(struct slabname *s);\t\t\t\t\\\n+void deep_clear_ ##slabname(struct slabname *s, void (*free_fn)(elemtype *ptr)); \\\n elemtype *slabname## _at_peek(struct slabname *s, const struct commit *c, int add_if_missing); \\\n elemtype *slabname## _at(struct slabname *s, const struct commit *c);\t\\\n elemtype *slabname## _peek(struct slabname *s, const struct commit *c)\ndiff --git a/commit-slab-impl.h b/commit-slab-impl.h\nindex 5c0eb91a5d1..557738df271 100644\n--- a/commit-slab-impl.h\n+++ b/commit-slab-impl.h\n@@ -38,6 +38,19 @@ scope void clear_ ##slabname(struct slabname *s)\t\t\t\\\n \tFREE_AND_NULL(s->slab);\t\t\t\t\t\t\\\n }\t\t\t\t\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n+scope void deep_clear_ ##slabname(struct slabname *s, void (*free_fn)(elemtype *)) \\\n+{\t\t\t\t\t\t\t\t\t\\\n+\tunsigned int i;\t\t\t\t\t\t\t\\\n+\tfor (i = 0; i < s->slab_count; i++) {\t\t\t\t\\\n+\t\tunsigned int j;\t\t\t\t\t\t\\\n+\t\tif (!s->slab[i])\t\t\t\t\t\\\n+\t\t\tcontinue;\t\t\t\t\t\\\n+\t\tfor (j = 0; j < s->slab_size; j++)\t\t\t\\\n+\t\t\tfree_fn(&s->slab[i][j * s->stride]);\t\t\\\n+\t}\t\t\t\t\t\t\t\t\\\n+\tclear_ ##slabname(s);\t\t\t\t\t\t\\\n+}\t\t\t\t\t\t\t\t\t\\\n+\t\t\t\t\t\t\t\t\t\\\n scope elemtype *slabname## _at_peek(struct slabname *s,\t\t\t\\\n \t\t\t\t\t\t const struct commit *c, \\\n \t\t\t\t\t\t int add_if_missing) \\\ndiff --git a/commit-slab.h b/commit-slab.h\nindex 05b3f2804e7..8e72a305365 100644\n--- a/commit-slab.h\n+++ b/commit-slab.h\n@@ -47,6 +47,16 @@\n *\n * Call this function before the slab falls out of scope to avoid\n * leaking memory.\n+ *\n+ * - void deep_clear_indegree(struct indegree *, void (*free_fn)(int*))\n+ *\n+ * Empties the slab, similar to clear_indegree(), but in addition it\n+ * calls the given 'free_fn' for each slab entry to release any\n+ * additional memory that might be owned by the entry (but not the\n+ * entry itself!).\n+ * Note that 'free_fn' might be called even for entries for which no\n+ * indegree_at() call has been made; in this case 'free_fn' is invoked\n+ * with a pointer to a zero-initialized location.\n */\n \n #define define_commit_slab(slabname, elemtype) \\\ndiff --git a/shallow.c b/shallow.c\nindex 7fd04afed19..c4ac8a73273 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -84,6 +84,10 @@ int is_repository_shallow(struct repository *r)\n * supports a \"valid\" flag.\n */\n define_commit_slab(commit_depth, int *);\n+static void free_depth_in_slab(int **ptr)\n+{\n+\tFREE_AND_NULL(*ptr);\n+}\n struct commit_list *get_shallow_commits(struct object_array *heads, int depth,\n \t\tint shallow_flag, int not_shallow_flag)\n {\n@@ -150,15 +154,7 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,\n \t\t\t}\n \t\t}\n \t}\n-\tfor (i = 0; i < depths.slab_count; i++) {\n-\t\tint j;\n-\n-\t\tif (!depths.slab[i])\n-\t\t\tcontinue;\n-\t\tfor (j = 0; j < depths.slab_size; j++)\n-\t\t\tfree(depths.slab[i][j]);\n-\t}\n-\tclear_commit_depth(&depths);\n+\tdeep_clear_commit_depth(&depths, free_depth_in_slab);\n \n \treturn result;\n }\n-- \ngitgitgadget\n\n"},{"id":"399177","messageId":"1a70ff05aea11cb479d9ba7529ccad6dcd79e23d.1591362033.git.gitgitgadget@gmail.com","threadId":"53617","inReplyTo":"pull.650.git.1591362032.gitgitgadget@gmail.com","subject":"[PATCH 05/10] diff.h: drop diff_tree_oid() & friends' return value","fromName":"SZEDER Gábor via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-05T13:00:27Z","receivedAt":"2020-06-05T13:00:52Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= \n\nll_diff_tree_oid() has only ever returned 0 [1], so it's return value\nis basically useless. It's only caller diff_tree_oid() has only ever\nreturned the return value of ll_diff_tree_oid() as-is [2], so its\nreturn value is just as useless. Most of diff_tree_oid()'s callers\nsimply ignore its return value, except:\n\n - diff_root_tree_oid() is a thin wrapper around diff_tree_oid() and\n returns with its return value, but all of diff_root_tree_oid()'s\n callers ignore its return value.\n\n - rev_compare_tree() and rev_same_tree_as_empty() do look at the\n return value in a condition, but, since the return value is always\n 0, the former's < 0 condition is never fulfilled, while the\n latter's >= 0 condition is always fulfilled.\n\nSo let's drop the return value of ll_diff_tree_oid(), diff_tree_oid()\nand diff_root_tree_oid(), and drop those conditions from\nrev_compare_tree() and rev_same_tree_as_empty() as well.\n\n[1] ll_diff_tree_oid() and its ancestors have been returning only 0\n ever since it was introduced as diff_tree() in 9174026cfe (Add\n \"diff-tree\" program to show which files have changed between two\n trees., 2005-04-09).\n[2] diff_tree_oid() traces back to diff-tree.c:main() in 9174026cfe as\n well.\n\nSigned-off-by: SZEDER Gábor \nSigned-off-by: Derrick Stolee \n---\n diff.h | 10 +++++-----\n revision.c | 9 +++------\n tree-diff.c | 30 ++++++++++++++----------------\n 3 files changed, 22 insertions(+), 27 deletions(-)\n\ndiff --git a/diff.h b/diff.h\nindex 9443dc1b003..e0c0af6286b 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -431,11 +431,11 @@ struct combine_diff_path *diff_tree_paths(\n \tstruct combine_diff_path *p, const struct object_id *oid,\n \tconst struct object_id **parents_oid, int nparent,\n \tstruct strbuf *base, struct diff_options *opt);\n-int diff_tree_oid(const struct object_id *old_oid,\n-\t\t const struct object_id *new_oid,\n-\t\t const char *base, struct diff_options *opt);\n-int diff_root_tree_oid(const struct object_id *new_oid, const char *base,\n-\t\t struct diff_options *opt);\n+void diff_tree_oid(const struct object_id *old_oid,\n+\t\t const struct object_id *new_oid,\n+\t\t const char *base, struct diff_options *opt);\n+void diff_root_tree_oid(const struct object_id *new_oid, const char *base,\n+\t\t\tstruct diff_options *opt);\n \n struct combine_diff_path {\n \tstruct combine_diff_path *next;\ndiff --git a/revision.c b/revision.c\nindex cbf4b61aa67..c644c660917 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -791,9 +791,7 @@ static int rev_compare_tree(struct rev_info *revs,\n \n \ttree_difference = REV_TREE_SAME;\n \trevs->pruning.flags.has_changes = 0;\n-\tif (diff_tree_oid(&t1->object.oid, &t2->object.oid, \"\",\n-\t\t\t &revs->pruning) < 0)\n-\t\treturn REV_TREE_DIFFERENT;\n+\tdiff_tree_oid(&t1->object.oid, &t2->object.oid, \"\", &revs->pruning);\n \n \tif (!nth_parent)\n \t\tif (bloom_ret == 1 && tree_difference == REV_TREE_SAME)\n@@ -804,7 +802,6 @@ static int rev_compare_tree(struct rev_info *revs,\n \n static int rev_same_tree_as_empty(struct rev_info *revs, struct commit *commit)\n {\n-\tint retval;\n \tstruct tree *t1 = get_commit_tree(commit);\n \n \tif (!t1)\n@@ -812,9 +809,9 @@ static int rev_same_tree_as_empty(struct rev_info *revs, struct commit *commit)\n \n \ttree_difference = REV_TREE_SAME;\n \trevs->pruning.flags.has_changes = 0;\n-\tretval = diff_tree_oid(NULL, &t1->object.oid, \"\", &revs->pruning);\n+\tdiff_tree_oid(NULL, &t1->object.oid, \"\", &revs->pruning);\n \n-\treturn retval >= 0 && (tree_difference == REV_TREE_SAME);\n+\treturn tree_difference == REV_TREE_SAME;\n }\n \n struct treesame_state {\ndiff --git a/tree-diff.c b/tree-diff.c\nindex f3d303c6e54..6ebad1a46f3 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -29,9 +29,9 @@ static struct combine_diff_path *ll_diff_tree_paths(\n \tstruct combine_diff_path *p, const struct object_id *oid,\n \tconst struct object_id **parents_oid, int nparent,\n \tstruct strbuf *base, struct diff_options *opt);\n-static int ll_diff_tree_oid(const struct object_id *old_oid,\n-\t\t\t const struct object_id *new_oid,\n-\t\t\t struct strbuf *base, struct diff_options *opt);\n+static void ll_diff_tree_oid(const struct object_id *old_oid,\n+\t\t\t const struct object_id *new_oid,\n+\t\t\t struct strbuf *base, struct diff_options *opt);\n \n /*\n * Compare two tree entries, taking into account only path/S_ISDIR(mode),\n@@ -679,9 +679,9 @@ static void try_to_follow_renames(const struct object_id *old_oid,\n \tq->nr = 1;\n }\n \n-static int ll_diff_tree_oid(const struct object_id *old_oid,\n-\t\t\t const struct object_id *new_oid,\n-\t\t\t struct strbuf *base, struct diff_options *opt)\n+static void ll_diff_tree_oid(const struct object_id *old_oid,\n+\t\t\t const struct object_id *new_oid,\n+\t\t\t struct strbuf *base, struct diff_options *opt)\n {\n \tstruct combine_diff_path phead, *p;\n \tpathchange_fn_t pathchange_old = opt->pathchange;\n@@ -697,29 +697,27 @@ static int ll_diff_tree_oid(const struct object_id *old_oid,\n \t}\n \n \topt->pathchange = pathchange_old;\n-\treturn 0;\n }\n \n-int diff_tree_oid(const struct object_id *old_oid,\n-\t\t const struct object_id *new_oid,\n-\t\t const char *base_str, struct diff_options *opt)\n+void diff_tree_oid(const struct object_id *old_oid,\n+\t\t const struct object_id *new_oid,\n+\t\t const char *base_str, struct diff_options *opt)\n {\n \tstruct strbuf base;\n-\tint retval;\n \n \tstrbuf_init(&base, PATH_MAX);\n \tstrbuf_addstr(&base, base_str);\n \n-\tretval = ll_diff_tree_oid(old_oid, new_oid, &base, opt);\n+\tll_diff_tree_oid(old_oid, new_oid, &base, opt);\n \tif (!*base_str && opt->flags.follow_renames && diff_might_be_rename())\n \t\ttry_to_follow_renames(old_oid, new_oid, &base, opt);\n \n \tstrbuf_release(&base);\n-\n-\treturn retval;\n }\n \n-int diff_root_tree_oid(const struct object_id *new_oid, const char *base, struct diff_options *opt)\n+void diff_root_tree_oid(const struct object_id *new_oid,\n+\t\t\tconst char *base,\n+\t\t\tstruct diff_options *opt)\n {\n-\treturn diff_tree_oid(NULL, new_oid, base, opt);\n+\tdiff_tree_oid(NULL, new_oid, base, opt);\n }\n-- \ngitgitgadget\n\n"},{"id":"399178","messageId":"cd9e033d1b1b4cfb0d5fe485b8b37608c2227ec2.1591362033.git.gitgitgadget@gmail.com","threadId":"53617","inReplyTo":"pull.650.git.1591362032.gitgitgadget@gmail.com","subject":"[PATCH 07/10] commit-graph: simplify parse_commit_graph() #1","fromName":"SZEDER Gábor via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-05T13:00:29Z","receivedAt":"2020-06-05T13:00:53Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= \n\nWhile we iterate over all entries of the Chunk Lookup table we make\nsure that we don't attempt to read past the end of the mmap-ed\ncommit-graph file, and check in each iteration that the chunk ID and\noffset we are about to read is still within the mmap-ed memory region.\nHowever, these checks in each iteration are not really necessary,\nbecause the number of chunks in the commit-graph file is already known\nbefore this loop from the just parsed commit-graph header.\n\nSo let's check that the commit-graph file is large enough for all\nentries in the Chunk Lookup table before we start iterating over those\nentries, and drop those per-iteration checks. While at it, take into\naccount the size of everything that is necessary to have a valid\ncommit-graph file, i.e. the size of the header, the size of the\nmandatory OID Fanout chunk, and the size of the signature in the\ntrailer as well.\n\nNote that this necessitates the change of the error message as well,\nand, consequently, have to update the 'detect incorrect chunk count'\ntest in 't5318-commit-graph.sh' as well.\n\nSigned-off-by: SZEDER Gábor \nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 16 +++++++++-------\n t/t5318-commit-graph.sh | 3 ++-\n 2 files changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 6ed649388d6..9927762f18c 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -272,6 +272,15 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \tgraph->data = graph_map;\n \tgraph->data_len = graph_size;\n \n+\tif (graph_size < GRAPH_HEADER_SIZE +\n+\t\t\t (graph->num_chunks + 1) * GRAPH_CHUNKLOOKUP_WIDTH +\n+\t\t\t GRAPH_FANOUT_SIZE + the_hash_algo->rawsz) {\n+\t\terror(_(\"commit-graph file is too small to hold %u chunks\"),\n+\t\t graph->num_chunks);\n+\t\tfree(graph);\n+\t\treturn NULL;\n+\t}\n+\n \tlast_chunk_id = 0;\n \tlast_chunk_offset = 8;\n \tchunk_lookup = data + 8;\n@@ -280,13 +289,6 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\tuint64_t chunk_offset;\n \t\tint chunk_repeated = 0;\n \n-\t\tif (data + graph_size - chunk_lookup <\n-\t\t GRAPH_CHUNKLOOKUP_WIDTH) {\n-\t\t\terror(_(\"commit-graph chunk lookup table entry missing; file may be incomplete\"));\n-\t\t\tfree(graph);\n-\t\t\treturn NULL;\n-\t\t}\n-\n \t\tchunk_id = get_be32(chunk_lookup + 0);\n \t\tchunk_offset = get_be64(chunk_lookup + 4);\n \ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 79e7fbcd40e..1073f9e3cf2 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -574,7 +574,8 @@ test_expect_success 'detect invalid checksum hash' '\n \n test_expect_success 'detect incorrect chunk count' '\n \tcorrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT \"\\377\" \\\n-\t\t\"chunk lookup table entry missing\" $GRAPH_CHUNK_LOOKUP_OFFSET\n+\t\t\"commit-graph file is too small to hold [0-9]* chunks\" \\\n+\t\t$GRAPH_CHUNK_LOOKUP_OFFSET\n '\n \n test_expect_success 'git fsck (checks commit-graph)' '\n-- \ngitgitgadget\n\n"},{"id":"399179","messageId":"636c20696592cc8f3ed6d300daa13f51cab360d7.1591362033.git.gitgitgadget@gmail.com","threadId":"53617","inReplyTo":"pull.650.git.1591362032.gitgitgadget@gmail.com","subject":"[PATCH 06/10] commit-graph: clean up #includes","fromName":"SZEDER Gábor via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-05T13:00:28Z","receivedAt":"2020-06-05T13:00:53Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= \n\nOur CodingGuidelines says that it's sufficient to include one of\n'git-compat-util.h' and 'cache.h', but both 'commit-graph.c' and\n'commit-graph.h' include both. Let's include only 'git-compat-util.h'\nto loose a bunch of unnecessary dependencies; but include 'hash.h',\nbecause 'commit-graph.h' does require the definition of 'struct\nobject_id'.\n\n'commit-graph.h' explicitly includes 'repository.h' and\n'string-list.h', but only needs the declaration of a few structs from\nthem. Drop these includes and forward-declare the necessary structs\ninstead.\n\n'commit-graph.c' includes 'dir.h', but doesn't actually use anything\nfrom there, so let's drop that #include as well.\n\nSigned-off-by: SZEDER Gábor \nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 4 +---\n commit-graph.h | 6 +++---\n 2 files changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 7807d945626..6ed649388d6 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1,7 +1,5 @@\n-#include \"cache.h\"\n-#include \"config.h\"\n-#include \"dir.h\"\n #include \"git-compat-util.h\"\n+#include \"config.h\"\n #include \"lockfile.h\"\n #include \"pack.h\"\n #include \"packfile.h\"\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 39484482cc1..881c9b46e57 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -2,9 +2,6 @@\n #define COMMIT_GRAPH_H\n \n #include \"git-compat-util.h\"\n-#include \"repository.h\"\n-#include \"string-list.h\"\n-#include \"cache.h\"\n #include \"object-store.h\"\n \n #define GIT_TEST_COMMIT_GRAPH \"GIT_TEST_COMMIT_GRAPH\"\n@@ -22,6 +19,9 @@ void git_test_write_commit_graph_or_die(void);\n \n struct commit;\n struct bloom_filter_settings;\n+struct repository;\n+struct raw_object_store;\n+struct string_list;\n \n char *get_commit_graph_filename(struct object_directory *odb);\n int open_commit_graph(const char *graph_file, int *fd, struct stat *st);\n-- \ngitgitgadget\n\n"},{"id":"399180","messageId":"83641b5e49e4f2abd2185fc1cde58eadad496cd4.1591362033.git.gitgitgadget@gmail.com","threadId":"53617","inReplyTo":"pull.650.git.1591362032.gitgitgadget@gmail.com","subject":"[PATCH 08/10] commit-graph: simplify parse_commit_graph() #2","fromName":"SZEDER Gábor via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-05T13:00:30Z","receivedAt":"2020-06-05T13:00:56Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= \n\nThe Chunk Lookup table stores the chunks' starting offset in the\ncommit-graph file, not their sizes. Consequently, the size of a chunk\ncan only be calculated by subtracting its offset from the offset of\nthe subsequent chunk (or that of the terminating label). This is\ncurrenly implemented in a bit complicated way: as we iterate over the\nentries of the Chunk Lookup table, we check the id of each chunk and\nstore its starting offset, then we check the id of the last seen chunk\nand calculate its size using its previously saved offset. At the\nmoment there is only one chunk for which we calculate its size, but\nthis patch series will add more, and the repeated chunk id checks are\nnot that pretty.\n\nInstead let's read ahead the offset of the next chunk on each\niteration, so we can calculate the size of each chunk right away,\nright where we store its starting offset.\n\nSigned-off-by: SZEDER Gábor \nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 26 +++++++++-----------------\n 1 file changed, 9 insertions(+), 17 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 9927762f18c..84206f0f512 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -230,8 +230,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \tconst unsigned char *data, *chunk_lookup;\n \tuint32_t i;\n \tstruct commit_graph *graph;\n-\tuint64_t last_chunk_offset;\n-\tuint32_t last_chunk_id;\n+\tuint64_t next_chunk_offset;\n \tuint32_t graph_signature;\n \tunsigned char graph_version, hash_version;\n \n@@ -281,18 +280,17 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\treturn NULL;\n \t}\n \n-\tlast_chunk_id = 0;\n-\tlast_chunk_offset = 8;\n \tchunk_lookup = data + 8;\n-\tfor (i = 0; i <= graph->num_chunks; i++) {\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;\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-\t\tchunk_offset = get_be64(chunk_lookup + 4);\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@@ -312,8 +310,11 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\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\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@@ -368,15 +369,6 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\t\tfree(graph);\n \t\t\treturn NULL;\n \t\t}\n-\n-\t\tif (last_chunk_id == GRAPH_CHUNKID_OIDLOOKUP)\n-\t\t{\n-\t\t\tgraph->num_commits = (chunk_offset - last_chunk_offset)\n-\t\t\t\t\t / graph->hash_len;\n-\t\t}\n-\n-\t\tlast_chunk_id = chunk_id;\n-\t\tlast_chunk_offset = chunk_offset;\n \t}\n \n \tif (graph->chunk_bloom_indexes && graph->chunk_bloom_data) {\n-- \ngitgitgadget\n\n"},{"id":"399181","messageId":"9b818b9cb9128d19f3ce6087cb34cdf4dd601977.1591362033.git.gitgitgadget@gmail.com","threadId":"53617","inReplyTo":"pull.650.git.1591362032.gitgitgadget@gmail.com","subject":"[PATCH 09/10] commit-graph: simplify write_commit_graph_file() #1","fromName":"SZEDER Gábor via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-05T13:00:31Z","receivedAt":"2020-06-05T13:00:57Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= \n\nIn write_commit_graph_file() one block of code fills the array of\nchunk IDs, another block of code fills the array of chunk offsets,\nthen the chunk IDs and offsets are written to the Chunk Lookup table,\nand finally a third block of code writes the actual chunks. In case\nof optional chunks like Extra Edge List and Base Graphs List there is\nalso a condition checking whether that chunk is necessary/desired, and\nthat same condition is repeated in all those three blocks of code.\nThis patch series is about to add more optional chunks, so there would\nbe even more repeated conditions.\n\nThose chunk offsets are relative to the beginning of the file, so they\ninherently depend on the size of the Chunk Lookup table, which in turn\ndepends on the number of chunks that are to be written to the\ncommit-graph file. IOW at the time we set the first chunk's ID we\ncan't yet know its offset, because we don't yet know how many chunks\nthere are.\n\nSimplify this by initially filling an array of chunk sizes, not\noffsets, and calculate the offsets based on the chunk sizes only\nlater, while we are writing the Chunk Lookup table. This way we can\nfill the arrays of chunk IDs and sizes in one go, eliminating one set\nof repeated conditions.\n\nSigned-off-by: SZEDER Gábor \nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 46 +++++++++++++++++-----------------------------\n 1 file changed, 17 insertions(+), 29 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 84206f0f512..79cddabcd12 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1529,10 +1529,11 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \tstruct hashfile *f;\n \tstruct lock_file lk = LOCK_INIT;\n \tuint32_t chunk_ids[MAX_NUM_CHUNKS + 1];\n-\tuint64_t chunk_offsets[MAX_NUM_CHUNKS + 1];\n+\tuint64_t chunk_sizes[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+\tuint64_t chunk_offset;\n \tstruct object_id file_hash;\n \tconst struct bloom_filter_settings bloom_settings = DEFAULT_BLOOM_FILTER_SETTINGS;\n \n@@ -1573,50 +1574,34 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t}\n \n \tchunk_ids[0] = GRAPH_CHUNKID_OIDFANOUT;\n+\tchunk_sizes[0] = GRAPH_FANOUT_SIZE;\n \tchunk_ids[1] = GRAPH_CHUNKID_OIDLOOKUP;\n+\tchunk_sizes[1] = hashsz * ctx->commits.nr;\n \tchunk_ids[2] = GRAPH_CHUNKID_DATA;\n+\tchunk_sizes[2] = (hashsz + 16) * ctx->commits.nr;\n+\n \tif (ctx->num_extra_edges) {\n \t\tchunk_ids[num_chunks] = GRAPH_CHUNKID_EXTRAEDGES;\n+\t\tchunk_sizes[num_chunks] = 4 * ctx->num_extra_edges;\n \t\tnum_chunks++;\n \t}\n \tif (ctx->changed_paths) {\n \t\tchunk_ids[num_chunks] = GRAPH_CHUNKID_BLOOMINDEXES;\n+\t\tchunk_sizes[num_chunks] = sizeof(uint32_t) * ctx->commits.nr;\n \t\tnum_chunks++;\n \t\tchunk_ids[num_chunks] = GRAPH_CHUNKID_BLOOMDATA;\n+\t\tchunk_sizes[num_chunks] = sizeof(uint32_t) * 3\n+\t\t\t\t\t + ctx->total_bloom_filter_data_size;\n \t\tnum_chunks++;\n \t}\n \tif (ctx->num_commit_graphs_after > 1) {\n \t\tchunk_ids[num_chunks] = GRAPH_CHUNKID_BASE;\n+\t\tchunk_sizes[num_chunks] = hashsz * (ctx->num_commit_graphs_after - 1);\n \t\tnum_chunks++;\n \t}\n \n \tchunk_ids[num_chunks] = 0;\n-\n-\tchunk_offsets[0] = 8 + (num_chunks + 1) * GRAPH_CHUNKLOOKUP_WIDTH;\n-\tchunk_offsets[1] = chunk_offsets[0] + GRAPH_FANOUT_SIZE;\n-\tchunk_offsets[2] = chunk_offsets[1] + hashsz * ctx->commits.nr;\n-\tchunk_offsets[3] = chunk_offsets[2] + (hashsz + 16) * ctx->commits.nr;\n-\n-\tnum_chunks = 3;\n-\tif (ctx->num_extra_edges) {\n-\t\tchunk_offsets[num_chunks + 1] = chunk_offsets[num_chunks] +\n-\t\t\t\t\t\t4 * ctx->num_extra_edges;\n-\t\tnum_chunks++;\n-\t}\n-\tif (ctx->changed_paths) {\n-\t\tchunk_offsets[num_chunks + 1] = chunk_offsets[num_chunks] +\n-\t\t\t\t\t\tsizeof(uint32_t) * ctx->commits.nr;\n-\t\tnum_chunks++;\n-\n-\t\tchunk_offsets[num_chunks + 1] = chunk_offsets[num_chunks] +\n-\t\t\t\t\t\tsizeof(uint32_t) * 3 + ctx->total_bloom_filter_data_size;\n-\t\tnum_chunks++;\n-\t}\n-\tif (ctx->num_commit_graphs_after > 1) {\n-\t\tchunk_offsets[num_chunks + 1] = chunk_offsets[num_chunks] +\n-\t\t\t\t\t\thashsz * (ctx->num_commit_graphs_after - 1);\n-\t\tnum_chunks++;\n-\t}\n+\tchunk_sizes[num_chunks] = 0;\n \n \thashwrite_be32(f, GRAPH_SIGNATURE);\n \n@@ -1625,13 +1610,16 @@ 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(chunk_ids[i]);\n-\t\tchunk_write[1] = htonl(chunk_offsets[i] >> 32);\n-\t\tchunk_write[2] = htonl(chunk_offsets[i] & 0xffffffff);\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 += chunk_sizes[i];\n \t}\n \n \tif (ctx->report_progress) {\n-- \ngitgitgadget\n\n"},{"id":"399182","messageId":"5984fb01ebcff326fe137720e5fcf784cf321e34.1591362033.git.gitgitgadget@gmail.com","threadId":"53617","inReplyTo":"pull.650.git.1591362032.gitgitgadget@gmail.com","subject":"[PATCH 10/10] commit-graph: simplify write_commit_graph_file() #2","fromName":"SZEDER Gábor via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-05T13:00:32Z","receivedAt":"2020-06-05T13:00:58Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= \n\nUnify the 'chunk_ids' and 'chunk_sizes' arrays into an array of\n'struct chunk_info'. This will allow more cleanups in the following\npatches.\n\nSigned-off-by: SZEDER Gábor \nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 45 ++++++++++++++++++++++++---------------------\n 1 file changed, 24 insertions(+), 21 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 79cddabcd12..887837e8826 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1522,14 +1522,18 @@ static int write_graph_chunk_base(struct hashfile *f,\n \treturn 0;\n }\n \n+struct chunk_info {\n+\tuint32_t id;\n+\tuint64_t size;\n+};\n+\n static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n {\n \tuint32_t i;\n \tint fd;\n \tstruct hashfile *f;\n \tstruct lock_file lk = LOCK_INIT;\n-\tuint32_t chunk_ids[MAX_NUM_CHUNKS + 1];\n-\tuint64_t chunk_sizes[MAX_NUM_CHUNKS + 1];\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@@ -1573,35 +1577,34 @@ 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-\tchunk_ids[0] = GRAPH_CHUNKID_OIDFANOUT;\n-\tchunk_sizes[0] = GRAPH_FANOUT_SIZE;\n-\tchunk_ids[1] = GRAPH_CHUNKID_OIDLOOKUP;\n-\tchunk_sizes[1] = hashsz * ctx->commits.nr;\n-\tchunk_ids[2] = GRAPH_CHUNKID_DATA;\n-\tchunk_sizes[2] = (hashsz + 16) * ctx->commits.nr;\n-\n+\tchunks[0].id = GRAPH_CHUNKID_OIDFANOUT;\n+\tchunks[0].size = GRAPH_FANOUT_SIZE;\n+\tchunks[1].id = GRAPH_CHUNKID_OIDLOOKUP;\n+\tchunks[1].size = hashsz * ctx->commits.nr;\n+\tchunks[2].id = GRAPH_CHUNKID_DATA;\n+\tchunks[2].size = (hashsz + 16) * ctx->commits.nr;\n \tif (ctx->num_extra_edges) {\n-\t\tchunk_ids[num_chunks] = GRAPH_CHUNKID_EXTRAEDGES;\n-\t\tchunk_sizes[num_chunks] = 4 * 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\tnum_chunks++;\n \t}\n \tif (ctx->changed_paths) {\n-\t\tchunk_ids[num_chunks] = GRAPH_CHUNKID_BLOOMINDEXES;\n-\t\tchunk_sizes[num_chunks] = sizeof(uint32_t) * ctx->commits.nr;\n+\t\tchunks[num_chunks].id = GRAPH_CHUNKID_BLOOMINDEXES;\n+\t\tchunks[num_chunks].size = sizeof(uint32_t) * ctx->commits.nr;\n \t\tnum_chunks++;\n-\t\tchunk_ids[num_chunks] = GRAPH_CHUNKID_BLOOMDATA;\n-\t\tchunk_sizes[num_chunks] = sizeof(uint32_t) * 3\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\tnum_chunks++;\n \t}\n \tif (ctx->num_commit_graphs_after > 1) {\n-\t\tchunk_ids[num_chunks] = GRAPH_CHUNKID_BASE;\n-\t\tchunk_sizes[num_chunks] = hashsz * (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\tnum_chunks++;\n \t}\n \n-\tchunk_ids[num_chunks] = 0;\n-\tchunk_sizes[num_chunks] = 0;\n+\tchunks[num_chunks].id = 0;\n+\tchunks[num_chunks].size = 0;\n \n \thashwrite_be32(f, GRAPH_SIGNATURE);\n \n@@ -1614,12 +1617,12 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \tfor (i = 0; i <= num_chunks; i++) {\n \t\tuint32_t chunk_write[3];\n \n-\t\tchunk_write[0] = htonl(chunk_ids[i]);\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 += chunk_sizes[i];\n+\t\tchunk_offset += chunks[i].size;\n \t}\n \n \tif (ctx->report_progress) {\n-- \ngitgitgadget\n"},{"id":"399309","messageId":"xmqq8sgxlan2.fsf@gitster.c.googlers.com","threadId":"53617","inReplyTo":"pull.650.git.1591362032.gitgitgadget@gmail.com","subject":"Re: [PATCH 00/10] Szeder's commit-graph cleanups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-08T17:39:45Z","receivedAt":"2020-06-08T17:39:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> This is based on ds/line-log-on-bloom.\n>\n> Since Szeder so kindly shared his alternate Bloom filter implementation [1],\n> I thought it worth my time to start the process of updating the patches to\n> apply to more recent code in Git. Here is the effort to update the almost\n> obviously-good commit-graph cleanups that he presented in that series.\n\nVery pleased to see collaboration.\n"},{"id":"400019","messageId":"2e5594f5-5da9-0b7a-7bdf-122d8e615cd4@gmail.com","threadId":"53617","inReplyTo":"pull.650.git.1591362032.gitgitgadget@gmail.com","subject":"Re: [PATCH 00/10] Szeder's commit-graph cleanups","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-06-18T01:48:56Z","receivedAt":"2020-06-18T01:49:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/5/2020 9:00 AM, Derrick Stolee via GitGitGadget wrote:\n> This is based on ds/line-log-on-bloom.\n> \n> Since Szeder so kindly shared his alternate Bloom filter implementation [1],\n> I thought it worth my time to start the process of updating the patches to\n> apply to more recent code in Git. Here is the effort to update the almost\n> obviously-good commit-graph cleanups that he presented in that series.\n> \n> [1] https://lore.kernel.org/git/20200529085038.26008-1-szeder.dev@gmail.com/\n\nHello everyone,\n\nIs anyone available to look at this series? Hopefully all of the changes\nare simple to read. Szeder did a great job crafting small patches, and\nhopefully I didn't butcher them too much applying them to recent commits.\n\nThanks,\n-Stolee\n"},{"id":"400089","messageId":"1c982d57-a446-b655-af95-1e4aeabf30ef@web.de","threadId":"53617","inReplyTo":"32a6f11cc475f5292075093dcdc3b2a5c6d02ed0.1591362033.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 04/10] commit-slab: add a function to deep free entries on the slab","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-06-18T20:59:48Z","receivedAt":"2020-06-18T20:59:59Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.06.20 um 15:00 schrieb SZEDER Gábor via GitGitGadget:\n> From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= \n>\n> clear_##slabname() frees only the memory allocated for a commit slab\n> itself, but entries in the commit slab might own additional memory\n> outside the slab that should be freed as well. We already have (at\n> least) one such commit slab, and this patch series is about to add one\n> more.\n>\n> To free all additional memory owned by entries on the commit slab the\n> user of such a slab could iterate over all commits it knows about,\n> peek whether there is a valid entry associated with each commit, and\n> free the additional memory, if any. Or it could rely on intimate\n> knowledge about the internals of the commit slab implementation, and\n> could itself iterate directly through all entries in the slab, and\n> free the additional memory. Or it could just leak the additional\n> memory...\n>\n> Introduce deep_clear_##slabname() to allow releasing memory owned by\n> commit slab entries by invoking the 'void free_fn(elemtype *ptr)'\n> function specified as parameter for each entry in the slab.\n\nAdding a new function instead of extending the existing ones makes\nsense, as this is a rare requirement.\n\n>\n> Use it in get_shallow_commits() in 'shallow.c' to replace an\n> open-coded iteration over a commit slab's entries.\n>\n> Signed-off-by: SZEDER Gábor \n> Signed-off-by: Derrick Stolee \n> ---\n> commit-slab-decl.h | 1 +\n> commit-slab-impl.h | 13 +++++++++++++\n> commit-slab.h | 10 ++++++++++\n> shallow.c | 14 +++++---------\n> 4 files changed, 29 insertions(+), 9 deletions(-)\n>\n> diff --git a/commit-slab-decl.h b/commit-slab-decl.h\n> index adc7b46c83b..286164b7e27 100644\n> --- a/commit-slab-decl.h\n> +++ b/commit-slab-decl.h\n> @@ -32,6 +32,7 @@ struct slabname {\t\t\t\t\t\t\t\\\n> void init_ ##slabname## _with_stride(struct slabname *s, unsigned stride); \\\n> void init_ ##slabname(struct slabname *s);\t\t\t\t\\\n> void clear_ ##slabname(struct slabname *s);\t\t\t\t\\\n> +void deep_clear_ ##slabname(struct slabname *s, void (*free_fn)(elemtype *ptr)); \\\n> elemtype *slabname## _at_peek(struct slabname *s, const struct commit *c, int add_if_missing); \\\n> elemtype *slabname## _at(struct slabname *s, const struct commit *c);\t\\\n> elemtype *slabname## _peek(struct slabname *s, const struct commit *c)\n> diff --git a/commit-slab-impl.h b/commit-slab-impl.h\n> index 5c0eb91a5d1..557738df271 100644\n> --- a/commit-slab-impl.h\n> +++ b/commit-slab-impl.h\n> @@ -38,6 +38,19 @@ scope void clear_ ##slabname(struct slabname *s)\t\t\t\\\n> \tFREE_AND_NULL(s->slab);\t\t\t\t\t\t\\\n> }\t\t\t\t\t\t\t\t\t\\\n> \t\t\t\t\t\t\t\t\t\\\n> +scope void deep_clear_ ##slabname(struct slabname *s, void (*free_fn)(elemtype *)) \\\n> +{\t\t\t\t\t\t\t\t\t\\\n> +\tunsigned int i;\t\t\t\t\t\t\t\\\n> +\tfor (i = 0; i < s->slab_count; i++) {\t\t\t\t\\\n> +\t\tunsigned int j;\t\t\t\t\t\t\\\n> +\t\tif (!s->slab[i])\t\t\t\t\t\\\n> +\t\t\tcontinue;\t\t\t\t\t\\\n> +\t\tfor (j = 0; j < s->slab_size; j++)\t\t\t\\\n> +\t\t\tfree_fn(&s->slab[i][j * s->stride]);\t\t\\\n> +\t}\t\t\t\t\t\t\t\t\\\n> +\tclear_ ##slabname(s);\t\t\t\t\t\t> +}\t\t\t\t\t\t\t\t\t\\\n\n\nWhy pass an elemtype pointer to the callback function instead of\na plain elemtype? Because it matches the return type of _at() and\n_peek(). Consistency, good. Handing it a pointer allows the\ncallback to pass it on to free(), though, which would be bad,\nsince we do that in clear_() as well. Hmm.\n\n> +\t\t\t\t\t\t\t\t\t\\\n> scope elemtype *slabname## _at_peek(struct slabname *s,\t\t\t\\\n> \t\t\t\t\t\t const struct commit *c, \\\n> \t\t\t\t\t\t int add_if_missing) \\\n> diff --git a/commit-slab.h b/commit-slab.h\n> index 05b3f2804e7..8e72a305365 100644\n> --- a/commit-slab.h\n> +++ b/commit-slab.h\n> @@ -47,6 +47,16 @@\n> *\n> * Call this function before the slab falls out of scope to avoid\n> * leaking memory.\n> + *\n> + * - void deep_clear_indegree(struct indegree *, void (*free_fn)(int*))\n> + *\n> + * Empties the slab, similar to clear_indegree(), but in addition it\n> + * calls the given 'free_fn' for each slab entry to release any\n> + * additional memory that might be owned by the entry (but not the\n> + * entry itself!).\n> + * Note that 'free_fn' might be called even for entries for which no\n> + * indegree_at() call has been made; in this case 'free_fn' is invoked\n> + * with a pointer to a zero-initialized location.\n> */\n>\n> #define define_commit_slab(slabname, elemtype) \\\n> diff --git a/shallow.c b/shallow.c\n> index 7fd04afed19..c4ac8a73273 100644\n> --- a/shallow.c\n> +++ b/shallow.c\n> @@ -84,6 +84,10 @@ int is_repository_shallow(struct repository *r)\n> * supports a \"valid\" flag.\n> */\n> define_commit_slab(commit_depth, int *);\n> +static void free_depth_in_slab(int **ptr)\n> +{\n> +\tFREE_AND_NULL(*ptr);\n> +}\n\nWhy FREE_AND_NULL? The original loop below called free(). The slabs\nare all released by deep_clear_() immediately after the callbacks are\ndone anyway, so what's the point in zeroing these pointers?\n\n> struct commit_list *get_shallow_commits(struct object_array *heads, int depth,\n> \t\tint shallow_flag, int not_shallow_flag)\n> {\n> @@ -150,15 +154,7 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,\n> \t\t\t}\n> \t\t}\n> \t}\n> -\tfor (i = 0; i < depths.slab_count; i++) {\n> -\t\tint j;\n> -\n> -\t\tif (!depths.slab[i])\n> -\t\t\tcontinue;\n> -\t\tfor (j = 0; j < depths.slab_size; j++)\n> -\t\t\tfree(depths.slab[i][j]);\n> -\t}\n> -\tclear_commit_depth(&depths);\n> +\tdeep_clear_commit_depth(&depths, free_depth_in_slab);\n\nWhat a relief!\n\n>\n> \treturn result;\n> }\n>\n\n"},{"id":"400109","messageId":"b275791b-0875-75e2-7fe4-4366b7399ee8@gmail.com","threadId":"53617","inReplyTo":"1c982d57-a446-b655-af95-1e4aeabf30ef@web.de","subject":"Re: [PATCH 04/10] commit-slab: add a function to deep free entries on the slab","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-06-19T12:52:13Z","receivedAt":"2020-06-19T12:52:20Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/18/2020 4:59 PM, René Scharfe wrote:\n> Am 05.06.20 um 15:00 schrieb SZEDER Gábor via GitGitGadget:\n>> From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= \n>>\n>> clear_##slabname() frees only the memory allocated for a commit slab\n>> itself, but entries in the commit slab might own additional memory\n>> outside the slab that should be freed as well. We already have (at\n>> least) one such commit slab, and this patch series is about to add one\n>> more.\n>>\n>> To free all additional memory owned by entries on the commit slab the\n>> user of such a slab could iterate over all commits it knows about,\n>> peek whether there is a valid entry associated with each commit, and\n>> free the additional memory, if any. Or it could rely on intimate\n>> knowledge about the internals of the commit slab implementation, and\n>> could itself iterate directly through all entries in the slab, and\n>> free the additional memory. Or it could just leak the additional\n>> memory...\n>>\n>> Introduce deep_clear_##slabname() to allow releasing memory owned by\n>> commit slab entries by invoking the 'void free_fn(elemtype *ptr)'\n>> function specified as parameter for each entry in the slab.\n> \n> Adding a new function instead of extending the existing ones makes\n> sense, as this is a rare requirement.\n> \n>>\n>> Use it in get_shallow_commits() in 'shallow.c' to replace an\n>> open-coded iteration over a commit slab's entries.\n>>\n>> Signed-off-by: SZEDER Gábor \n>> Signed-off-by: Derrick Stolee \n>> ---\n>> commit-slab-decl.h | 1 +\n>> commit-slab-impl.h | 13 +++++++++++++\n>> commit-slab.h | 10 ++++++++++\n>> shallow.c | 14 +++++---------\n>> 4 files changed, 29 insertions(+), 9 deletions(-)\n>>\n>> diff --git a/commit-slab-decl.h b/commit-slab-decl.h\n>> index adc7b46c83b..286164b7e27 100644\n>> --- a/commit-slab-decl.h\n>> +++ b/commit-slab-decl.h\n>> @@ -32,6 +32,7 @@ struct slabname {\t\t\t\t\t\t\t\\\n>> void init_ ##slabname## _with_stride(struct slabname *s, unsigned stride); \\\n>> void init_ ##slabname(struct slabname *s);\t\t\t\t\\\n>> void clear_ ##slabname(struct slabname *s);\t\t\t\t\\\n>> +void deep_clear_ ##slabname(struct slabname *s, void (*free_fn)(elemtype *ptr)); \\\n>> elemtype *slabname## _at_peek(struct slabname *s, const struct commit *c, int add_if_missing); \\\n>> elemtype *slabname## _at(struct slabname *s, const struct commit *c);\t\\\n>> elemtype *slabname## _peek(struct slabname *s, const struct commit *c)\n>> diff --git a/commit-slab-impl.h b/commit-slab-impl.h\n>> index 5c0eb91a5d1..557738df271 100644\n>> --- a/commit-slab-impl.h\n>> +++ b/commit-slab-impl.h\n>> @@ -38,6 +38,19 @@ scope void clear_ ##slabname(struct slabname *s)\t\t\t\\\n>> \tFREE_AND_NULL(s->slab);\t\t\t\t\t\t\\\n>> }\t\t\t\t\t\t\t\t\t\\\n>> \t\t\t\t\t\t\t\t\t\\\n>> +scope void deep_clear_ ##slabname(struct slabname *s, void (*free_fn)(elemtype *)) \\\n>> +{\t\t\t\t\t\t\t\t\t\\\n>> +\tunsigned int i;\t\t\t\t\t\t\t\\\n>> +\tfor (i = 0; i < s->slab_count; i++) {\t\t\t\t\\\n>> +\t\tunsigned int j;\t\t\t\t\t\t\\\n>> +\t\tif (!s->slab[i])\t\t\t\t\t\\\n>> +\t\t\tcontinue;\t\t\t\t\t\\\n>> +\t\tfor (j = 0; j < s->slab_size; j++)\t\t\t\\\n>> +\t\t\tfree_fn(&s->slab[i][j * s->stride]);\t\t\\\n>> +\t}\t\t\t\t\t\t\t\t\\\n>> +\tclear_ ##slabname(s);\t\t\t\t\t\t> +}\t\t\t\t\t\t\t\t\t\\\n> \n> \n> Why pass an elemtype pointer to the callback function instead of\n> a plain elemtype? Because it matches the return type of _at() and\n> _peek(). Consistency, good. Handing it a pointer allows the\n> callback to pass it on to free(), though, which would be bad,\n> since we do that in clear_() as well. Hmm.\n> \n>> +\t\t\t\t\t\t\t\t\t\\\n>> scope elemtype *slabname## _at_peek(struct slabname *s,\t\t\t\\\n>> \t\t\t\t\t\t const struct commit *c, \\\n>> \t\t\t\t\t\t int add_if_missing) \\\n>> diff --git a/commit-slab.h b/commit-slab.h\n>> index 05b3f2804e7..8e72a305365 100644\n>> --- a/commit-slab.h\n>> +++ b/commit-slab.h\n>> @@ -47,6 +47,16 @@\n>> *\n>> * Call this function before the slab falls out of scope to avoid\n>> * leaking memory.\n>> + *\n>> + * - void deep_clear_indegree(struct indegree *, void (*free_fn)(int*))\n>> + *\n>> + * Empties the slab, similar to clear_indegree(), but in addition it\n>> + * calls the given 'free_fn' for each slab entry to release any\n>> + * additional memory that might be owned by the entry (but not the\n>> + * entry itself!).\n>> + * Note that 'free_fn' might be called even for entries for which no\n>> + * indegree_at() call has been made; in this case 'free_fn' is invoked\n>> + * with a pointer to a zero-initialized location.\n>> */\n>>\n>> #define define_commit_slab(slabname, elemtype) \\\n>> diff --git a/shallow.c b/shallow.c\n>> index 7fd04afed19..c4ac8a73273 100644\n>> --- a/shallow.c\n>> +++ b/shallow.c\n>> @@ -84,6 +84,10 @@ int is_repository_shallow(struct repository *r)\n>> * supports a \"valid\" flag.\n>> */\n>> define_commit_slab(commit_depth, int *);\n>> +static void free_depth_in_slab(int **ptr)\n>> +{\n>> +\tFREE_AND_NULL(*ptr);\n>> +}\n> \n> Why FREE_AND_NULL? The original loop below called free(). The slabs\n> are all released by deep_clear_() immediately after the callbacks are\n> done anyway, so what's the point in zeroing these pointers?\n\nI think the point was that a later change was going to free\nelements in the slab on a one-by-one basis while computing\nthe filters, to save memory overall. To be future-proof\nagainst such a change, we need to NULL the pointers here.\n\nPerhaps that viewpoint also answers your other comment about\n\"why pass the pointer?\"\n\nThanks,\n-Stolee\n\n"},{"id":"400729","messageId":"20200627155348.GM2898@szeder.dev","threadId":"53617","inReplyTo":"32a6f11cc475f5292075093dcdc3b2a5c6d02ed0.1591362033.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 04/10] commit-slab: add a function to deep free entries on the slab","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-06-27T15:53:48Z","receivedAt":"2020-06-27T15:53:54Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jun 05, 2020 at 01:00:26PM +0000, SZEDER Gábor via GitGitGadget wrote:\n> From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= \n> \n> clear_##slabname() frees only the memory allocated for a commit slab\n> itself, but entries in the commit slab might own additional memory\n> outside the slab that should be freed as well. We already have (at\n> least) one such commit slab, and this patch series is about to add one\n> more.\n\nThis was only true in my original submission, but not anymore: this\npatch series doesn't add another such slab, and, more importantly, now\nwe have at least two such commit slabs. deep_clear_##slabnmae() could\nbe used to clear the bloom_filter_slab and all memory attached to it,\nwhich at the moment is just leaked.\n\n> To free all additional memory owned by entries on the commit slab the\n> user of such a slab could iterate over all commits it knows about,\n> peek whether there is a valid entry associated with each commit, and\n> free the additional memory, if any. Or it could rely on intimate\n> knowledge about the internals of the commit slab implementation, and\n> could itself iterate directly through all entries in the slab, and\n> free the additional memory. Or it could just leak the additional\n> memory...\n> \n> Introduce deep_clear_##slabname() to allow releasing memory owned by\n> commit slab entries by invoking the 'void free_fn(elemtype *ptr)'\n> function specified as parameter for each entry in the slab.\n> \n> Use it in get_shallow_commits() in 'shallow.c' to replace an\n> open-coded iteration over a commit slab's entries.\n> \n> Signed-off-by: SZEDER Gábor \n> Signed-off-by: Derrick Stolee \n> ---\n> commit-slab-decl.h | 1 +\n> commit-slab-impl.h | 13 +++++++++++++\n> commit-slab.h | 10 ++++++++++\n> shallow.c | 14 +++++---------\n> 4 files changed, 29 insertions(+), 9 deletions(-)\n> \n> diff --git a/commit-slab-decl.h b/commit-slab-decl.h\n> index adc7b46c83b..286164b7e27 100644\n> --- a/commit-slab-decl.h\n> +++ b/commit-slab-decl.h\n> @@ -32,6 +32,7 @@ struct slabname {\t\t\t\t\t\t\t\\\n> void init_ ##slabname## _with_stride(struct slabname *s, unsigned stride); \\\n> void init_ ##slabname(struct slabname *s);\t\t\t\t\\\n> void clear_ ##slabname(struct slabname *s);\t\t\t\t\\\n> +void deep_clear_ ##slabname(struct slabname *s, void (*free_fn)(elemtype *ptr)); \\\n> elemtype *slabname## _at_peek(struct slabname *s, const struct commit *c, int add_if_missing); \\\n> elemtype *slabname## _at(struct slabname *s, const struct commit *c);\t\\\n> elemtype *slabname## _peek(struct slabname *s, const struct commit *c)\n> diff --git a/commit-slab-impl.h b/commit-slab-impl.h\n> index 5c0eb91a5d1..557738df271 100644\n> --- a/commit-slab-impl.h\n> +++ b/commit-slab-impl.h\n> @@ -38,6 +38,19 @@ scope void clear_ ##slabname(struct slabname *s)\t\t\t\\\n> \tFREE_AND_NULL(s->slab);\t\t\t\t\t\t\\\n> }\t\t\t\t\t\t\t\t\t\\\n> \t\t\t\t\t\t\t\t\t\\\n> +scope void deep_clear_ ##slabname(struct slabname *s, void (*free_fn)(elemtype *)) \\\n> +{\t\t\t\t\t\t\t\t\t\\\n> +\tunsigned int i;\t\t\t\t\t\t\t\\\n> +\tfor (i = 0; i < s->slab_count; i++) {\t\t\t\t\\\n> +\t\tunsigned int j;\t\t\t\t\t\t\\\n> +\t\tif (!s->slab[i])\t\t\t\t\t\\\n> +\t\t\tcontinue;\t\t\t\t\t\\\n> +\t\tfor (j = 0; j < s->slab_size; j++)\t\t\t\\\n> +\t\t\tfree_fn(&s->slab[i][j * s->stride]);\t\t\\\n> +\t}\t\t\t\t\t\t\t\t\\\n> +\tclear_ ##slabname(s);\t\t\t\t\t\t\\\n> +}\t\t\t\t\t\t\t\t\t\\\n> +\t\t\t\t\t\t\t\t\t\\\n> scope elemtype *slabname## _at_peek(struct slabname *s,\t\t\t\\\n> \t\t\t\t\t\t const struct commit *c, \\\n> \t\t\t\t\t\t int add_if_missing) \\\n> diff --git a/commit-slab.h b/commit-slab.h\n> index 05b3f2804e7..8e72a305365 100644\n> --- a/commit-slab.h\n> +++ b/commit-slab.h\n> @@ -47,6 +47,16 @@\n> *\n> * Call this function before the slab falls out of scope to avoid\n> * leaking memory.\n> + *\n> + * - void deep_clear_indegree(struct indegree *, void (*free_fn)(int*))\n> + *\n> + * Empties the slab, similar to clear_indegree(), but in addition it\n> + * calls the given 'free_fn' for each slab entry to release any\n> + * additional memory that might be owned by the entry (but not the\n> + * entry itself!).\n> + * Note that 'free_fn' might be called even for entries for which no\n> + * indegree_at() call has been made; in this case 'free_fn' is invoked\n> + * with a pointer to a zero-initialized location.\n> */\n> \n> #define define_commit_slab(slabname, elemtype) \\\n> diff --git a/shallow.c b/shallow.c\n> index 7fd04afed19..c4ac8a73273 100644\n> --- a/shallow.c\n> +++ b/shallow.c\n> @@ -84,6 +84,10 @@ int is_repository_shallow(struct repository *r)\n> * supports a \"valid\" flag.\n> */\n> define_commit_slab(commit_depth, int *);\n> +static void free_depth_in_slab(int **ptr)\n> +{\n> +\tFREE_AND_NULL(*ptr);\n> +}\n> struct commit_list *get_shallow_commits(struct object_array *heads, int depth,\n> \t\tint shallow_flag, int not_shallow_flag)\n> {\n> @@ -150,15 +154,7 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,\n> \t\t\t}\n> \t\t}\n> \t}\n> -\tfor (i = 0; i < depths.slab_count; i++) {\n> -\t\tint j;\n> -\n> -\t\tif (!depths.slab[i])\n> -\t\t\tcontinue;\n> -\t\tfor (j = 0; j < depths.slab_size; j++)\n> -\t\t\tfree(depths.slab[i][j]);\n> -\t}\n> -\tclear_commit_depth(&depths);\n> +\tdeep_clear_commit_depth(&depths, free_depth_in_slab);\n> \n> \treturn result;\n> }\n> -- \n> gitgitgadget\n> \n"}]}