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

Re: line_filter and line_transformer callbacks [was: [PATCH] v4. line_transformer callback]

From: Stefan Sperling <stsp_at_elego.de>
Date: Fri, 28 Aug 2009 11:23:33 +0100

On Fri, Aug 28, 2009 at 10:29:25AM +0100, Julian Foad wrote:
> Hi Daniel and others.
> We have recently added a "line filter" callback and a "line transformer"
> callback to svn_stream_t's "readline" function. I can see they're
> useful, but now I'm wondering whether they are the right approach to
> extending a stream's functionality.

I did the line filter originally, and the transformer was modeled after
it. Back when I added the line filter I saw svn_stream_readline() as a
separate utility function for streams, since the stream has no function
pointer to it. The way I saw it was that it's just a helper. But see below.

> The idea of a generic stream is that the API is generic and a particular
> stream implementation provides any desired functionality. The "readline"
> function is a concession to how often streams are used for plain text,
> but in most respects special data handling is done by creating a wrapper
> stream which does the special handling during reads and/or writes and
> passes all the other calls straight through to the wrapped stream.
> My stream of thoughts since the new callbacks were added has gone like
> this.
> First: All callbacks should take a baton. Patch attached. (Shall I just
> commit this?)

Possibly, though right now, the readline function is not part of the
stream itself, so passing the stream baton into it feels a bit weird.

> Second: These two callbacks do a very similar job: optionally modify the
> line, and optionally delete it entirely. I don't think it makes much
> sense for them to be separate. We should just have one callback that can
> modify or delete the line as it wishes.

> Third: If svn_stream_readline() can pass each line through a generic
> filter callback, that's pretty similar to just having replaced the
> "readline" function with a different "readline" function, in the same
> way that svn_stream_set_read() replaces the stream's basic "read"
> function with a different "read" function. Should we not use the same
> approach here, and provide svn_stream_set_readline() instead of
> svn_stream_set_line_filter_callback()?
> So, ultimately, I'm thinking that this filtering/modifying should be
> done by a wrapper stream.
> Does this all make sense?

Yes, it might make sense now to promote the readline function from a helper
to a proper stream method. However with the current mechanism we don't
duplicate the "read a line" logic anywhere, and I'd like to keep it that
way. So if we make streams define their own readline() method, we should
do it in a way that avoids duplicating code, by factoring out the
"read a line" functionality into a static helper function which can be
used by svn_stream_readline() implementations.

The standard stream's readline method would just call this helper.

Then, we implement two new types of streams:

- A stream to read original text from a patch file, overriding the
  readline method.
- A stream to read modified text from a patch file, overriding the
  readline method.

A stream to read latest text from a file already exists :)

And then we use those in the unit tests and patch code instead of
the filter/transformer callbacks.

I guess this can be made to work nicely.
So +1 on the approach, someone just has to do it.
(/me looks at dannas for a blink of a second)


Received on 2009-08-28 12:24:17 CEST

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

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.