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
