Re: [PATCH 18/20] pack-revindex: remove unused 'find_revindex_position()'

[Taylor Blau <me@ttaylorr.com>]

On Mon, Jan 11, 2021 at 06:57:00AM -0500, Derrick Stolee wrote:
> On 1/8/2021 1:17 PM, Taylor Blau wrote:
> > -int find_revindex_position(struct packed_git *p, off_t ofs)
> > +int offset_to_pack_pos(struct packed_git *p, off_t ofs, uint32_t *pos)
> >  {
> >  	int lo = 0;
> > -	int hi = p->num_objects + 1;
> > -	const struct revindex_entry *revindex = p->revindex;
> > +	int hi;
> > +	const struct revindex_entry *revindex;
> > +
> > +	if (load_pack_revindex(p) < 0)
> > +		return -1;
> > +
> > +	hi = p->num_objects + 1;
> > +	revindex = p->revindex;
> >
> >  	do {
> >  		const unsigned mi = lo + (hi - lo) / 2;
> >  		if (revindex[mi].offset == ofs) {
> > -			return mi;
> > +			*pos = mi;
> > +			return 0;
> >  		} else if (ofs < revindex[mi].offset)
> >  			hi = mi;
> >  		else
> > @@ -189,20 +196,6 @@ int find_revindex_position(struct packed_git *p, off_t ofs)
> >  	return -1;
> >  }
> >
> > -int offset_to_pack_pos(struct packed_git *p, off_t ofs, uint32_t *pos)
> > -{
> > -	int ret;
> > -
> > -	if (load_pack_revindex(p) < 0)
> > -		return -1;
> > -
> > -	ret = find_revindex_position(p, ofs);
> > -	if (ret < 0)
> > -		return -1;
> > -	*pos = ret;
> > -	return 0;
> > -}
> > -
>
> Not that this is new to the current patch, but this patch made me
> wonder if we should initialize *pos = -1 in the case of a failure
> to find the position? A correct caller should not use the value
> if they are checking for the fail-to-find case properly. But, I
> could see someone making a mistake and having trouble diagnosing
> the problem because their position variable was initialized to
> zero or a previous successful case.

*pos = -1 may be more confusing than clarifying since pos is unsigned.

It would be nice if there was a clear signal beyond returning a negative
value. I guess you could take a double pointer here which would allow
you to assign NULL, but that feels rather cumbersome as a means to catch
callers who failed to check the return value.

It does raise the argument of whether or not we should allow the
program to continue at all if 'ret < 0' (i.e., 'offset_to_pack_pos()'
either 'die()'s or returns a usable uint32_t), but I'm OK with the
current behavior.

> Thanks,
> -Stolee

Thanks,
Taylor