{"thread":{"id":"61477","subject":"[PATCH 0/6] pack-bitmap: various pack-bitmap-write cleanups","startedAt":"2024-05-14T19:56:49Z","lastAt":"2024-05-15T15:58:38Z","messageCount":14,"participants":["Taylor Blau","Jeff King","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"494738","messageId":"cover.1715716605.git.me@ttaylorr.com","threadId":"61477","inReplyTo":null,"subject":"[PATCH 0/6] pack-bitmap: various pack-bitmap-write cleanups","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-14T19:56:46Z","receivedAt":"2024-05-14T19:56:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This topic was born out of tb/pseudo-merge-bitmaps, which has a few\nquality-of-life improvements in the pack-bitmap machinery.\n\nThe main changes are to remove the static 'struct bitmap_writer', drop\none unused fields, and start using another unused one (see \"move\ncommit_positions into commit_pos fields\").\n\nThere are other smaller changes, too, like cleaning up the flag\nallocation table in object.h, as well as introducing a new\nbitmap_writer_free() function.\n\nThe next round of tb/pseudo-merge-bitmaps will be based on this branch.\n\nThanks in advance for your review!\n\nTaylor Blau (6):\n  object.h: add flags allocated by pack-bitmap.h\n  pack-bitmap-write.c: move commit_positions into commit_pos fields\n  pack-bitmap: avoid use of static `bitmap_writer`\n  pack-bitmap: drop unused `max_bitmaps` parameter\n  pack-bitmap-write.c: avoid uninitialized 'write_as' field\n  pack-bitmap: introduce `bitmap_writer_free()`\n\n builtin/pack-objects.c |  19 +++-\n midx-write.c           |  17 ++-\n object.h               |   1 +\n pack-bitmap-write.c    | 248 +++++++++++++++++++++--------------------\n pack-bitmap.h          |  38 +++++--\n 5 files changed, 185 insertions(+), 138 deletions(-)\n\n\nbase-commit: 83f1add914c6b4682de1e944ec0d1ac043d53d78\n-- \n2.45.1.151.g7cc3499008c\n"},{"id":"494739","messageId":"38c96fc1909162a4d9c188f55b7c708cfc1b14b9.1715716605.git.me@ttaylorr.com","threadId":"61477","inReplyTo":"cover.1715716605.git.me@ttaylorr.com","subject":"[PATCH 1/6] object.h: add flags allocated by pack-bitmap.h","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-14T19:56:50Z","receivedAt":"2024-05-14T19:56:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In commit 7cc8f971085 (pack-objects: implement bitmap writing,\n2013-12-21) the NEEDS_BITMAP flag was introduced into pack-bitmap.h, but\nno object flags allocation table existed at the time.\n\nIn 208acbfb82f (object.h: centralize object flag allocation, 2014-03-25)\nwhen that table was first introduced, we never added the flags from\n7cc8f971085, which has remained the case since.\n\nRectify this by including the flag bit used by pack-bitmap.h into the\ncentralized table in object.h.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n object.h | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/object.h b/object.h\nindex 9293e703cc..99b9c8f114 100644\n--- a/object.h\n+++ b/object.h\n@@ -81,6 +81,7 @@ void object_array_init(struct object_array *array);\n  * reflog.c:                           10--12\n  * builtin/show-branch.c:    0-------------------------------------------26\n  * builtin/unpack-objects.c:                                 2021\n+ * pack-bitmap.h:                                                22\n  */\n #define FLAG_BITS  28\n \n-- \n2.45.1.151.g7cc3499008c\n\n"},{"id":"494740","messageId":"cbedff02ed1409c2b5f97596fe0b43d0a4a4d0ff.1715716605.git.me@ttaylorr.com","threadId":"61477","inReplyTo":"cover.1715716605.git.me@ttaylorr.com","subject":"[PATCH 2/6] pack-bitmap-write.c: move commit_positions into commit_pos fields","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-14T19:56:53Z","receivedAt":"2024-05-14T19:56:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In 7cc8f971085 (pack-objects: implement bitmap writing, 2013-12-21), the\nbitmapped_commit struct was introduced, including the 'commit_pos'\nfield, which has been unused ever since its introduction more than a\ndecade ago.\n\nInstead, we have used the nearby `commit_positions` array leaving the\nbitmapped_commit struct with an unused 4-byte field.\n\nWe could drop the `commit_pos` field as unused, and continue to store\nthe values in the auxiliary array. But we could also drop the array and\nstore the data for each bitmapped_commit struct inside of the structure\nitself, which is what this patch does.\n\nIn any spot that we previously read `commit_positions[i]`, we can now\ninstead read `writer.selected[i].commit_pos`. There are a few spots that\nneed changing as a result:\n\n  - write_selected_commits_v1() is a simple transformation, since we're\n    just reading the field. As a result, the function no longer needs an\n    explicit argument to pass the commit_positions array.\n\n  - write_lookup_table() also no longer needs the explicit\n    commit_positions array passed in as an argument. But it still needs\n    to sort an array of indices into the writer.selected array to read\n    them in commit_pos order, so table_cmp() is adjusted accordingly.\n\n  - bitmap_writer_finish() no longer needs to allocate, populate, and\n    free the commit_positions table. Instead, we can just write the data\n    directly into each struct bitmapped_commit.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n pack-bitmap-write.c | 41 ++++++++++++++++-------------------------\n 1 file changed, 16 insertions(+), 25 deletions(-)\n\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex c6c8f94cc5..2ae82b8696 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -670,9 +670,7 @@ static const struct object_id *oid_access(size_t pos, const void *table)\n \treturn &index[pos]->oid;\n }\n \n-static void write_selected_commits_v1(struct hashfile *f,\n-\t\t\t\t      uint32_t *commit_positions,\n-\t\t\t\t      off_t *offsets)\n+static void write_selected_commits_v1(struct hashfile *f, off_t *offsets)\n {\n \tint i;\n \n@@ -682,7 +680,7 @@ static void write_selected_commits_v1(struct hashfile *f,\n \t\tif (offsets)\n \t\t\toffsets[i] = hashfile_total(f);\n \n-\t\thashwrite_be32(f, commit_positions[i]);\n+\t\thashwrite_be32(f, stored->commit_pos);\n \t\thashwrite_u8(f, stored->xor_offset);\n \t\thashwrite_u8(f, stored->flags);\n \n@@ -690,23 +688,20 @@ static void write_selected_commits_v1(struct hashfile *f,\n \t}\n }\n \n-static int table_cmp(const void *_va, const void *_vb, void *_data)\n+static int table_cmp(const void *_va, const void *_vb)\n {\n-\tuint32_t *commit_positions = _data;\n-\tuint32_t a = commit_positions[*(uint32_t *)_va];\n-\tuint32_t b = commit_positions[*(uint32_t *)_vb];\n+\tstruct bitmapped_commit *a = &writer.selected[*(uint32_t *)_va];\n+\tstruct bitmapped_commit *b = &writer.selected[*(uint32_t *)_vb];\n \n-\tif (a > b)\n+\tif (a->commit_pos < b->commit_pos)\n+\t\treturn -1;\n+\telse if (a->commit_pos > b->commit_pos)\n \t\treturn 1;\n-\telse if (a < b)\n-\t\treturn -1;\n \n \treturn 0;\n }\n \n-static void write_lookup_table(struct hashfile *f,\n-\t\t\t       uint32_t *commit_positions,\n-\t\t\t       off_t *offsets)\n+static void write_lookup_table(struct hashfile *f, off_t *offsets)\n {\n \tuint32_t i;\n \tuint32_t *table, *table_inv;\n@@ -722,7 +717,7 @@ static void write_lookup_table(struct hashfile *f,\n \t * bitmap corresponds to j'th bitmapped commit (among the selected\n \t * commits) in lex order of OIDs.\n \t */\n-\tQSORT_S(table, writer.selected_nr, table_cmp, commit_positions);\n+\tQSORT(table, writer.selected_nr, table_cmp);\n \n \t/* table_inv helps us discover that relationship (i'th bitmap\n \t * to j'th commit by j = table_inv[i])\n@@ -753,7 +748,7 @@ static void write_lookup_table(struct hashfile *f,\n \t\t\txor_row = 0xffffffff;\n \t\t}\n \n-\t\thashwrite_be32(f, commit_positions[table[i]]);\n+\t\thashwrite_be32(f, writer.selected[table[i]].commit_pos);\n \t\thashwrite_be64(f, (uint64_t)offsets[table[i]]);\n \t\thashwrite_be32(f, xor_row);\n \t}\n@@ -789,7 +784,6 @@ void bitmap_writer_finish(struct pack_idx_entry **index,\n \tstatic uint16_t flags = BITMAP_OPT_FULL_DAG;\n \tstruct strbuf tmp_file = STRBUF_INIT;\n \tstruct hashfile *f;\n-\tuint32_t *commit_positions = NULL;\n \toff_t *offsets = NULL;\n \tuint32_t i;\n \n@@ -814,22 +808,20 @@ void bitmap_writer_finish(struct pack_idx_entry **index,\n \tif (options & BITMAP_OPT_LOOKUP_TABLE)\n \t\tCALLOC_ARRAY(offsets, index_nr);\n \n-\tALLOC_ARRAY(commit_positions, writer.selected_nr);\n-\n \tfor (i = 0; i < writer.selected_nr; i++) {\n \t\tstruct bitmapped_commit *stored = &writer.selected[i];\n-\t\tint commit_pos = oid_pos(&stored->commit->object.oid, index, index_nr, oid_access);\n+\t\tint commit_pos = oid_pos(&stored->commit->object.oid, index,\n+\t\t\t\t\t index_nr, oid_access);\n \n \t\tif (commit_pos < 0)\n \t\t\tBUG(_(\"trying to write commit not in index\"));\n-\n-\t\tcommit_positions[i] = commit_pos;\n+\t\tstored->commit_pos = commit_pos;\n \t}\n \n-\twrite_selected_commits_v1(f, commit_positions, offsets);\n+\twrite_selected_commits_v1(f, offsets);\n \n \tif (options & BITMAP_OPT_LOOKUP_TABLE)\n-\t\twrite_lookup_table(f, commit_positions, offsets);\n+\t\twrite_lookup_table(f, offsets);\n \n \tif (options & BITMAP_OPT_HASH_CACHE)\n \t\twrite_hash_cache(f, index, index_nr);\n@@ -844,6 +836,5 @@ void bitmap_writer_finish(struct pack_idx_entry **index,\n \t\tdie_errno(\"unable to rename temporary bitmap file to '%s'\", filename);\n \n \tstrbuf_release(&tmp_file);\n-\tfree(commit_positions);\n \tfree(offsets);\n }\n-- \n2.45.1.151.g7cc3499008c\n\n"},{"id":"494741","messageId":"65ee69acfeb9c66404aa85f0c4b09891d3b8fc27.1715716605.git.me@ttaylorr.com","threadId":"61477","inReplyTo":"cover.1715716605.git.me@ttaylorr.com","subject":"[PATCH 3/6] pack-bitmap: avoid use of static `bitmap_writer`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-14T19:56:56Z","receivedAt":"2024-05-14T19:56:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The pack-bitmap machinery uses a structure called 'bitmap_writer' to\ncollect the data necessary to write out .bitmap files. Since its\nintroduction in 7cc8f971085 (pack-objects: implement bitmap writing,\n2013-12-21), there has been a single static bitmap_writer structure,\nwhich is responsible for all bitmap writing-related operations.\n\nIn practice, this is OK, since we are only ever writing a single .bitmap\nfile in a single process (e.g., `git multi-pack-index write --bitmap`,\n`git pack-objects --write-bitmap-index`, `git repack -b`, etc.).\n\nHowever, having a single static variable makes issues like data\nownership unclear, when to free variables, what has/hasn't been\ninitialized unclear.\n\nRefactor this code to be written in terms of a given bitmap_writer\nstructure instead of relying on a static global.\n\nNote that this exposes the structure definition of the bitmap_writer at\nthe pack-bitmap.h level. We could work around this by, e.g., forcing\ncallers to declare their writers as:\n\n    struct bitmap_writer *writer;\n    bitmap_writer_init(&bitmap_writer);\n\nand then declaring `bitmap_writer_init()` as taking in a double-pointer\nlike so:\n\n    void bitmap_writer_init(struct bitmap_writer **writer);\n\nwhich would avoid us having to expose the definition of the structure\nitself. This patch takes a different approach, since future patches\n(like for the ongoing pseudo-merge bitmaps work) will want to modify the\ninnards of this structure (in the previous example, via pseudo-merge.c).\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/pack-objects.c |  19 ++--\n midx-write.c           |  16 ++--\n pack-bitmap-write.c    | 209 +++++++++++++++++++++--------------------\n pack-bitmap.h          |  38 ++++++--\n 4 files changed, 159 insertions(+), 123 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex baf0090fc8..ba4c93d241 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1245,6 +1245,7 @@ static void write_pack_file(void)\n \tuint32_t nr_remaining = nr_result;\n \ttime_t last_mtime = 0;\n \tstruct object_entry **write_order;\n+\tstruct bitmap_writer bitmap_writer;\n \n \tif (progress > pack_to_stdout)\n \t\tprogress_state = start_progress(_(\"Writing objects\"), nr_result);\n@@ -1339,8 +1340,9 @@ static void write_pack_file(void)\n \t\t\t\t    hash_to_hex(hash));\n \n \t\t\tif (write_bitmap_index) {\n-\t\t\t\tbitmap_writer_set_checksum(hash);\n-\t\t\t\tbitmap_writer_build_type_index(\n+\t\t\t\tbitmap_writer_init(&bitmap_writer);\n+\t\t\t\tbitmap_writer_set_checksum(&bitmap_writer, hash);\n+\t\t\t\tbitmap_writer_build_type_index(&bitmap_writer,\n \t\t\t\t\t&to_pack, written_list, nr_written);\n \t\t\t}\n \n@@ -1358,11 +1360,16 @@ static void write_pack_file(void)\n \t\t\t\tstrbuf_addstr(&tmpname, \"bitmap\");\n \t\t\t\tstop_progress(&progress_state);\n \n-\t\t\t\tbitmap_writer_show_progress(progress);\n-\t\t\t\tbitmap_writer_select_commits(indexed_commits, indexed_commits_nr, -1);\n-\t\t\t\tif (bitmap_writer_build(&to_pack) < 0)\n+\t\t\t\tbitmap_writer_show_progress(&bitmap_writer,\n+\t\t\t\t\t\t\t    progress);\n+\t\t\t\tbitmap_writer_select_commits(&bitmap_writer,\n+\t\t\t\t\t\t\t     indexed_commits,\n+\t\t\t\t\t\t\t     indexed_commits_nr,\n+\t\t\t\t\t\t\t     -1);\n+\t\t\t\tif (bitmap_writer_build(&bitmap_writer, &to_pack) < 0)\n \t\t\t\t\tdie(_(\"failed to write bitmap index\"));\n-\t\t\t\tbitmap_writer_finish(written_list, nr_written,\n+\t\t\t\tbitmap_writer_finish(&bitmap_writer,\n+\t\t\t\t\t\t     written_list, nr_written,\n \t\t\t\t\t\t     tmpname.buf, write_bitmap_options);\n \t\t\t\twrite_bitmap_index = 0;\n \t\t\t\tstrbuf_setlen(&tmpname, tmpname_len);\ndiff --git a/midx-write.c b/midx-write.c\nindex 65e69d2de7..5fdc8f2ff5 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -798,6 +798,7 @@ static int write_midx_bitmap(const char *midx_name,\n {\n \tint ret, i;\n \tuint16_t options = 0;\n+\tstruct bitmap_writer writer;\n \tstruct pack_idx_entry **index;\n \tchar *bitmap_name = xstrfmt(\"%s-%s.bitmap\", midx_name,\n \t\t\t\t\thash_to_hex(midx_hash));\n@@ -819,8 +820,10 @@ static int write_midx_bitmap(const char *midx_name,\n \tfor (i = 0; i < pdata->nr_objects; i++)\n \t\tindex[i] = &pdata->objects[i].idx;\n \n-\tbitmap_writer_show_progress(flags & MIDX_PROGRESS);\n-\tbitmap_writer_build_type_index(pdata, index, pdata->nr_objects);\n+\tbitmap_writer_init(&writer);\n+\tbitmap_writer_show_progress(&writer, flags & MIDX_PROGRESS);\n+\tbitmap_writer_build_type_index(&writer, pdata, index,\n+\t\t\t\t       pdata->nr_objects);\n \n \t/*\n \t * bitmap_writer_finish expects objects in lex order, but pack_order\n@@ -838,13 +841,14 @@ static int write_midx_bitmap(const char *midx_name,\n \tfor (i = 0; i < pdata->nr_objects; i++)\n \t\tindex[pack_order[i]] = &pdata->objects[i].idx;\n \n-\tbitmap_writer_select_commits(commits, commits_nr, -1);\n-\tret = bitmap_writer_build(pdata);\n+\tbitmap_writer_select_commits(&writer, commits, commits_nr, -1);\n+\tret = bitmap_writer_build(&writer, pdata);\n \tif (ret < 0)\n \t\tgoto cleanup;\n \n-\tbitmap_writer_set_checksum(midx_hash);\n-\tbitmap_writer_finish(index, pdata->nr_objects, bitmap_name, options);\n+\tbitmap_writer_set_checksum(&writer, midx_hash);\n+\tbitmap_writer_finish(&writer, index, pdata->nr_objects, bitmap_name,\n+\t\t\t     options);\n \n cleanup:\n \tfree(index);\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex 2ae82b8696..e22fa70745 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -27,43 +27,30 @@ struct bitmapped_commit {\n \tuint32_t commit_pos;\n };\n \n-struct bitmap_writer {\n-\tstruct ewah_bitmap *commits;\n-\tstruct ewah_bitmap *trees;\n-\tstruct ewah_bitmap *blobs;\n-\tstruct ewah_bitmap *tags;\n-\n-\tkh_oid_map_t *bitmaps;\n-\tstruct packing_data *to_pack;\n-\n-\tstruct bitmapped_commit *selected;\n-\tunsigned int selected_nr, selected_alloc;\n-\n-\tstruct progress *progress;\n-\tint show_progress;\n-\tunsigned char pack_checksum[GIT_MAX_RAWSZ];\n-};\n-\n-static struct bitmap_writer writer;\n+void bitmap_writer_init(struct bitmap_writer *writer)\n+{\n+\tmemset(writer, 0, sizeof(struct bitmap_writer));\n+}\n \n-void bitmap_writer_show_progress(int show)\n+void bitmap_writer_show_progress(struct bitmap_writer *writer, int show)\n {\n-\twriter.show_progress = show;\n+\twriter->show_progress = show;\n }\n \n /**\n  * Build the initial type index for the packfile or multi-pack-index\n  */\n-void bitmap_writer_build_type_index(struct packing_data *to_pack,\n+void bitmap_writer_build_type_index(struct bitmap_writer *writer,\n+\t\t\t\t    struct packing_data *to_pack,\n \t\t\t\t    struct pack_idx_entry **index,\n \t\t\t\t    uint32_t index_nr)\n {\n \tuint32_t i;\n \n-\twriter.commits = ewah_new();\n-\twriter.trees = ewah_new();\n-\twriter.blobs = ewah_new();\n-\twriter.tags = ewah_new();\n+\twriter->commits = ewah_new();\n+\twriter->trees = ewah_new();\n+\twriter->blobs = ewah_new();\n+\twriter->tags = ewah_new();\n \tALLOC_ARRAY(to_pack->in_pack_pos, to_pack->nr_objects);\n \n \tfor (i = 0; i < index_nr; ++i) {\n@@ -88,19 +75,19 @@ void bitmap_writer_build_type_index(struct packing_data *to_pack,\n \n \t\tswitch (real_type) {\n \t\tcase OBJ_COMMIT:\n-\t\t\tewah_set(writer.commits, i);\n+\t\t\tewah_set(writer->commits, i);\n \t\t\tbreak;\n \n \t\tcase OBJ_TREE:\n-\t\t\tewah_set(writer.trees, i);\n+\t\t\tewah_set(writer->trees, i);\n \t\t\tbreak;\n \n \t\tcase OBJ_BLOB:\n-\t\t\tewah_set(writer.blobs, i);\n+\t\t\tewah_set(writer->blobs, i);\n \t\t\tbreak;\n \n \t\tcase OBJ_TAG:\n-\t\t\tewah_set(writer.tags, i);\n+\t\t\tewah_set(writer->tags, i);\n \t\t\tbreak;\n \n \t\tdefault:\n@@ -115,23 +102,25 @@ void bitmap_writer_build_type_index(struct packing_data *to_pack,\n  * Compute the actual bitmaps\n  */\n \n-static inline void push_bitmapped_commit(struct commit *commit)\n+static inline void push_bitmapped_commit(struct bitmap_writer *writer,\n+\t\t\t\t\t struct commit *commit)\n {\n-\tif (writer.selected_nr >= writer.selected_alloc) {\n-\t\twriter.selected_alloc = (writer.selected_alloc + 32) * 2;\n-\t\tREALLOC_ARRAY(writer.selected, writer.selected_alloc);\n+\tif (writer->selected_nr >= writer->selected_alloc) {\n+\t\twriter->selected_alloc = (writer->selected_alloc + 32) * 2;\n+\t\tREALLOC_ARRAY(writer->selected, writer->selected_alloc);\n \t}\n \n-\twriter.selected[writer.selected_nr].commit = commit;\n-\twriter.selected[writer.selected_nr].bitmap = NULL;\n-\twriter.selected[writer.selected_nr].flags = 0;\n+\twriter->selected[writer->selected_nr].commit = commit;\n+\twriter->selected[writer->selected_nr].bitmap = NULL;\n+\twriter->selected[writer->selected_nr].flags = 0;\n \n-\twriter.selected_nr++;\n+\twriter->selected_nr++;\n }\n \n-static uint32_t find_object_pos(const struct object_id *oid, int *found)\n+static uint32_t find_object_pos(struct bitmap_writer *writer,\n+\t\t\t\tconst struct object_id *oid, int *found)\n {\n-\tstruct object_entry *entry = packlist_find(writer.to_pack, oid);\n+\tstruct object_entry *entry = packlist_find(writer->to_pack, oid);\n \n \tif (!entry) {\n \t\tif (found)\n@@ -143,17 +132,17 @@ static uint32_t find_object_pos(const struct object_id *oid, int *found)\n \n \tif (found)\n \t\t*found = 1;\n-\treturn oe_in_pack_pos(writer.to_pack, entry);\n+\treturn oe_in_pack_pos(writer->to_pack, entry);\n }\n \n-static void compute_xor_offsets(void)\n+static void compute_xor_offsets(struct bitmap_writer *writer)\n {\n \tstatic const int MAX_XOR_OFFSET_SEARCH = 10;\n \n \tint i, next = 0;\n \n-\twhile (next < writer.selected_nr) {\n-\t\tstruct bitmapped_commit *stored = &writer.selected[next];\n+\twhile (next < writer->selected_nr) {\n+\t\tstruct bitmapped_commit *stored = &writer->selected[next];\n \n \t\tint best_offset = 0;\n \t\tstruct ewah_bitmap *best_bitmap = stored->bitmap;\n@@ -166,7 +155,7 @@ static void compute_xor_offsets(void)\n \t\t\t\tbreak;\n \n \t\t\ttest_xor = ewah_pool_new();\n-\t\t\tewah_xor(writer.selected[curr].bitmap, stored->bitmap, test_xor);\n+\t\t\tewah_xor(writer->selected[curr].bitmap, stored->bitmap, test_xor);\n \n \t\t\tif (test_xor->buffer_size < best_bitmap->buffer_size) {\n \t\t\t\tif (best_bitmap != stored->bitmap)\n@@ -348,7 +337,8 @@ static void bitmap_builder_clear(struct bitmap_builder *bb)\n \tbb->commits_nr = bb->commits_alloc = 0;\n }\n \n-static int fill_bitmap_tree(struct bitmap *bitmap,\n+static int fill_bitmap_tree(struct bitmap_writer *writer,\n+\t\t\t    struct bitmap *bitmap,\n \t\t\t    struct tree *tree)\n {\n \tint found;\n@@ -360,7 +350,7 @@ static int fill_bitmap_tree(struct bitmap *bitmap,\n \t * If our bit is already set, then there is nothing to do. Both this\n \t * tree and all of its children will be set.\n \t */\n-\tpos = find_object_pos(&tree->object.oid, &found);\n+\tpos = find_object_pos(writer, &tree->object.oid, &found);\n \tif (!found)\n \t\treturn -1;\n \tif (bitmap_get(bitmap, pos))\n@@ -375,12 +365,12 @@ static int fill_bitmap_tree(struct bitmap *bitmap,\n \twhile (tree_entry(&desc, &entry)) {\n \t\tswitch (object_type(entry.mode)) {\n \t\tcase OBJ_TREE:\n-\t\t\tif (fill_bitmap_tree(bitmap,\n+\t\t\tif (fill_bitmap_tree(writer, bitmap,\n \t\t\t\t\t     lookup_tree(the_repository, &entry.oid)) < 0)\n \t\t\t\treturn -1;\n \t\t\tbreak;\n \t\tcase OBJ_BLOB:\n-\t\t\tpos = find_object_pos(&entry.oid, &found);\n+\t\t\tpos = find_object_pos(writer, &entry.oid, &found);\n \t\t\tif (!found)\n \t\t\t\treturn -1;\n \t\t\tbitmap_set(bitmap, pos);\n@@ -397,7 +387,8 @@ static int fill_bitmap_tree(struct bitmap *bitmap,\n \n static int reused_bitmaps_nr;\n \n-static int fill_bitmap_commit(struct bb_commit *ent,\n+static int fill_bitmap_commit(struct bitmap_writer *writer,\n+\t\t\t      struct bb_commit *ent,\n \t\t\t      struct commit *commit,\n \t\t\t      struct prio_queue *queue,\n \t\t\t      struct prio_queue *tree_queue,\n@@ -436,7 +427,7 @@ static int fill_bitmap_commit(struct bb_commit *ent,\n \t\t * Mark ourselves and queue our tree. The commit\n \t\t * walk ensures we cover all parents.\n \t\t */\n-\t\tpos = find_object_pos(&c->object.oid, &found);\n+\t\tpos = find_object_pos(writer, &c->object.oid, &found);\n \t\tif (!found)\n \t\t\treturn -1;\n \t\tbitmap_set(ent->bitmap, pos);\n@@ -444,7 +435,8 @@ static int fill_bitmap_commit(struct bb_commit *ent,\n \t\t\t       repo_get_commit_tree(the_repository, c));\n \n \t\tfor (p = c->parents; p; p = p->next) {\n-\t\t\tpos = find_object_pos(&p->item->object.oid, &found);\n+\t\t\tpos = find_object_pos(writer, &p->item->object.oid,\n+\t\t\t\t\t      &found);\n \t\t\tif (!found)\n \t\t\t\treturn -1;\n \t\t\tif (!bitmap_get(ent->bitmap, pos)) {\n@@ -455,29 +447,31 @@ static int fill_bitmap_commit(struct bb_commit *ent,\n \t}\n \n \twhile (tree_queue->nr) {\n-\t\tif (fill_bitmap_tree(ent->bitmap,\n+\t\tif (fill_bitmap_tree(writer, ent->bitmap,\n \t\t\t\t     prio_queue_get(tree_queue)) < 0)\n \t\t\treturn -1;\n \t}\n \treturn 0;\n }\n \n-static void store_selected(struct bb_commit *ent, struct commit *commit)\n+static void store_selected(struct bitmap_writer *writer,\n+\t\t\t   struct bb_commit *ent, struct commit *commit)\n {\n-\tstruct bitmapped_commit *stored = &writer.selected[ent->idx];\n+\tstruct bitmapped_commit *stored = &writer->selected[ent->idx];\n \tkhiter_t hash_pos;\n \tint hash_ret;\n \n \tstored->bitmap = bitmap_to_ewah(ent->bitmap);\n \n-\thash_pos = kh_put_oid_map(writer.bitmaps, commit->object.oid, &hash_ret);\n+\thash_pos = kh_put_oid_map(writer->bitmaps, commit->object.oid, &hash_ret);\n \tif (hash_ret == 0)\n \t\tdie(\"Duplicate entry when writing index: %s\",\n \t\t    oid_to_hex(&commit->object.oid));\n-\tkh_value(writer.bitmaps, hash_pos) = stored;\n+\tkh_value(writer->bitmaps, hash_pos) = stored;\n }\n \n-int bitmap_writer_build(struct packing_data *to_pack)\n+int bitmap_writer_build(struct bitmap_writer *writer,\n+\t\t\tstruct packing_data *to_pack)\n {\n \tstruct bitmap_builder bb;\n \tsize_t i;\n@@ -488,11 +482,12 @@ int bitmap_writer_build(struct packing_data *to_pack)\n \tuint32_t *mapping;\n \tint closed = 1; /* until proven otherwise */\n \n-\twriter.bitmaps = kh_init_oid_map();\n-\twriter.to_pack = to_pack;\n+\twriter->bitmaps = kh_init_oid_map();\n+\twriter->to_pack = to_pack;\n \n-\tif (writer.show_progress)\n-\t\twriter.progress = start_progress(\"Building bitmaps\", writer.selected_nr);\n+\tif (writer->show_progress)\n+\t\twriter->progress = start_progress(\"Building bitmaps\",\n+\t\t\t\t\t\t  writer->selected_nr);\n \ttrace2_region_enter(\"pack-bitmap-write\", \"building_bitmaps_total\",\n \t\t\t    the_repository);\n \n@@ -502,23 +497,23 @@ int bitmap_writer_build(struct packing_data *to_pack)\n \telse\n \t\tmapping = NULL;\n \n-\tbitmap_builder_init(&bb, &writer, old_bitmap);\n+\tbitmap_builder_init(&bb, writer, old_bitmap);\n \tfor (i = bb.commits_nr; i > 0; i--) {\n \t\tstruct commit *commit = bb.commits[i-1];\n \t\tstruct bb_commit *ent = bb_data_at(&bb.data, commit);\n \t\tstruct commit *child;\n \t\tint reused = 0;\n \n-\t\tif (fill_bitmap_commit(ent, commit, &queue, &tree_queue,\n+\t\tif (fill_bitmap_commit(writer, ent, commit, &queue, &tree_queue,\n \t\t\t\t       old_bitmap, mapping) < 0) {\n \t\t\tclosed = 0;\n \t\t\tbreak;\n \t\t}\n \n \t\tif (ent->selected) {\n-\t\t\tstore_selected(ent, commit);\n+\t\t\tstore_selected(writer, ent, commit);\n \t\t\tnr_stored++;\n-\t\t\tdisplay_progress(writer.progress, nr_stored);\n+\t\t\tdisplay_progress(writer->progress, nr_stored);\n \t\t}\n \n \t\twhile ((child = pop_commit(&ent->reverse_edges))) {\n@@ -549,10 +544,10 @@ int bitmap_writer_build(struct packing_data *to_pack)\n \ttrace2_data_intmax(\"pack-bitmap-write\", the_repository,\n \t\t\t   \"building_bitmaps_reused\", reused_bitmaps_nr);\n \n-\tstop_progress(&writer.progress);\n+\tstop_progress(&writer->progress);\n \n \tif (closed)\n-\t\tcompute_xor_offsets();\n+\t\tcompute_xor_offsets(writer);\n \treturn closed ? 0 : -1;\n }\n \n@@ -590,7 +585,8 @@ static int date_compare(const void *_a, const void *_b)\n \treturn (long)b->date - (long)a->date;\n }\n \n-void bitmap_writer_select_commits(struct commit **indexed_commits,\n+void bitmap_writer_select_commits(struct bitmap_writer *writer,\n+\t\t\t\t  struct commit **indexed_commits,\n \t\t\t\t  unsigned int indexed_commits_nr,\n \t\t\t\t  int max_bitmaps)\n {\n@@ -600,12 +596,12 @@ void bitmap_writer_select_commits(struct commit **indexed_commits,\n \n \tif (indexed_commits_nr < 100) {\n \t\tfor (i = 0; i < indexed_commits_nr; ++i)\n-\t\t\tpush_bitmapped_commit(indexed_commits[i]);\n+\t\t\tpush_bitmapped_commit(writer, indexed_commits[i]);\n \t\treturn;\n \t}\n \n-\tif (writer.show_progress)\n-\t\twriter.progress = start_progress(\"Selecting bitmap commits\", 0);\n+\tif (writer->show_progress)\n+\t\twriter->progress = start_progress(\"Selecting bitmap commits\", 0);\n \n \tfor (;;) {\n \t\tstruct commit *chosen = NULL;\n@@ -615,8 +611,8 @@ void bitmap_writer_select_commits(struct commit **indexed_commits,\n \t\tif (i + next >= indexed_commits_nr)\n \t\t\tbreak;\n \n-\t\tif (max_bitmaps > 0 && writer.selected_nr >= max_bitmaps) {\n-\t\t\twriter.selected_nr = max_bitmaps;\n+\t\tif (max_bitmaps > 0 && writer->selected_nr >= max_bitmaps) {\n+\t\t\twriter->selected_nr = max_bitmaps;\n \t\t\tbreak;\n \t\t}\n \n@@ -638,13 +634,13 @@ void bitmap_writer_select_commits(struct commit **indexed_commits,\n \t\t\t}\n \t\t}\n \n-\t\tpush_bitmapped_commit(chosen);\n+\t\tpush_bitmapped_commit(writer, chosen);\n \n \t\ti += next + 1;\n-\t\tdisplay_progress(writer.progress, i);\n+\t\tdisplay_progress(writer->progress, i);\n \t}\n \n-\tstop_progress(&writer.progress);\n+\tstop_progress(&writer->progress);\n }\n \n \n@@ -670,12 +666,13 @@ static const struct object_id *oid_access(size_t pos, const void *table)\n \treturn &index[pos]->oid;\n }\n \n-static void write_selected_commits_v1(struct hashfile *f, off_t *offsets)\n+static void write_selected_commits_v1(struct bitmap_writer *writer,\n+\t\t\t\t      struct hashfile *f, off_t *offsets)\n {\n \tint i;\n \n-\tfor (i = 0; i < writer.selected_nr; ++i) {\n-\t\tstruct bitmapped_commit *stored = &writer.selected[i];\n+\tfor (i = 0; i < writer->selected_nr; ++i) {\n+\t\tstruct bitmapped_commit *stored = &writer->selected[i];\n \n \t\tif (offsets)\n \t\t\toffsets[i] = hashfile_total(f);\n@@ -688,10 +685,11 @@ static void write_selected_commits_v1(struct hashfile *f, off_t *offsets)\n \t}\n }\n \n-static int table_cmp(const void *_va, const void *_vb)\n+static int table_cmp(const void *_va, const void *_vb, void *_data)\n {\n-\tstruct bitmapped_commit *a = &writer.selected[*(uint32_t *)_va];\n-\tstruct bitmapped_commit *b = &writer.selected[*(uint32_t *)_vb];\n+\tstruct bitmap_writer *writer = _data;\n+\tstruct bitmapped_commit *a = &writer->selected[*(uint32_t *)_va];\n+\tstruct bitmapped_commit *b = &writer->selected[*(uint32_t *)_vb];\n \n \tif (a->commit_pos < b->commit_pos)\n \t\treturn -1;\n@@ -701,15 +699,16 @@ static int table_cmp(const void *_va, const void *_vb)\n \treturn 0;\n }\n \n-static void write_lookup_table(struct hashfile *f, off_t *offsets)\n+static void write_lookup_table(struct bitmap_writer *writer, struct hashfile *f,\n+\t\t\t       off_t *offsets)\n {\n \tuint32_t i;\n \tuint32_t *table, *table_inv;\n \n-\tALLOC_ARRAY(table, writer.selected_nr);\n-\tALLOC_ARRAY(table_inv, writer.selected_nr);\n+\tALLOC_ARRAY(table, writer->selected_nr);\n+\tALLOC_ARRAY(table_inv, writer->selected_nr);\n \n-\tfor (i = 0; i < writer.selected_nr; i++)\n+\tfor (i = 0; i < writer->selected_nr; i++)\n \t\ttable[i] = i;\n \n \t/*\n@@ -717,17 +716,17 @@ static void write_lookup_table(struct hashfile *f, off_t *offsets)\n \t * bitmap corresponds to j'th bitmapped commit (among the selected\n \t * commits) in lex order of OIDs.\n \t */\n-\tQSORT(table, writer.selected_nr, table_cmp);\n+\tQSORT_S(table, writer->selected_nr, table_cmp, writer);\n \n \t/* table_inv helps us discover that relationship (i'th bitmap\n \t * to j'th commit by j = table_inv[i])\n \t */\n-\tfor (i = 0; i < writer.selected_nr; i++)\n+\tfor (i = 0; i < writer->selected_nr; i++)\n \t\ttable_inv[table[i]] = i;\n \n \ttrace2_region_enter(\"pack-bitmap-write\", \"writing_lookup_table\", the_repository);\n-\tfor (i = 0; i < writer.selected_nr; i++) {\n-\t\tstruct bitmapped_commit *selected = &writer.selected[table[i]];\n+\tfor (i = 0; i < writer->selected_nr; i++) {\n+\t\tstruct bitmapped_commit *selected = &writer->selected[table[i]];\n \t\tuint32_t xor_offset = selected->xor_offset;\n \t\tuint32_t xor_row;\n \n@@ -748,7 +747,7 @@ static void write_lookup_table(struct hashfile *f, off_t *offsets)\n \t\t\txor_row = 0xffffffff;\n \t\t}\n \n-\t\thashwrite_be32(f, writer.selected[table[i]].commit_pos);\n+\t\thashwrite_be32(f, writer->selected[table[i]].commit_pos);\n \t\thashwrite_be64(f, (uint64_t)offsets[table[i]]);\n \t\thashwrite_be32(f, xor_row);\n \t}\n@@ -770,12 +769,14 @@ static void write_hash_cache(struct hashfile *f,\n \t}\n }\n \n-void bitmap_writer_set_checksum(const unsigned char *sha1)\n+void bitmap_writer_set_checksum(struct bitmap_writer *writer,\n+\t\t\t\tconst unsigned char *sha1)\n {\n-\thashcpy(writer.pack_checksum, sha1);\n+\thashcpy(writer->pack_checksum, sha1);\n }\n \n-void bitmap_writer_finish(struct pack_idx_entry **index,\n+void bitmap_writer_finish(struct bitmap_writer *writer,\n+\t\t\t  struct pack_idx_entry **index,\n \t\t\t  uint32_t index_nr,\n \t\t\t  const char *filename,\n \t\t\t  uint16_t options)\n@@ -796,20 +797,20 @@ void bitmap_writer_finish(struct pack_idx_entry **index,\n \tmemcpy(header.magic, BITMAP_IDX_SIGNATURE, sizeof(BITMAP_IDX_SIGNATURE));\n \theader.version = htons(default_version);\n \theader.options = htons(flags | options);\n-\theader.entry_count = htonl(writer.selected_nr);\n-\thashcpy(header.checksum, writer.pack_checksum);\n+\theader.entry_count = htonl(writer->selected_nr);\n+\thashcpy(header.checksum, writer->pack_checksum);\n \n \thashwrite(f, &header, sizeof(header) - GIT_MAX_RAWSZ + the_hash_algo->rawsz);\n-\tdump_bitmap(f, writer.commits);\n-\tdump_bitmap(f, writer.trees);\n-\tdump_bitmap(f, writer.blobs);\n-\tdump_bitmap(f, writer.tags);\n+\tdump_bitmap(f, writer->commits);\n+\tdump_bitmap(f, writer->trees);\n+\tdump_bitmap(f, writer->blobs);\n+\tdump_bitmap(f, writer->tags);\n \n \tif (options & BITMAP_OPT_LOOKUP_TABLE)\n \t\tCALLOC_ARRAY(offsets, index_nr);\n \n-\tfor (i = 0; i < writer.selected_nr; i++) {\n-\t\tstruct bitmapped_commit *stored = &writer.selected[i];\n+\tfor (i = 0; i < writer->selected_nr; i++) {\n+\t\tstruct bitmapped_commit *stored = &writer->selected[i];\n \t\tint commit_pos = oid_pos(&stored->commit->object.oid, index,\n \t\t\t\t\t index_nr, oid_access);\n \n@@ -818,10 +819,10 @@ void bitmap_writer_finish(struct pack_idx_entry **index,\n \t\tstored->commit_pos = commit_pos;\n \t}\n \n-\twrite_selected_commits_v1(f, offsets);\n+\twrite_selected_commits_v1(writer, f, offsets);\n \n \tif (options & BITMAP_OPT_LOOKUP_TABLE)\n-\t\twrite_lookup_table(f, offsets);\n+\t\twrite_lookup_table(writer, f, offsets);\n \n \tif (options & BITMAP_OPT_HASH_CACHE)\n \t\twrite_hash_cache(f, index, index_nr);\ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex c7dea13217..5a1890a2c5 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -97,9 +97,29 @@ int bitmap_has_oid_in_uninteresting(struct bitmap_index *, const struct object_i\n \n off_t get_disk_usage_from_bitmap(struct bitmap_index *, struct rev_info *);\n \n-void bitmap_writer_show_progress(int show);\n-void bitmap_writer_set_checksum(const unsigned char *sha1);\n-void bitmap_writer_build_type_index(struct packing_data *to_pack,\n+struct bitmap_writer {\n+\tstruct ewah_bitmap *commits;\n+\tstruct ewah_bitmap *trees;\n+\tstruct ewah_bitmap *blobs;\n+\tstruct ewah_bitmap *tags;\n+\n+\tkh_oid_map_t *bitmaps;\n+\tstruct packing_data *to_pack;\n+\n+\tstruct bitmapped_commit *selected;\n+\tunsigned int selected_nr, selected_alloc;\n+\n+\tstruct progress *progress;\n+\tint show_progress;\n+\tunsigned char pack_checksum[GIT_MAX_RAWSZ];\n+};\n+\n+void bitmap_writer_init(struct bitmap_writer *writer);\n+void bitmap_writer_show_progress(struct bitmap_writer *writer, int show);\n+void bitmap_writer_set_checksum(struct bitmap_writer *writer,\n+\t\t\t\tconst unsigned char *sha1);\n+void bitmap_writer_build_type_index(struct bitmap_writer *writer,\n+\t\t\t\t    struct packing_data *to_pack,\n \t\t\t\t    struct pack_idx_entry **index,\n \t\t\t\t    uint32_t index_nr);\n uint32_t *create_bitmap_mapping(struct bitmap_index *bitmap_git,\n@@ -109,10 +129,14 @@ int rebuild_bitmap(const uint32_t *reposition,\n \t\t   struct bitmap *dest);\n struct ewah_bitmap *bitmap_for_commit(struct bitmap_index *bitmap_git,\n \t\t\t\t      struct commit *commit);\n-void bitmap_writer_select_commits(struct commit **indexed_commits,\n-\t\tunsigned int indexed_commits_nr, int max_bitmaps);\n-int bitmap_writer_build(struct packing_data *to_pack);\n-void bitmap_writer_finish(struct pack_idx_entry **index,\n+void bitmap_writer_select_commits(struct bitmap_writer *writer,\n+\t\t\t\t  struct commit **indexed_commits,\n+\t\t\t\t  unsigned int indexed_commits_nr,\n+\t\t\t\t  int max_bitmaps);\n+int bitmap_writer_build(struct bitmap_writer *writer,\n+\t\t\tstruct packing_data *to_pack);\n+void bitmap_writer_finish(struct bitmap_writer *writer,\n+\t\t\t  struct pack_idx_entry **index,\n \t\t\t  uint32_t index_nr,\n \t\t\t  const char *filename,\n \t\t\t  uint16_t options);\n-- \n2.45.1.151.g7cc3499008c\n\n"},{"id":"494742","messageId":"b38dd5464d5423783ef6bb7863977d3051d5eab2.1715716605.git.me@ttaylorr.com","threadId":"61477","inReplyTo":"cover.1715716605.git.me@ttaylorr.com","subject":"[PATCH 4/6] pack-bitmap: drop unused `max_bitmaps` parameter","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-14T19:57:00Z","receivedAt":"2024-05-14T19:57:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The `max_bitmaps` parameter in `bitmap_writer_select_commits()` was\nintroduced back in 7cc8f97108 (pack-objects: implement bitmap writing,\n2013-12-21), making it original to the bitmap implementation in Git\nitself.\n\nWhen that patch was merged via 0f9e62e084 (Merge branch\n'jk/pack-bitmap', 2014-02-27), its sole caller in builtin/pack-objects.c\npassed a value of \"-1\" for `max_bitmaps`, indicating no limit.\n\nSince then, the only other caller (in midx.c, added via c528e17966\n(pack-bitmap: write multi-pack bitmaps, 2021-08-31)) also uses a value\nof \"-1\" for `max_bitmaps`.\n\nSince no callers have needed a finite limit for the `max_bitmaps`\nparameter in the nearly decade that has passed since 0f9e62e084, let's\nremove the parameter and any dead pieces of code connected to it.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/pack-objects.c | 3 +--\n midx-write.c           | 2 +-\n pack-bitmap-write.c    | 8 +-------\n pack-bitmap.h          | 3 +--\n 4 files changed, 4 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex ba4c93d241..10e69fdc8e 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1364,8 +1364,7 @@ static void write_pack_file(void)\n \t\t\t\t\t\t\t    progress);\n \t\t\t\tbitmap_writer_select_commits(&bitmap_writer,\n \t\t\t\t\t\t\t     indexed_commits,\n-\t\t\t\t\t\t\t     indexed_commits_nr,\n-\t\t\t\t\t\t\t     -1);\n+\t\t\t\t\t\t\t     indexed_commits_nr);\n \t\t\t\tif (bitmap_writer_build(&bitmap_writer, &to_pack) < 0)\n \t\t\t\t\tdie(_(\"failed to write bitmap index\"));\n \t\t\t\tbitmap_writer_finish(&bitmap_writer,\ndiff --git a/midx-write.c b/midx-write.c\nindex 5fdc8f2ff5..78fb0a2c8c 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -841,7 +841,7 @@ static int write_midx_bitmap(const char *midx_name,\n \tfor (i = 0; i < pdata->nr_objects; i++)\n \t\tindex[pack_order[i]] = &pdata->objects[i].idx;\n \n-\tbitmap_writer_select_commits(&writer, commits, commits_nr, -1);\n+\tbitmap_writer_select_commits(&writer, commits, commits_nr);\n \tret = bitmap_writer_build(&writer, pdata);\n \tif (ret < 0)\n \t\tgoto cleanup;\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex e22fa70745..c0087dab12 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -587,8 +587,7 @@ static int date_compare(const void *_a, const void *_b)\n \n void bitmap_writer_select_commits(struct bitmap_writer *writer,\n \t\t\t\t  struct commit **indexed_commits,\n-\t\t\t\t  unsigned int indexed_commits_nr,\n-\t\t\t\t  int max_bitmaps)\n+\t\t\t\t  unsigned int indexed_commits_nr)\n {\n \tunsigned int i = 0, j, next;\n \n@@ -611,11 +610,6 @@ void bitmap_writer_select_commits(struct bitmap_writer *writer,\n \t\tif (i + next >= indexed_commits_nr)\n \t\t\tbreak;\n \n-\t\tif (max_bitmaps > 0 && writer->selected_nr >= max_bitmaps) {\n-\t\t\twriter->selected_nr = max_bitmaps;\n-\t\t\tbreak;\n-\t\t}\n-\n \t\tif (next == 0) {\n \t\t\tchosen = indexed_commits[i];\n \t\t} else {\ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex 5a1890a2c5..b2d13d40eb 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -131,8 +131,7 @@ struct ewah_bitmap *bitmap_for_commit(struct bitmap_index *bitmap_git,\n \t\t\t\t      struct commit *commit);\n void bitmap_writer_select_commits(struct bitmap_writer *writer,\n \t\t\t\t  struct commit **indexed_commits,\n-\t\t\t\t  unsigned int indexed_commits_nr,\n-\t\t\t\t  int max_bitmaps);\n+\t\t\t\t  unsigned int indexed_commits_nr);\n int bitmap_writer_build(struct bitmap_writer *writer,\n \t\t\tstruct packing_data *to_pack);\n void bitmap_writer_finish(struct bitmap_writer *writer,\n-- \n2.45.1.151.g7cc3499008c\n\n"},{"id":"494743","messageId":"f16175295f5c786fea2d56ebffc2b9a6beb07aa0.1715716605.git.me@ttaylorr.com","threadId":"61477","inReplyTo":"cover.1715716605.git.me@ttaylorr.com","subject":"[PATCH 5/6] pack-bitmap-write.c: avoid uninitialized 'write_as' field","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-14T19:57:03Z","receivedAt":"2024-05-14T19:57:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Prepare to free() memory associated with bitmapped_commit structs by\nzero'ing the 'write_as' field.\n\nIn ideal cases, it is fine to do something like:\n\n    for (i = 0; i < writer->selected_nr; i++) {\n        struct bitmapped_commit *bc = &writer->selected[i];\n        if (bc->write_as != bc->bitmap)\n            ewah_free(bc->write_as);\n        ewah_free(bc->bitmap);\n    }\n\nbut if not all of the 'write_as' fields were populated (e.g., because\nthe packing_data given does not form a reachability closure), then we\nmay attempt to free uninitialized memory.\n\nGuard against this by preemptively zero'ing this field just in case.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n pack-bitmap-write.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex c0087dab12..420f17c2e0 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -112,6 +112,7 @@ static inline void push_bitmapped_commit(struct bitmap_writer *writer,\n \n \twriter->selected[writer->selected_nr].commit = commit;\n \twriter->selected[writer->selected_nr].bitmap = NULL;\n+\twriter->selected[writer->selected_nr].write_as = NULL;\n \twriter->selected[writer->selected_nr].flags = 0;\n \n \twriter->selected_nr++;\n-- \n2.45.1.151.g7cc3499008c\n\n"},{"id":"494744","messageId":"bf65967764f34adc2ca00d4c8195840ad3e4e127.1715716605.git.me@ttaylorr.com","threadId":"61477","inReplyTo":"cover.1715716605.git.me@ttaylorr.com","subject":"[PATCH 6/6] pack-bitmap: introduce `bitmap_writer_free()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-14T19:57:06Z","receivedAt":"2024-05-14T19:57:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Now that there is clearer memory ownership around the bitmap_writer\nstructure, introduce a bitmap_writer_free() function that callers may\nuse to free any memory associated with their instance of the\nbitmap_writer structure.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/pack-objects.c |  3 ++-\n midx-write.c           |  1 +\n pack-bitmap-write.c    | 23 +++++++++++++++++++++++\n pack-bitmap.h          |  1 +\n 4 files changed, 27 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 10e69fdc8e..26a6d0d791 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1245,7 +1245,6 @@ static void write_pack_file(void)\n \tuint32_t nr_remaining = nr_result;\n \ttime_t last_mtime = 0;\n \tstruct object_entry **write_order;\n-\tstruct bitmap_writer bitmap_writer;\n \n \tif (progress > pack_to_stdout)\n \t\tprogress_state = start_progress(_(\"Writing objects\"), nr_result);\n@@ -1315,6 +1314,7 @@ static void write_pack_file(void)\n \t\tif (!pack_to_stdout) {\n \t\t\tstruct stat st;\n \t\t\tstruct strbuf tmpname = STRBUF_INIT;\n+\t\t\tstruct bitmap_writer bitmap_writer;\n \t\t\tchar *idx_tmp_name = NULL;\n \n \t\t\t/*\n@@ -1370,6 +1370,7 @@ static void write_pack_file(void)\n \t\t\t\tbitmap_writer_finish(&bitmap_writer,\n \t\t\t\t\t\t     written_list, nr_written,\n \t\t\t\t\t\t     tmpname.buf, write_bitmap_options);\n+\t\t\t\tbitmap_writer_free(&bitmap_writer);\n \t\t\t\twrite_bitmap_index = 0;\n \t\t\t\tstrbuf_setlen(&tmpname, tmpname_len);\n \t\t\t}\ndiff --git a/midx-write.c b/midx-write.c\nindex 78fb0a2c8c..7c0c08c64b 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -853,6 +853,7 @@ static int write_midx_bitmap(const char *midx_name,\n cleanup:\n \tfree(index);\n \tfree(bitmap_name);\n+\tbitmap_writer_free(&writer);\n \n \ttrace2_region_leave(\"midx\", \"write_midx_bitmap\", the_repository);\n \ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex 420f17c2e0..6cae670412 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -32,6 +32,29 @@ void bitmap_writer_init(struct bitmap_writer *writer)\n \tmemset(writer, 0, sizeof(struct bitmap_writer));\n }\n \n+void bitmap_writer_free(struct bitmap_writer *writer)\n+{\n+\tuint32_t i;\n+\n+\tif (!writer)\n+\t\treturn;\n+\n+\tewah_free(writer->commits);\n+\tewah_free(writer->trees);\n+\tewah_free(writer->blobs);\n+\tewah_free(writer->tags);\n+\n+\tkh_destroy_oid_map(writer->bitmaps);\n+\n+\tfor (i = 0; i < writer->selected_nr; i++) {\n+\t\tstruct bitmapped_commit *bc = &writer->selected[i];\n+\t\tif (bc->write_as != bc->bitmap)\n+\t\t\tewah_free(bc->write_as);\n+\t\tewah_free(bc->bitmap);\n+\t}\n+\tfree(writer->selected);\n+}\n+\n void bitmap_writer_show_progress(struct bitmap_writer *writer, int show)\n {\n \twriter->show_progress = show;\ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex b2d13d40eb..3091095f33 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -139,6 +139,7 @@ void bitmap_writer_finish(struct bitmap_writer *writer,\n \t\t\t  uint32_t index_nr,\n \t\t\t  const char *filename,\n \t\t\t  uint16_t options);\n+void bitmap_writer_free(struct bitmap_writer *writer);\n char *midx_bitmap_filename(struct multi_pack_index *midx);\n char *pack_bitmap_filename(struct packed_git *p);\n \n-- \n2.45.1.151.g7cc3499008c\n"},{"id":"494760","messageId":"20240515061832.GA110841@coredump.intra.peff.net","threadId":"61477","inReplyTo":"cover.1715716605.git.me@ttaylorr.com","subject":"Re: [PATCH 0/6] pack-bitmap: various pack-bitmap-write cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-15T06:18:32Z","receivedAt":"2024-05-15T06:18:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 14, 2024 at 03:56:46PM -0400, Taylor Blau wrote:\n\n> This topic was born out of tb/pseudo-merge-bitmaps, which has a few\n> quality-of-life improvements in the pack-bitmap machinery.\n> \n> The main changes are to remove the static 'struct bitmap_writer', drop\n> one unused fields, and start using another unused one (see \"move\n> commit_positions into commit_pos fields\").\n> \n> There are other smaller changes, too, like cleaning up the flag\n> allocation table in object.h, as well as introducing a new\n> bitmap_writer_free() function.\n> \n> The next round of tb/pseudo-merge-bitmaps will be based on this branch.\n\nThese all look good to me. And I think it is nice to pull them out of\nthe larger series, to keep the independent steps to a more digestible\nsize.\n\n-Peff\n"},{"id":"494801","messageId":"ZkR6xqHgrIvoV6OR@tanuki","threadId":"61477","inReplyTo":"f16175295f5c786fea2d56ebffc2b9a6beb07aa0.1715716605.git.me@ttaylorr.com","subject":"Re: [PATCH 5/6] pack-bitmap-write.c: avoid uninitialized 'write_as' field","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-15T09:05:10Z","receivedAt":"2024-05-15T09:05:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, May 14, 2024 at 03:57:03PM -0400, Taylor Blau wrote:\n> Prepare to free() memory associated with bitmapped_commit structs by\n> zero'ing the 'write_as' field.\n> \n> In ideal cases, it is fine to do something like:\n> \n>     for (i = 0; i < writer->selected_nr; i++) {\n>         struct bitmapped_commit *bc = &writer->selected[i];\n>         if (bc->write_as != bc->bitmap)\n>             ewah_free(bc->write_as);\n>         ewah_free(bc->bitmap);\n>     }\n> \n> but if not all of the 'write_as' fields were populated (e.g., because\n> the packing_data given does not form a reachability closure), then we\n> may attempt to free uninitialized memory.\n> \n> Guard against this by preemptively zero'ing this field just in case.\n> \n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>  pack-bitmap-write.c | 1 +\n>  1 file changed, 1 insertion(+)\n> \n> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\n> index c0087dab12..420f17c2e0 100644\n> --- a/pack-bitmap-write.c\n> +++ b/pack-bitmap-write.c\n> @@ -112,6 +112,7 @@ static inline void push_bitmapped_commit(struct bitmap_writer *writer,\n>  \n>  \twriter->selected[writer->selected_nr].commit = commit;\n>  \twriter->selected[writer->selected_nr].bitmap = NULL;\n> +\twriter->selected[writer->selected_nr].write_as = NULL;\n>  \twriter->selected[writer->selected_nr].flags = 0;\n\nInstead of having to ensure that all fields are initialized we could\nalso set the whole structure to zero via `memset()`, which might be a\nbit more sustainable in the future. That alone doesn't really warrant a\nreroll though.\n\nPatrick\n"},{"id":"494802","messageId":"ZkR6zkUfW6Fosqyn@tanuki","threadId":"61477","inReplyTo":"bf65967764f34adc2ca00d4c8195840ad3e4e127.1715716605.git.me@ttaylorr.com","subject":"Re: [PATCH 6/6] pack-bitmap: introduce `bitmap_writer_free()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-15T09:05:18Z","receivedAt":"2024-05-15T09:05:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, May 14, 2024 at 03:57:06PM -0400, Taylor Blau wrote:\n> Now that there is clearer memory ownership around the bitmap_writer\n> structure, introduce a bitmap_writer_free() function that callers may\n> use to free any memory associated with their instance of the\n> bitmap_writer structure.\n\nGreat. I wanted to ask about this in preceding commits already, good to\nsee that you already thought of if.\n\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>  builtin/pack-objects.c |  3 ++-\n>  midx-write.c           |  1 +\n>  pack-bitmap-write.c    | 23 +++++++++++++++++++++++\n>  pack-bitmap.h          |  1 +\n>  4 files changed, 27 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 10e69fdc8e..26a6d0d791 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -1245,7 +1245,6 @@ static void write_pack_file(void)\n>  \tuint32_t nr_remaining = nr_result;\n>  \ttime_t last_mtime = 0;\n>  \tstruct object_entry **write_order;\n> -\tstruct bitmap_writer bitmap_writer;\n>  \n>  \tif (progress > pack_to_stdout)\n>  \t\tprogress_state = start_progress(_(\"Writing objects\"), nr_result);\n> @@ -1315,6 +1314,7 @@ static void write_pack_file(void)\n>  \t\tif (!pack_to_stdout) {\n>  \t\t\tstruct stat st;\n>  \t\t\tstruct strbuf tmpname = STRBUF_INIT;\n> +\t\t\tstruct bitmap_writer bitmap_writer;\n>  \t\t\tchar *idx_tmp_name = NULL;\n>  \n>  \t\t\t/*\n\nNit: we could have avoided moving the struct if it was introduced in\nthis spot in the preceding patch.\n\n[snip]\n> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\n> index 420f17c2e0..6cae670412 100644\n> --- a/pack-bitmap-write.c\n> +++ b/pack-bitmap-write.c\n> @@ -32,6 +32,29 @@ void bitmap_writer_init(struct bitmap_writer *writer)\n>  \tmemset(writer, 0, sizeof(struct bitmap_writer));\n>  }\n>  \n> +void bitmap_writer_free(struct bitmap_writer *writer)\n> +{\n> +\tuint32_t i;\n> +\n> +\tif (!writer)\n> +\t\treturn;\n> +\n> +\tewah_free(writer->commits);\n> +\tewah_free(writer->trees);\n> +\tewah_free(writer->blobs);\n> +\tewah_free(writer->tags);\n> +\n> +\tkh_destroy_oid_map(writer->bitmaps);\n> +\n> +\tfor (i = 0; i < writer->selected_nr; i++) {\n> +\t\tstruct bitmapped_commit *bc = &writer->selected[i];\n> +\t\tif (bc->write_as != bc->bitmap)\n> +\t\t\tewah_free(bc->write_as);\n> +\t\tewah_free(bc->bitmap);\n> +\t}\n> +\tfree(writer->selected);\n\nI was wondering whether we also want to set all those fields to `NULL`\nat the end, or just `memset()` the whole structure. But there probably\nisn't much of a reason to do it currently, so I don't mind if your\nanswer is \"no\".\n\nPatrick\n"},{"id":"494803","messageId":"ZkR61b5fWoRCv18D@tanuki","threadId":"61477","inReplyTo":"cover.1715716605.git.me@ttaylorr.com","subject":"Re: [PATCH 0/6] pack-bitmap: various pack-bitmap-write cleanups","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-15T09:05:25Z","receivedAt":"2024-05-15T09:05:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, May 14, 2024 at 03:56:46PM -0400, Taylor Blau wrote:\n> This topic was born out of tb/pseudo-merge-bitmaps, which has a few\n> quality-of-life improvements in the pack-bitmap machinery.\n> \n> The main changes are to remove the static 'struct bitmap_writer', drop\n> one unused fields, and start using another unused one (see \"move\n> commit_positions into commit_pos fields\").\n> \n> There are other smaller changes, too, like cleaning up the flag\n> allocation table in object.h, as well as introducing a new\n> bitmap_writer_free() function.\n> \n> The next round of tb/pseudo-merge-bitmaps will be based on this branch.\n> \n> Thanks in advance for your review!\n\nI've got some tiny nits, but overall this series looks good to me\nalready. I don't mind if it's merged in the current state if you don't\nthink any of the nits are worthy to be addressed.\n\nThanks for this cleanup!\n\nPatrick\n"},{"id":"494814","messageId":"ZkS4qi5TOThOwQLO@nand.local","threadId":"61477","inReplyTo":"ZkR6xqHgrIvoV6OR@tanuki","subject":"Re: [PATCH 5/6] pack-bitmap-write.c: avoid uninitialized 'write_as' field","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-15T13:29:14Z","receivedAt":"2024-05-15T13:29:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 15, 2024 at 11:05:10AM +0200, Patrick Steinhardt wrote:\n> > diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\n> > index c0087dab12..420f17c2e0 100644\n> > --- a/pack-bitmap-write.c\n> > +++ b/pack-bitmap-write.c\n> > @@ -112,6 +112,7 @@ static inline void push_bitmapped_commit(struct bitmap_writer *writer,\n> >\n> >  \twriter->selected[writer->selected_nr].commit = commit;\n> >  \twriter->selected[writer->selected_nr].bitmap = NULL;\n> > +\twriter->selected[writer->selected_nr].write_as = NULL;\n> >  \twriter->selected[writer->selected_nr].flags = 0;\n>\n> Instead of having to ensure that all fields are initialized we could\n> also set the whole structure to zero via `memset()`, which might be a\n> bit more sustainable in the future. That alone doesn't really warrant a\n> reroll though.\n\nI had considered it, but decided against it as it seemed wasteful to\n\n    memset(&writer->selected[writer->selected_nr], 0,\n           sizeof(struct bitmapped_commit))\n\nand then immediately un-zero some of its memory by assigning the commit\nfield.\n\nObviously not actually all that wasteful, as we're only talking about a\ncouple of hundred CPU cycles ;-).\n\nThanks,\nTaylor\n"},{"id":"494823","messageId":"ZkTblLnr53lSKZ8Q@nand.local","threadId":"61477","inReplyTo":"ZkR6zkUfW6Fosqyn@tanuki","subject":"Re: [PATCH 6/6] pack-bitmap: introduce `bitmap_writer_free()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-15T15:58:12Z","receivedAt":"2024-05-15T15:58:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 15, 2024 at 11:05:18AM +0200, Patrick Steinhardt wrote:\n> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> > index 10e69fdc8e..26a6d0d791 100644\n> > --- a/builtin/pack-objects.c\n> > +++ b/builtin/pack-objects.c\n> > @@ -1245,7 +1245,6 @@ static void write_pack_file(void)\n> >  \tuint32_t nr_remaining = nr_result;\n> >  \ttime_t last_mtime = 0;\n> >  \tstruct object_entry **write_order;\n> > -\tstruct bitmap_writer bitmap_writer;\n> >\n> >  \tif (progress > pack_to_stdout)\n> >  \t\tprogress_state = start_progress(_(\"Writing objects\"), nr_result);\n> > @@ -1315,6 +1314,7 @@ static void write_pack_file(void)\n> >  \t\tif (!pack_to_stdout) {\n> >  \t\t\tstruct stat st;\n> >  \t\t\tstruct strbuf tmpname = STRBUF_INIT;\n> > +\t\t\tstruct bitmap_writer bitmap_writer;\n> >  \t\t\tchar *idx_tmp_name = NULL;\n> >\n> >  \t\t\t/*\n>\n> Nit: we could have avoided moving the struct if it was introduced in\n> this spot in the preceding patch.\n\nUgh, I meant to move the declaration here in the previous patch, but\napparently didn't. I don't think it's worth sending another round just\nto fix that minor issue, but happy to do so if you feel strongly.\n\nThanks,\nTaylor\n"},{"id":"494824","messageId":"ZkTbrPjTqMEQGonv@nand.local","threadId":"61477","inReplyTo":"ZkR61b5fWoRCv18D@tanuki","subject":"Re: [PATCH 0/6] pack-bitmap: various pack-bitmap-write cleanups","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-15T15:58:36Z","receivedAt":"2024-05-15T15:58:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 15, 2024 at 11:05:25AM +0200, Patrick Steinhardt wrote:\n> On Tue, May 14, 2024 at 03:56:46PM -0400, Taylor Blau wrote:\n> > This topic was born out of tb/pseudo-merge-bitmaps, which has a few\n> > quality-of-life improvements in the pack-bitmap machinery.\n> >\n> > The main changes are to remove the static 'struct bitmap_writer', drop\n> > one unused fields, and start using another unused one (see \"move\n> > commit_positions into commit_pos fields\").\n> >\n> > There are other smaller changes, too, like cleaning up the flag\n> > allocation table in object.h, as well as introducing a new\n> > bitmap_writer_free() function.\n> >\n> > The next round of tb/pseudo-merge-bitmaps will be based on this branch.\n> >\n> > Thanks in advance for your review!\n>\n> I've got some tiny nits, but overall this series looks good to me\n> already. I don't mind if it's merged in the current state if you don't\n> think any of the nits are worthy to be addressed.\n\nThanks, both, for your review!\n\nThanks,\nTaylor\n"}]}