From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 21 Nov 2025 06:32:45 GMT
Subject: Re: [PATCH 05/18] streaming: allocate stream inside the backend-specific logic
Message-ID: <aSAHjQKO7R1TvPgj@pks.im>
In-Reply-To: <CAOLa=ZTF+xzhZv2yXp8L_URk8cjscycheD=Xgdxd=eRGtvpt2A@mail.gmail.com>

On Wed, Nov 19, 2025 at 02:11:40AM -0800, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > that is specific to backends in "streaming.c".
> >
> > Ideally though, the infrastructure would be reversed: we have a generic
> > `struct odb_read_stream` and some helper functions in "streaming.c",
> > whereas the backend-specific logic sits in the backend's subsystem
> > itself.
> >
> 
> Will this also mean that we move the backend specific functions like
> `open_istream_loose()` away from 'streaming.c'? Let's read on.

Yup, exactly.

> > This can be realized by using a design that is similar to how we handle
> > reference databases: instead of having a union of members, we instead
> > have backend-specific structures with a `struct odb_read_stream base`
> > as its first member. The backends would thus hand out the pointer to the
> > base, but internally they know to cast back to the backend-specific
> > type.
> >
> 
> Right.
> 
> > This means though that we need to allocate different structures
> > depending on the backend. To prepare for this, move allocation of the
> > structure into the backend-specific functions that open a new stream.
> > Subsequent commits will then create those new backend-specific structs.
> >
> 
> Who's in charge of free'ing these structs? I see that `close_istream()`
> calls the assigned `close()` function. So this could be handled on the
> backend level. But it also does `free(st)`.

Yeah, this'll be changed later: the `close()` callback will then only
close and release the backend-specific data. `odb_read_stream_close()`
is then responsible for freeing the stream itself.

> > @@ -338,12 +354,16 @@ static int close_istream_pack_non_delta(struct odb_read_stream *st)
> >  	return 0;
> >  }
> >
> > -static int open_istream_pack_non_delta(struct odb_read_stream *st,
> > +static int open_istream_pack_non_delta(struct odb_read_stream **out,
> >  				       struct repository *r UNUSED,
> >  				       const struct object_id *oid UNUSED,
> >  				       struct packed_git *pack,
> >  				       off_t offset)
> >  {
> > +	struct odb_read_stream stream = {
> > +		.close = close_istream_pack_non_delta,
> > +		.read = read_istream_pack_non_delta,
> > +	};
> 
> So this is now statically defined. Won't this cause an issue?

No, it doesn't, as we eventually copy the local stream weh ave here into
the allocated `out` pointer.

Patrick