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

From: Jeff King <peff@peff.net>
Date: Jan 12, 2021, 08:54 UTC
Message-ID: <X/1jrSMU71BLWgm5@coredump.intra.peff.net>
In-Reply-To: <X/x5tdbM3PzkqbFQ@nand.local>

On Mon, Jan 11, 2021 at 11:15:49AM -0500, Taylor Blau wrote:

Show 22 quoted lines

> On Mon, Jan 11, 2021 at 06:43:23AM -0500, Derrick Stolee wrote:
> > > @@ -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.

I'll throw in my opinion that "< 0" to me much more clearly signals "did an error occur". And that same form can be used consistently with functions which _do_ have a positive return value on success, too. So I prefer it for readability.

-Peff