On Wed, Jan 13, 2021 at 10:43:37PM -0800, Junio C Hamano wrote: > Taylor Blau <me@xxxxxxxxxxxx> writes: > > > Avoid looking at the 'revindex' pointer directly and instead call > > 'pack_pos_to_index()'. > > > > Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx> > > --- > > packfile.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/packfile.c b/packfile.c > > index 936ab3def5..7bb1750934 100644 > > --- a/packfile.c > > +++ b/packfile.c > > @@ -2086,7 +2086,7 @@ int for_each_object_in_pack(struct packed_git *p, > > struct object_id oid; > > > > if (flags & FOR_EACH_OBJECT_PACK_ORDER) > > - pos = p->revindex[i].nr; > > + pos = pack_pos_to_index(p, i); > > It wasn't too bad before this series formally defined what > "position", "index" and "offset" mean, but now this has become > highly misleading. The variable "pos" here holds what we consider > "index" while "i" holds what we call "position" [*1*]. > > > else > > pos = i; > > Perhaps renaming "uint32_t pos" to "nth" would avoid confusion? I agree that it can be confusing. Unfortunately in this spot, this variable really does mean two things. If we set the FOR_EACH_OBJECT_PACK_ORDER bit in our flags, then the caller really wants the index position (and the objects to be delivered in pack order). But if we didn't set it, then the caller wants it in index order. > - if (nth_packed_object_id(&oid, p, pos) < 0) > + if (nth_packed_object_id(&oid, p, nth) < 0) > return error(...); This suggested diff makes me think that you understand all of that, so I'm mostly saying this for the benefit of others that haven't looked at this code closely in the recent past. I'd be happy to send a replacement patch if you would like [1], but I'm hopeful that this is clear enough since there isn't much code between the declaration, assignment(s), and use of 'pos'. Thanks, Taylor [1]: I understand your general disdain for single replacement patches, but I'd like to avoid sending the other 19 patches if possible to avoid delivering more mail to list subscribers than is necessary.
