From: Karthik Nayak Date: Wed, 19 Nov 2025 09:39:05 GMT Subject: Re: [PATCH 02/18] streaming: drop the `open()` callback function Message-ID: In-Reply-To: <20251119-b4-pks-odb-read-stream-v1-2-adacf03c2ccf@pks.im> Patrick Steinhardt writes: > diff --git a/streaming.c b/streaming.c > index 1fb4b7c1c0..5ce6350123 100644 > --- a/streaming.c > +++ b/streaming.c > @@ -14,10 +14,6 @@ > #include "replace-object.h" > #include "packfile.h" > > -typedef int (*open_istream_fn)(struct odb_read_stream *, > - struct repository *, > - const struct object_id *, > - enum object_type *); > typedef int (*close_istream_fn)(struct odb_read_stream *); > typedef ssize_t (*read_istream_fn)(struct odb_read_stream *, char *, size_t); > > @@ -34,7 +30,6 @@ struct filtered_istream { > }; > > struct odb_read_stream { > - open_istream_fn open; > close_istream_fn close; > read_istream_fn read; > > @@ -437,21 +432,25 @@ static int istream_source(struct odb_read_stream *st, > > switch (oi.whence) { > case OI_LOOSE: > - st->open = open_istream_loose; > + if (open_istream_loose(st, r, oid, type) < 0) > + break; Earlier we were checking for `if (st->open(st, r, real, type))` so there is a slight change in behavior here. But both `open_istream_loose()` and `open_istream_pack_non_delta()` return either -1 or 0. So this is okay. > return 0; > case OI_PACKED: > - if (!oi.u.packed.is_delta && > - repo_settings_get_big_file_threshold(the_repository) < size) { > - st->u.in_pack.pack = oi.u.packed.pack; > - st->u.in_pack.pos = oi.u.packed.offset; > - st->open = open_istream_pack_non_delta; > - return 0; > - } > - /* fallthru */ > - default: > - st->open = open_istream_incore; > + if (oi.u.packed.is_delta || > + repo_settings_get_big_file_threshold(the_repository) >= size) > + break; > + So we switch the branch flow to break the switch early. Makes sense. The patch looks good. [snip]