[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] issue #3555: make svn_stream_readline() a stream method

From: Julian Foad <julian.foad_at_wandisco.com>
Date: Thu, 08 Jul 2010 12:39:22 +0100

(Oops, I forgot to trim 60 KB of quoted text in my previous reply :-( )

Would you mind writing a test. The thing that most needs testing, I
should think, is interleaving read(), readline(), mark() and seek().
Here's an idea for a test:

  Stream content = "Line one.\nLine two.\n"

  # Some points where we will mark and seek:
[[[<1>Line one.
<2>Line <3>two.<4>
<5>]]]

  # Sequentially read, mixing read() with readline(), and make marks.
  mark(<1>) # before any read or readline
  readline() == "Line one."
  mark(<2>) # after readline
  read(5 bytes) == "Line "
  mark(<3>) # after read
  read(4 bytes) == "two."
  mark(<4>) # after read at EOL
  readline() == ""
  mark(<5>) # after read after EOL
  readline() == EOF

  # Jump around with seeks, and check read() and readline().
  seek(<1>)
  readline() == "Line one."
  seek(<5>)
  readline() == EOF
  seek(<2>)
  read(5 bytes) == "Line "
  seek(<4>)
  readline() == ""
  seek(<3>)
  readline() == "two."

  # And then a test with one stream wrapped in another would be ...
  # useful, though harder to write.

Can you see a convenient way to keep the transformer/filter tests? It
seems a bit of a pity to delete them.

> +/* Invoke the READ_FN function of a stream to scan for an end-of-line
> + * indicatior, and return it in *EOL.

s/indicatior/indicator

> + * Use MARK_FN and SEEK_FN to seek in the stream.

Why should it seek in the stream? Ah, because it returns to the
original position. Let's say so:

  Set *EOL to the first end-of-line string found in the stream
  accessed through READ_FN, MARK_FN and SEEK_FN, whose stream baton
  is BATON. Leave the stream read position unchanged. Allocate
  *EOL statically; POOL is a scratch pool.

That also documents the other parameters.

> * Set *EOL to NULL if the stream runs out before an end-of-line indicator
> * is found. */
> static svn_error_t *
> -scan_eol(const char **eol, svn_stream_t *stream, apr_pool_t *pool)
> +scan_eol(void *baton,
> + svn_read_fn_t read_fn,
> + svn_io_mark_fn_t mark_fn,
> + svn_io_seek_fn_t seek_fn,
> + const char **eol,
> + apr_pool_t *pool)
> {

Would it make more sense for stream_create() to initialize the readline
member to point to the default stream_readline() function, instead of
writing "if (readline == NULL) X else Y" in two places? That would also
allow a custom stream impl to set the readline handler to NULL if
readline doesn't make sense, e.g. on a compress-while-reading stream.
Not sure if that's better, just asking.

> @@ -1225,11 +1196,14 [...]
> /**
> - * Similar to svn_stream_readline(). The line-terminator is detected
> - * automatically. If @a eol is not NULL, the detected line-terminator
> - * is returned in @a *eol. If EOF is reached and the stream does not
> - * end with a newline character, @a *eol will be NULL.
> + * Similar to svn_stream_readline(). If @a *eol is not NULL, it is used
> + * as the expected line terminator. If @a eol is NULL, the line-terminator
> + * is detected automatically. If @a *eol is NULL, the line-terminator is
> + * detected automatically and is returned in @a *eol.
> [...]
> */
> svn_error_t *
> svn_stream_readline_detect_eol([...]

Did you mean to change the API here to allow specifying the expected EOL
string? That seems an unnecessary change, and your impl. doesn't
support that on non-seekable streams, so it looks unintended:

> @@ -416,13 +359,35 @@ svn_stream_readline_detect_eol([...]
> [...]
> + /* EOL-detection requires mark/seek support. */
> + if (stream->mark_fn == NULL)
> + return svn_error_create(SVN_ERR_STREAM_SEEK_NOT_SUPPORTED, NULL, NULL);
> + if (stream->seek_fn == NULL)
> + return svn_error_create(SVN_ERR_STREAM_SEEK_NOT_SUPPORTED, NULL, NULL);

The empty stream impl looks wrong: who says readline_handler_empty()
should return APR_EOL_STR as the "detected" EOL string? If that's
proper behaviour for svn_io_readline_fn_t, it needs to be documented in
svn_io_readline_fn_t.

The forwarding in the several filter streams looks wrong:

> @@ -652,6 +639,7 @@ svn_stream_disown(svn_stream_t *stream, apr_pool_t
> svn_stream_set_reset(s, reset_handler_disown);
> svn_stream_set_mark(s, mark_handler_disown);
> svn_stream_set_seek(s, seek_handler_disown);
> + svn_stream_set_readline(s, stream_readline);

It should forward to the wrapped stream's readline function, not the
default readline, shouldn't it? (Several similar places.)

I've only skimmed through it so far, and probably can't do a full review
before the end of the month. However from what I've seen it looks good
enough to go ahead and then improve/fix later.

- Julian

I (Julian Foad) wrote:
> Hi Stefan. I've just started looking at this. Overall impression is it
> looks like you've done it very well - thank you!
>
> Could you possibly get the readline_detect_eol() API changes out of this
> patch, and make them in a separate patch either before or after this?
> Or if that change belongs in this patch, please explain why in the log
> msg.
>
> Thanks.
> - Julian
>
>
> On Wed, 2010-07-07 at 20:39 +0200, Stefan Sperling wrote:
[...]
Received on 2010-07-08 13:41:07 CEST

This is an archived mail posted to the Subversion Dev mailing list.