Re: [PATCH 06/11] streaming: a new API to read from the object store

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> I assume the "sz" parameter is meant to be an output parameter with the
> total size of the object. The open_istream_incore function fills it in
> properly. But later,...
> may or may not have "size" meaningful at this point, which seems like a
> bug.

Thanks for spotting.

I would want to revamp the API implementation a bit further.

The "u.*.sz" fields should probably become the first level field in
"struct git_istream" [*1*].

The open_istream_$backend() methods should lose "sz" parameter, as it
should be the same as what would be stored in st->size after the above
change. The public API open_istream() would fill the caller supplied *sz
word with what is stored in st->sz by the backend method.

I do not think the "read" method implementations (other than "incore")
currently make sure that what comes out of the z_stream really has the
size. A check probably needs to be added somewhere in the codepath.

I haven't looked at the revindex issue yet.


[Footnote]

*1* Actually I am a bit torn on this. As I am envisioning that we would
later (much later) reuse this API to implement "streaming filters" by
adding a new struct "filter" as a member of the st->u union, and such a
filter would not have "size" known in advance, I am a bit reluctant to
change it to have sz in the common part of istream. It can be argued that
it may not be a big deal to move "size" to the generic part and have only
certain backends look at it while allowing other backends to ignore it,
just like "z" and "z_state" are unnecessary for "incore" but they are
shared among three backends already.