Re: [PATCH v2 02/19] streaming: drop the `open()` callback function
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 21, 2025, 18:08 UTC
- Message-ID
- <xmqqqztr45t5.fsf@gitster.g>
- In-Reply-To
- <20251121-b4-pks-odb-read-stream-v2-2-ca8534963150@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 6 quoted lines
> 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.
Show 12 quoted lines
> @@ -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?