Stefan Fuhrmann <stefan.fuhrm...@wandisco.com> writes: > So, you are absolutely right about APR buffering being enough for our > non-interactive "read stdin as a single large file" usage in load & friends. > > My last concern is as follows. We need an alternative implementation > for svn_stream_for_stdin. I see 3 options: > > 1. svn_stream_for_stdin2 with buffered option. > 2. Always enable buffering in svn_stream_for_stdin. > 3. Some private API.
I very much prefer using the standard APR_BUFFERED buffering, as opposed to rolling our own buffering adapter. I also prefer the idea of exposing this flag through the new variant of the API, svn_stream_for_stdin2(), and using it in the three mentioned subcommands (option 1). I don't see an abstraction leak in this approach. Rather than that, exposing this option is a necessary thing. Only the caller knows if buffering makes sense at the particular moment, and that is, for instance, what we know when we enable it for commands like 'svnadmin load-revprops'. Enabling buffering when you're about to do something interactive is always a bad idea, but, again, only the caller knows these details. Furthermore, it is not that we invent something new and push it through the API, as APR_BUFFERED is a well-known and a quite stable concept. Regarding other two options: - We can't do 2), because doing so would induce a behavior change in all our command-line tools, and this is going to happen without any reason. As we know that buffering makes sense for three mentioned subcommands, I'd say that we should limit the scope to them. - I find 3) an unnecessary complication, as opposed to 1). As we are just exposing a standard APR concept through our API, I think that we can make it public. To sum everything up, I think that we should: - Replace the custom buffering with APR_BUFFERED, and expose it through the new svn_stream_for_stdin2() API. We could also place a @note in the API documentation saying that currently enabled buffering is equivalent to how the APR_BUFFERED flag behaves. - Enable buffering in three subcommands: svnadmin load, svnadmin load-revprops and in svnfsfs load-index. I could do that, if it works for you. Regards, Evgeny Kotkov