I (Julian Foad) wrote: > Just a couple of comments, from a quick read of the patch. > > +/** Flushes all available internal buffers in a generic @a stream. If > + * @sync is non-zero, it causes any buffered data to be written to the > + * underlying device (for example, to the disk for a file stream). > > I'm not sure about having the 'sync' flag. What does that mean for a > non-disk stream? Does the API really need to support two different > ways of flushing a generic stream? I feel probably not. If we need to > support two different ways of flushing a disk stream, could we achieve > that in a better way? Perhaps the code that opens the disk stream > should specify this option when opening the disk stream, rather than a > generic writer specifying this option when it calls 'flush'?
Closing a stream always implies first flushing its own buffers. This proposed svn_stream_flush() API supports flushing a generic stream's buffers at any (and multiple) points in time. That's potentially useful, but the present use case only requires us to sync a disk file's data to disk when closing it. That's a slightly different thing. I note that in the MSDN documentation that Evgeny linked to, the plain 'Stream' class has a plain 'Flush' method, while the 'FileStream' class has (in addition) a Flush(bool) method that supports a 'sync to disk' option. Another small point. Ivan mentioned at the beginning of this thread: > 3. It's unclear how FLUSH_TO_DISK flag should interact with DISOWN=TRUE. The current patch chooses one of the two possible behaviours for svn_stream_disown() -- it chooses to forward all flush() method calls to the 'disowned' stream, and only blocks the close() method. Nothing wrong with that; I'm just noting that one behaviour was chosen. - Julian
