> 2025年5月29日 23:45，Junio C Hamano <gitster@xxxxxxxxx> 写道：
>
> "Lidong Yan via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
>> From: Lidong Yan <502024330056@xxxxxxxxxxxxxxxx>
>>
>> This patch add test_bitmap_list_commits_offset() in patch-bitmap.c,
>
> "pack-bitmap.c"?
>
>> a new test helper command `test-tool bitmap list-commits-offset`,
>> and a `load corrupt bitmap` test case in t5310.
>>
>> The `load corrupt bitmap` test case intentionally corrupt the
>> "xor_offset" field of the first entry. And the newly added helper
>> can help to find position of "xor_offset" in bitmap file.
>
> [the structure of a log message]
>
> The usual way to compose a log message of this project is to
>
> - Give an observation on how the current system works in the
> present tense (so no need to say "Currently X is Y", or
> "Previously X was Y" to describe the state before your change;
> just "X is Y" is enough), and discuss what you perceive as a
> problem in it.
>
> - Propose a solution (optional---often, problem description
> trivially leads to an obvious solution in reader's minds).
>
> - Give commands to somebody editing the codebase to "make it so".
>
> in this order.
>
> The proposed log message lacks the motivation and only talks about
> what the patch does. We add a test-only code in a file, intermixed
> with production code. Let's explain why it is the best arrangement.
I see. I am trying to validate these patch series and test further patch won’t leak
memory under the condition that bitmap is corrupted, Anyway I will pay attention
to motivation in my following log messages.
>
>> Signed-off-by: Lidong Yan <502024330056@xxxxxxxxxxxxxxxx>
>> ---
>> pack-bitmap.c | 73 +++++++++++++++++++++++++++++++++++++----
>> pack-bitmap.h | 1 +
>> t/helper/test-bitmap.c | 8 +++++
>> t/t5310-pack-bitmaps.sh | 27 +++++++++++++++
>> 4 files changed, 103 insertions(+), 6 deletions(-)
>
> After the second round of the series, no review comments seem to
> have been sent to the list. Is everybody happy with the latest
> iteration?
>
> Thanks.
>
>> diff --git a/pack-bitmap.c b/pack-bitmap.c
>> index fd19c2255163..39c1c1bc4ce1 100644
>> --- a/pack-bitmap.c
>> +++ b/pack-bitmap.c
>> @@ -34,6 +34,11 @@ struct stored_bitmap {
>> int flags;
>> };
>>
>> +struct stored_bitmap_tag_pos {
>> + struct stored_bitmap stored;
>> + size_t map_pos;
>> +};
>> +
>> /*
>> * The active bitmap index for a repository. By design, repositories only have
>> * a single bitmap index available (the index for the biggest packfile in
>> @@ -148,6 +153,7 @@ static int existing_bitmaps_hits_nr;
>> static int existing_bitmaps_misses_nr;
>> static int roots_with_bitmaps_nr;
>> static int roots_without_bitmaps_nr;
>> +static int tag_pos_on_bitmap;
>>
>> static struct ewah_bitmap *lookup_stored_bitmap(struct stored_bitmap *st)
>> {
>> @@ -314,13 +320,18 @@ static struct stored_bitmap *store_bitmap(struct bitmap_index *index,
>> struct ewah_bitmap *root,
>> const struct object_id *oid,
>> struct stored_bitmap *xor_with,
>> - int flags)
>> + int flags, size_t map_pos)
>> {
>> struct stored_bitmap *stored;
>> + struct stored_bitmap_tag_pos *tagged;
>> khiter_t hash_pos;
>> int ret;
>>
>> - stored = xmalloc(sizeof(struct stored_bitmap));
>> + tagged = xmalloc(tag_pos_on_bitmap ? sizeof(struct stored_bitmap_tag_pos) :
>> + sizeof(struct stored_bitmap));
>> + stored = &tagged->stored;
>> + if (tag_pos_on_bitmap)
>> + tagged->map_pos = map_pos;
>> stored->root = root;
>> stored->xor = xor_with;
>> stored->flags = flags;
>> @@ -376,10 +387,12 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)
>> struct stored_bitmap *xor_bitmap = NULL;
>> uint32_t commit_idx_pos;
>> struct object_id oid;
>> + size_t entry_map_pos;
>>
>> if (index->map_size - index->map_pos < 6)
>> return error(_("corrupt ewah bitmap: truncated header for entry %d"), i);
>>
>> + entry_map_pos = index->map_pos;
>> commit_idx_pos = read_be32(index->map, &index->map_pos);
>> xor_offset = read_u8(index->map, &index->map_pos);
>> flags = read_u8(index->map, &index->map_pos);
>> @@ -402,8 +415,9 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)
>> if (!bitmap)
>> return -1;
>>
>> - recent_bitmaps[i % MAX_XOR_OFFSET] = store_bitmap(
>> - index, bitmap, &oid, xor_bitmap, flags);
>> + recent_bitmaps[i % MAX_XOR_OFFSET] =
>> + store_bitmap(index, bitmap, &oid, xor_bitmap, flags,
>> + entry_map_pos);
>> }
>>
>> return 0;
>> @@ -869,6 +883,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_
>> int xor_flags;
>> khiter_t hash_pos;
>> struct bitmap_lookup_table_xor_item *xor_item;
>> + size_t entry_map_pos;
>>
>> if (is_corrupt)
>> return NULL;
>> @@ -928,6 +943,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_
>> goto corrupt;
>> }
>>
>> + entry_map_pos = bitmap_git->map_pos;
>> bitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);
>> xor_flags = read_u8(bitmap_git->map, &bitmap_git->map_pos);
>> bitmap = read_bitmap_1(bitmap_git);
>> @@ -935,7 +951,8 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_
>> if (!bitmap)
>> goto corrupt;
>>
>> - xor_bitmap = store_bitmap(bitmap_git, bitmap, &xor_item->oid, xor_bitmap, xor_flags);
>> + xor_bitmap = store_bitmap(bitmap_git, bitmap, &xor_item->oid,
>> + xor_bitmap, xor_flags, entry_map_pos);
>> xor_items_nr--;
>> }
>>
>> @@ -969,6 +986,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_
>> * Instead, we can skip ahead and immediately read the flags and
>> * ewah bitmap.
>> */
>> + entry_map_pos = bitmap_git->map_pos;
>> bitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);
>> flags = read_u8(bitmap_git->map, &bitmap_git->map_pos);
>> bitmap = read_bitmap_1(bitmap_git);
>> @@ -976,7 +994,8 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_
>> if (!bitmap)
>> goto corrupt;
>>
>> - return store_bitmap(bitmap_git, bitmap, oid, xor_bitmap, flags);
>> + return store_bitmap(bitmap_git, bitmap, oid, xor_bitmap, flags,
>> + entry_map_pos);
>>
>> corrupt:
>> free(xor_items);
>> @@ -2856,6 +2875,48 @@ int test_bitmap_commits(struct repository *r)
>> return 0;
>> }
>>
>> +int test_bitmap_commits_offset(struct repository *r)
>> +{
>> + struct object_id oid;
>> + struct stored_bitmap_tag_pos *tagged;
>> + struct bitmap_index *bitmap_git;
>> + size_t commit_idx_pos_map_pos, xor_offset_map_pos, flag_map_pos,
>> + ewah_bitmap_map_pos;
>> +
>> + tag_pos_on_bitmap = 1;
>> + bitmap_git = prepare_bitmap_git(r);
>> + if (!bitmap_git)
>> + die(_("failed to load bitmap indexes"));
>> +
>> + /*
>> + * As this function is only used to print bitmap selected
>> + * commits, we don't have to read the commit table.
>> + */
>> + if (bitmap_git->table_lookup) {
>> + if (load_bitmap_entries_v1(bitmap_git) < 0)
>> + die(_("failed to load bitmap indexes"));
>> + }
>> +
>> + kh_foreach (bitmap_git->bitmaps, oid, tagged, {
>> + commit_idx_pos_map_pos = tagged->map_pos;
>> + xor_offset_map_pos = tagged->map_pos + sizeof(uint32_t);
>> + flag_map_pos = xor_offset_map_pos + sizeof(uint8_t);
>> + ewah_bitmap_map_pos = flag_map_pos + sizeof(uint8_t);
>> +
>> + printf_ln("%s %"PRIuMAX" %"PRIuMAX" %"PRIuMAX" %"PRIuMAX,
>> + oid_to_hex(&oid),
>> + (uintmax_t)commit_idx_pos_map_pos,
>> + (uintmax_t)xor_offset_map_pos,
>> + (uintmax_t)flag_map_pos,
>> + (uintmax_t)ewah_bitmap_map_pos);
>> + })
>> + ;
>> +
>> + free_bitmap_index(bitmap_git);
>> +
>> + return 0;
>> +}
>> +
>> int test_bitmap_hashes(struct repository *r)
>> {
>> struct bitmap_index *bitmap_git = prepare_bitmap_git(r);
>> diff --git a/pack-bitmap.h b/pack-bitmap.h
>> index 382d39499af2..96880ba3d72d 100644
>> --- a/pack-bitmap.h
>> +++ b/pack-bitmap.h
>> @@ -81,6 +81,7 @@ void traverse_bitmap_commit_list(struct bitmap_index *,
>> show_reachable_fn show_reachable);
>> void test_bitmap_walk(struct rev_info *revs);
>> int test_bitmap_commits(struct repository *r);
>> +int test_bitmap_commits_offset(struct repository *r);
>> int test_bitmap_hashes(struct repository *r);
>> int test_bitmap_pseudo_merges(struct repository *r);
>> int test_bitmap_pseudo_merge_commits(struct repository *r, uint32_t n);
>> diff --git a/t/helper/test-bitmap.c b/t/helper/test-bitmap.c
>> index 3f23f2107268..65a1ab29192b 100644
>> --- a/t/helper/test-bitmap.c
>> +++ b/t/helper/test-bitmap.c
>> @@ -10,6 +10,11 @@ static int bitmap_list_commits(void)
>> return test_bitmap_commits(the_repository);
>> }
>>
>> +static int bitmap_list_commits_offset(void)
>> +{
>> + return test_bitmap_commits_offset(the_repository);
>> +}
>> +
>> static int bitmap_dump_hashes(void)
>> {
>> return test_bitmap_hashes(the_repository);
>> @@ -36,6 +41,8 @@ int cmd__bitmap(int argc, const char **argv)
>>
>> if (argc == 2 && !strcmp(argv[1], "list-commits"))
>> return bitmap_list_commits();
>> + if (argc == 2 && !strcmp(argv[1], "list-commits-offset"))
>> + return bitmap_list_commits_offset();
>> if (argc == 2 && !strcmp(argv[1], "dump-hashes"))
>> return bitmap_dump_hashes();
>> if (argc == 2 && !strcmp(argv[1], "dump-pseudo-merges"))
>> @@ -46,6 +53,7 @@ int cmd__bitmap(int argc, const char **argv)
>> return bitmap_dump_pseudo_merge_objects(atoi(argv[2]));
>>
>> usage("\ttest-tool bitmap list-commits\n"
>> + "\ttest-tool bitmap list-commits-offset\n"
>> "\ttest-tool bitmap dump-hashes\n"
>> "\ttest-tool bitmap dump-pseudo-merges\n"
>> "\ttest-tool bitmap dump-pseudo-merge-commits <n>\n"
>> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
>> index a62b463eaf09..ef4c5fbaae83 100755
>> --- a/t/t5310-pack-bitmaps.sh
>> +++ b/t/t5310-pack-bitmaps.sh
>> @@ -486,6 +486,33 @@ test_bitmap_cases () {
>> grep "ignoring extra bitmap" trace2.txt
>> )
>> '
>> +
>> + test_expect_success 'load corrupt bitmap' '
>> + rm -fr repo &&
>> + git init repo &&
>> + test_when_finished "rm -fr repo" &&
>> + (
>> + cd repo &&
>> + git config pack.writeBitmapLookupTable '"$writeLookupTable"' &&
>> +
>> + test_commit base &&
>> +
>> + git repack -adb &&
>> + bitmap="$(ls .git/objects/pack/pack-*.bitmap)" &&
>> + chmod +w $bitmap &&
>> +
>> + read oid commit_off xor_off flag_off ewah_off <<-EOF &&
>> + $(test-tool bitmap list-commits-offset | head -n 1)
>> + EOF
>> + printf '\161' |
>> + dd of=$bitmap count=1 bs=1 conv=notrunc seek=$xor_off &&
>> +
>> +
>> + git rev-list --count HEAD > expect &&
>> + git rev-list --use-bitmap-index --count HEAD > actual &&
>> + test_cmp expect actual
>> + )
>> + '
>> }
>>
>> test_bitmap_cases
>
