From: Karthik Nayak <karthik.188@gmail.com>
Date: Thu, 08 Jan 2026 09:30:16 GMT
Subject: Re: [PATCH v4 1/7] object-file: always set OI_LOOSE when reading object info
Message-ID: <CAOLa=ZSWKzOzN103CyuVstnaiviFDm8KB6mQOQLyyExy4TiUzA@mail.gmail.com>
In-Reply-To: <20260107-b4-pks-odb-read-object-info-improvements-v4-1-b5d55c47082a@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> There are some early returns in `odb_source_loose_read_object_info()`
> 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.
>
> The root cause of this really is that we have so many different return
> paths in the function. As a consequence, it's harder than necessary to
> make sure that all successful exit paths sot up the `whence` field as
> expected.
>
> Address this by refactoring the function to have a single exit path.
> Like this, we can trivially set up the `whence` field when we exit
> successfully from the function.
>
> Note that we also:
>
>   - Rename `status` to `ret` to match our usual coding style, but also
>     to show that the old `status` variable is now always getting the
>     expected value. Furthermore, the value is not initialized anymore,
>     which has the consequence that most compilers will warn for exit
>     paths where we forgot to set it.
>
>   - Move the setup of scratch pointers closer to `parse_loose_header()`
>     to show where it's needed.
>
>   - Guard a couple of variables on cleanup so that they only get
>     released in case they have been set up.
>
>   - Reset `oi->delta_base_oid` towards the end of the function, together
>     with all the other object info pointers.
>
> Overall, all these changes result in a diff that is somewhat hard to
> read. But the end result is significantly easier to read and reason
> about, so I'd argue this one-time churn is worth it.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  object-file.c | 115 ++++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 71 insertions(+), 44 deletions(-)
>
> diff --git a/object-file.c b/object-file.c
> index 6280e42f34..e7e4c3348f 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -416,19 +416,16 @@ int odb_source_loose_read_object_info(struct odb_source *source,
>  				      const struct object_id *oid,
>  				      struct object_info *oi, int flags)
>  {
> -	int status = 0;
> +	int ret;
>  	int fd;
>  	unsigned long mapsize;
>  	const char *path;
> -	void *map;
> -	git_zstream stream;
> +	void *map = NULL;
> +	git_zstream stream, *stream_to_end = NULL;
>  	char hdr[MAX_HEADER_LEN];
>  	unsigned long size_scratch;
>  	enum object_type type_scratch;
>
> -	if (oi && oi->delta_base_oid)
> -		oidclr(oi->delta_base_oid, source->odb->repo->hash_algo);
> -
>  	/*
>  	 * If we don't care about type or size, then we don't
>  	 * need to look inside the object at all. Note that we
> @@ -439,71 +436,101 @@ 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 (stat_loose_object(source->loose, oid, &st, &path) < 0)
> -			return -1;
> +
> +		if ((!oi || !oi->disk_sizep) && (flags & OBJECT_INFO_QUICK)) {
> +			ret = quick_has_loose(source->loose, oid) ? 0 : -1;
> +			goto out;
> +		}
> +
> +		if (stat_loose_object(source->loose, oid, &st, &path) < 0) {
> +			ret = -1;
> +			goto out;
> +		}
> +
>  		if (oi && oi->disk_sizep)
>  			*oi->disk_sizep = st.st_size;
> -		return 0;
> +
> +		ret = 0;
> +		goto out;
>  	}
>
>  	fd = open_loose_object(source->loose, oid, &path);
>  	if (fd < 0) {
>  		if (errno != ENOENT)
>  			error_errno(_("unable to open loose object %s"), oid_to_hex(oid));
> -		return -1;
> +		ret = -1;
> +		goto out;
>  	}
> -	map = map_fd(fd, path, &mapsize);
> -	if (!map)
> -		return -1;
>
> -	if (!oi->sizep)
> -		oi->sizep = &size_scratch;
> -	if (!oi->typep)
> -		oi->typep = &type_scratch;
> +	map = map_fd(fd, path, &mapsize);
> +	if (!map) {
> +		ret = -1;
> +		goto out;
> +	}
>
>  	if (oi->disk_sizep)
>  		*oi->disk_sizep = mapsize;
>
> +	stream_to_end = &stream;
> +
>

Okay we use `stream_to_end` to simply identify if the stream needs to be
cleared.

The changes look good and indeed the final outcome is better here.
Thanks.