You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Gabriela Gibson <ga...@gmail.com> on 2012/12/10 23:32:50 UTC
[PATCH] Test for line ending bug in svnrdump (issue 4263)
[[[
Test for issue #4263: svnrdump: E125005: Cannot accept non-LF line
endings in 'svn:log' property
* subversion/tests/cmdline/svnrdump_tests.py
copy_bad_line_endings_load: Test for \r line ending bug in svnrdump
(issue 4263)
]]]
Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)
Posted by Daniel Shahaf <da...@elego.de>.
Gabriela Gibson wrote on Wed, Dec 12, 2012 at 00:50:41 +0000:
> On 11/12/12 00:46, Daniel Shahaf wrote:
>
> <snip>
> >
> > subversion/svnrdump/svnrdump.c:554: (apr_err=125005)
> > subversion/libsvn_repos/load.c:583: (apr_err=125005)
> > subversion/libsvn_repos/load.c:260: (apr_err=125005)
> > subversion/svnrdump/load_editor.c:858: (apr_err=125005)
> > subversion/libsvn_repos/fs-wrap.c:193: (apr_err=125005)
> > svnrdump: E125005: Cannot accept non-LF line endings in 'svn:log'
> > property
> >
> Thanks for this. This morphs into:
>
I got the stack trace by building --enable-maintainer-mode (which you
should be using) and then running just the individual test (r1420511
extends the documentation on this).
Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)
Posted by Daniel Shahaf <da...@elego.de>.
Ben Reser wrote on Tue, Dec 11, 2012 at 17:44:59 -0800:
> I tracked that down by looking at the issue that's referenced in the
> issue you're looking at, which then says it is fixed in r37795. When
> we migrated from tigris.org to apache.org for our repo hosting our
> revision numbers changed. Fortunately our bot on IRC is helpful with
> this:
> 17:22 [msg(wayita)] #svn r37795
> 17:22 [wayita(~wayita@svn-qavm.apache.org )] breser: r37795 <-> r877869
This is documented in ^/subversion/README. The transition happened 2-3
years ago (so we run into the need for translation proportionally
rarely).
Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)
Posted by Ben Reser <be...@reser.org>.
On Tue, Dec 11, 2012 at 4:50 PM, Gabriela Gibson
<ga...@gmail.com> wrote:
> I'm concerned that I shouldn't be altering fs-wrap.c. So a logical
> place to put a fix is probably in (set_revision_property).
>
> I could either hand code a "for" loop, or call the function
> (svn_rdump__normalize_props) in svnrdump/util.c
>
> So, to summarise, my options seem to be:
>
> 1. Alter (svn_repos__validate_prop) to replace '\r' with '<space>'.
> 2. Hand code a loop at load_editor.c:857
> 3. Make a call to (svn_rdump__normalize_props) at load_editor.c:857
> 4. Make a call to to (svn_subst_translate_cstring2) at
> load_editor.c:857
>
> Which is the preferred option?
Option 5.
Refactor the svn_rdump__normalize_props to use a function that you can
also use from the load editor to normalize a single property.
I think what was done to svnsync in r877869 should be instructive to you.
I tracked that down by looking at the issue that's referenced in the
issue you're looking at, which then says it is fixed in r37795. When
we migrated from tigris.org to apache.org for our repo hosting our
revision numbers changed. Fortunately our bot on IRC is helpful with
this:
17:22 [msg(wayita)] #svn r37795
17:22 [wayita(~wayita@svn-qavm.apache.org )] breser: r37795 <-> r877869
Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)
Posted by Gabriela Gibson <ga...@gmail.com>.
On 11/12/12 00:46, Daniel Shahaf wrote:
<snip>
>
> subversion/svnrdump/svnrdump.c:554: (apr_err=125005)
> subversion/libsvn_repos/load.c:583: (apr_err=125005)
> subversion/libsvn_repos/load.c:260: (apr_err=125005)
> subversion/svnrdump/load_editor.c:858: (apr_err=125005)
> subversion/libsvn_repos/fs-wrap.c:193: (apr_err=125005)
> svnrdump: E125005: Cannot accept non-LF line endings in 'svn:log'
> property
>
Thanks for this. This morphs into:
subversion/svnrdump/svnrdump.c:554: (apr_err=125005)
This is (load_cmd)
subversion/libsvn_repos/load.c:583: (apr_err=125005)
(svn_repos_parse_dumpstream3)-> (parse_property_block)
subversion/libsvn_repos/load.c:260: (apr_err=125005)
(parse_property_block)-> (parse_fns->set_revision_property)
subversion/svnrdump/load_editor.c:858: (apr_err=125005)
(set_revision_property)-> (svn_repos__validate_prop)
subversion/libsvn_repos/fs-wrap.c:193: (apr_err=125005)
(svn_repos__validate_prop)
I'm concerned that I shouldn't be altering fs-wrap.c. So a logical
place to put a fix is probably in (set_revision_property).
I could either hand code a "for" loop, or call the function
(svn_rdump__normalize_props) in svnrdump/util.c
So, to summarise, my options seem to be:
1. Alter (svn_repos__validate_prop) to replace '\r' with '<space>'.
2. Hand code a loop at load_editor.c:857
3. Make a call to (svn_rdump__normalize_props) at load_editor.c:857
4. Make a call to to (svn_subst_translate_cstring2) at
load_editor.c:857
Which is the preferred option?
Regards
Gabriela
Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)
Posted by Stefan Sperling <st...@elego.de>.
I cannot build/test this right now but both patch and log message
look great to me. Thanks!
On Tue, Dec 11, 2012 at 10:18:54PM +0000, Gabriela Gibson wrote:
> Index: subversion/tests/cmdline/svnrdump_tests.py
> ===================================================================
> --- subversion/tests/cmdline/svnrdump_tests.py (revision 1420388)
> +++ subversion/tests/cmdline/svnrdump_tests.py (working copy)
> @@ -356,6 +356,12 @@ def copy_bad_line_endings_dump(sbox):
> expected_dumpfile_name="copy-bad-line-endings.expected.dump",
> bypass_prop_validation=True)
>
> +@XFail()
> +@Issue(4263)
> +def copy_bad_line_endings_load(sbox):
> + "load: inconsistent line endings in svn:* props"
> + run_load_test(sbox, "copy-bad-line-endings.dump")
> +
> def copy_bad_line_endings2_dump(sbox):
> "dump: non-LF line endings in svn:* props"
> run_dump_test(sbox, "copy-bad-line-endings2.dump",
> @@ -771,6 +777,7 @@ test_list = [ None,
> move_and_modify_in_the_same_revision_dump,
> move_and_modify_in_the_same_revision_load,
> copy_bad_line_endings_dump,
> + copy_bad_line_endings_load,
> copy_bad_line_endings2_dump,
> commit_a_copy_of_root_dump,
> commit_a_copy_of_root_load,
> [[[
> Test for issue #4263: svnrdump: E125005: Cannot accept non-LF line endings in
> 'svn:log' property
>
> * subversion/tests/cmdline/svnrdump_tests.py
> (copy_bad_line_endings_load): Test for '\r' line ending bug in svnrdump
> (issue 4263)
> ]]]
Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)
Posted by Ben Reser <be...@reser.org>.
On Tue, Dec 11, 2012 at 3:14 PM, C. Michael Pilato <cm...@collab.net> wrote:
> We should probably link to the "Coding Conventions" section from the "Patch
> submission guidelines" section just to be thorough.
Done in r1420516.
Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 12/11/2012 06:01 PM, Daniel Shahaf wrote:
> Gabriela Gibson wrote on Tue, Dec 11, 2012 at 22:18:54 +0000:
>> On 11/12/12 00:46, Daniel Shahaf wrote:
>>
>>> Need parentheses around the symbol name. Lines should be wrapped at 80
>>> characters and subsequent lines indented.
>>
>> The web page instructions[1] need updating because they doesn't mention
>> this and so, I was trying to stay under a 72 character limit for the
>> mailing list.
>>
>
> It seems
> http://subversion.apache.org/docs/community-guide/conventions#log-messages
> doesn't mention that either. Though I believe we recommend 79 columns
> for code.
http://subversion.apache.org/docs/community-guide/conventions.html#other-conventions
recommends 79 columns for code. "Restrict lines to 79 columns, so that code
will display well in a minimal standard display window."
We should probably link to the "Coding Conventions" section from the "Patch
submission guidelines" section just to be thorough.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)
Posted by Daniel Shahaf <da...@elego.de>.
Gabriela Gibson wrote on Wed, Dec 12, 2012 at 01:16:34 +0000:
> The differences between copy-bad-line-endings.expected.dump and
> copy-bad-line-endings.dump appear to be:
>
> 1. '\r' in the middle of a line is replaced by '\n'.
> 2. '\r' at the end of a line is deleted.
>
Actually what happens in (2) is that a CRLF at the end of the line
becomes LF. (How to tell? Look at svn_hash_write2() for the
serialisation format description, then count the bytes within the field.
The second \r is byte 48 in a 49-byte field. Byte 49 is a \n.)
> Let's call this "option 1".
>
> I had in mind to replace '\r' with '<space>'.
> This would be option 2.
>
> Which is the prefered option?
>
Whatever is consistent with how we handle \r elsewhere --- eg, in
svnsync, 'svnadmin dump' (which I think dumps \r literally), and
'svnrdump dump'.
We might just need to reuse copy-bad-line-endings.expected.dump --- but
you're correct that that is not a priori obvious.
Cheers
Daniel
> Regards
>
> Gabriela
>
Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)
Posted by Ben Reser <be...@reser.org>.
On Tue, Dec 11, 2012 at 6:02 PM, Daniel Shahaf <da...@elego.de> wrote:
> We have code on the client side that removes \r from the log message
> supplied by the user. (I don't really remember whether that is in 'svn' (the
> cmdline client) or in libsvn_client.)
That would be the svn_subst_translate_string2() call in
svn_cl__get_log_message() which is part of the cmdline client.
I was thinking we had some code to remove trailing whitespace from
logs, but I just looked and we don't.
I'd missed that copy-bad-line-endings.dump had a trailing CRLF not
just a trailing CR. The CRLF is being converted to just a LF because
we are passing TRUE as the repair argument to the
svn_subst_translate_string2() call in svnsync.
Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)
Posted by Daniel Shahaf <da...@apache.org>.
On Wed, Dec 12, 2012 at 04:02:01AM +0200, Daniel Shahaf wrote:
> Ben Reser wrote on Tue, Dec 11, 2012 at 17:59:47 -0800:
> > Your \r at the end of a line being deleted is in a log message. I
> > suspect we have some code someplace that removes trailing new lines
> > from svn:log. But I haven't dug too far on that.
>
> We have code on the client side that removes \r from the log message
> supplied by the user. (I don't really what that is in 'svn' (the
s/what/remember whether/
> cmdline client) or in libsvn_client.)
>
> But I guess you meant, code on the server side?
Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)
Posted by Daniel Shahaf <da...@elego.de>.
Ben Reser wrote on Tue, Dec 11, 2012 at 17:59:47 -0800:
> Your \r at the end of a line being deleted is in a log message. I
> suspect we have some code someplace that removes trailing new lines
> from svn:log. But I haven't dug too far on that.
We have code on the client side that removes \r from the log message
supplied by the user. (I don't really what that is in 'svn' (the
cmdline client) or in libsvn_client.)
But I guess you meant, code on the server side?
Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On Wed, Dec 12, 2012 at 5:06 PM, Daniel Shahaf <da...@elego.de> wrote:
> P.S. This thread was an unusually long one, for a patch that adds about
> a dozen lines of code.
>
Uh, how is that at all unusual for this crowd? =) -- justin
Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)
Posted by Daniel Shahaf <da...@elego.de>.
Ben Reser wrote on Wed, Dec 12, 2012 at 13:57:30 -0800:
> On Tue, Dec 11, 2012 at 5:59 PM, Ben Reser <be...@reser.org> wrote:
> > I'd say that replacing '\r' with a '<space>' is wrong. That would
> > change the meaning of some properties. E.G. svn:ignore, svn:externals
> > which use lines to handle individual records within them.
>
> To be more explicit, I think you should change CR or CRLF into LF.
I've applied your patch Gabriela, with a tweak to the log message and
with the addition of an expected_dumpfile_path parameter. Currently the
test checks what Ben said --- which is also consistent with what you and
danielsh suggested in earlier emails.
Naturally we can change the test's expectations if down the road we
decide the correct behaviour is something else.
Thanks for the patch!
Daniel
P.S. This thread was an unusually long one, for a patch that adds about
a dozen lines of code.
Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)
Posted by Ben Reser <be...@reser.org>.
On Tue, Dec 11, 2012 at 5:59 PM, Ben Reser <be...@reser.org> wrote:
> I'd say that replacing '\r' with a '<space>' is wrong. That would
> change the meaning of some properties. E.G. svn:ignore, svn:externals
> which use lines to handle individual records within them.
To be more explicit, I think you should change CR or CRLF into LF.
Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)
Posted by Ben Reser <be...@reser.org>.
On Tue, Dec 11, 2012 at 5:16 PM, Gabriela Gibson
<ga...@gmail.com> wrote:
> The differences between copy-bad-line-endings.expected.dump and
> copy-bad-line-endings.dump appear to be:
>
> 1. '\r' in the middle of a line is replaced by '\n'.
> 2. '\r' at the end of a line is deleted.
>
> Let's call this "option 1".
>
> I had in mind to replace '\r' with '<space>'.
> This would be option 2.
>
> Which is the prefered option?
I'd say that replacing '\r' with a '<space>' is wrong. That would
change the meaning of some properties. E.G. svn:ignore, svn:externals
which use lines to handle individual records within them.
Your \r at the end of a line being deleted is in a log message. I
suspect we have some code someplace that removes trailing new lines
from svn:log. But I haven't dug too far on that.
Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)
Posted by Gabriela Gibson <ga...@gmail.com>.
On 11/12/12 23:01, Daniel Shahaf wrote:
> Gabriela Gibson wrote on Tue, Dec 11, 2012 at 22:18:54 +0000:
>> On 11/12/12 00:46, Daniel Shahaf wrote:
>>
>>
>> I will attempt to do just this. Also your tip with the libtool was
>> much appreciated, thank you very much :)
>>
>
> Welcome.
>
>> Index: subversion/tests/cmdline/svnrdump_tests.py
>> ===================================================================
>> --- subversion/tests/cmdline/svnrdump_tests.py (revision 1420388)
>> +++ subversion/tests/cmdline/svnrdump_tests.py (working copy)
>> @@ -356,6 +356,12 @@ def copy_bad_line_endings_dump(sbox):
>>
expected_dumpfile_name="copy-bad-line-endings.expected.dump",
>> bypass_prop_validation=True)
>>
>> +@XFail()
>> +@Issue(4263)
>> +def copy_bad_line_endings_load(sbox):
>> + "load: inconsistent line endings in svn:* props"
>> + run_load_test(sbox, "copy-bad-line-endings.dump")
>> +
>
> OK, sorry, I missed it yesterday, but there's a problem here. Looking
> at the docstring of run_load_test():
>
> def run_load_test(sbox, dumpfile_name, expected_dumpfile_name = None,
> expect_deltas = True):
> """Load a dumpfile using 'svnrdump load', dump it with 'svnadmin
> dump' and check that the same dumpfile is produced"""
>
> It checks for identity. However, the problem here is \r in an svn:*
> property; as of 1.6, the server doesn't allow any new instances of
> this to enter a repository, so the resulting dumpfile won't be
> equal to the input one.
>
> I think you need to pass expected_dumpfile_name= to run_load_test().
>
> Does that make sense?
>
Yes.
There is a candidate for expected_dumpfile_name already in the tree:
/tests/cmdline/svnrdump_tests/copy-bad-line-endings.expected.dump
However, it's not clear that this defines the desired behaviour when
loading.
The differences between copy-bad-line-endings.expected.dump and
copy-bad-line-endings.dump appear to be:
1. '\r' in the middle of a line is replaced by '\n'.
2. '\r' at the end of a line is deleted.
Let's call this "option 1".
I had in mind to replace '\r' with '<space>'.
This would be option 2.
Which is the prefered option?
Regards
Gabriela
Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)
Posted by Daniel Shahaf <da...@elego.de>.
Gabriela Gibson wrote on Tue, Dec 11, 2012 at 22:18:54 +0000:
> On 11/12/12 00:46, Daniel Shahaf wrote:
>
>> Need parentheses around the symbol name. Lines should be wrapped at 80
>> characters and subsequent lines indented.
>
> The web page instructions[1] need updating because they doesn't mention
> this and so, I was trying to stay under a 72 character limit for the
> mailing list.
>
It seems
http://subversion.apache.org/docs/community-guide/conventions#log-messages
doesn't mention that either. Though I believe we recommend 79 columns
for code.
Anyway, the original issue I saw was that the log message had a ~full
line and the next line was aligned to column zero. The log message on
this iteration is fine.
>> Re your other mail about OPW, you shouldn't let yourself be blocked by
>> this --- while this patch is outstanding, you should feel free to work
>> on another patch. The natural choice would be the C patch that turns
>> this test from XFAIL to PASS.
>
> I will attempt to do just this. Also your tip with the libtool was much
> appreciated, thank you very much :)
>
Welcome.
> Index: subversion/tests/cmdline/svnrdump_tests.py
> ===================================================================
> --- subversion/tests/cmdline/svnrdump_tests.py (revision 1420388)
> +++ subversion/tests/cmdline/svnrdump_tests.py (working copy)
> @@ -356,6 +356,12 @@ def copy_bad_line_endings_dump(sbox):
> expected_dumpfile_name="copy-bad-line-endings.expected.dump",
> bypass_prop_validation=True)
>
> +@XFail()
> +@Issue(4263)
> +def copy_bad_line_endings_load(sbox):
> + "load: inconsistent line endings in svn:* props"
> + run_load_test(sbox, "copy-bad-line-endings.dump")
> +
OK, sorry, I missed it yesterday, but there's a problem here. Looking
at the docstring of run_load_test():
def run_load_test(sbox, dumpfile_name, expected_dumpfile_name = None,
expect_deltas = True):
"""Load a dumpfile using 'svnrdump load', dump it with 'svnadmin
dump' and check that the same dumpfile is produced"""
It checks for identity. However, the problem here is \r in an svn:*
property; as of 1.6, the server doesn't allow any new instances of this
to enter a repository, so the resulting dumpfile won't be equal to the
input one.
I think you need to pass expected_dumpfile_name= to run_load_test().
Does that make sense?
>
Cheers
Daniel
Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)
Posted by Gabriela Gibson <ga...@gmail.com>.
On 11/12/12 00:46, Daniel Shahaf wrote:
> Need parentheses around the symbol name. Lines should be wrapped at 80
> characters and subsequent lines indented.
The web page instructions[1] need updating because they doesn't mention
this and so, I was trying to stay under a 72 character limit for the
mailing list.
> The test needs an @XFail decorator, since it currently FAILs. And an
> @Issue decorator, to associate it with #4263.
I hope to have corrected all outstanding issues in the attached files.
> Re your other mail about OPW, you shouldn't let yourself be blocked by
> this --- while this patch is outstanding, you should feel free to work
> on another patch. The natural choice would be the C patch that turns
> this test from XFAIL to PASS.
I will attempt to do just this. Also your tip with the libtool was much
appreciated, thank you very much :)
regards,
Gabriela
[1] http://subversion.apache.org/docs/community-guide/general.html#patches
Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)
Posted by Daniel Shahaf <da...@elego.de>.
Gabriela Gibson wrote on Tue, Dec 11, 2012 at 00:21:46 +0000:
>
Thanks for the patch, a few quick comments:
> [[[
> Test for issue #4263: svnrdump: E125005: Cannot accept non-LF line
> endings in 'svn:log' property
>
> * subversion/tests/cmdline/svnrdump_tests.py
> copy_bad_line_endings_load: Test for \r line ending bug in svnrdump
> (issue 4263)
Need parentheses around the symbol name. Lines should be wrapped at 80
characters and subsequent lines indented.
> ]]]
>
>
>
> Index: svnrdump_tests.py
> ===================================================================
> --- svnrdump_tests.py (revision 1419536)
> +++ svnrdump_tests.py (working copy)
> @@ -356,6 +356,10 @@ def copy_bad_line_endings_dump(sbox):
> expected_dumpfile_name="copy-bad-line-endings.expected.dump",
> bypass_prop_validation=True)
>
> +def copy_bad_line_endings_load(sbox):
The test needs an @XFail decorator, since it currently FAILs. And an
@Issue decorator, to associate it with #4263.
> + "load: inconsistent line endings in svn:* props"
> + run_load_test(sbox, "copy-bad-line-endings.dump")
FTR, when run over svn://, this currently errors as:
subversion/svnrdump/svnrdump.c:554: (apr_err=125005)
subversion/libsvn_repos/load.c:583: (apr_err=125005)
subversion/libsvn_repos/load.c:260: (apr_err=125005)
subversion/svnrdump/load_editor.c:858: (apr_err=125005)
subversion/libsvn_repos/fs-wrap.c:193: (apr_err=125005)
svnrdump: E125005: Cannot accept non-LF line endings in 'svn:log' property
> +
> def copy_bad_line_endings2_dump(sbox):
> "dump: non-LF line endings in svn:* props"
> run_dump_test(sbox, "copy-bad-line-endings2.dump",
> @@ -771,6 +775,7 @@ test_list = [ None,
> move_and_modify_in_the_same_revision_dump,
> move_and_modify_in_the_same_revision_load,
> copy_bad_line_endings_dump,
> + copy_bad_line_endings_load,
> copy_bad_line_endings2_dump,
> commit_a_copy_of_root_dump,
> commit_a_copy_of_root_load,
>
In summary, thanks for this contribution. I guess it's correct but
I don't want to make that judgement at 2am, so I'll probably apply the
patch Wed or Thu after reviewing the relevant parts of the C code too.
Re your other mail about OPW, you shouldn't let yourself be blocked by
this --- while this patch is outstanding, you should feel free to work
on another patch. The natural choice would be the C patch that turns
this test from XFAIL to PASS.
Cheers,
Daniel
[PATCH] Test for line ending bug in svnrdump (issue 4263)
Posted by Gabriela Gibson <ga...@gmail.com>.
[[[
Test for issue #4263: svnrdump: E125005: Cannot accept non-LF line
endings in 'svn:log' property
* subversion/tests/cmdline/svnrdump_tests.py
copy_bad_line_endings_load: Test for \r line ending bug in svnrdump
(issue 4263)
]]]