git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v4 2/2] pack-bitmap: add load corrupt bitmap test

From
Junio C Hamano <gitster@pobox.com>
Date
May 29, 2025, 15:45 UTC
Message-ID
<xmqqbjrbmndn.fsf@gitster.g>
In-Reply-To
<7876d9a9014ea6a0657f440f7fa1efd496a4a15a.1748140983.git.gitgitgadget@gmail.com>
"Lidong Yan via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Lidong Yan <502024330056@smail.nju.edu.cn>
>
> This patch add test_bitmap_list_commits_offset() in patch-bitmap.c,
"pack-bitmap.c"?
Show 6 quoted lines
> 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.

Show 7 quoted lines
> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
> ---
>  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.
Show 246 quoted lines
> 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
Previous: Lidong Yan via GitGitGadgetNext: Taylor Blau
Message 28 of 45 in “pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed”
  1. pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failedLidong Yan via GitGitGadget, May 12, 2025
  2. Jeff KingMay 12, 2025
  3. Taylor BlauMay 13, 2025
  4. Junio C HamanoMay 14, 2025
  5. Jeff KingMay 14, 2025
  6. lidongyanMay 15, 2025
  7. 0/3 pack-bitmap: fix memory leak if load_bitmap_entries_v1 failedLidong Yan via GitGitGadget, May 20, 2025
  8. 1/3 pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failedLidong Yan via GitGitGadget, May 20, 2025
  9. 2/3 pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failedTaylor Blau via GitGitGadget, May 20, 2025
  10. Taylor BlauMay 21, 2025
  11. lidongyanMay 22, 2025
  12. Junio C HamanoMay 22, 2025
  13. 3/3 pack-bitmap: add loading corrupt bitmap_index testLidong Yan via GitGitGadget, May 20, 2025
  14. Taylor BlauMay 22, 2025
  15. lidongyanMay 22, 2025
  16. Taylor BlauMay 23, 2025
  17. lidongyanMay 23, 2025
  18. 0/2 pack-bitmap: fix memory leak if load_bitmap_entries_v1 failedLidong Yan via GitGitGadget, May 25, 2025
  19. 1/2 pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failedTaylor Blau via GitGitGadget, May 25, 2025
  20. 2/2 pack-bitmap: add load corrupt bitmap testLidong Yan via GitGitGadget, May 25, 2025
  21. 0/2 pack-bitmap: fix memory leak if load_bitmap failedLidong Yan via GitGitGadget, May 25, 2025
  22. 1/2 pack-bitmap: fix memory leak if load_bitmap() failedTaylor Blau via GitGitGadget, May 25, 2025
  23. Junio C HamanoMay 29, 2025
  24. Taylor BlauMay 29, 2025
  25. Junio C HamanoMay 29, 2025
  26. lidongyanMay 30, 2025
  27. 2/2 pack-bitmap: add load corrupt bitmap testLidong Yan via GitGitGadget, May 25, 2025
  28. Junio C HamanoMay 29, 2025
  29. Taylor BlauMay 29, 2025
  30. lidongyanMay 30, 2025
  31. Taylor BlauMay 29, 2025
  32. lidongyanMay 30, 2025
  33. 0/3 pack-bitmap: fix memory leak if load_bitmap failedLidong Yan via GitGitGadget, Jun 3, 2025
  34. 1/3 pack-bitmap: fix memory leak if load_bitmap() failedTaylor Blau via GitGitGadget, Jun 3, 2025
  35. 2/3 pack-bitmap: reword comments in test_bitmap_commits()Lidong Yan via GitGitGadget, Jun 3, 2025
  36. Taylor BlauJun 3, 2025
  37. 3/3 pack-bitmap: add load corrupt bitmap testLidong Yan via GitGitGadget, Jun 3, 2025
  38. Taylor BlauJun 3, 2025
  39. 0/3 pack-bitmap: fix memory leak if load_bitmap failedLidong Yan via GitGitGadget, Jul 1, 2025
  40. 1/3 pack-bitmap: fix memory leak if load_bitmap() failedTaylor Blau via GitGitGadget, Jul 1, 2025
  41. 2/3 pack-bitmap: reword comments in test_bitmap_commits()Lidong Yan via GitGitGadget, Jul 1, 2025
  42. 3/3 pack-bitmap: add load corrupt bitmap testLidong Yan via GitGitGadget, Jul 1, 2025
  43. Junio C HamanoJul 7, 2025
  44. Taylor BlauJul 8, 2025
  45. Junio C HamanoJul 8, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.