Re: svn commit: rev 2186 - trunk/subversion/svnadmin

sussman@tigris.org wrote:

>Author: sussman
>Date: 2002-06-13 17:22 GMT
>New Revision: 2186
>
>Modified:
> trunk/subversion/svnadmin/main.c
>Log:
>
>Brane's solution to make fs dump/load work on Win32: create stdio
>streams that avoid the CRT. (Factorized by sussman.)
>
[snip]

>+/* NOTE: this is a temporary fix.
>+
>+ This function *should* simply be calling svn_stream_from_stdio(),
>+ which wraps a stream a standard stdio.h FILE pointer. The problem
>+ is that these pointers operate through Common Run Time (CRT) on
>+ Win32, which does all sorts of translation on them: LF's become
>+ CRLF's, and ctrl-Z's embedded in Word documents are interpreted as
>+ premature EOF's. The Right Solution is to have APR open files in
>+ 'binary' mode, but it doesn't honor that flag yet.
>+
>+ The short-term solution below calls apr_file_open_std*, which
>+ bypass the CRT and directly wrap the OS's file-handles, which don't
>+ know or care about translation. Thus dump/load works correctly on
>+ Win32.
>+ */
>
This comment is totally bogus. *Why* would this not be the correct
solution? It is absolutely kosher for us to use APR files instead of
STDIO files. Also, I don't see how you expect APR to open STDIO files in
_any_ mode -- after all, APR is not a wrapper around STDIO.

The issue about APR_BINARY being ignored is totally orthogonal and has
nothing to do with this problem, as I've pointed out elsewhere.

>+static svn_error_t *
>+create_stdio_stream (svn_stream_t **stream,
>+ int io_kind,
>+ apr_pool_t *pool)
>+{
>+ apr_file_t *stdio_file;
>+ apr_status_t apr_err;
>+
>+ switch (io_kind)
>+ {
>+ case svnadmin_stdout:
>+ apr_err = apr_file_open_stdout (&stdio_file, pool);
>+ break;
>
How about something simpler instead:

static svn_error_t *
create_stdio_stream (svn_stream_t **stream,
                      apr_status_t open_fn (apr_file_t **, apr_pool_t *),
                      apr_pool_t *pool)
{
   apr_file_t *stdio_file;
   apr_status_t apr_err = open_fn (&stdio_file, pool);
   if (apr_err)
     etc. ...
}

And later:

     INT_ERR (create_stdio_stream (&stdout_stream, apr_file_open_stdout, pool));

Then you can lose the switch, and the enum, and make the code vastly
more readable.

[snip]

>-
>- fflush(stdout);
>
Are you sure we don't need the flush?
(I don't know, it might be safer to keep it. If we do, it might make
sense to add a flush handler to the svn streams.)

-- 
Brane Čibej   <brane_at_xbc.nu>   http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org