[PATCH] Test for line ending bug in svnrdump (issue 4263)

2012-12-10 Thread Gabriela Gibson
[[[ 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) ]]] Index: svnrdump_tests.py ==

[PATCH] Test for line ending bug in svnrdump (issue 4263)

2012-12-10 Thread Gabriela Gibson
[[[ 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) ]]] Index: svnrdump_tests.py ==

Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)

2012-12-10 Thread Daniel Shahaf
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:

Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)

2012-12-11 Thread Gabriela Gibson
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 th

Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)

2012-12-11 Thread Stefan Sperling
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/te

Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)

2012-12-11 Thread Daniel Shahaf
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 menti

Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)

2012-12-11 Thread C. Michael Pilato
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 ins

Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)

2012-12-11 Thread Gabriela Gibson
On 11/12/12 00:46, Daniel Shahaf wrote: > > 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-wr

Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)

2012-12-11 Thread Gabriela Gibson
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: su

Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)

2012-12-11 Thread Ben Reser
On Tue, Dec 11, 2012 at 4:50 PM, Gabriela Gibson 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

Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)

2012-12-11 Thread Daniel Shahaf
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. Fo

Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)

2012-12-11 Thread Daniel Shahaf
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 i

Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)

2012-12-11 Thread Ben Reser
On Tue, Dec 11, 2012 at 5:16 PM, Gabriela Gibson 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".

Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)

2012-12-11 Thread Daniel Shahaf
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 fro

Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)

2012-12-11 Thread Daniel Shahaf
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 du

Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)

2012-12-11 Thread Daniel Shahaf
Gabriela Gibson wrote on Wed, Dec 12, 2012 at 00:50:41 +: > On 11/12/12 00:46, Daniel Shahaf wrote: > > > > > > 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/

Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)

2012-12-11 Thread Ben Reser
On Tue, Dec 11, 2012 at 6:02 PM, Daniel Shahaf 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

Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)

2012-12-11 Thread Ben Reser
On Tue, Dec 11, 2012 at 3:14 PM, C. Michael Pilato 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)

2012-12-12 Thread Ben Reser
On Tue, Dec 11, 2012 at 5:59 PM, Ben Reser wrote: > I'd say that replacing '\r' with a '' 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

Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)

2012-12-12 Thread Daniel Shahaf
Ben Reser wrote on Wed, Dec 12, 2012 at 13:57:30 -0800: > On Tue, Dec 11, 2012 at 5:59 PM, Ben Reser wrote: > > I'd say that replacing '\r' with a '' is wrong. That would > > change the meaning of some properties. E.G. svn:ignore, svn:externals > > which use lines to handle individual records wi

Re: [PATCH] Test for line ending bug in svnrdump (issue 4263)

2012-12-13 Thread Justin Erenkrantz
On Wed, Dec 12, 2012 at 5:06 PM, Daniel Shahaf 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