On 28 May 2015 at 15:03, Branko Čibej <brane_at_wandisco.com> wrote:
> On 28.05.2015 13:07, Julian Foad wrote:
>> Philip Martin wrote:
>>> I still prefer the stream patch I posted earlier, and it can be extended
>>> to support compressed streams. Something like:
>>>
>>> svn_error_t *
>>> svn_stream_flush_to_disk_on_close(svn_stream_t *stream)
>>> {
>>> if (stream->close_fn == close_handler_apr)
>>> {
>>> svn_stream_set_close(stream, close_handler_flush);
>>> }
>>> else if (stream->close_fn == close_handler_gz)
>>> {
>>> struct zbaton *zbaton = stream->baton;
>>> SVN_ERR(svn_stream_flush_to_disk_on_close(zbaton->substream));
>>> }
>>> else [...]
>>> }
>>>
>>> That only allows flushing the stream on close but I do not see any need
>>> at present to support flushing at arbitrary positions.
>> The point about the generic stream API is you should always be able to
>> define a new stream class that wraps a stream (examples: a 'tee'
>> stream wraps one stream while copying to another; a checksumming
>> stream, etc.).
>>
>> And you should always be able to use the wrapping stream in place of
>> the original stream.
>>
>> The 'svn_stream_flush_to_disk_on_close()' that you suggest breaks that.
>>
>> The implementation you suggest in your email an hour ago needs direct
>> access to the implementation methods of all the stream classes that it
>> may possibly encounter (close_handler_gz, for example).
>>
>> And functionality supported by streams should be provided as a virtual
>> method, overridden in each stream class.
>
> I still think that we should do this on the level of stream
> implementations, not on the level of the abstract stream API.
>
> In the case we're talking about here, that would mean revising the
> svn_stream_from_aprfile function and adding a flush-on-close flag, or
> adding a similar function that has the flush-on-close semantics. That
> would leave the generic stream API completely unchanged and only affect
> the properties of the stream itself, at the point where it's created.
I still think that flush should happen explicit when neded: it should
not happen as flag when file/stream is opened. Someone who opens
file/stream may doesn't know whether flush will be needed or not.
> Nicely encapsulated, and far better than replacing existing stream-based
> code with messy file-based code.
First of all current FSFS code uses apr_file_t in many places for
different reasons: to control offset in file, to control buffering
etc. See pack_log_addressed() in libsvn_fsfs\pack.c for example.
Also I don't understand what do you mean "messy file-based code"? Imho
code using svn_stream_write() that require pointer to length is more
messy.
--
Ivan Zhakov
Received on 2015-05-28 14:18:22 CEST