From: Justin Tobler <jltobler@gmail.com>
Date: Wed, 19 Nov 2025 19:01:03 GMT
Subject: Re: [PATCH 02/18] streaming: drop the `open()` callback function
Message-ID: <g74hupkwedtclb3gxomhxj6w4rqqzn3tsostdriauvn3gu2cw2@wxgwulitxbtq>
In-Reply-To: <20251119-b4-pks-odb-read-stream-v1-2-adacf03c2ccf@pks.im>

On 25/11/19 08:47AM, Patrick Steinhardt wrote:
> When creating a read stream we first populate the structure with the
> open callback function and then subsequently call the function. This
> layout is somewhat weird though:
> 
>   - The structure needs to be allocated and partially populated with the
>     open function before we can properly initialize it.
> 
>   - We never use the `open()` callback after having opened it initially.
> 
> Especially the first point creates a problem for us. In subsequent
> commits we'll want to fully move construction of the read source into
> the respective object sources. E.g., the loose object source will be the
> one that is responsible for creating the structure. But this creates a
> problem: if we first need to create the structure so that we can call
> the source-specific callback we cannot fully handle creation of the
> structure in the source itself.
> 
> We could of course work around that and have the loose object source
> create the structure and populate it's `open()` callback, only. But

s/it's/its/

> this doesn't really buy us anything due to the second bullet point
> above.
> 
> Instead, drop the callback entirely and refactor `istream_source()` so
> that we open the streams immediately. This unblocks a subsequent step,
> where we'll also start to allocate the structure in the source-specific
> logic.

Out of curiousity, is there any reason we would ever want to delay
opening the source read stream? If not, then I agree it makes more sense
to just open the stream at time of its initialization.

> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  streaming.c | 40 +++++++++++++++++-----------------------
>  1 file changed, 17 insertions(+), 23 deletions(-)
> 
> diff --git a/streaming.c b/streaming.c
> index 1fb4b7c1c0..5ce6350123 100644
> --- a/streaming.c
> +++ b/streaming.c
> @@ -14,10 +14,6 @@
>  #include "replace-object.h"
>  #include "packfile.h"
>  
> -typedef int (*open_istream_fn)(struct odb_read_stream *,
> -			       struct repository *,
> -			       const struct object_id *,
> -			       enum object_type *);
>  typedef int (*close_istream_fn)(struct odb_read_stream *);
>  typedef ssize_t (*read_istream_fn)(struct odb_read_stream *, char *, size_t);
>  
> @@ -34,7 +30,6 @@ struct filtered_istream {
>  };
>  
>  struct odb_read_stream {
> -	open_istream_fn open;
>  	close_istream_fn close;
>  	read_istream_fn read;
>  
> @@ -437,21 +432,25 @@ static int istream_source(struct odb_read_stream *st,
>  
>  	switch (oi.whence) {
>  	case OI_LOOSE:
> -		st->open = open_istream_loose;
> +		if (open_istream_loose(st, r, oid, type) < 0)
> +			break;

Previously, if an error happened when executing the callback,
`open_istream_incore()` would be invoked as a fallback. Now we handle
that here during initialization by breaking early. This preserves the
original behavior. Makes sense. 

>  		return 0;
>  	case OI_PACKED:
> -		if (!oi.u.packed.is_delta &&
> -		    repo_settings_get_big_file_threshold(the_repository) < size) {
> -			st->u.in_pack.pack = oi.u.packed.pack;
> -			st->u.in_pack.pos = oi.u.packed.offset;
> -			st->open = open_istream_pack_non_delta;
> -			return 0;
> -		}
> -		/* fallthru */
> -	default:
> -		st->open = open_istream_incore;
> +		if (oi.u.packed.is_delta ||
> +		    repo_settings_get_big_file_threshold(the_repository) >= size)
> +			break;
> +
> +		st->u.in_pack.pack = oi.u.packed.pack;
> +		st->u.in_pack.pos = oi.u.packed.offset;
> +		if (open_istream_pack_non_delta(st, r, oid, type) < 0)
> +			break;
> +
>  		return 0;
> +	default:
> +		break;
>  	}
> +
> +	return open_istream_incore(st, r, oid, type);
>  }
>  
>  /****************************************************************
> @@ -478,19 +477,14 @@ struct odb_read_stream *open_istream(struct repository *r,
>  {
>  	struct odb_read_stream *st = xmalloc(sizeof(*st));
>  	const struct object_id *real = lookup_replace_object(r, oid);
> -	int ret = istream_source(st, r, real, type);
> +	int ret;
>  
> +	ret = istream_source(st, r, real, type);
>  	if (ret) {
>  		free(st);
>  		return NULL;
>  	}
>  
> -	if (st->open(st, r, real, type)) {
> -		if (open_istream_incore(st, r, real, type)) {
> -			free(st);
> -			return NULL;
> -		}
> -	}

Now that opening the read stream in handled during initialization, we
can drop the explicit call to the open callback.

-Justin