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/05 20:46:44 UTC
issue 3459 - svn patch does not tolerate empty lines
Hi!
[Sorry for not putting the shell script as an attachment. Could not
create a new thread with mutt.]
I'm getting a conflict when using svn patch with the shell script
below. According to the issue all lines after the empty line should be
ignored. Did I do something wrong?
My explanation to the behavior is that svn patch in
subversion/libsvn_client/patch.c (match_hunk) only compares hunk lines
that begin with '-' or ' '. I must add a ' ' if the hunk line does not
begin with one.
#!/bin/sh
# Tries to apply a diff where leading space on one line is dropped.
BASE_DIR=~/Code/Subversion/trunk
SVN=${BASE_DIR}/subversion/svn/svn
SVNADMIN=${BASE_DIR}/subversion/svnadmin/svnadmin
URL=file:///`pwd`/repos
rm -rf repos wc* import-me
${SVNADMIN} create repos
mkdir import-me;
cd import-me;
cat >> foo << EOF
bar
baz
EOF
${SVN} import -q -m "" ${URL}/trunk
cd ..;
${SVN} co ${URL}/trunk wc -q
cd wc
cat > foo << EOF
foo
EOF
${SVN} di > p
${SVN} revert foo;
# Remove the space from the empty line in the diff
cat p | sed -e "s/^[ ]$//" > p2
${SVN} patch p2
cat foo
My first ideas briefly (not implemented):
* subversion/libsvn_diff/parse_diff.c
(svn_diff__parse_next_hunk): Allow empty lines to be part of hunk.
Should use some library function.
* subversion/libsvn_client/patch.c
(apply_one_hunk): Allow for applying diffs with fuzz. Issue 3460.
* subversion/libsvn_client/patch.c
(match_hunk): Right now it demands that hunk lines begin with ' ' or
'-' and that the hunk line is one character longer than the target line.
That must be fixed.
Am I on the right track?
My idea for allowing empty lines:
Index: subversion/libsvn_diff/parse-diff.c
===================================================================
--- subversion/libsvn_diff/parse-diff.c (revision 38568)
+++ subversion/libsvn_diff/parse-diff.c (arbetskopia)
@@ -358,7 +358,10 @@
}
c = line->data[0];
- if (c == ' ' || c == '-' || c == '+')
+ if (c == ' ' || c == '-' || c == '+'
+ || (line->len == 1 && c == '\n')
+ || (line->len == 1 && c == '\r')
+ || (line->len == 2 && c == '\r' && line->data[1] == '\n'))
hunk_seen = TRUE;
else
Mvh
Daniel
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2380627
Re: [PATCH] v2. char_filter_callback for streams (or
line_transformer)
Posted by Julian Foad <ju...@btopenworld.com>.
Julian Foad wrote:
> Daniel Näslund wrote:
> > [[[
> > Creata a new callback for svn_stream_readline() that can replace, add or
> > delete characters from a line.
> >
> > * subversion/include/svn_io.h
> > (svn_stream_set_line_transformer_callback): Declare
> >
> > * subversion/libsvn_subr/stream.c
> > (svn_stream_set_line_transformer_callback): Sets the new callback
> >
> > * subversion/tests/libsvn_subr/stream-tests.c
> > (line_transformer): A callback of the new function. Removes leading
> > '!' characters. Does not allocate a new line.
> > (test_stream_line_transformer): tests the above callback.
> > ]]]
One more thing. You need to define and state whether the transform is
applied before or after the filter, and implement it accordingly. In
this v2 implementation, you have a mixture of both: you filter first,
but then go on to call line_transformer() on the resulting stringbuf
which may be empty as a result of the filter.
It's great that you already included a test for the transformer. It
would be really good if you could add a test for transforming and
filtering at the same time, as that seems to be an areas where there is
a lot of room for confusion and bugs.
Thanks.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2387899
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
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
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
Re: [PATCH] v4. line_transformer callback
Posted by Daniel Näslund <da...@longitudo.com>.
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: [PATCH] v3. line_transformer callback
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Aug 27, 2009 at 06:51:28PM +0200, Daniel Näslund wrote:
> [[[
> Create a new callback for svn_stream_readline() that can replace, add or
> delete characters from a line.
Comments inline. I'm mainly suggesting changes for the way some things
are worded in the log message and in the docstrings, but that's all
just the way *I* would say it -- there are many ways to say things :)
There is only one more code change which I've suggested below.
Please digest this input to your liking and re-send, I will then
test and commit it.
>
> * subversion/include/svn_io.h
> (svn_io_line_transformer_cb_t): Declare
> (svn_stream_set_line_transformer_callback): Declare
> (svn_stream_readline): Changed doc string.
I'd say something like "Document line transforming in docstring" here.
> * subversion/libsvn_subr/stream.c
You forgot:
(svn_stream_t): Add new field 'transformer_cb'.
> (svn_stream_create): Set transformer_cb to NULL.
"Initialise the new field."
> (svn_stream_set_line_transformer_callback): Sets the new callback.
"New function to set the line transformer callback on a stream."
> * subversion/tests/libsvn_subr/stream-tests.c
> (line_transformer): A callback of the new function. Swaps the order of
> the supplied line.
"An implementation of svn_io_line_transformer_cb_t, reverses the
supplied line."
> (test_stream_line_transformer): tests the line_transformer callback.
"New test testing line transformations on a stream".
> (test_stream_line_filter_and_transformer): tests both line_transformer
> and line_filter callbacks.
"New test testing line transformations as well as line filtering on a stream".
> ]]]
> Index: subversion/libsvn_subr/stream.c
> ===================================================================
> --- subversion/libsvn_subr/stream.c (revision 38964)
> +++ subversion/libsvn_subr/stream.c (arbetskopia)
> @@ -51,6 +51,7 @@
> svn_close_fn_t close_fn;
> svn_io_reset_fn_t reset_fn;
> svn_io_line_filter_cb_t line_filter_cb;
> + svn_io_line_transformer_cb_t line_transformer_cb;
> };
>
>
> @@ -69,6 +70,7 @@
> stream->reset_fn = NULL;
> stream->close_fn = NULL;
> stream->line_filter_cb = NULL;
> + stream->line_transformer_cb = NULL;
> return stream;
> }
>
> @@ -112,6 +114,14 @@
> stream->line_filter_cb = line_filter_cb;
> }
>
> +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;
> +}
> +
> svn_error_t *
> svn_stream_read(svn_stream_t *stream, char *buffer, apr_size_t *len)
> {
> @@ -207,6 +217,25 @@
> return SVN_NO_ERROR;
> }
>
> +static svn_error_t *
> +line_transformer(svn_stream_t *stream, svn_stringbuf_t **buf,
> + const char *line, apr_pool_t *pool)
> +{
> + apr_pool_t *scratch_pool;
> +
> + if (! stream->line_transformer_cb)
> + {
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.
> + *buf = svn_stringbuf_create(line, pool);
> + return SVN_NO_ERROR;
> + }
> +
> + scratch_pool = svn_pool_create(pool);
> + SVN_ERR(stream->line_transformer_cb(buf, line, pool, scratch_pool));
> + svn_pool_destroy(scratch_pool);
> +
> + return SVN_NO_ERROR;
> +}
> +
> svn_error_t *
> svn_stream_readline(svn_stream_t *stream,
> svn_stringbuf_t **stringbuf,
> @@ -244,10 +273,12 @@
> *eof = TRUE;
>
> SVN_ERR(line_filter(stream, &filtered, str->data, iterpool));
> +
> if (filtered)
> *stringbuf = svn_stringbuf_create_ensure(0, pool);
> - else
> - *stringbuf = svn_stringbuf_dup(str, pool);
> + else
> + SVN_ERR(line_transformer(stream, stringbuf, str->data,
> + iterpool));
So here, something like:
if (filtered)
*stringbuf = svn_stringbuf_create_ensure(0, pool);
else if (stream->line_transformer_cb)
SVN_ERR(line_transformer(stream, stringbuf, str->data, iterpool));
else
*stringbuf = svn_stringbuf_dup(str, pool);
and drop the non-transformed stringbuf creating logic inside of
line_transformer().
>
> return SVN_NO_ERROR;
> }
> @@ -268,6 +299,8 @@
> while (filtered);
> *stringbuf = svn_stringbuf_dup(str, pool);
>
> + SVN_ERR(line_transformer(stream, stringbuf, str->data, iterpool));
> +
> svn_pool_destroy(iterpool);
> return SVN_NO_ERROR;
> }
> Index: subversion/tests/libsvn_subr/stream-test.c
> ===================================================================
> --- subversion/tests/libsvn_subr/stream-test.c (revision 38964)
> +++ subversion/tests/libsvn_subr/stream-test.c (arbetskopia)
> @@ -345,6 +345,105 @@
> return SVN_NO_ERROR;
> }
>
> +/* An implementation of svn_io_line_transformer_cb_t */
> +static svn_error_t *
> +line_transformer(svn_stringbuf_t **buf, const char *line,
> + apr_pool_t *result_pool, apr_pool_t *scratch_pool)
> +{
> + int i, len = strlen(line);
> + char *temp = apr_palloc(scratch_pool, len + 1 );
> +
> + for (i = 0; i < len; i++)
> + {
> + temp[i] = line[len - 1 - i];
> + }
> +
> + temp[len] = '\0';
> +
> + *buf = svn_stringbuf_create(temp, result_pool);
> +
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> +test_stream_line_transformer(apr_pool_t *pool)
> +{
> + static const char *lines[4] = {"gamma", "",
> + "iota", "!"};
> +
> + static const char *inv_lines[4] = {"ammag", "",
> + "atoi", "!"};
> + svn_string_t *string;
> + svn_stream_t *stream;
> + svn_stringbuf_t *line;
> + svn_boolean_t eof;
> +
> + string = svn_string_createf(pool, "%s\n%s\n%s\n%s", lines[0], lines[1],
> + lines[2], lines[3]);
> +
> + stream = svn_stream_from_string(string, pool);
> +
> + svn_stream_set_line_transformer_callback(stream, line_transformer);
> +
> + svn_stream_readline(stream, &line, "\n", &eof, pool);
> + SVN_ERR_ASSERT(strcmp(line->data, inv_lines[0]) == 0);
> +
> + svn_stream_readline(stream, &line, "\n", &eof, pool);
> + SVN_ERR_ASSERT(strcmp(line->data, inv_lines[1]) == 0);
> +
> + svn_stream_readline(stream, &line, "\n", &eof, pool);
> + SVN_ERR_ASSERT(strcmp(line->data, inv_lines[2]) == 0);
> +
> + svn_stream_readline(stream, &line, "\n", &eof, pool);
> + SVN_ERR_ASSERT(strcmp(line->data, inv_lines[3]) == 0);
> +
> + /* We should have reached eof and the stringbuf should be emtpy. */
> + svn_stream_readline(stream, &line, "\n", &eof, pool);
> + SVN_ERR_ASSERT(eof && svn_stringbuf_isempty(line));
> +
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> +test_stream_line_filter_and_transformer(apr_pool_t *pool)
> +{
> + static const char *lines[4] = {"!gamma", "",
> + "iota", "!"};
> +
> + static const char *inv_lines[4] = {"ammag", "",
> + "atoi", "!"};
> + svn_string_t *string;
> + svn_stream_t *stream;
> + svn_stringbuf_t *line;
> + svn_boolean_t eof;
> +
> + string = svn_string_createf(pool, "%s\n%s\n%s\n%s", lines[0], lines[1],
> + lines[2], lines[3]);
> +
> + stream = svn_stream_from_string(string, pool);
> +
> + svn_stream_set_line_filter_callback(stream, line_filter);
> +
> + svn_stream_set_line_transformer_callback(stream, line_transformer);
> +
> + /* Line one should be filtered. */
> + svn_stream_readline(stream, &line, "\n", &eof, pool);
> + SVN_ERR_ASSERT(strcmp(line->data, inv_lines[1]) == 0);
> +
> + svn_stream_readline(stream, &line, "\n", &eof, pool);
> + SVN_ERR_ASSERT(strcmp(line->data, inv_lines[2]) == 0);
> +
> + /* The last line should also be filtered, and the resulting
> + * stringbuf should be empty. */
> + svn_stream_readline(stream, &line, "\n", &eof, pool);
> + SVN_ERR_ASSERT(eof && svn_stringbuf_isempty(line));
> +
> + return SVN_NO_ERROR;
> +
> +}
> +
> +
> +
>
> /* The test table. */
>
> @@ -359,5 +458,9 @@
> "test streams reading from range of file"),
> SVN_TEST_PASS2(test_stream_line_filter,
> "test stream line filtering"),
> + SVN_TEST_PASS2(test_stream_line_transformer,
> + "test stream line transforming"),
> + SVN_TEST_PASS2(test_stream_line_filter_and_transformer,
> + "test stream line filtering and transforming"),
> SVN_TEST_NULL
> };
> Index: subversion/include/svn_io.h
> ===================================================================
> --- subversion/include/svn_io.h (revision 38964)
> +++ subversion/include/svn_io.h (arbetskopia)
> @@ -705,6 +705,20 @@
> const char *line,
> apr_pool_t *scratch_pool);
>
> +/** Character-filtering callback function for a generic
> + * stream.
"A callback function, invoked by svn_stream_readline(), which can perform
arbitrary transformations on the line before it is passed back to the
caller of svn_stream_readline()."
> + * Returns a transformed @a buf allocated in @a result_pool.
"Returns a transformed stringbuf in @a buf, allocated in @a result_pool."
> + * This function gets called on the lines that was not filtered by the
> + * line-filtering callback function.
"This callback gets invoked only on lines which were not filtered by the
line-filtering callback function."
> + *
> + * @see svn_stream_t and svn_stream_readline().
"@see svn_stream_t, svn_io_line_filter_cb_t, and svn_stream_readline()."
> + *
> + * @since New in 1.7. */
> +typedef svn_error_t *(*svn_io_line_transformer_cb_t)(svn_stringbuf_t **buf,
> + const char *line,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool);
> +
> /** Create a generic stream. @see svn_stream_t. */
> svn_stream_t *
> svn_stream_create(void *baton,
> @@ -742,6 +756,14 @@
> svn_stream_set_line_filter_callback(svn_stream_t *stream,
> svn_io_line_filter_cb_t line_filter_cb);
>
> +/** Set @a streams's line-transforming callback function to @a
> + * line_transformer_cb
"@a line_transformer_cb" (both on the same line).
> + * @since New in 1.7. */
> +void
> +svn_stream_set_line_transformer_callback(svn_stream_t *stream,
> + svn_io_line_transformer_cb_t
> + line_transformer_cb);
> +
> /** Create a stream that is empty for reading and infinite for writing. */
> svn_stream_t *
> svn_stream_empty(apr_pool_t *pool);
> @@ -1030,6 +1052,13 @@
> * if they pass the filtering decision of the callback function.
> * If end-of-file is encountered while reading the line and the line
> * is filtered, @a *stringbuf will be empty.
> + *
> + * If a line-transformer callback function was set on the stream using
> + * svn_stream_set_line_transformer_callback(), lines will be returned
> + * transformed in a way determined by the callback.
I'd put a comma after "transformed" to make this sentence easier to
parse.
> + *
> + * Note that the line-transformer callback gets called after the line-filter
> + * callback.
", not before."
> */
> svn_error_t *
> svn_stream_readline(svn_stream_t *stream,
Thanks, looks very good!
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2387976
Re: [PATCH] v3. line_transformer callback
Posted by Daniel Näslund <da...@longitudo.com>.
On Thu, Aug 27, 2009 at 03:08:35PM +0100, Julian Foad wrote:
> You need to state the required behaviour of an
> svn_io_line_transformer_cb_t callback function in that type's
> doc-string. Mention all the parameters and what it must do with them.
> For example, one thing that isn't clear is whether such a callback
> function must set '*line' to anything at all if it doesn't want to
> transform it. Another is whether the resulting '*line' must be allocated
> in 'pool' or can alternatively point to the input string or a static
> string.
>
> Please give the new function 'line_transformer' a doc string, even
> though 'line_filter' doesn't have one. Or, as it is very simple, you
> could perhaps remove it and write the necessary test in line.
Doc string added.
> Your code is transforming 'str->data' but the previous lines (not
> visible in this quote) have just decided that 'str' might not be the
> right result because it might have been filtered out. I think you need
> '*stringbuf' instead.
>
> Re-using the boolean variable 'filtered' to also indicate whether it
> was transformed is a little confusing and error-prone. Please create a
> second variable.
I have changed the callback to return a new stringbuf. You're right,
the flag was just confusing.
> > @@ -266,8 +298,14 @@
> > SVN_ERR(line_filter(stream, &filtered, str->data, iterpool));
> > }
> > while (filtered);
> > - *stringbuf = svn_stringbuf_dup(str, pool);
> >
> > + SVN_ERR(line_transformer(stream, &filtered, &temp, str->data,
> > + iterpool));
> > + if (filtered)
> > + *stringbuf = svn_stringbuf_create(temp, pool);
> > + else
> > + *stringbuf = svn_stringbuf_dup(str, pool);
> > +
> > svn_pool_destroy(iterpool);
> > return SVN_NO_ERROR;
> > }
>
> In this block, which is thandling the normal case, it looks like the 'if
> (filtered)' test (which means "if transformed") is redundant because
> your line_transformer() function does that job.
Fixed.
> One more thing. You need to define and state whether the transform is
> applied before or after the filter, and implement it accordingly. In
> this v2 implementation, you have a mixture of both: you filter first,
> but then go on to call line_transformer() on the resulting stringbuf
> which may be empty as a result of the filter.
Yeah, that was not intended. Fixed.
>
> It's great that you already included a test for the transformer. It
> would be really good if you could add a test for transforming and
> filtering at the same time, as that seems to be an areas where there is
> a lot of room for confusion and bugs.
>
Added another test that uses both callbacks.
A new 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): Changed doc string.
* subversion/libsvn_subr/stream.c
(svn_stream_create): Set transformer_cb to NULL.
(svn_stream_set_line_transformer_callback): Sets the new callback.
* subversion/tests/libsvn_subr/stream-tests.c
(line_transformer): A callback of the new function. Swaps the order of
the supplied line.
(test_stream_line_transformer): tests the line_transformer callback.
(test_stream_line_filter_and_transformer): tests both line_transformer
and line_filter callbacks.
]]]
Thanks Julian and stsp for the feedback!
Mvh
Daniel
Re: [PATCH] v2. char_filter_callback for streams (or
line_transformer)
Posted by Julian Foad <ju...@btopenworld.com>.
Daniel Näslund wrote:
> [[[
> Creata a new callback for svn_stream_readline() that can replace, add or
> delete characters from a line.
>
> * subversion/include/svn_io.h
> (svn_stream_set_line_transformer_callback): Declare
>
> * subversion/libsvn_subr/stream.c
> (svn_stream_set_line_transformer_callback): Sets the new callback
>
> * subversion/tests/libsvn_subr/stream-tests.c
> (line_transformer): A callback of the new function. Removes leading
> '!' characters. Does not allocate a new line.
> (test_stream_line_transformer): tests the above callback.
> ]]]
Hi Daniel. This looks good in principle.
You need to state the required behaviour of an
svn_io_line_transformer_cb_t callback function in that type's
doc-string. Mention all the parameters and what it must do with them.
For example, one thing that isn't clear is whether such a callback
function must set '*line' to anything at all if it doesn't want to
transform it. Another is whether the resulting '*line' must be allocated
in 'pool' or can alternatively point to the input string or a static
string.
Please give the new function 'line_transformer' a doc string, even
though 'line_filter' doesn't have one. Or, as it is very simple, you
could perhaps remove it and write the necessary test in line.
> svn_error_t *
> svn_stream_readline(svn_stream_t *stream,
> svn_stringbuf_t **stringbuf,
> @@ -217,6 +243,7 @@
> svn_stringbuf_t *str;
> apr_pool_t *iterpool;
> svn_boolean_t filtered;
> + const char *temp;
>
> iterpool = svn_pool_create(pool);
> do
> @@ -249,6 +276,11 @@
> else
> *stringbuf = svn_stringbuf_dup(str, pool);
>
> + SVN_ERR(line_transformer(stream, &filtered, &temp, str->data,
> + iterpool));
> + if (filtered)
> + *stringbuf = svn_stringbuf_create(temp, pool);
> +
> return SVN_NO_ERROR;
> }
>
This block is handling the end-of-stream case. Your code is transforming
'str->data' but the previous lines (not visible in this quote) have just
decided that 'str' might not be the right result because it might have
been filtered out. I think you need '*stringbuf' instead.
Re-using the boolean variable 'filtered' to also indicate whether it was
transformed is a little confusing and error-prone. Please create a
second variable.
> @@ -266,8 +298,14 @@
> SVN_ERR(line_filter(stream, &filtered, str->data, iterpool));
> }
> while (filtered);
> - *stringbuf = svn_stringbuf_dup(str, pool);
>
> + SVN_ERR(line_transformer(stream, &filtered, &temp, str->data,
> + iterpool));
> + if (filtered)
> + *stringbuf = svn_stringbuf_create(temp, pool);
> + else
> + *stringbuf = svn_stringbuf_dup(str, pool);
> +
> svn_pool_destroy(iterpool);
> return SVN_NO_ERROR;
> }
In this block, which is thandling the normal case, it looks like the 'if
(filtered)' test (which means "if transformed") is redundant because
your line_transformer() function does that job.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2387892
Re: [PATCH] v2. char_filter_callback for streams (or
line_transformer)
Posted by Daniel Näslund <da...@longitudo.com>.
Fixed some formatting issues.
[[[
Creata a new callback for svn_stream_readline() that can replace, add or
delete characters from a line.
* subversion/include/svn_io.h
(svn_stream_set_line_transformer_callback): Declare
* subversion/libsvn_subr/stream.c
(svn_stream_set_line_transformer_callback): Sets the new callback
* subversion/tests/libsvn_subr/stream-tests.c
(line_transformer): A callback of the new function. Removes leading
'!' characters. Does not allocate a new line.
(test_stream_line_transformer): tests the above callback.
]]]
Mvh
Daniel
[PATCH] char_filter_callback for streams
Posted by Daniel Näslund <da...@longitudo.com>.
Hi!
We have line_filter_callback that can exclude certain lines. The
line_transformer can replace, add or delete characters from a line.
[[[
Creata a new callback for svn_stream_readline() that can replace, add or
delete characters from a line.
* subversion/include/svn_io.h
(svn_stream_set_line_transformer_callback): Declare
* subversion/libsvn_subr/stream.c
(svn_stream_set_line_transformer_callback): Sets the new callback
* subversion/tests/libsvn_subr/stream-tests.c
(line_transformer): A callback of the new function. Removes leading
'!' characters. Does not allocate a new line.
(test_stream_line_transformer): tests the above callback.
]]]
Mvh
Daniel
Re: [PATCH] issue 3459 - svn patch does not tolerate empty lines
Posted by Daniel Näslund <da...@longitudo.com>.
On Sat, Aug 15, 2009 at 11:27:38AM +1000, Gavin Baumanis wrote:
> Hi Daniel.
>
> Just as an FYI - there is absolutely nothing wrong with you pinging
> your own patch.
> You are definitely the best person to be taking control of your
> submissions.
>
> But also please realise that - just like you - "nearly" everyone else
> is a volunteer of their own time to the Subversion Project.
> Sometimes it just takes a little while for other people to get around
> to looking at a patch submission.
> As the Patch Manager it is my role within the project to ensure that a
> patch submission doesn't fall through the cracks. (I.e. if you don't
> ping your own patch.)
> I normally ping the list for uncommented / unreviewed patches at about
> a week after it is submitted or last commented on..
>
> I'm not scolding you or anything like that....
> It's more a case of just trying to make sue that your expectations of
> the "timeliness" to having your patch submissions reviewed are "in-
> line" with the "tradition" of how things normally happen within the
> mailing list.
>
> I'm certainly not telling you to stop pinging the list, I openly
> encourage all patch submitters to take control of their own work - I
> just don't want you to think that your submission(s) are being ignored
> or that your work is not seen as being valuable by the project -
> because neither of those are true - sometimes it just takes a little
> while for a review to occur - especially when people are busy doing
> their own contributions at the same time, during their volunteered time.
Thanks for pointing this out. I got a bit carried away, a beginners rush
perhaps?
I hope no one has taken offence, it was not my intention to be intrusive.
Mvh
Daniel
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2383841
Re: [PATCH] issue 3459 - svn patch does not tolerate empty lines
Posted by Gavin Baumanis <ga...@thespidernet.com>.
Hi Daniel.
Just as an FYI - there is absolutely nothing wrong with you pinging
your own patch.
You are definitely the best person to be taking control of your
submissions.
But also please realise that - just like you - "nearly" everyone else
is a volunteer of their own time to the Subversion Project.
Sometimes it just takes a little while for other people to get around
to looking at a patch submission.
As the Patch Manager it is my role within the project to ensure that a
patch submission doesn't fall through the cracks. (I.e. if you don't
ping your own patch.)
I normally ping the list for uncommented / unreviewed patches at about
a week after it is submitted or last commented on..
I'm not scolding you or anything like that....
It's more a case of just trying to make sue that your expectations of
the "timeliness" to having your patch submissions reviewed are "in-
line" with the "tradition" of how things normally happen within the
mailing list.
I'm certainly not telling you to stop pinging the list, I openly
encourage all patch submitters to take control of their own work - I
just don't want you to think that your submission(s) are being ignored
or that your work is not seen as being valuable by the project -
because neither of those are true - sometimes it just takes a little
while for a review to occur - especially when people are busy doing
their own contributions at the same time, during their volunteered time.
Gavin.
On 14/08/2009, at 15:27 , Daniel Näslund wrote:
> Ping! This patch hasn't been reviewed.
>
> On Sun, Aug 09, 2009 at 09:25:50PM +0200, Daniel Näslund wrote:
>> On Fri, Aug 07, 2009 at 01:23:51PM +0100, Stefan Sperling wrote:
>>>>> Maybe libsvn_client should be shielded from the unidiff specifics
>>>>> entirely, and just get original and modified hunk lines from
>>>>> libsvn_diff
>>>>> with the extra '+', '-' etc. already removed.
>>>>
>>>> Involves changing svn_hunk_t to use some other classification than
>>>> original_text and modified_text? Beeing lazy I would prefer not
>>>> having
>>>> to do that. :-)
>>>
>>> I'd still prefer a solution where libsvn_client conceptually
>>> ends up in a situation like:
>>>
>>> original_text = "foo"
>>> modified_text = "bar"
>>> latest_text = "foo"
>>>
>>> which would be much more easy to deal with instead of the current:
>>>
>>> original_text = "-foo"
>>> modified_text = "+bar"
>>> latest_text = "foo"
>>
>> I was lucky. There was no need to change anything in svn_hunk_t. Just
>> removing the leading '+', '-' and ' ' was enough.
>>>
>>> Say we'd want to add a parser for context diffs (diff -c) at some
>>> point.
>>> That would be easier if libsvn_client was already shielded from the
>>> specifics of the unidiff format.
>>
>> I agree.
>>
>>>> Now I am able to patch the simple example given in the issue
>>>> description
>>>> but when trying to create a file using this patch I get a conflict
>>>> (There is one empty line at the end of the patch) :
>>>>
>>>> Index: foo2
>>>> ===================================================================
>>>> --- foo2 (revision 0)
>>>> +++ foo2 (revision 0)
>>>> @@ -0,0 +1 @@
>>>> +test
>>>>
>>>> Is this an issue with the fuzz? If svn patch would allow at least
>>>> one
>>>> line of fuzz then this patch would work?
>>>>
>>>> Or do I need a way to find out when an empty line is there for
>>>> readability (as the blank line before Property changes on ...) or
>>>> when
>>>> the leading space has been chopped?
>>>
>>> There are two cases:
>>>
>>> 1) trailing empty line is part of the hunk's context
>>> 2) trailing empty line is part of the noise surrounding patches
>>> in the unidiff
>>>
>>> I guess we can assume that the number of lines of leading context
>>> in a hunk is equal to the number of lines of trailing context in
>>> a hunk. That should provide a way to resolve the ambiguity.
>>
>> I used another method. Original_lines in svn_hunk_t is the number of
>> lines with leading ' ' or '-'. I read the number at the beginning of
>> each hunk and decrease for each ' ' or '+'. If an empty line is
>> found -
>> it is added as long as the number of original_lines is not zero.
>>
>> In subversion/test/libsvn_diff/parse-diff-tests.c I rewrote the hunk
>> header. According to me it was wrong, am I wrong?
>>
>> Should I write a new test in subversion/tests/cmdline/patch-tests.py
>> with empty lines?
>>
>> [[[
>> Fix issue 3459 - svn patch does not tolerate empty lines.
>>
>> * subversion/libsvn_diff/parse-diff.c
>> (set_hunk_text): Removes leading character before setting hunk text.
>> (svn_diff__parse_next_hunk): Allow empty lines in hunk.
>>
>> * subversion/tests/libsvn_diff/parse-diff-test.c
>> (test_parse_unidiff): Fix changed representation of hunks.
>>
>> * subversion/libsvn_client/patch.c
>> (match_hunk, copy_hunk_text): Fix changed representation of hunks.
>> ]]]
>>
>> Mvh
>> Daniel
>>
>
>> Index: subversion/libsvn_diff/parse-diff.c
>> ===================================================================
>> --- subversion/libsvn_diff/parse-diff.c (revision 38646)
>> +++ subversion/libsvn_diff/parse-diff.c (arbetskopia)
>> @@ -30,6 +30,7 @@
>> #include "svn_pools.h"
>> #include "svn_utf.h"
>> #include "svn_dirent_uri.h"
>> +#include "svn_cmdline.h"
>>
>> #include "private/svn_diff_private.h"
>>
>> @@ -273,6 +274,42 @@
>> return TRUE;
>> }
>>
>> +static svn_error_t *
>> +set_hunk_text(svn_stream_t **hunk_text, svn_stream_t **text,
>> + apr_pool_t *pool)
>> +{
>> + int offset;
>> + svn_stringbuf_t *line, *buf;
>> + svn_boolean_t eof;
>> + char *c_line;
>> +
>> + buf = svn_stringbuf_create("", pool);
>> +
>> + do
>> + {
>> + SVN_ERR(svn_stream_readline(*text, &line, APR_EOL_STR, &eof,
>> pool));
>> +
>> + char c = line->data[0];
>> +
>> + if ( c == ' ' || c == '-' || c == '+')
>> + offset = 1;
>> + else
>> + offset = 0;
>> +
>> + if (! eof)
>> + c_line = apr_psprintf(pool, "%s%s", line->data + offset,
>> APR_EOL_STR);
>> + else
>> + c_line = apr_psprintf(pool, "%s", line->data + offset);
>> +
>> + svn_stringbuf_appendcstr(buf, c_line);
>> +
>> + } while (! eof);
>> +
>> + *hunk_text = svn_stream_from_stringbuf(buf, pool);
>> +
>> + return SVN_NO_ERROR;
>> +}
>> +
>> /* A stream line-filter which allows only original text from a
>> hunk. */
>> static svn_error_t *
>> original_line_filter(svn_boolean_t *filtered, const char *line,
>> @@ -305,6 +342,7 @@
>> svn_stream_t *diff_text;
>> svn_stream_t *original_text;
>> svn_stream_t *modified_text;
>> + svn_linenum_t original_lines;
>> svn_stream_t *s;
>> apr_pool_t *iterpool;
>>
>> @@ -358,8 +396,22 @@
>> }
>>
>> c = line->data[0];
>> - if (c == ' ' || c == '-' || c == '+')
>> - hunk_seen = TRUE;
>> +
>> + if (c == ' ' || c == '-')
>> + {
>> + hunk_seen = TRUE;
>> + original_lines--;
>> + }
>> + else if (c == '+')
>> + {
>> + hunk_seen = TRUE;
>> + }
>> + /* Should tolerate chopped leading spaces on empty
>> lines. */
>> + else if (original_lines > 0 && ! eof && line->len == 0)
>> + {
>> + hunk_seen = TRUE;
>> + original_lines--;
>> + }
>> else
>> {
>> in_hunk = FALSE;
>> @@ -376,6 +428,10 @@
>> if (starts_with(line->data, atat))
>> /* Looks like we have a hunk header, let's try to rip
>> it apart. */
>> in_hunk = parse_hunk_header(line->data, *hunk, iterpool);
>> +
>> + if (in_hunk)
>> + original_lines = (*hunk)->original_length;
>> +
>> else if (starts_with(line->data, minus))
>> /* This could be a header of another patch. Bail out. */
>> break;
>> @@ -421,9 +477,13 @@
>> 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;
>> + SVN_ERR(set_hunk_text(&(*hunk)->diff_text, &diff_text,
>> + result_pool));
>> + SVN_ERR(set_hunk_text(&(*hunk)->original_text, &original_text,
>> + result_pool));
>> + SVN_ERR(set_hunk_text(&(*hunk)->modified_text, &modified_text,
>> + result_pool));
>> +
>> }
>> else
>> /* Something went wrong, just discard the result. */
>> Index: subversion/tests/libsvn_diff/parse-diff-test.c
>> ===================================================================
>> --- subversion/tests/libsvn_diff/parse-diff-test.c (revision 38646)
>> +++ subversion/tests/libsvn_diff/parse-diff-test.c (arbetskopia)
>> @@ -46,7 +46,7 @@
>>
>> "=
>> =================================================================="
>> NL
>> "--- A/D/
>> gamma.orig" NL
>> "+++ A/D/
>> gamma" NL
>> - "@@ -1 +1,2
>> @@" NL
>> + "@@ -1,2 +1
>> @@" NL
>> " This is the file
>> 'gamma'." NL
>> "-some less bytes to
>> 'gamma'" NL
>>
>> "" NL
>> @@ -99,7 +99,7 @@
>> /* Make sure original text was parsed correctly. */
>> SVN_ERR(svn_stream_readline(hunk->original_text, &buf, NL, &eof,
>> pool));
>> SVN_ERR_ASSERT(! eof);
>> - SVN_ERR_ASSERT(! strcmp(buf->data, " This is the file 'gamma'."));
>> + SVN_ERR_ASSERT(! strcmp(buf->data, "This is the file 'gamma'."));
>> /* Now we should get EOF. */
>> SVN_ERR(svn_stream_readline(hunk->original_text, &buf, NL, &eof,
>> pool));
>> SVN_ERR_ASSERT(eof);
>> @@ -108,10 +108,10 @@
>> /* Make sure modified text was parsed correctly. */
>> SVN_ERR(svn_stream_readline(hunk->modified_text, &buf, NL, &eof,
>> pool));
>> SVN_ERR_ASSERT(! eof);
>> - SVN_ERR_ASSERT(! strcmp(buf->data, " This is the file 'gamma'."));
>> + SVN_ERR_ASSERT(! strcmp(buf->data, "This is the file 'gamma'."));
>> SVN_ERR(svn_stream_readline(hunk->modified_text, &buf, NL, &eof,
>> pool));
>> SVN_ERR_ASSERT(! eof);
>> - SVN_ERR_ASSERT(! strcmp(buf->data, "+some more bytes to
>> 'gamma'"));
>> + SVN_ERR_ASSERT(! strcmp(buf->data, "some more bytes to 'gamma'"));
>> /* Now we should get EOF. */
>> SVN_ERR(svn_stream_readline(hunk->modified_text, &buf, NL, &eof,
>> pool));
>> SVN_ERR_ASSERT(eof);
>> @@ -128,10 +128,10 @@
>> /* Make sure original text was parsed correctly. */
>> SVN_ERR(svn_stream_readline(hunk->original_text, &buf, NL, &eof,
>> pool));
>> SVN_ERR_ASSERT(! eof);
>> - SVN_ERR_ASSERT(! strcmp(buf->data, " This is the file 'gamma'."));
>> + SVN_ERR_ASSERT(! strcmp(buf->data, "This is the file 'gamma'."));
>> SVN_ERR(svn_stream_readline(hunk->original_text, &buf, NL, &eof,
>> pool));
>> SVN_ERR_ASSERT(! eof);
>> - SVN_ERR_ASSERT(! strcmp(buf->data, "-some less bytes to
>> 'gamma'"));
>> + SVN_ERR_ASSERT(! strcmp(buf->data, "some less bytes to 'gamma'"));
>> /* Now we should get EOF. */
>> SVN_ERR(svn_stream_readline(hunk->original_text, &buf, NL, &eof,
>> pool));
>> SVN_ERR_ASSERT(eof);
>> @@ -140,7 +140,7 @@
>> /* Make sure modified text was parsed correctly. */
>> SVN_ERR(svn_stream_readline(hunk->modified_text, &buf, NL, &eof,
>> pool));
>> SVN_ERR_ASSERT(! eof);
>> - SVN_ERR_ASSERT(! strcmp(buf->data, " This is the file 'gamma'."));
>> + SVN_ERR_ASSERT(! strcmp(buf->data, "This is the file 'gamma'."));
>> /* Now we should get EOF. */
>> SVN_ERR(svn_stream_readline(hunk->modified_text, &buf, NL, &eof,
>> pool));
>> SVN_ERR_ASSERT(eof);
>> Index: subversion/libsvn_client/patch.c
>> ===================================================================
>> --- subversion/libsvn_client/patch.c (revision 38646)
>> +++ subversion/libsvn_client/patch.c (arbetskopia)
>> @@ -2108,12 +2108,11 @@
>> target->patch->eol_str,
>> &hunk_eof, iterpool));
>> SVN_ERR(svn_stream_readline(target->stream, &target_line,
>> target->eol_str,
>> &target->eof, iterpool));
>> +
>> if (! hunk_eof && hunk_line->len > 0)
>> {
>> - char c = hunk_line->data[0];
>> - SVN_ERR_ASSERT(c == ' ' || c == '-');
>> - lines_matched = (hunk_line->len == target_line->len + 1 &&
>> - ! strcmp(hunk_line->data + 1,
>> target_line->data));
>> + lines_matched = (hunk_line->len == target_line->len &&
>> + ! strcmp(hunk_line->data, target_line-
>> >data));
>> }
>> }
>> while (lines_matched && ! (hunk_eof || target->eof));
>> @@ -2386,13 +2385,9 @@
>> {
>> if (line->len >= 1)
>> {
>> - char c = line->data[0];
>> -
>> - SVN_ERR_ASSERT(c == ' ' || c == '+' || c == '-');
>> - len = line->len - 1;
>> - SVN_ERR(svn_io_file_write_full(file, line->data + 1,
>> len, &len,
>> + len = line->len;
>> + SVN_ERR(svn_io_file_write_full(file, line->data,
>> len, &len,
>> iterpool));
>> - SVN_ERR_ASSERT(len == line->len - 1);
>> }
>>
>> /* Add newline. */
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2383529
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2383805
Re: [PATCH] issue 3459 - svn patch does not tolerate empty lines
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Aug 14, 2009 at 07:27:23AM +0200, Daniel Näslund wrote:
> Ping! This patch hasn't been reviewed.
Reviewed now, see below.
> > [[[
> > Fix issue 3459 - svn patch does not tolerate empty lines.
> >
> > * subversion/libsvn_diff/parse-diff.c
> > (set_hunk_text): Removes leading character before setting hunk text.
> > (svn_diff__parse_next_hunk): Allow empty lines in hunk.
> >
> > * subversion/tests/libsvn_diff/parse-diff-test.c
> > (test_parse_unidiff): Fix changed representation of hunks.
> >
> > * subversion/libsvn_client/patch.c
> > (match_hunk, copy_hunk_text): Fix changed representation of hunks.
> > ]]]
> >
> > Mvh
> > Daniel
> >
>
> > Index: subversion/libsvn_diff/parse-diff.c
> > ===================================================================
> > --- subversion/libsvn_diff/parse-diff.c (revision 38646)
> > +++ subversion/libsvn_diff/parse-diff.c (arbetskopia)
> > @@ -30,6 +30,7 @@
> > #include "svn_pools.h"
> > #include "svn_utf.h"
> > #include "svn_dirent_uri.h"
> > +#include "svn_cmdline.h"
> >
> > #include "private/svn_diff_private.h"
> >
> > @@ -273,6 +274,42 @@
> > return TRUE;
> > }
> >
> > +static svn_error_t *
> > +set_hunk_text(svn_stream_t **hunk_text, svn_stream_t **text,
Why isn't text just a pointer, ie. *text instead of **text?
You're not using it as an output parameter.
> > + apr_pool_t *pool)
> > +{
> > + int offset;
> > + svn_stringbuf_t *line, *buf;
> > + svn_boolean_t eof;
> > + char *c_line;
> > +
> > + buf = svn_stringbuf_create("", pool);
> > +
> > + do
> > + {
> > + SVN_ERR(svn_stream_readline(*text, &line, APR_EOL_STR, &eof, pool));
What if the patch and target file use an eol-style other than
APR_EOL_STR? We need to support that.
> > +
> > + char c = line->data[0];
> > +
> > + if ( c == ' ' || c == '-' || c == '+')
> > + offset = 1;
> > + else
> > + offset = 0;
> > +
> > + if (! eof)
> > + c_line = apr_psprintf(pool, "%s%s", line->data + offset, APR_EOL_STR);
Same here.
> > + else
> > + c_line = apr_psprintf(pool, "%s", line->data + offset);
> > +
> > + svn_stringbuf_appendcstr(buf, c_line);
I went to great length to avoid sucking an entire hunk into memory,
but this change would make us suck an entire hunk into memory again.
A hunk may be very large, e.g. when adding a large file.
The stream 'text' that comes into this function is backed by a range
of the patch file instead of having its content allocated in a pool.
This way, 'svn patch' will only allocate memory for one line at a time
instead of one hunk at a time. (Someone could still pass us a 4GB long
line and crash 'svn patch', we might want to add a sanity check for
that condition before release -- but that's for another patch :)
Instead of putting the hunk into memory, you could try wrapping the
text stream in another stream that shaves leading characters from
every line read (in svn_stream_readline()). That stream could then
be passed to libsvn_client. Such a stream does not yet exist so you
need to create it. See include/svn_io.h for more information about
streams, and see the existing examples in libsvn_subr/stream.c.
> > +
> > + } while (! eof);
> > +
> > + *hunk_text = svn_stream_from_stringbuf(buf, pool);
> > +
> > + return SVN_NO_ERROR;
> > +}
> > +
> > /* A stream line-filter which allows only original text from a hunk. */
> > static svn_error_t *
> > original_line_filter(svn_boolean_t *filtered, const char *line,
> > @@ -305,6 +342,7 @@
> > svn_stream_t *diff_text;
> > svn_stream_t *original_text;
> > svn_stream_t *modified_text;
> > + svn_linenum_t original_lines;
> > svn_stream_t *s;
> > apr_pool_t *iterpool;
> >
> > @@ -358,8 +396,22 @@
> > }
> >
> > c = line->data[0];
> > - if (c == ' ' || c == '-' || c == '+')
> > - hunk_seen = TRUE;
> > +
> > + if (c == ' ' || c == '-')
> > + {
> > + hunk_seen = TRUE;
> > + original_lines--;
What happens if this wraps around? svn_linenum_t is unsigned.
> > + }
> > + else if (c == '+')
> > + {
> > + hunk_seen = TRUE;
> > + }
> > + /* Should tolerate chopped leading spaces on empty lines. */
> > + else if (original_lines > 0 && ! eof && line->len == 0)
> > + {
> > + hunk_seen = TRUE;
> > + original_lines--;
This can't wrap around, good.
> > + }
> > else
> > {
> > in_hunk = FALSE;
> > @@ -376,6 +428,10 @@
> > if (starts_with(line->data, atat))
> > /* Looks like we have a hunk header, let's try to rip it apart. */
> > in_hunk = parse_hunk_header(line->data, *hunk, iterpool);
> > +
> > + if (in_hunk)
> > + original_lines = (*hunk)->original_length;
> > +
> > else if (starts_with(line->data, minus))
> > /* This could be a header of another patch. Bail out. */
> > break;
> > @@ -421,9 +477,13 @@
> > 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;
> > + SVN_ERR(set_hunk_text(&(*hunk)->diff_text, &diff_text,
> > + result_pool));
> > + SVN_ERR(set_hunk_text(&(*hunk)->original_text, &original_text,
> > + result_pool));
> > + SVN_ERR(set_hunk_text(&(*hunk)->modified_text, &modified_text,
> > + result_pool));
> > +
> > }
> > else
> > /* Something went wrong, just discard the result. */
The test adjustments below look good, that's exactly how we want it
to behave!
Thanks,
Stefan
> > Index: subversion/tests/libsvn_diff/parse-diff-test.c
> > ===================================================================
> > --- subversion/tests/libsvn_diff/parse-diff-test.c (revision 38646)
> > +++ subversion/tests/libsvn_diff/parse-diff-test.c (arbetskopia)
> > @@ -46,7 +46,7 @@
> > "===================================================================" NL
> > "--- A/D/gamma.orig" NL
> > "+++ A/D/gamma" NL
> > - "@@ -1 +1,2 @@" NL
> > + "@@ -1,2 +1 @@" NL
> > " This is the file 'gamma'." NL
> > "-some less bytes to 'gamma'" NL
> > "" NL
> > @@ -99,7 +99,7 @@
> > /* Make sure original text was parsed correctly. */
> > SVN_ERR(svn_stream_readline(hunk->original_text, &buf, NL, &eof, pool));
> > SVN_ERR_ASSERT(! eof);
> > - SVN_ERR_ASSERT(! strcmp(buf->data, " This is the file 'gamma'."));
> > + SVN_ERR_ASSERT(! strcmp(buf->data, "This is the file 'gamma'."));
> > /* Now we should get EOF. */
> > SVN_ERR(svn_stream_readline(hunk->original_text, &buf, NL, &eof, pool));
> > SVN_ERR_ASSERT(eof);
> > @@ -108,10 +108,10 @@
> > /* Make sure modified text was parsed correctly. */
> > SVN_ERR(svn_stream_readline(hunk->modified_text, &buf, NL, &eof, pool));
> > SVN_ERR_ASSERT(! eof);
> > - SVN_ERR_ASSERT(! strcmp(buf->data, " This is the file 'gamma'."));
> > + SVN_ERR_ASSERT(! strcmp(buf->data, "This is the file 'gamma'."));
> > SVN_ERR(svn_stream_readline(hunk->modified_text, &buf, NL, &eof, pool));
> > SVN_ERR_ASSERT(! eof);
> > - SVN_ERR_ASSERT(! strcmp(buf->data, "+some more bytes to 'gamma'"));
> > + SVN_ERR_ASSERT(! strcmp(buf->data, "some more bytes to 'gamma'"));
> > /* Now we should get EOF. */
> > SVN_ERR(svn_stream_readline(hunk->modified_text, &buf, NL, &eof, pool));
> > SVN_ERR_ASSERT(eof);
> > @@ -128,10 +128,10 @@
> > /* Make sure original text was parsed correctly. */
> > SVN_ERR(svn_stream_readline(hunk->original_text, &buf, NL, &eof, pool));
> > SVN_ERR_ASSERT(! eof);
> > - SVN_ERR_ASSERT(! strcmp(buf->data, " This is the file 'gamma'."));
> > + SVN_ERR_ASSERT(! strcmp(buf->data, "This is the file 'gamma'."));
> > SVN_ERR(svn_stream_readline(hunk->original_text, &buf, NL, &eof, pool));
> > SVN_ERR_ASSERT(! eof);
> > - SVN_ERR_ASSERT(! strcmp(buf->data, "-some less bytes to 'gamma'"));
> > + SVN_ERR_ASSERT(! strcmp(buf->data, "some less bytes to 'gamma'"));
> > /* Now we should get EOF. */
> > SVN_ERR(svn_stream_readline(hunk->original_text, &buf, NL, &eof, pool));
> > SVN_ERR_ASSERT(eof);
> > @@ -140,7 +140,7 @@
> > /* Make sure modified text was parsed correctly. */
> > SVN_ERR(svn_stream_readline(hunk->modified_text, &buf, NL, &eof, pool));
> > SVN_ERR_ASSERT(! eof);
> > - SVN_ERR_ASSERT(! strcmp(buf->data, " This is the file 'gamma'."));
> > + SVN_ERR_ASSERT(! strcmp(buf->data, "This is the file 'gamma'."));
> > /* Now we should get EOF. */
> > SVN_ERR(svn_stream_readline(hunk->modified_text, &buf, NL, &eof, pool));
> > SVN_ERR_ASSERT(eof);
> > Index: subversion/libsvn_client/patch.c
> > ===================================================================
> > --- subversion/libsvn_client/patch.c (revision 38646)
> > +++ subversion/libsvn_client/patch.c (arbetskopia)
> > @@ -2108,12 +2108,11 @@
> > target->patch->eol_str, &hunk_eof, iterpool));
> > SVN_ERR(svn_stream_readline(target->stream, &target_line, target->eol_str,
> > &target->eof, iterpool));
> > +
> > if (! hunk_eof && hunk_line->len > 0)
> > {
> > - char c = hunk_line->data[0];
> > - SVN_ERR_ASSERT(c == ' ' || c == '-');
> > - lines_matched = (hunk_line->len == target_line->len + 1 &&
> > - ! strcmp(hunk_line->data + 1, target_line->data));
> > + lines_matched = (hunk_line->len == target_line->len &&
> > + ! strcmp(hunk_line->data, target_line->data));
> > }
> > }
> > while (lines_matched && ! (hunk_eof || target->eof));
> > @@ -2386,13 +2385,9 @@
> > {
> > if (line->len >= 1)
> > {
> > - char c = line->data[0];
> > -
> > - SVN_ERR_ASSERT(c == ' ' || c == '+' || c == '-');
> > - len = line->len - 1;
> > - SVN_ERR(svn_io_file_write_full(file, line->data + 1, len, &len,
> > + len = line->len;
> > + SVN_ERR(svn_io_file_write_full(file, line->data, len, &len,
> > iterpool));
> > - SVN_ERR_ASSERT(len == line->len - 1);
> > }
> >
> > /* Add newline. */
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2383529
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2383544
Re: [PATCH] issue 3459 - svn patch does not tolerate empty lines
Posted by Daniel Näslund <da...@longitudo.com>.
Ping! This patch hasn't been reviewed.
On Sun, Aug 09, 2009 at 09:25:50PM +0200, Daniel Näslund wrote:
> On Fri, Aug 07, 2009 at 01:23:51PM +0100, Stefan Sperling wrote:
> > > > Maybe libsvn_client should be shielded from the unidiff specifics
> > > > entirely, and just get original and modified hunk lines from libsvn_diff
> > > > with the extra '+', '-' etc. already removed.
> > >
> > > Involves changing svn_hunk_t to use some other classification than
> > > original_text and modified_text? Beeing lazy I would prefer not having
> > > to do that. :-)
> >
> > I'd still prefer a solution where libsvn_client conceptually
> > ends up in a situation like:
> >
> > original_text = "foo"
> > modified_text = "bar"
> > latest_text = "foo"
> >
> > which would be much more easy to deal with instead of the current:
> >
> > original_text = "-foo"
> > modified_text = "+bar"
> > latest_text = "foo"
>
> I was lucky. There was no need to change anything in svn_hunk_t. Just
> removing the leading '+', '-' and ' ' was enough.
> >
> > Say we'd want to add a parser for context diffs (diff -c) at some point.
> > That would be easier if libsvn_client was already shielded from the
> > specifics of the unidiff format.
>
> I agree.
>
> > > Now I am able to patch the simple example given in the issue description
> > > but when trying to create a file using this patch I get a conflict
> > > (There is one empty line at the end of the patch) :
> > >
> > > Index: foo2
> > > ===================================================================
> > > --- foo2 (revision 0)
> > > +++ foo2 (revision 0)
> > > @@ -0,0 +1 @@
> > > +test
> > >
> > > Is this an issue with the fuzz? If svn patch would allow at least one
> > > line of fuzz then this patch would work?
> > >
> > > Or do I need a way to find out when an empty line is there for
> > > readability (as the blank line before Property changes on ...) or when
> > > the leading space has been chopped?
> >
> > There are two cases:
> >
> > 1) trailing empty line is part of the hunk's context
> > 2) trailing empty line is part of the noise surrounding patches
> > in the unidiff
> >
> > I guess we can assume that the number of lines of leading context
> > in a hunk is equal to the number of lines of trailing context in
> > a hunk. That should provide a way to resolve the ambiguity.
>
> I used another method. Original_lines in svn_hunk_t is the number of
> lines with leading ' ' or '-'. I read the number at the beginning of
> each hunk and decrease for each ' ' or '+'. If an empty line is found -
> it is added as long as the number of original_lines is not zero.
>
> In subversion/test/libsvn_diff/parse-diff-tests.c I rewrote the hunk
> header. According to me it was wrong, am I wrong?
>
> Should I write a new test in subversion/tests/cmdline/patch-tests.py
> with empty lines?
>
> [[[
> Fix issue 3459 - svn patch does not tolerate empty lines.
>
> * subversion/libsvn_diff/parse-diff.c
> (set_hunk_text): Removes leading character before setting hunk text.
> (svn_diff__parse_next_hunk): Allow empty lines in hunk.
>
> * subversion/tests/libsvn_diff/parse-diff-test.c
> (test_parse_unidiff): Fix changed representation of hunks.
>
> * subversion/libsvn_client/patch.c
> (match_hunk, copy_hunk_text): Fix changed representation of hunks.
> ]]]
>
> Mvh
> Daniel
>
> Index: subversion/libsvn_diff/parse-diff.c
> ===================================================================
> --- subversion/libsvn_diff/parse-diff.c (revision 38646)
> +++ subversion/libsvn_diff/parse-diff.c (arbetskopia)
> @@ -30,6 +30,7 @@
> #include "svn_pools.h"
> #include "svn_utf.h"
> #include "svn_dirent_uri.h"
> +#include "svn_cmdline.h"
>
> #include "private/svn_diff_private.h"
>
> @@ -273,6 +274,42 @@
> return TRUE;
> }
>
> +static svn_error_t *
> +set_hunk_text(svn_stream_t **hunk_text, svn_stream_t **text,
> + apr_pool_t *pool)
> +{
> + int offset;
> + svn_stringbuf_t *line, *buf;
> + svn_boolean_t eof;
> + char *c_line;
> +
> + buf = svn_stringbuf_create("", pool);
> +
> + do
> + {
> + SVN_ERR(svn_stream_readline(*text, &line, APR_EOL_STR, &eof, pool));
> +
> + char c = line->data[0];
> +
> + if ( c == ' ' || c == '-' || c == '+')
> + offset = 1;
> + else
> + offset = 0;
> +
> + if (! eof)
> + c_line = apr_psprintf(pool, "%s%s", line->data + offset, APR_EOL_STR);
> + else
> + c_line = apr_psprintf(pool, "%s", line->data + offset);
> +
> + svn_stringbuf_appendcstr(buf, c_line);
> +
> + } while (! eof);
> +
> + *hunk_text = svn_stream_from_stringbuf(buf, pool);
> +
> + return SVN_NO_ERROR;
> +}
> +
> /* A stream line-filter which allows only original text from a hunk. */
> static svn_error_t *
> original_line_filter(svn_boolean_t *filtered, const char *line,
> @@ -305,6 +342,7 @@
> svn_stream_t *diff_text;
> svn_stream_t *original_text;
> svn_stream_t *modified_text;
> + svn_linenum_t original_lines;
> svn_stream_t *s;
> apr_pool_t *iterpool;
>
> @@ -358,8 +396,22 @@
> }
>
> c = line->data[0];
> - if (c == ' ' || c == '-' || c == '+')
> - hunk_seen = TRUE;
> +
> + if (c == ' ' || c == '-')
> + {
> + hunk_seen = TRUE;
> + original_lines--;
> + }
> + else if (c == '+')
> + {
> + hunk_seen = TRUE;
> + }
> + /* Should tolerate chopped leading spaces on empty lines. */
> + else if (original_lines > 0 && ! eof && line->len == 0)
> + {
> + hunk_seen = TRUE;
> + original_lines--;
> + }
> else
> {
> in_hunk = FALSE;
> @@ -376,6 +428,10 @@
> if (starts_with(line->data, atat))
> /* Looks like we have a hunk header, let's try to rip it apart. */
> in_hunk = parse_hunk_header(line->data, *hunk, iterpool);
> +
> + if (in_hunk)
> + original_lines = (*hunk)->original_length;
> +
> else if (starts_with(line->data, minus))
> /* This could be a header of another patch. Bail out. */
> break;
> @@ -421,9 +477,13 @@
> 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;
> + SVN_ERR(set_hunk_text(&(*hunk)->diff_text, &diff_text,
> + result_pool));
> + SVN_ERR(set_hunk_text(&(*hunk)->original_text, &original_text,
> + result_pool));
> + SVN_ERR(set_hunk_text(&(*hunk)->modified_text, &modified_text,
> + result_pool));
> +
> }
> else
> /* Something went wrong, just discard the result. */
> Index: subversion/tests/libsvn_diff/parse-diff-test.c
> ===================================================================
> --- subversion/tests/libsvn_diff/parse-diff-test.c (revision 38646)
> +++ subversion/tests/libsvn_diff/parse-diff-test.c (arbetskopia)
> @@ -46,7 +46,7 @@
> "===================================================================" NL
> "--- A/D/gamma.orig" NL
> "+++ A/D/gamma" NL
> - "@@ -1 +1,2 @@" NL
> + "@@ -1,2 +1 @@" NL
> " This is the file 'gamma'." NL
> "-some less bytes to 'gamma'" NL
> "" NL
> @@ -99,7 +99,7 @@
> /* Make sure original text was parsed correctly. */
> SVN_ERR(svn_stream_readline(hunk->original_text, &buf, NL, &eof, pool));
> SVN_ERR_ASSERT(! eof);
> - SVN_ERR_ASSERT(! strcmp(buf->data, " This is the file 'gamma'."));
> + SVN_ERR_ASSERT(! strcmp(buf->data, "This is the file 'gamma'."));
> /* Now we should get EOF. */
> SVN_ERR(svn_stream_readline(hunk->original_text, &buf, NL, &eof, pool));
> SVN_ERR_ASSERT(eof);
> @@ -108,10 +108,10 @@
> /* Make sure modified text was parsed correctly. */
> SVN_ERR(svn_stream_readline(hunk->modified_text, &buf, NL, &eof, pool));
> SVN_ERR_ASSERT(! eof);
> - SVN_ERR_ASSERT(! strcmp(buf->data, " This is the file 'gamma'."));
> + SVN_ERR_ASSERT(! strcmp(buf->data, "This is the file 'gamma'."));
> SVN_ERR(svn_stream_readline(hunk->modified_text, &buf, NL, &eof, pool));
> SVN_ERR_ASSERT(! eof);
> - SVN_ERR_ASSERT(! strcmp(buf->data, "+some more bytes to 'gamma'"));
> + SVN_ERR_ASSERT(! strcmp(buf->data, "some more bytes to 'gamma'"));
> /* Now we should get EOF. */
> SVN_ERR(svn_stream_readline(hunk->modified_text, &buf, NL, &eof, pool));
> SVN_ERR_ASSERT(eof);
> @@ -128,10 +128,10 @@
> /* Make sure original text was parsed correctly. */
> SVN_ERR(svn_stream_readline(hunk->original_text, &buf, NL, &eof, pool));
> SVN_ERR_ASSERT(! eof);
> - SVN_ERR_ASSERT(! strcmp(buf->data, " This is the file 'gamma'."));
> + SVN_ERR_ASSERT(! strcmp(buf->data, "This is the file 'gamma'."));
> SVN_ERR(svn_stream_readline(hunk->original_text, &buf, NL, &eof, pool));
> SVN_ERR_ASSERT(! eof);
> - SVN_ERR_ASSERT(! strcmp(buf->data, "-some less bytes to 'gamma'"));
> + SVN_ERR_ASSERT(! strcmp(buf->data, "some less bytes to 'gamma'"));
> /* Now we should get EOF. */
> SVN_ERR(svn_stream_readline(hunk->original_text, &buf, NL, &eof, pool));
> SVN_ERR_ASSERT(eof);
> @@ -140,7 +140,7 @@
> /* Make sure modified text was parsed correctly. */
> SVN_ERR(svn_stream_readline(hunk->modified_text, &buf, NL, &eof, pool));
> SVN_ERR_ASSERT(! eof);
> - SVN_ERR_ASSERT(! strcmp(buf->data, " This is the file 'gamma'."));
> + SVN_ERR_ASSERT(! strcmp(buf->data, "This is the file 'gamma'."));
> /* Now we should get EOF. */
> SVN_ERR(svn_stream_readline(hunk->modified_text, &buf, NL, &eof, pool));
> SVN_ERR_ASSERT(eof);
> Index: subversion/libsvn_client/patch.c
> ===================================================================
> --- subversion/libsvn_client/patch.c (revision 38646)
> +++ subversion/libsvn_client/patch.c (arbetskopia)
> @@ -2108,12 +2108,11 @@
> target->patch->eol_str, &hunk_eof, iterpool));
> SVN_ERR(svn_stream_readline(target->stream, &target_line, target->eol_str,
> &target->eof, iterpool));
> +
> if (! hunk_eof && hunk_line->len > 0)
> {
> - char c = hunk_line->data[0];
> - SVN_ERR_ASSERT(c == ' ' || c == '-');
> - lines_matched = (hunk_line->len == target_line->len + 1 &&
> - ! strcmp(hunk_line->data + 1, target_line->data));
> + lines_matched = (hunk_line->len == target_line->len &&
> + ! strcmp(hunk_line->data, target_line->data));
> }
> }
> while (lines_matched && ! (hunk_eof || target->eof));
> @@ -2386,13 +2385,9 @@
> {
> if (line->len >= 1)
> {
> - char c = line->data[0];
> -
> - SVN_ERR_ASSERT(c == ' ' || c == '+' || c == '-');
> - len = line->len - 1;
> - SVN_ERR(svn_io_file_write_full(file, line->data + 1, len, &len,
> + len = line->len;
> + SVN_ERR(svn_io_file_write_full(file, line->data, len, &len,
> iterpool));
> - SVN_ERR_ASSERT(len == line->len - 1);
> }
>
> /* Add newline. */
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2383529
[PATCH] issue 3459 - svn patch does not tolerate empty lines
Posted by Daniel Näslund <da...@longitudo.com>.
On Fri, Aug 07, 2009 at 01:23:51PM +0100, Stefan Sperling wrote:
> > > Maybe libsvn_client should be shielded from the unidiff specifics
> > > entirely, and just get original and modified hunk lines from libsvn_diff
> > > with the extra '+', '-' etc. already removed.
> >
> > Involves changing svn_hunk_t to use some other classification than
> > original_text and modified_text? Beeing lazy I would prefer not having
> > to do that. :-)
>
> I'd still prefer a solution where libsvn_client conceptually
> ends up in a situation like:
>
> original_text = "foo"
> modified_text = "bar"
> latest_text = "foo"
>
> which would be much more easy to deal with instead of the current:
>
> original_text = "-foo"
> modified_text = "+bar"
> latest_text = "foo"
I was lucky. There was no need to change anything in svn_hunk_t. Just
removing the leading '+', '-' and ' ' was enough.
>
> Say we'd want to add a parser for context diffs (diff -c) at some point.
> That would be easier if libsvn_client was already shielded from the
> specifics of the unidiff format.
I agree.
> > Now I am able to patch the simple example given in the issue description
> > but when trying to create a file using this patch I get a conflict
> > (There is one empty line at the end of the patch) :
> >
> > Index: foo2
> > ===================================================================
> > --- foo2 (revision 0)
> > +++ foo2 (revision 0)
> > @@ -0,0 +1 @@
> > +test
> >
> > Is this an issue with the fuzz? If svn patch would allow at least one
> > line of fuzz then this patch would work?
> >
> > Or do I need a way to find out when an empty line is there for
> > readability (as the blank line before Property changes on ...) or when
> > the leading space has been chopped?
>
> There are two cases:
>
> 1) trailing empty line is part of the hunk's context
> 2) trailing empty line is part of the noise surrounding patches
> in the unidiff
>
> I guess we can assume that the number of lines of leading context
> in a hunk is equal to the number of lines of trailing context in
> a hunk. That should provide a way to resolve the ambiguity.
I used another method. Original_lines in svn_hunk_t is the number of
lines with leading ' ' or '-'. I read the number at the beginning of
each hunk and decrease for each ' ' or '+'. If an empty line is found -
it is added as long as the number of original_lines is not zero.
In subversion/test/libsvn_diff/parse-diff-tests.c I rewrote the hunk
header. According to me it was wrong, am I wrong?
Should I write a new test in subversion/tests/cmdline/patch-tests.py
with empty lines?
[[[
Fix issue 3459 - svn patch does not tolerate empty lines.
* subversion/libsvn_diff/parse-diff.c
(set_hunk_text): Removes leading character before setting hunk text.
(svn_diff__parse_next_hunk): Allow empty lines in hunk.
* subversion/tests/libsvn_diff/parse-diff-test.c
(test_parse_unidiff): Fix changed representation of hunks.
* subversion/libsvn_client/patch.c
(match_hunk, copy_hunk_text): Fix changed representation of hunks.
]]]
Mvh
Daniel
Re: issue 3459 - svn patch does not tolerate empty lines
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Aug 07, 2009 at 01:40:10PM +0200, Daniel Näslund wrote:
> On Thu, Aug 06, 2009 at 01:10:43PM +0100, Stefan Sperling wrote:
> > The parser right now assumes that every line which is part
> > of a hunk starts with '+', '-', or ' '.
> >
> > This works for correctly formatted diffs as output by 'svn diff'
> > and 'diff', but not for mangled diffs.
> >
>
> My second attempt. I allow empty lines to be included in the hunk. And
> when reading the hunk in subversion/libsvn_client/patch.c (match_hunk) I
> insert a space if missing.
>
> It would be better to shield libsvn_client from the empty lines handling
> and insert an extra space when needed in the streams in
> subversion/libsvn_diff/parse-diff.c (svn_diff__parse_next_hunk. Should I
> do that instead? I do miss Java IO (thought I would never say that).
>
> Index: subversion/libsvn_diff/parse-diff.c
> ===================================================================
> --- subversion/libsvn_diff/parse-diff.c (revision 38611)
> +++ subversion/libsvn_diff/parse-diff.c (arbetskopia)
> @@ -358,8 +358,11 @@
> }
>
> c = line->data[0];
> - if (c == ' ' || c == '-' || c == '+')
> - hunk_seen = TRUE;
> +
> + /* It is allowed for a line to be empty. */
> + if (c == ' ' || c == '-' || c == '+'
> + || (! eof && line->len == 0))
> + hunk_seen = TRUE;
> else
> {
> in_hunk = FALSE;
> Index: subversion/libsvn_client/patch.c
> ===================================================================
> --- subversion/libsvn_client/patch.c (revision 38611)
> +++ subversion/libsvn_client/patch.c (arbetskopia)
> @@ -2108,6 +2108,13 @@
> target->patch->eol_str, &hunk_eof, iterpool));
> SVN_ERR(svn_stream_readline(target->stream, &target_line, target->eol_str,
> &target->eof, iterpool));
> +
> + /* If leading spaces has been dropped, the patch should still be
> + * applied. Started out as an issue with the mailing software
> + * tigris that for some reason ate extra spaces. */
Just say that UNIX patch accepts this, too. There's no need to bash
tigris in comments in our code. It's getting enough bashing elsewhere as is.
> + if (hunk_line->len == 0)
> + svn_stringbuf_appendcstr(hunk_line, " ");
> +
> if (! hunk_eof && hunk_line->len > 0)
> {
> char c = hunk_line->data[0];
>
>
> > Maybe libsvn_client should be shielded from the unidiff specifics
> > entirely, and just get original and modified hunk lines from libsvn_diff
> > with the extra '+', '-' etc. already removed.
>
> Involves changing svn_hunk_t to use some other classification than
> original_text and modified_text? Beeing lazy I would prefer not having
> to do that. :-)
It may involve changing that.
I was hoping you'd figure out what's needed :-P
I'd still prefer a solution where libsvn_client conceptually
ends up in a situation like:
original_text = "foo"
modified_text = "bar"
latest_text = "foo"
which would be much more easy to deal with instead of the current:
original_text = "-foo"
modified_text = "+bar"
latest_text = "foo"
Right now, the information about a pattern being original or modified
is encoded twice, both in the variable name and the first character
(except for latest_text which needs special-casing). libsvn_client
doesn't really care about the plusses and minusses. It does the text
string matching, not the parsing.
Say we'd want to add a parser for context diffs (diff -c) at some point.
That would be easier if libsvn_client was already shielded from the
specifics of the unidiff format.
> Now I am able to patch the simple example given in the issue description
> but when trying to create a file using this patch I get a conflict
> (There is one empty line at the end of the patch) :
>
> Index: foo2
> ===================================================================
> --- foo2 (revision 0)
> +++ foo2 (revision 0)
> @@ -0,0 +1 @@
> +test
>
> Is this an issue with the fuzz? If svn patch would allow at least one
> line of fuzz then this patch would work?
>
> Or do I need a way to find out when an empty line is there for
> readability (as the blank line before Property changes on ...) or when
> the leading space has been chopped?
There are two cases:
1) trailing empty line is part of the hunk's context
2) trailing empty line is part of the noise surrounding patches
in the unidiff
I guess we can assume that the number of lines of leading context
in a hunk is equal to the number of lines of trailing context in
a hunk. That should provide a way to resolve the ambiguity.
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2381272
Re: issue 3459 - svn patch does not tolerate empty lines
Posted by Daniel Näslund <da...@longitudo.com>.
On Thu, Aug 06, 2009 at 01:10:43PM +0100, Stefan Sperling wrote:
> The parser right now assumes that every line which is part
> of a hunk starts with '+', '-', or ' '.
>
> This works for correctly formatted diffs as output by 'svn diff'
> and 'diff', but not for mangled diffs.
>
My second attempt. I allow empty lines to be included in the hunk. And
when reading the hunk in subversion/libsvn_client/patch.c (match_hunk) I
insert a space if missing.
It would be better to shield libsvn_client from the empty lines handling
and insert an extra space when needed in the streams in
subversion/libsvn_diff/parse-diff.c (svn_diff__parse_next_hunk. Should I
do that instead? I do miss Java IO (thought I would never say that).
Index: subversion/libsvn_diff/parse-diff.c
===================================================================
--- subversion/libsvn_diff/parse-diff.c (revision 38611)
+++ subversion/libsvn_diff/parse-diff.c (arbetskopia)
@@ -358,8 +358,11 @@
}
c = line->data[0];
- if (c == ' ' || c == '-' || c == '+')
- hunk_seen = TRUE;
+
+ /* It is allowed for a line to be empty. */
+ if (c == ' ' || c == '-' || c == '+'
+ || (! eof && line->len == 0))
+ hunk_seen = TRUE;
else
{
in_hunk = FALSE;
Index: subversion/libsvn_client/patch.c
===================================================================
--- subversion/libsvn_client/patch.c (revision 38611)
+++ subversion/libsvn_client/patch.c (arbetskopia)
@@ -2108,6 +2108,13 @@
target->patch->eol_str, &hunk_eof, iterpool));
SVN_ERR(svn_stream_readline(target->stream, &target_line, target->eol_str,
&target->eof, iterpool));
+
+ /* If leading spaces has been dropped, the patch should still be
+ * applied. Started out as an issue with the mailing software
+ * tigris that for some reason ate extra spaces. */
+ if (hunk_line->len == 0)
+ svn_stringbuf_appendcstr(hunk_line, " ");
+
if (! hunk_eof && hunk_line->len > 0)
{
char c = hunk_line->data[0];
> Maybe libsvn_client should be shielded from the unidiff specifics
> entirely, and just get original and modified hunk lines from libsvn_diff
> with the extra '+', '-' etc. already removed.
Involves changing svn_hunk_t to use some other classification than
original_text and modified_text? Beeing lazy I would prefer not having
to do that. :-)
Now I am able to patch the simple example given in the issue description
but when trying to create a file using this patch I get a conflict
(There is one empty line at the end of the patch) :
Index: foo2
===================================================================
--- foo2 (revision 0)
+++ foo2 (revision 0)
@@ -0,0 +1 @@
+test
Is this an issue with the fuzz? If svn patch would allow at least one
line of fuzz then this patch would work?
Or do I need a way to find out when an empty line is there for
readability (as the blank line before Property changes on ...) or when
the leading space has been chopped?
Mvh
Daniel
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2381258
Re: issue 3459 - svn patch does not tolerate empty lines
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Aug 05, 2009 at 10:46:44PM +0200, Daniel Näslund wrote:
> Hi!
> [Sorry for not putting the shell script as an attachment. Could not
> create a new thread with mutt.]
>
> I'm getting a conflict when using svn patch with the shell script
> below. According to the issue all lines after the empty line should be
> ignored. Did I do something wrong?
I wrote the issue description from human memory so it might not
be entirely accurate. It may well be that not all lines are ignored.
I remember that one way to reproduce the problem was applying your
patch committed in r38568 to a working copy which pre-dates r38568.
Your patch was mangled by tigris so that it contained empty lines
of context. When applying the mangled patch with 'svn patch', application
would succeed but some hunks were (silently) applied incompletely or
not at all.
> My explanation to the behavior is that svn patch in
> subversion/libsvn_client/patch.c (match_hunk) only compares hunk lines
> that begin with '-' or ' '. I must add a ' ' if the hunk line does not
> begin with one.
Yes, the parser right now assumes that every line which is part
of a hunk starts with '+', '-', or ' '.
This works for correctly formatted diffs as output by 'svn diff'
and 'diff', but not for mangled diffs.
> My first ideas briefly (not implemented):
> * subversion/libsvn_diff/parse_diff.c
> (svn_diff__parse_next_hunk): Allow empty lines to be part of hunk.
> Should use some library function.
>
> * subversion/libsvn_client/patch.c
> (apply_one_hunk): Allow for applying diffs with fuzz. Issue 3460.
Are you planning on addressing both issues in a single patch?
I'd prefer separate patches for each.
> * subversion/libsvn_client/patch.c
> (match_hunk): Right now it demands that hunk lines begin with ' ' or
> '-' and that the hunk line is one character longer than the target line.
> That must be fixed.
>
> Am I on the right track?
Yes.
>
> My idea for allowing empty lines:
>
> Index: subversion/libsvn_diff/parse-diff.c
> ===================================================================
> --- subversion/libsvn_diff/parse-diff.c (revision 38568)
> +++ subversion/libsvn_diff/parse-diff.c (arbetskopia)
> @@ -358,7 +358,10 @@
> }
>
> c = line->data[0];
> - if (c == ' ' || c == '-' || c == '+')
> + if (c == ' ' || c == '-' || c == '+'
> + || (line->len == 1 && c == '\n')
> + || (line->len == 1 && c == '\r')
> + || (line->len == 2 && c == '\r' && line->data[1] == '\n'))
> hunk_seen = TRUE;
> else
I tried something like that briefly, too. When you run it you will
find that the patch_tests.py tests fail, because as you said the
assumptions about the hunk line syntax actually extend into
libsvn_client/patch.c.
Maybe libsvn_client should be shielded from the unidiff specifics
entirely, and just get original and modified hunk lines from libsvn_diff
with the extra '+', '-' etc. already removed.
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2380804