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

[Julian Foad <ju...@wandisco.com>]

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

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

[Julian Foad <ju...@wandisco.com>]

Stefan Sperling wrote:
> 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.

Ah. I didn't notice that. I saw it was linked in to the stream in some
way and assumed it was a "method" of the stream, but I was concerned
that it was already sort of outside the scope of a generic stream and
didn't seem to have full support (e.g. of being replaceable) and it's
not clear exactly how it interacts with the plain read() function and I
would assume there should be a writeline() counterpart.

I'm not sure I'd want it to be a standard function of a generic stream
if it isn't already.

> > 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.

Right.

> > 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.

Hmm. Hold the thought for now. That doesn't sound ideal. I'd like to
"mull it over" in my mind for a few days.

- Julian

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

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

[Stefan Sperling <st...@elego.de>]

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)

Stefan

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