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

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