Evgeny Kotkov wrote on Thu, 27 Jul 2017 18:50 +0300:
> Daniel Shahaf <d.s_at_daniel.shahaf.name> writes:
>
> >> +static svn_error_t *
> >> +svndiff_stream_write_fn(void *baton, const char *data, apr_size_t *len)
> >> +{
> >> + svndiff_stream_baton_t *b = baton;
> >> +
> >> + svn_stringbuf_appendbytes(b->window_buffer, data, *len);
> >> +
> >> + return SVN_NO_ERROR;
> >> +}
> >
> > Isn't this using O(filesize in bytes) memory? File content streams are
> > supposed to be O(1) memory.
>
> [...]
>
> >> +/** Return a readable generic stream which will produce svndiff-encoded
> >> + * text delta from the delta stream @a txstream. @a svndiff_version and
> >> + * @a compression_level are same as in svn_txdelta_to_svndiff3().
> >> + *
> >
> > Why is a write handler implemented, when the docstring doesn't promise
> > the returned stream would be writable?
>
> Thanks for the review.
>
> Could it be that you misinterpreted how the svn_txdelta_to_svndiff_stream()
> function works?
>
> Its implementation creates two streams, the writable push_stream, and the
> readable pull_stream. The writable stream is used to accumulate svndiff-
> encoded data in the shared buffer. The readable stream is returned to the
> caller, and reads it data from the same shared buffer.
I did misunderstand the relation between the two streams, …
> The internal buffer doesn't grow beyond the (header size + max window
> size in svndiff format), which is O(1).
… but I still don't see why this statement is true. The stream is
written to by [a callback provided by] svn_txdelta_to_svndiff3(), and
that function does not promise to write into the stream one svndiff
window per call. The API contract permits it to write into the stream
more than one svndiff window per call, in which case the buffer's size
will not be O(1).
I'm not sure what change to suggest. Maybe the function already does in
fact write one svndiff window per call, in which case making that an API
promise would be a valid (and simple) possible fix. Maybe the spillbuf
API is needed here as a buffer.
> As a side note, this function _could be_ implemented with just a single
> readable stream with an internal buffer that would be manually emitting
> the header, encoding delta windows and copying them to buffer supplied by
> the caller. However, I don't see why we'd want to have such implementation
> in favor of the existing one, at this time.
I'm just surprised that 17 years into the project, we don't _already_
have an API for pushing a txdelta into an svn_stream_t.
Thanks,
Daniel
Received on 2017-07-28 13:44:23 CEST