From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 07 Jan 2026 11:27:24 GMT
Subject: Re: [PATCH v3 1/7] object-file: always set OI_LOOSE when reading object info
Message-ID: <aV5DHI04KBs4GJn-@pks.im>
In-Reply-To: <CAOLa=ZSNmi_Lzb=3EdWks=mMOPvfijT2659y4YtxWnUKVUOXaA@mail.gmail.com>

On Wed, Jan 07, 2026 at 12:50:45AM -0800, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/object-file.c b/object-file.c
> > index 6280e42f34..d566df427a 100644
> > --- a/object-file.c
> > +++ b/object-file.c
> > @@ -439,12 +439,23 @@ int odb_source_loose_read_object_info(struct odb_source *source,
> >  	 */
> >  	if (!oi || (!oi->typep && !oi->sizep && !oi->contentp)) {
> >  		struct stat st;
> > -		if ((!oi || !oi->disk_sizep) && (flags & OBJECT_INFO_QUICK))
> > -			return quick_has_loose(source->loose, oid) ? 0 : -1;
> > +
> > +		if ((!oi || !oi->disk_sizep) && (flags & OBJECT_INFO_QUICK)) {
> > +			status = quick_has_loose(source->loose, oid) ? 0 : -1;
> > +			if (!status && oi)
> > +				oi->whence = OI_LOOSE;
> > +			return status;
> > +		}
> > +
> >  		if (stat_loose_object(source->loose, oid, &st, &path) < 0)
> >  			return -1;
> > -		if (oi && oi->disk_sizep)
> > -			*oi->disk_sizep = st.st_size;
> > +
> > +		if (oi) {
> > +			if (oi->disk_sizep)
> > +				*oi->disk_sizep = st.st_size;
> > +			oi->whence = OI_LOOSE;
> > +		}
> > +
> >  		return 0;
> >  	}
> >
> 
> The change looks good. I'm wary of early returns independently doing the
> cleanup, wonder if it'd be better to do `status = ...; goto cleanup`
> instead.

I share that sentiment, and I was in fact having a look at what it would
take to have a single exit path in this function. I eventually discarded
the work though because it required a bunch of changes to really make
this whole function more readable than it currently is.

But now that you're the second one thinking this I'll probably bite the
bullet and just do it.

Thanks!

Patrick