Re: [PATCH 05/20] check_object(): convert to new revindex API

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 11, 2021, 16:15 UTC
Message-ID: <X/x5tdbM3PzkqbFQ@nand.local>
In-Reply-To: <b1a6110a-a097-931f-5710-92a1f59a842b@gmail.com>

On Mon, Jan 11, 2021 at 06:43:23AM -0500, Derrick Stolee wrote:

Show 17 quoted lines

> > @@ -1813,11 +1813,11 @@ static void check_object(struct object_entry *entry, uint32_t object_index)
> >  				goto give_up;
> >  			}
> >  			if (reuse_delta && !entry->preferred_base) {
> > -				struct revindex_entry *revidx;
> > -				revidx = find_pack_revindex(p, ofs);
> > -				if (!revidx)
> > +				uint32_t pos;
> > +				if (offset_to_pack_pos(p, ofs, &pos) < 0)
>
> The current implementation does not return a positive value. Only
> -1 on error and 0 on success. Is this "< 0" doing anything important?
> Seems like it would be easiest to do
>
> 	if (offset_to_pack_pos(p, ofs, &pos))
>
> [snip]

Either would work, of course. I tend to find the '< 0' form easier to read, but I may be in the minority there. For me, the negative return value makes clear that the function encountered an error.

A secondary benefit is that if the function ever were to return a positive value that _didn't_ indicate an error, we would already be protected against it. That is probably a pretty weak argument, though, since any such refactoring would probably require the callers to change, too.

Anyway, that's all to say that I'm happy to leave it as-is, but I'm equally happy to change it, too.

Thanks, Taylor