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

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.

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?)

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?

For reference, here is where the filter is actually used, in
svn_diff__parse_next_hunk():

> /* Create a stream which returns the hunk text itself. */
> SVN_ERR(svn_io_file_open(&f, patch->path, [...]));
> diff_text = svn_stream_from_aprfile_range_readonly(f, [...]);
>
> /* Create a stream which returns the original hunk text. */
> SVN_ERR(svn_io_file_open(&f, patch->path, [...]));
> original_text = svn_stream_from_aprfile_range_readonly(f, [...]);
> svn_stream_set_line_filter_callback(original_text, original_line_filter);
>
> /* Create a stream which returns the modified hunk text. */
> SVN_ERR(svn_io_file_open(&f, patch->path, [...]));
> modified_text = svn_stream_from_aprfile_range_readonly(f, [...]);
> svn_stream_set_line_filter_callback(modified_text, modified_line_filter);
>
> /* Set the hunk's texts. */
> (*hunk)->diff_text = diff_text;
> (*hunk)->original_text = original_text;
> (*hunk)->modified_text = modified_text;

- Julian

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2388193