Re: [PATCH v2 02/19] streaming: drop the `open()` callback function

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> 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.

It is unclear what are left for delayed initialization from this
description.

>   - We never use the `open()` callback after having opened it initially.

I was not sure what this means in v1 and it still is not clear to
me.  Naively the above reads as if it is somehow desirable if we can
call open() after we have already called it on an object.  The flow
being a caller (e.g., stream_blob_to_fd()) first ask open_istream(),
which calls the open method after figuring out which backend knows
about the object and how to open a stream on it, I am not sure what
you want your second and subsequent uses of the open() calklbacks
do.  Puzzled.

> 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.

Because I do not think these open methods specific to each storage
mechanism cascades into each other, open-coding the logic to
dispatch into these open() methods in istream_source() itself,
instead of setting the method there and then have the caller call
it, is a perfectly fine simplification, I think.

> @@ -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;
>  	}

A patch noise?