Re: [PATCH 06/11] streaming: a new API to read from the object store
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 19, 2011, 01:52 UTC
- Message-ID
- <7vtycra23f.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <20110518080947.GE27482@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> 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.