Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)
On Wed, Dec 12, 2012 at 5:06 PM, Daniel Shahaf danie...@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)
On Tue, Dec 11, 2012 at 5:59 PM, Ben Reser b...@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)
Ben Reser wrote on Wed, Dec 12, 2012 at 13:57:30 -0800: On Tue, Dec 11, 2012 at 5:59 PM, Ben Reser b...@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)
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 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)
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 +, 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)
Gabriela Gibson wrote on Tue, Dec 11, 2012 at 22:18:54 +: 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)
On 12/11/2012 06:01 PM, Daniel Shahaf wrote: Gabriela Gibson wrote on Tue, Dec 11, 2012 at 22:18:54 +: 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 cmpil...@collab.net CollabNet www.collab.net Enterprise Cloud Development signature.asc Description: OpenPGP digital signature
Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)
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)
On 11/12/12 23:01, Daniel Shahaf wrote: Gabriela Gibson wrote on Tue, Dec 11, 2012 at 22:18:54 +: 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)
On Tue, Dec 11, 2012 at 4:50 PM, Gabriela Gibson gabriela.gib...@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(~way...@svn-qavm.apache.org )] breser: r37795 - r877869
Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)
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(~way...@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)
Gabriela Gibson wrote on Wed, Dec 12, 2012 at 01:16:34 +: 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)
On Tue, Dec 11, 2012 at 5:16 PM, Gabriela Gibson gabriela.gib...@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)
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)
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)
Gabriela Gibson wrote on Wed, Dec 12, 2012 at 00:50:41 +: 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)
On Tue, Dec 11, 2012 at 6:02 PM, Daniel Shahaf danie...@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)
On Tue, Dec 11, 2012 at 3:14 PM, C. Michael Pilato cmpil...@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)
Gabriela Gibson wrote on Tue, Dec 11, 2012 at 00:21:46 +: 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