You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Näslund <da...@longitudo.com> on 2009/08/27 19:24:53 UTC
Re: [PATCH] v4. line_transformer callback
On Thu, Aug 27, 2009 at 07:29:03PM +0100, Stefan Sperling wrote:
> On Thu, Aug 27, 2009 at 06:51:28PM +0200, Daniel Näslund wrote:
I used your suggestions for the log message.
[[[
Create a new callback for svn_stream_readline() that can replace, add or
delete characters from a line.
* subversion/include/svn_io.h
(svn_io_line_transformer_cb_t): Declare
(svn_stream_set_line_transformer_callback): Declare
(svn_stream_readline): Document line transforming.
* subversion/libsvn_subr/stream.c
(svn_stream_t): Add new field 'transformer_cb'.
(svn_stream_create): Initialize the new field.
(svn_stream_set_line_transformer_callback): New function to set the
line transformer callback on a stream.
* subversion/tests/libsvn_subr/stream-tests.c
(line_transformer): An implementation of
svn_io_line_transformer_cb_t, reverses the supplied line.
(test_stream_line_transformer): New test, testing line
transformations on a stream.
(test_stream_line_filter_and_transformer): New test, testing line
transformations as well as line filtering on a stream.
]]]
> Please check stream->line_transformer_cb != NULL outside of
> line_transformer(). The code flow is then easier to understand
> in svn_stream_readline() itself. And you don't need to create
> the stringbuf inside here.
Done
I used your suggestions for doc strings.
Once again, thanks for all the time you're "investing" in me. Hopefully
a will gain speed and accuracy and in the end be a good investment. :-).
Mvh
Daniel
Re: line_filter and line_transformer callbacks [was: [PATCH] v4.
line_transformer callback]
Posted by 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]
Posted by 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
Re: [PATCH] v4. line_transformer callback
Posted by Blair Zajac <bl...@orcaware.com>.
Stefan Sperling wrote:
> On Thu, Aug 27, 2009 at 01:07:20PM -0700, Blair Zajac wrote:
>>> I thought because of no-space-before paren, this would read:
>>>
>>> void
>>> svn_stream_set_line_transformer_callback(
>>> svn_stream_t *stream,
>>> svn_io_line_transformer_cb_t line_transformer_cb)
>>>
>>> Or is this bike-sheddy?
>> It's bike-sheddy, but I'm following convention in the other header files. In
>> particular, look through subversion/include/svn_auth.h.
>
> Hmmm... I chose to put the parenthesis on the same line.
> We can still change it later if needed.
Well, I don't think we should have both when there's precedent for the other.
Blair
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2388017
Re: [PATCH] v4. line_transformer callback
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Aug 27, 2009 at 01:07:20PM -0700, Blair Zajac wrote:
> > I thought because of no-space-before paren, this would read:
> >
> > void
> > svn_stream_set_line_transformer_callback(
> > svn_stream_t *stream,
> > svn_io_line_transformer_cb_t line_transformer_cb)
> >
> > Or is this bike-sheddy?
>
> It's bike-sheddy, but I'm following convention in the other header files. In
> particular, look through subversion/include/svn_auth.h.
Hmmm... I chose to put the parenthesis on the same line.
We can still change it later if needed.
Thanks for pointing out the indentation issue, I wouldn't have noticed it.
Committed in r38973.
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2388012
line_filter and line_transformer callbacks [was: [PATCH] v4.
line_transformer callback]
Posted by 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: [PATCH] v4. line_transformer callback
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Aug 27, 2009 at 08:49:21PM +0100, Stefan Sperling wrote:
> }
> while (filtered);
> svn_pool_destroy(iterpool);
>
> if (stream->line_transformer_cb)
> SVN_ERR(line_transformer(stream, stringbuf, str->data, pool));
> else
> *stringbuf = svn_stringbuf_dup(str, pool);
>
> return SVN_NO_ERROR;
My change doesn't work, because str is allocated in iterpool :-/
The former order of things thus makes sense, but I'll put a huge
fat comment there for the next person to stumble over this.
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2388008
Re: [PATCH] v4. line_transformer callback
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Aug 27, 2009 at 08:37:46PM +0100, Stefan Sperling wrote:
> On Thu, Aug 27, 2009 at 09:24:53PM +0200, Daniel Näslund wrote:
> > if (filtered)
> > *stringbuf = svn_stringbuf_create_ensure(0, pool);
> > + else if (stream->line_transformer_cb)
> > + SVN_ERR(line_transformer(stream, stringbuf, str->data,
> > + iterpool));
>
> Passing iterpool here can cause problems, since the result will go
> to the caller.
Same problem further down the function:
> @@ -268,6 +295,9 @@
> while (filtered);
> *stringbuf = svn_stringbuf_dup(str, pool);
>
> + if (stream->line_transformer_cb)
> + SVN_ERR(line_transformer(stream, stringbuf, str->data, iterpool));
> +
> svn_pool_destroy(iterpool);
> return SVN_NO_ERROR;
> }
That won't work. Making a habit of creating iterpools just before the
loop is entered, and destroying then right after the loop is exited
can help to avoid such problems. (Note how the existing code got this
wrong.)
Also, we should not create a stringbuf here we don't use if the
transformer is active, so I've changed this to:
}
while (filtered);
svn_pool_destroy(iterpool);
if (stream->line_transformer_cb)
SVN_ERR(line_transformer(stream, stringbuf, str->data, pool));
else
*stringbuf = svn_stringbuf_dup(str, pool);
return SVN_NO_ERROR;
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2387997
Re: [PATCH] v4. line_transformer callback
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Aug 27, 2009 at 09:24:53PM +0200, Daniel Näslund wrote:
> if (filtered)
> *stringbuf = svn_stringbuf_create_ensure(0, pool);
> + else if (stream->line_transformer_cb)
> + SVN_ERR(line_transformer(stream, stringbuf, str->data,
> + iterpool));
Passing iterpool here can cause problems, since the result will go
to the caller.
I'll fix this at my end, and test this patch now. Unless more issues
pop up unexpectedly I'll commit it.
Stefan
> else
> *stringbuf = svn_stringbuf_dup(str, pool);
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2387994
Re: [PATCH] v4. line_transformer callback
Posted by Blair Zajac <bl...@orcaware.com>.
Stefan Sperling wrote:
> On Thu, Aug 27, 2009 at 01:00:18PM -0700, Blair Zajac wrote:
>>> It took me a second to see that line_transformer_cb is not a type
>>> but the variable name from the previous line. For long lines, we
>>> use something like this style:
>>>
>>> void
>>> svn_stream_set_line_transformer_callback
>>> (svn_stream_t *stream,
>>> svn_io_line_transformer_cb_t line_transformer_cb)
>> Oops, make that a two space indent:
>>
>> void
>> svn_stream_set_line_transformer_callback
>> (svn_stream_t *stream,
>> svn_io_line_transformer_cb_t line_transformer_cb)
>
> I thought because of no-space-before paren, this would read:
>
> void
> svn_stream_set_line_transformer_callback(
> svn_stream_t *stream,
> svn_io_line_transformer_cb_t line_transformer_cb)
>
> Or is this bike-sheddy?
It's bike-sheddy, but I'm following convention in the other header files. In
particular, look through subversion/include/svn_auth.h.
Blair
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2388004
Re: [PATCH] v4. line_transformer callback
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Aug 27, 2009 at 01:00:18PM -0700, Blair Zajac wrote:
> >It took me a second to see that line_transformer_cb is not a type
> >but the variable name from the previous line. For long lines, we
> >use something like this style:
> >
> >void
> >svn_stream_set_line_transformer_callback
> > (svn_stream_t *stream,
> > svn_io_line_transformer_cb_t line_transformer_cb)
>
> Oops, make that a two space indent:
>
> void
> svn_stream_set_line_transformer_callback
> (svn_stream_t *stream,
> svn_io_line_transformer_cb_t line_transformer_cb)
I thought because of no-space-before paren, this would read:
void
svn_stream_set_line_transformer_callback(
svn_stream_t *stream,
svn_io_line_transformer_cb_t line_transformer_cb)
Or is this bike-sheddy?
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2388003
Re: [PATCH] v4. line_transformer callback
Posted by Blair Zajac <bl...@orcaware.com>.
Hyrum K. Wright wrote:
> On Aug 27, 2009, at 10:00 PM, Blair Zajac wrote:
>
>> Blair Zajac wrote:
>>> Daniel Näslund wrote:
>>>> On Thu, Aug 27, 2009 at 07:29:03PM +0100, Stefan Sperling wrote:
>>>>> On Thu, Aug 27, 2009 at 06:51:28PM +0200, Daniel Näslund wrote:
>>>> I used your suggestions for the log message.
>>> Hi Daniel,
>>>
>>> Thanks for all your work on svn.
>>>
>>> Minor nit. When you have a long line like this:
>>>
>>> void
>>> svn_stream_set_line_transformer_callback(svn_stream_t *stream,
>>> svn_io_line_transformer_cb_t
>>> line_transformer_cb)
>>> {
>>> stream->line_transformer_cb = line_transformer_cb;
>>> }
>>>
>>> It took me a second to see that line_transformer_cb is not a type
>>> but the
>>> variable name from the previous line. For long lines, we use
>>> something like
>>> this style:
>>>
>>> void
>>> svn_stream_set_line_transformer_callback
>>> (svn_stream_t *stream,
>>> svn_io_line_transformer_cb_t line_transformer_cb)
>> Oops, make that a two space indent:
>>
>> void
>> svn_stream_set_line_transformer_callback
>> (svn_stream_t *stream,
>> svn_io_line_transformer_cb_t line_transformer_cb)
>
> I think no-space-before-paren disallows this. One convention I've
> seen in other parameter lists is:
>
> void
> svn_stream_set_line_transformer_callback(svn_stream_t *stream,
> svn_io_line_transformer_cb_t
>
> line_transformer_cb)
Is that line wrapped? I can't tell what the mailer did to this line. Do you
have a function in mind where you've seen this.
Blair
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2388019
Re: [PATCH] v4. line_transformer callback
Posted by "Hyrum K. Wright" <hy...@hyrumwright.org>.
On Aug 27, 2009, at 10:00 PM, Blair Zajac wrote:
> Blair Zajac wrote:
>> Daniel Näslund wrote:
>>> On Thu, Aug 27, 2009 at 07:29:03PM +0100, Stefan Sperling wrote:
>>>> On Thu, Aug 27, 2009 at 06:51:28PM +0200, Daniel Näslund wrote:
>>> I used your suggestions for the log message.
>>
>> Hi Daniel,
>>
>> Thanks for all your work on svn.
>>
>> Minor nit. When you have a long line like this:
>>
>> void
>> svn_stream_set_line_transformer_callback(svn_stream_t *stream,
>> svn_io_line_transformer_cb_t
>> line_transformer_cb)
>> {
>> stream->line_transformer_cb = line_transformer_cb;
>> }
>>
>> It took me a second to see that line_transformer_cb is not a type
>> but the
>> variable name from the previous line. For long lines, we use
>> something like
>> this style:
>>
>> void
>> svn_stream_set_line_transformer_callback
>> (svn_stream_t *stream,
>> svn_io_line_transformer_cb_t line_transformer_cb)
>
> Oops, make that a two space indent:
>
> void
> svn_stream_set_line_transformer_callback
> (svn_stream_t *stream,
> svn_io_line_transformer_cb_t line_transformer_cb)
I think no-space-before-paren disallows this. One convention I've
seen in other parameter lists is:
void
svn_stream_set_line_transformer_callback(svn_stream_t *stream,
svn_io_line_transformer_cb_t
line_transformer_cb)
{
stream->line_transformer_cb = line_transformer_cb;
}
It keeps the indentation of typenames, but also ensures we don't
overflow the line. In any case, I feel like a green[1] bikeshed today.
-Hyrum
[1] http://green.bikeshed.com/
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2388014
Re: [PATCH] v4. line_transformer callback
Posted by Blair Zajac <bl...@orcaware.com>.
Blair Zajac wrote:
> Daniel Näslund wrote:
>> On Thu, Aug 27, 2009 at 07:29:03PM +0100, Stefan Sperling wrote:
>>> On Thu, Aug 27, 2009 at 06:51:28PM +0200, Daniel Näslund wrote:
>> I used your suggestions for the log message.
>
> Hi Daniel,
>
> Thanks for all your work on svn.
>
> Minor nit. When you have a long line like this:
>
> void
> svn_stream_set_line_transformer_callback(svn_stream_t *stream,
> svn_io_line_transformer_cb_t
> line_transformer_cb)
> {
> stream->line_transformer_cb = line_transformer_cb;
> }
>
> It took me a second to see that line_transformer_cb is not a type but the
> variable name from the previous line. For long lines, we use something like
> this style:
>
> void
> svn_stream_set_line_transformer_callback
> (svn_stream_t *stream,
> svn_io_line_transformer_cb_t line_transformer_cb)
Oops, make that a two space indent:
void
svn_stream_set_line_transformer_callback
(svn_stream_t *stream,
svn_io_line_transformer_cb_t line_transformer_cb)
Blair
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2388001
Re: [PATCH] v4. line_transformer callback
Posted by Blair Zajac <bl...@orcaware.com>.
Daniel Näslund wrote:
> On Thu, Aug 27, 2009 at 07:29:03PM +0100, Stefan Sperling wrote:
>> On Thu, Aug 27, 2009 at 06:51:28PM +0200, Daniel Näslund wrote:
>
> I used your suggestions for the log message.
Hi Daniel,
Thanks for all your work on svn.
Minor nit. When you have a long line like this:
void
svn_stream_set_line_transformer_callback(svn_stream_t *stream,
svn_io_line_transformer_cb_t
line_transformer_cb)
{
stream->line_transformer_cb = line_transformer_cb;
}
It took me a second to see that line_transformer_cb is not a type but the
variable name from the previous line. For long lines, we use something like
this style:
void
svn_stream_set_line_transformer_callback
(svn_stream_t *stream,
svn_io_line_transformer_cb_t line_transformer_cb)
{
stream->line_transformer_cb = line_transformer_cb;
}
Regards,
Blair
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2388000