Re: [PATCH v2 15/20] for_each_object_in_pack(): convert to new revindex API

[Taylor Blau <me@ttaylorr.com>]

On Wed, Jan 13, 2021 at 10:43:37PM -0800, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > Avoid looking at the 'revindex' pointer directly and instead call
> > 'pack_pos_to_index()'.
> >
> > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > ---
> >  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.