Re: svn commit: r1680819 - /subversion/trunk/subversion/libsvn_fs_fs/revprops.c

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.

Like Evgeny argued in his first email in the thread,
http://svn.haxx.se/dev/archive-2015-05/0154.shtml

He then proposed a virtualized method 'svn_stream_flush()' which
solves the abstraction/virtualization issue.

But then you have to define abstract semantics for 'flush', which that
attempt didn't do well.

It just doesn't all seem to fit together, the idea of telling a
generic stream "you must ensure the result of this generic stream
processing is written to *a*/*the*/*which?* phyical disk".

For example, should a 'tee' stream ensure that *both* output streams
are flushed to disk? That's a rhetorical question: the point is there
is an semantic mismatch.

- Julian
Received on 2015-05-28 13:08:34 CEST