On Wed, Sep 04, 2024 at 11:54:18AM -0700, Junio C Hamano wrote:
> Taylor Blau <me@xxxxxxxxxxxx> writes:
>
> > @@ -2055,17 +2055,18 @@ static int try_partial_reuse(struct bitmap_index *bitmap_git,
> > struct bitmapped_pack *pack,
> > size_t bitmap_pos,
> > uint32_t pack_pos,
> > + off_t offset,
> > struct bitmap *reuse,
> > struct pack_window **w_curs)
> > {
> > - off_t offset, delta_obj_offset;
> > + off_t delta_obj_offset;
> > enum object_type type;
> > unsigned long size;
> >
> > if (pack_pos >= pack->p->num_objects)
> > return -1; /* not actually in the pack */
> >
> > - offset = delta_obj_offset = pack_pos_to_offset(pack->p, pack_pos);
>
> We are now passing redundant piece of information "offset" that
> could be derived from two other parameters, which feels a bit icky,
Yeah, I agree. The only spot we use pack_pos in this function is in the
hunk above, and in the check where we reject any deltas whose base has a
greater than or equal to bit position than the delta object's bit
position.
But I think both of those are redundant. The only caller in
reuse_partial_packfile_from_bitmap_1() only passes bit positions that
are set, so we'll never trip the bounds check above. For the latter
check, it would suffice to compare object offsets in the single-pack
case, since the bit positions corresponding to objects in a single pack
are derived from the ordering of their actual offset.
> - The logic to punt on a delta pointing backwards can be done by
> comparing the base_offset and offset, instead of comparing the
> base_pos and pack_pos.
Ah, yes -- exactly ;-). I should have read the email in its entirety
before starting to respond.
> but it may not be worth the effort, unless we are going to do
> similar clean-up globally in all the codepaths that access
> packfiles.
I'm not so sure we can do it in all cases, but at least in the case of
try_partial_reuse(), doing something like this (which compiles and
passes "make test") is fine:
--- 8< ---
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 9d9b8c4bfb..fc51f298d5 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -2054,7 +2054,6 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs,
static int try_partial_reuse(struct bitmap_index *bitmap_git,
struct bitmapped_pack *pack,
size_t bitmap_pos,
- uint32_t pack_pos,
off_t offset,
struct bitmap *reuse,
struct pack_window **w_curs)
@@ -2063,9 +2062,6 @@ static int try_partial_reuse(struct bitmap_index *bitmap_git,
enum object_type type;
unsigned long size;
- if (pack_pos >= pack->p->num_objects)
- return -1; /* not actually in the pack */
-
delta_obj_offset = offset;
type = unpack_object_header(pack->p, w_curs, &offset, &size);
if (type < 0)
@@ -2121,8 +2117,13 @@ static int try_partial_reuse(struct bitmap_index *bitmap_git,
* OFS_DELTA is the default). But let's double check to
* make sure the pack wasn't written with odd
* parameters.
+ *
+ * Since we're working on a single-pack bitmap, we can
+ * use the object offset as a proxy for the bit
+ * position, since the bits are ordered by their
+ * positions within the pack.
*/
- if (base_pos >= pack_pos)
+ if (base_offset >= offset)
return 0;
base_bitmap_pos = pack->bitmap_pos + base_pos;
}
@@ -2218,8 +2219,8 @@ static void reuse_partial_packfile_from_bitmap_1(struct bitmap_index *bitmap_git
ofs = pack_pos_to_offset(pack->p, pack_pos);
}
- if (try_partial_reuse(bitmap_git, pack, bit_pos,
- pack_pos, ofs, reuse, &w_curs) < 0) {
+ if (try_partial_reuse(bitmap_git, pack, bit_pos, ofs,
+ reuse, &w_curs) < 0) {
/*
* try_partial_reuse indicated we couldn't reuse
* any bits, so there is no point in trying more
--- >8 ---
Thanks,
Taylor
