diff mbox series

[v2,11/24] pack-bitmap.c: introduce 'nth_bitmap_object_oid()'

Message ID ac1f46aa1f0dbc7fba45229555b11b390fe104a0.1624314293.git.me@ttaylorr.com (mailing list archive)
State New
Headers show
Series multi-pack reachability bitmaps | expand

Commit Message

Taylor Blau June 21, 2021, 10:25 p.m. UTC
A subsequent patch to support reading MIDX bitmaps will be less noisy
after extracting a generic function to fetch the nth OID contained in
the bitmap.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 pack-bitmap.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Comments

Taylor Blau June 24, 2021, 2:59 p.m. UTC | #1
On Mon, Jun 21, 2021 at 06:25:26PM -0400, Taylor Blau wrote:
>  static int load_bitmap_entries_v1(struct bitmap_index *index)
>  {
>  	uint32_t i;
> @@ -242,9 +249,7 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)
>  		xor_offset = read_u8(index->map, &index->map_pos);
>  		flags = read_u8(index->map, &index->map_pos);
>
> -		if (nth_packed_object_id(&oid, index->pack, commit_idx_pos) < 0)
> -			return error("corrupt ewah bitmap: commit index %u out of range",
> -				     (unsigned)commit_idx_pos);
> +		nth_bitmap_object_oid(index, &oid, commit_idx_pos);

Oops. I was reading code in this area and noticed that this patch drops
the check introduced by c6b0c3910c (pack-bitmap.c: check reads more
aggressively when loading, 2020-12-08).

I fixed it up locally by restoring the check (but on the new function
nth_bitmap_object_oid() instead), and will send it in a reroll.

Thanks,
Taylor
Jeff King July 21, 2021, 10:37 a.m. UTC | #2
On Mon, Jun 21, 2021 at 06:25:26PM -0400, Taylor Blau wrote:

> A subsequent patch to support reading MIDX bitmaps will be less noisy
> after extracting a generic function to fetch the nth OID contained in
> the bitmap.

Makes sense, but...

> +static void nth_bitmap_object_oid(struct bitmap_index *index,
> +				  struct object_id *oid,
> +				  uint32_t n)
> +{
> +	nth_packed_object_id(oid, index->pack, n);
> +}
> +
>  static int load_bitmap_entries_v1(struct bitmap_index *index)
>  {
>  	uint32_t i;
> @@ -242,9 +249,7 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)
>  		xor_offset = read_u8(index->map, &index->map_pos);
>  		flags = read_u8(index->map, &index->map_pos);
>  
> -		if (nth_packed_object_id(&oid, index->pack, commit_idx_pos) < 0)
> -			return error("corrupt ewah bitmap: commit index %u out of range",
> -				     (unsigned)commit_idx_pos);
> +		nth_bitmap_object_oid(index, &oid, commit_idx_pos);

What happened to our error check here?

Should nth_bitmap_object_oid() be returning the value from
nth_packed_object_id()?

-Peff
Jeff King July 21, 2021, 10:38 a.m. UTC | #3
On Wed, Jul 21, 2021 at 06:37:41AM -0400, Jeff King wrote:

> > @@ -242,9 +249,7 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)
> >  		xor_offset = read_u8(index->map, &index->map_pos);
> >  		flags = read_u8(index->map, &index->map_pos);
> >  
> > -		if (nth_packed_object_id(&oid, index->pack, commit_idx_pos) < 0)
> > -			return error("corrupt ewah bitmap: commit index %u out of range",
> > -				     (unsigned)commit_idx_pos);
> > +		nth_bitmap_object_oid(index, &oid, commit_idx_pos);
> 
> What happened to our error check here?
> 
> Should nth_bitmap_object_oid() be returning the value from
> nth_packed_object_id()?

Ah, sorry, I just saw your followup message. I'll look for the fix in
the re-roll.

-Peff
diff mbox series

Patch

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 2dc135d34a..9757cd0fbb 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -223,6 +223,13 @@  static inline uint8_t read_u8(const unsigned char *buffer, size_t *pos)
 
 #define MAX_XOR_OFFSET 160
 
+static void nth_bitmap_object_oid(struct bitmap_index *index,
+				  struct object_id *oid,
+				  uint32_t n)
+{
+	nth_packed_object_id(oid, index->pack, n);
+}
+
 static int load_bitmap_entries_v1(struct bitmap_index *index)
 {
 	uint32_t i;
@@ -242,9 +249,7 @@  static int load_bitmap_entries_v1(struct bitmap_index *index)
 		xor_offset = read_u8(index->map, &index->map_pos);
 		flags = read_u8(index->map, &index->map_pos);
 
-		if (nth_packed_object_id(&oid, index->pack, commit_idx_pos) < 0)
-			return error("corrupt ewah bitmap: commit index %u out of range",
-				     (unsigned)commit_idx_pos);
+		nth_bitmap_object_oid(index, &oid, commit_idx_pos);
 
 		bitmap = read_bitmap_1(index);
 		if (!bitmap)
@@ -844,8 +849,8 @@  static unsigned long get_size_by_pos(struct bitmap_index *bitmap_git,
 		off_t ofs = pack_pos_to_offset(pack, pos);
 		if (packed_object_info(the_repository, pack, ofs, &oi) < 0) {
 			struct object_id oid;
-			nth_packed_object_id(&oid, pack,
-					     pack_pos_to_index(pack, pos));
+			nth_bitmap_object_oid(bitmap_git, &oid,
+					      pack_pos_to_index(pack, pos));
 			die(_("unable to get size of %s"), oid_to_hex(&oid));
 		}
 	} else {