| Message ID | 54f4ad329f56808432549aa885f2847d5c9a8ac6.1610129796.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pack-revindex: prepare for on-disk reverse index | expand |
On Fri, Jan 08, 2021 at 01:17:17PM -0500, Taylor Blau wrote: > Remove another instance of direct revindex manipulation by calling > 'pack_pos_to_offset()' instead (the caller here does not care about the > index position of the object at position 'pos'). > > Somewhat confusingly, the subsequent call to unpack_object_header() > takes a pointer to &offset and then updates it with a new value. But, > try_partial_reuse() cares about the offset of both the base's header and > contents. The existing code made a copy of the offset field, and only > addresses and manipulates one of them. > > Instead, store the return of pack_pos_to_offset twice: once in header > and another in offset. Header will be left untouched, but offset will be > addressed and modified by unpack_object_header(). I had to read these second two paragraphs a few times to parse them. Really we are just replacing revidx->offset with "header", and "offset" retains its same role within the function. So it's definitely doing the right thing, but it makes more sense to me as: Note that we cannot just use the existing "offset" variable to store the value we get from pack_pos_to_offset(). It is incremented by unpack_object_header(), but we later need the original value. Since we'll no longer have revindex->offset to read it from, we'll store that in a separate variable ("header" since it points to the entry's header bytes). Another option would be to just call pack_pos_to_offset() again for the later call. Like the code it's replacing, it's constant-time anyway. But I think the "header" variable actually makes things more readable. -Peff
On Tue, Jan 12, 2021 at 04:06:46AM -0500, Jeff King wrote: > On Fri, Jan 08, 2021 at 01:17:17PM -0500, Taylor Blau wrote: > > Somewhat confusingly, the subsequent call to unpack_object_header() > > takes a pointer to &offset and then updates it with a new value. But, > > try_partial_reuse() cares about the offset of both the base's header and > > contents. The existing code made a copy of the offset field, and only > > addresses and manipulates one of them. > > > > Instead, store the return of pack_pos_to_offset twice: once in header > > and another in offset. Header will be left untouched, but offset will be > > addressed and modified by unpack_object_header(). > > I had to read these second two paragraphs a few times to parse them. > Really we are just replacing revidx->offset with "header", and "offset" > retains its same role within the function. > > So it's definitely doing the right thing, but it makes more sense to me > as: [...] Thanks. Reading these both back-to-back, I agree that yours is much clearer in describing what is actually going on. Thanks, Taylor
diff --git a/pack-bitmap.c b/pack-bitmap.c index 422505d7af..3f5cd4e77d 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -1069,23 +1069,21 @@ static void try_partial_reuse(struct bitmap_index *bitmap_git, struct bitmap *reuse, struct pack_window **w_curs) { - struct revindex_entry *revidx; - off_t offset; + off_t offset, header; enum object_type type; unsigned long size; if (pos >= bitmap_git->pack->num_objects) return; /* not actually in the pack */ - revidx = &bitmap_git->pack->revindex[pos]; - offset = revidx->offset; + offset = header = pack_pos_to_offset(bitmap_git->pack, pos); type = unpack_object_header(bitmap_git->pack, w_curs, &offset, &size); if (type < 0) return; /* broken packfile, punt */ if (type == OBJ_REF_DELTA || type == OBJ_OFS_DELTA) { off_t base_offset; - int base_pos; + uint32_t base_pos; /* * Find the position of the base object so we can look it up @@ -1096,11 +1094,10 @@ static void try_partial_reuse(struct bitmap_index *bitmap_git, * more detail. */ base_offset = get_delta_base(bitmap_git->pack, w_curs, - &offset, type, revidx->offset); + &offset, type, header); if (!base_offset) return; - base_pos = find_revindex_position(bitmap_git->pack, base_offset); - if (base_pos < 0) + if (offset_to_pack_pos(bitmap_git->pack, base_offset, &base_pos) < 0) return; /*
Remove another instance of direct revindex manipulation by calling 'pack_pos_to_offset()' instead (the caller here does not care about the index position of the object at position 'pos'). Somewhat confusingly, the subsequent call to unpack_object_header() takes a pointer to &offset and then updates it with a new value. But, try_partial_reuse() cares about the offset of both the base's header and contents. The existing code made a copy of the offset field, and only addresses and manipulates one of them. Instead, store the return of pack_pos_to_offset twice: once in header and another in offset. Header will be left untouched, but offset will be addressed and modified by unpack_object_header(). Signed-off-by: Taylor Blau <me@ttaylorr.com> --- pack-bitmap.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)