From: Karthik Nayak <karthik.188@gmail.com>
Date: Wed, 19 Nov 2025 16:10:41 GMT
Subject: Re: [PATCH 12/18] streaming: rely on object sources to create object stream
Message-ID: <CAOLa=ZRwnsYeHDpdL+uvnw0YMTbG1Gx2SKsq+0hTWMto+QZ+Lg@mail.gmail.com>
In-Reply-To: <20251119-b4-pks-odb-read-stream-v1-12-adacf03c2ccf@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> When creating an object stream we first look up the object info and, if
> it's present, we call into the respective backend that contains the
> object to create a new stream for it.
>
> This has the consequence that, for loose object source, we basically
> iterate through the object sources twice: we first discover that the
> file exists as a loose object in the first place by iterating through
> all sources. And, once we have discovered it, we again walk through all
> sources to try and map the object. The same issue will eventually also
> surface once the packfile store becomes per-object-source.
>
> Furthermore, it feels rather pointless to first look up the object only
> to then try and read it.
>
> Refactor the logic to be centered around sources instead. Instead of
> first reading the object, we immediately ask the source to create the
> object stream for us. If the object exists we get stream, otherwise
> we'll try the next source.
>
> Like this we only have to iterate through sources once. But even more
> importantly, this change also helps us to make the whole logic
> pluggable. The object read stream subsystem does not need to be aware of
> the different source backends anymore, but eventually it'll only have to
> call the source's callback function.
>
> Note that at the current poin in time we aren't full there yet:
>

s/poin/point
s/full/fully

>   - The packfile store still sits on the object database level and is
>     thus agnostic of the sources.
>
>   - We still have to call into both the packfile store and the loose
>     object source.
>
> But both of these issues will soon be addressed.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  streaming.c | 65 +++++++++++++++++++++++--------------------------------------
>  1 file changed, 24 insertions(+), 41 deletions(-)
>
> diff --git a/streaming.c b/streaming.c
> index 572be98248..bebb434cd1 100644
> --- a/streaming.c
> +++ b/streaming.c
> @@ -204,21 +204,15 @@ static int close_istream_loose(struct odb_read_stream *_st)
>  }
>
>  static int open_istream_loose(struct odb_read_stream **out,
> -			      struct repository *r,
> +			      struct odb_source *source,
>  			      const struct object_id *oid)
>  {
>  	struct object_info oi = OBJECT_INFO_INIT;
>  	struct odb_loose_read_stream *st;
> -	struct odb_source *source;
>  	unsigned long mapsize;
>  	void *mapped;
>
> -	odb_prepare_alternates(r->objects);
> -	for (source = r->objects->sources; source; source = source->next) {
> -		mapped = odb_source_loose_map_object(source, oid, &mapsize);
> -		if (mapped)
> -			break;
> -	}
> +	mapped = odb_source_loose_map_object(source, oid, &mapsize);
>  	if (!mapped)
>  		return -1;
>

So instead of going over the sources, we simply check for the given
source. Nice.

[snip]

> @@ -462,30 +460,15 @@ static int istream_source(struct odb_read_stream **out,
>  			  struct repository *r,
>  			  const struct object_id *oid)
>  {
> -	unsigned long size;
> -	int status;
> -	struct object_info oi = OBJECT_INFO_INIT;
> -
> -	oi.sizep = &size;
> -	status = odb_read_object_info_extended(r->objects, oid, &oi, 0);
> -	if (status < 0)
> -		return status;
> +	struct odb_source *source;
>
> -	switch (oi.whence) {
> -	case OI_LOOSE:
> -		if (open_istream_loose(out, r, oid) < 0)
> -			break;
> -		return 0;
> -	case OI_PACKED:
> -		if (oi.u.packed.is_delta ||
> -		    repo_settings_get_big_file_threshold(the_repository) >= size ||
> -		    open_istream_pack_non_delta(out, r, oid, oi.u.packed.pack,
> -						oi.u.packed.offset) < 0)
> -			break;
> +	if (!open_istream_pack_non_delta(out, r->objects, oid))
>  		return 0;
> -	default:
> -		break;
> -	}
> +
> +	odb_prepare_alternates(r->objects);
> +	for (source = r->objects->sources; source; source = source->next)
> +		if (!open_istream_loose(out, source, oid))
> +			return 0;
>

This seem to be the crux of it, where earlier we depended on
`odb_read_object_info_extended()` to tell us which backend to rely on
and then we re-fetched from that backed, now we simply go over the
different sources and try to get the object stream. Makes sense.

>  	return open_istream_incore(out, r, oid);
>  }
>
> --
> 2.52.0.rc2.482.gaa765fefd0.dirty