From: Patrick Steinhardt <ps@pks.im>
Date: Sun, 23 Nov 2025 18:59:30 GMT
Subject: Re: [PATCH v2 02/19] streaming: drop the `open()` callback function
Message-ID: <aSNZklOl98TYRTUg@pks.im>
In-Reply-To: <xmqqqztr45t5.fsf@gitster.g>

On Fri, Nov 21, 2025 at 10:08:22AM -0800, Junio C Hamano wrote:
> 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.

I actually mean the opposite, so exactly what you describe: why do we
store the `open()` callback in a member variable of the stream if it's
only ever called a single time, only, and is never called a second time
thereafter?

I'll rephrase this.

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

Will drop this line.

Thanks!

Patrick