Re: svn commit: r1700799 - in /subversion/trunk/subversion: include/svn_io.h libsvn_subr/stream.c svnadmin/svnadmin.c svnfsfs/load-index-cmd.c tests/libsvn_subr/stream-test.c

Stefan Fuhrmann <stefan.fuhrmann_at_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
Received on 2015-09-11 13:17:27 CEST