From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 21 Nov 2025 19:32:38 GMT
Subject: Re: [PATCH v2 12/19] streaming: rely on object sources to create object stream
Message-ID: <xmqqldjz41wp.fsf@gitster.g>
In-Reply-To: <20251121-b4-pks-odb-read-stream-v2-12-ca8534963150@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.

Very nicely done.

> Note that at the current point in time we aren't fully there yet:
>
>   - 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.

;-)

> @@ -463,30 +461,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;

Hmph.

Earlier we let odb_read_object_info_extended() decide which one of
the duplicated objects (e.g., perhaps a loose object is still there
after packing), and then used the one it picked.  I think the
odb_read_object_info_extended() encodes a particular order with with
solid reasons like "do in-core cached one first", "favor objects in
pack over loose ones".

Now we instead let the first one with the object in the linked list
of sources, which may be different, unless the linked list is
created with the same "why one source needs to be given precedence
over the others" reasoning.

I do not know if/how it matters, this somewhat changes the
semantics, no?