From: Karthik Nayak <karthik.188@gmail.com>
Date: Wed, 07 Jan 2026 08:50:45 GMT
Subject: Re: [PATCH v3 1/7] object-file: always set OI_LOOSE when reading object info
Message-ID: <CAOLa=ZSNmi_Lzb=3EdWks=mMOPvfijT2659y4YtxWnUKVUOXaA@mail.gmail.com>
In-Reply-To: <20260106-b4-pks-odb-read-object-info-improvements-v3-1-b5e02fae1fb0@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> There are some early returns in ``odb_source_loose_read_object_info()`

Nit: s/``/`

> in cases where we don't have to open the loose object. These return
> paths do not set `struct object_info::whence` to `OI_LOOSE` though, so
> it becomes impossible for the caller to tell the format of such an
> object.
>
> Nobody seems to care about this right now, but it's a bug waiting to
> happen. Fix this by always setting `whence` on success.
>

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  object-file.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> 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.