Re: [PATCH] Improve svn_checksum_t bindings in SWIG
Shivani Poddar wrote on Tue, Dec 11, 2012 at 11:51:19 +0530:
On Tue, Dec 11, 2012 at 4:08 AM, Daniel Shahaf danie...@elego.de wrote:
Shivani Poddar wrote on Tue, Dec 11, 2012 at 02:22:28 +0530:
Index: subversion/bindings/swig/python/tests/checksum.py
===
--- subversion/bindings/swig/python/tests/checksum.py (revision 1419694)
+++ subversion/bindings/swig/python/tests/checksum.py (working copy)
@@ -20,22 +20,25 @@
#
import unittest, setup_path
import svn.core
-
+LENGTH = 32 or 40;
This is wrong in two different ways:
- 32 or 40 is a constant expression that evaluates to 32.
- You hardcode two values, when you should be hardcoding neither of
them. (The bindings shouldn't need to change if the C library grows
a third svn_checksum_kind_t.)
the symbolic constants in python are declared as this one. However in this
test, since we are checking by only svn_checksum_md5 , we only need the
length to be = 32, i dont know why we would want to include 40 in the
I didn't ask you to include 40. And if didn't know why to include it,
why _did_ you include it?
length here , since atleast in this test length should always be 32.
so maybe
LENGTH =
svn.core.svn_checksum_to_cstring_display(svn_checksum_create(svn_checksum_md5))
would have been a better thing to do
+if(int(check_val) != 0):
+raise
This bare raise statement without arguments is itself an error.
See for yourself:
% python -c 'raise'
TypeError: exceptions must be old-style classes or derived from
BaseException, not NoneType
This exception signifies a bug in your program. It has become
a RuntimeError in recent Pythons (and, frankly, could become
a compile-time error as well --- the compiler knows there's no except:
block surrounding this statement). It might work, but not because it's
correct.
Yes it was working for me in the program, will check how i can fix this one
Please don't your own lines with a character. Most people's clients
(including mine) will filter(lambda line: line.startswith(''), lines)
the emails they display, so if you prepend a to anything but the
quoted text you're replying to, it simply won't be seen or read by most
folks.
Re: OWP: Introduction for Gabriela Gibson
Gabriela Gibson gabriela.gib...@gmail.com writes: BTW, I noticed that the python tests in subversion/tests/cmdline/ don't support a list parameter as suggested in subversion/tests/README. Is there a reason for this? Otherwise, this might something I could fix. Out-of-date documentation. They do support --list. -- Certified Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
Re: Literals in wc SQLite queries
Daniel Shahaf danie...@elego.de writes: Philip Martin wrote on Mon, Dec 10, 2012 at 17:49:44 +: Daniel Shahaf danie...@elego.de writes: Philip - perhaps you can move the relevant definitions to that header? Then I'll follow up with a transform_sql.py patch. OK, I've done that. I didn't annotate the maps or elements in any way. Implemented. Please note the question embedded in the r1419771 log message. (The code added in r1419817 might be relevant, too, in case both the node-kind and the depth maps have an unknown map member.) 'unknown' for depth corresponds to svn_depth_unknown while 'unknown' for kind corresponds to svn_kind_unknown. As C enums these are different values. You changed STMT_HAS_SPARSE_NODES to use the kind mapping for depth. This will work since the numeric values are not involved, but it feels wrong to me: we should introduce a svn_depth_t map. -- Certified Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
Re: Literals in wc SQLite queries
Philip Martin philip.mar...@wandisco.com writes: Daniel Shahaf danie...@elego.de writes: Philip Martin wrote on Mon, Dec 10, 2012 at 17:49:44 +: Daniel Shahaf danie...@elego.de writes: Philip - perhaps you can move the relevant definitions to that header? Then I'll follow up with a transform_sql.py patch. OK, I've done that. I didn't annotate the maps or elements in any way. Implemented. Please note the question embedded in the r1419771 log message. (The code added in r1419817 might be relevant, too, in case both the node-kind and the depth maps have an unknown map member.) 'unknown' for depth corresponds to svn_depth_unknown while 'unknown' for kind corresponds to svn_kind_unknown. As C enums these are different values. You changed STMT_HAS_SPARSE_NODES to use the kind mapping for depth. This will work since the numeric values are not involved, but it feels wrong to me: we should introduce a svn_depth_t map. The reason we don't currently have a depth map is that we are reusing svn_depth_from_word which converts command-line literals to an enum. I see no reason why we should not introduce a depth map and use svn_sqlite__column_token instead. -- Certified Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
Re: Literals in wc SQLite queries
Philip Martin philip.mar...@wandisco.com writes: I see no reason why we should not introduce a depth map and use svn_sqlite__column_token instead. For both 1.7 and 1.8 an excluded file has null depth: $ rm -rf wc svn co file://`pwd`/repo wc $ svn up --set-depth exclude wc/A/f $ sqlite3 -nullvalue - wc/.svn/wc.db select depth from nodes where local_relpath='A/f' - Directories are different: $ svnadmin create repo $ svn import -mm repo/format file://`pwd`/repo/A/f $ svn1.7 co file://`pwd`/repo wc $ svn1.7 up --set-depth exclude wc/A $ sqlite3 wc/.svn/wc.db select depth from nodes where local_relpath='A' unknown $ rm -rf wc svn1.8 co file://`pwd`/repo wc $ svn1.8 up --set-depth exclude wc/A $ sqlite3 wc/.svn/wc.db select depth from nodes where local_relpath='A' infinity -- Certified Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
Re: [PATCH] Improve svn_checksum_t bindings in SWIG
On Tue, Dec 11, 2012 at 2:17 PM, Daniel Shahaf danie...@elego.de wrote:
Shivani Poddar wrote on Tue, Dec 11, 2012 at 11:51:19 +0530:
On Tue, Dec 11, 2012 at 4:08 AM, Daniel Shahaf danie...@elego.de
wrote:
Shivani Poddar wrote on Tue, Dec 11, 2012 at 02:22:28 +0530:
Index: subversion/bindings/swig/python/tests/checksum.py
===
--- subversion/bindings/swig/python/tests/checksum.py (revision
1419694)
+++ subversion/bindings/swig/python/tests/checksum.py (working copy)
@@ -20,22 +20,25 @@
#
import unittest, setup_path
import svn.core
-
+LENGTH = 32 or 40;
This is wrong in two different ways:
- 32 or 40 is a constant expression that evaluates to 32
i included 40 here because svn_checksum_sha1 gave me a digest of length 40
- You hardcode two values, when you should be hardcoding neither of
them. (The bindings shouldn't need to change if the C library grows
a third svn_checksum_kind_t.)
the symbolic constants in python are declared as this one. However in
this
test, since we are checking by only svn_checksum_md5 , we only need the
length to be = 32, i dont know why we would want to include 40 in the
I didn't ask you to include 40. And if didn't know why to include it,
why _did_ you include it?
length here , since atleast in this test length should always be 32.
so maybe
LENGTH =
svn.core.svn_checksum_to_cstring_display(svn_checksum_create(svn_checksum_md5))
would have been a better thing to do
+if(int(check_val) != 0):
+raise
This bare raise statement without arguments is itself an error.
See for yourself:
% python -c 'raise'
TypeError: exceptions must be old-style classes or derived from
BaseException, not NoneType
This exception signifies a bug in your program. It has become
a RuntimeError in recent Pythons (and, frankly, could become
a compile-time error as well --- the compiler knows there's no except:
block surrounding this statement). It might work, but not because it's
correct.
Yes it was working for me in the program, will check how i can fix
this one
Please don't your own lines with a character. Most people's clients
(including mine) will filter(lambda line: line.startswith(''), lines)
the emails they display, so if you prepend a to anything but the
quoted text you're replying to, it simply won't be seen or read by most
folks.
--
Shivani Poddar,
Bachelors in Computer Sciences and MS in Exact Humanities, Sophomore
International Institute of Information Technology, Hyderabad
[PATCH] Improve svn_checksum_t bindings in SWIG
Log Message: Improve support for svn_checksum.h in SWIG bindings * subversion/bindings/swig/python/tests/checksum.py: Improved test_checksum Modified: subversion/trunk/subversion/bindings/swig/python/tests/checksum.py Regards, Shivani Poddar Bachelors in Computer Sciences and MS in Exact Humanities, Sophomore International Institute of Information Technology, Hyderabad Index: subversion/bindings/swig/python/tests/checksum.py === --- subversion/bindings/swig/python/tests/checksum.py (revision 1419694) +++ subversion/bindings/swig/python/tests/checksum.py (working copy) @@ -20,22 +20,25 @@ # import unittest, setup_path import svn.core - +LENGTH = svn.core.svn_checksum_to_cstring_display(svn.core.svn_checksum_create(svn.core.svn_checksum_md5)) class ChecksumTestCases(unittest.TestCase): def test_checksum(self): # Checking primarily the return type for the svn_checksum_create # function val = svn.core.svn_checksum_create(svn.core.svn_checksum_md5) check_val = svn.core.svn_checksum_to_cstring_display(val) - # The svn_checksum_to_cstring_display should return a str type object # from the check_val object passed to it if(type(check_val) == str): -# The intialized value created from a checksum should be 0 -if(int(check_val) != 0): -self.assertRaises(AssertionError) +#Check the length of the string check_val +if(len(check_val) == LENGTH): +# The intialized value created from a checksum should be 0 +if(int(check_val) != 0): +raise Exception (Value of Initialized digest is not 0) +else: + raise Exception (Length of Initialized digest does not match kind) else: -self.assertRaises(TypeError, test_checksum) +raise Exception(Type of digest is not str) def suite(): return unittest.defaultTestLoader.loadTestsFromTestCase(ChecksumTestCases) @@ -45,4 +48,3 @@ -
Re: 1.7.8 up for testing/signing
On 11/12/12 03:03, Paul Burba wrote: On Mon, Dec 10, 2012 at 5:07 PM, Ben Reser b...@reser.org wrote: The 1.7.8 release artifacts are now available for testing/signing. Please get the tarballs from https://dist.apache.org/repos/dist/dev/subversion and add your signatures there. Thanks! SUMMARY: - +1 to release VERIFIED: - Other than the expected differences in subversion/include/svn_version.h, https://dist.apache.org/repos/dist/dev/subversion/subversion-1.7.8.zip is identical to https://svn.apache.org/repos/asf/subversion/branches/1.7.x@1419691. SHA1 checksum of https://dist.apache.org/repos/dist/dev/subversion/subversion-1.7.8.zip is 65985725f8138cc18993a9088d4ad70df6c0d816 TESTED: --- [Release-Build] x[ fsfs | bdb ] x [ file | svn | http (neon) | http (serf) ] Ruby bindings (patched as described here: http://svn.haxx.se/dev/archive-2011-06/0682.shtml) JavaHL Bindings RESULTS: All Tests Pass PLATFORM: - MS Windows 7 Home Premium Service Pack 1 Microsoft Visual Studio 2008 Version 9.0.30729.1 SP DEPENDENCIES: - APR: 1.4.6 APR-UTIL: 1.4.1 Neon: 0.29.5 zlib: 1.2.4 OpenSSL: 0.9.8q Apache: 2.2.22 BDB: 4.8.30 sqlite: 3.7.7.1 Python: 2.7.2 (ActivePython 2.7.2.5) Perl: 5.14.2 Ruby: ruby 1.8.7 java: 1.6.0_21 junit: 4.8.2 swig: 1.3.40 serf: 1.1.1 SIGNATURE: -- https://dist.apache.org/repos/dist/dev/subversion/subversion-1.7.8.zip: -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.5 (MingW32) iD8DBQBQxnLZ2RaJMFP83FURAhdgAJ4uhjXgujUQK5MCObZWanCv0y06dgCfcip4 9wYkft1k86LMlDaXYBRZfvk= =EbgM -END PGP SIGNATURE- Looks cool, been building alot of packages only thing i noticed is the libraries are re-named from debian/tmp/usr/lib/libsvn_ra*.so.1* usr/lib debian/tmp/usr/lib/libsvn_fs*.so.1* usr/lib debian/tmp/usr/lib/libsvn_wc-1.so.1*usr/lib debian/tmp/usr/lib/libsvn_delta-1.so.1* usr/lib debian/tmp/usr/lib/libsvn_subr-1.so.1* usr/lib debian/tmp/usr/lib/libsvn_client-1.so.1*usr/lib debian/tmp/usr/lib/libsvn_repos-1.so.1* usr/lib debian/tmp/usr/lib/libsvn_diff-1.so.1* usr/lib debian/tmp/usr/lib/libsvn_auth_*-1.so.1*usr/lib to debian/tmp/usr/lib/libsvn_ra*.so.0* usr/lib debian/tmp/usr/lib/libsvn_fs*.so.0* usr/lib debian/tmp/usr/lib/libsvn_wc-1.so.0*usr/lib debian/tmp/usr/lib/libsvn_delta-1.so.0* usr/lib debian/tmp/usr/lib/libsvn_subr-1.so.0* usr/lib debian/tmp/usr/lib/libsvn_client-1.so.0*usr/lib debian/tmp/usr/lib/libsvn_repos-1.so.0* usr/lib debian/tmp/usr/lib/libsvn_diff-1.so.0* usr/lib debian/tmp/usr/lib/libsvn_auth_*-1.so.0*usr/lib Oh and windows worked well i only had to add python include and lib path to VC 2008 project settings. gen-make.py seemed to pick this up before. --Phil
Re: 1.7.8 up for testing/signing
Philip Herron philip.her...@wandisco.com writes: Looks cool, been building alot of packages only thing i noticed is the libraries are re-named from debian/tmp/usr/lib/libsvn_ra*.so.1* usr/lib debian/tmp/usr/lib/libsvn_fs*.so.1* usr/lib debian/tmp/usr/lib/libsvn_wc-1.so.1*usr/lib debian/tmp/usr/lib/libsvn_delta-1.so.1* usr/lib debian/tmp/usr/lib/libsvn_subr-1.so.1* usr/lib debian/tmp/usr/lib/libsvn_client-1.so.1*usr/lib debian/tmp/usr/lib/libsvn_repos-1.so.1* usr/lib debian/tmp/usr/lib/libsvn_diff-1.so.1* usr/lib debian/tmp/usr/lib/libsvn_auth_*-1.so.1*usr/lib to debian/tmp/usr/lib/libsvn_ra*.so.0* usr/lib debian/tmp/usr/lib/libsvn_fs*.so.0* usr/lib debian/tmp/usr/lib/libsvn_wc-1.so.0*usr/lib debian/tmp/usr/lib/libsvn_delta-1.so.0* usr/lib debian/tmp/usr/lib/libsvn_subr-1.so.0* usr/lib debian/tmp/usr/lib/libsvn_client-1.so.0*usr/lib debian/tmp/usr/lib/libsvn_repos-1.so.0* usr/lib debian/tmp/usr/lib/libsvn_diff-1.so.0* usr/lib debian/tmp/usr/lib/libsvn_auth_*-1.so.0*usr/lib An out-of-the-box Subversion build produces libraries named -1.so.0. Debian and Ubuntu patch their build to produce -1.so.1 because they bumped the library version number when they switched from apr-0.9 to apr-1.0 as that was an ABI change. I assume WANdisco's package is intended to replace the libraries in the standard Debian or Ubuntu libsvn1 package, in which case you need to use the same patch. If you ship -1.so.0 libraries then applications linked to the -1.so.1 libraries will fail to start. -- Certified Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
Re: OWP: Introduction for Gabriela Gibson
Philip Martin wrote: Gabriela Gibson gabriela.gib...@gmail.com writes: BTW, I noticed that the python tests in subversion/tests/cmdline/ don't support a list parameter as suggested in subversion/tests/README. Is there a reason for this? Otherwise, this might something I could fix. Out-of-date documentation. They do support --list. Fixed: http://svn.apache.org/r1420210. - Julian
Re: 1.7.8 up for testing/signing
[Philip Martin] I assume WANdisco's package is intended to replace the libraries in the standard Debian or Ubuntu libsvn1 package, in which case you need to use the same patch. If you ship -1.so.0 libraries then applications linked to the -1.so.1 libraries will fail to start. Well, they'll fail to start _if_ your package is actually named 'libsvn1' and thus replaces the system libsvn1. Calling a package 'libsvn1' implies that you intend to ship libsvn_*-1.so.1. And certainly the obvious course is to use my ABI patch, but if you'd rather stay truer to upstream Subversion, just name your library package something else (we called it libsvn0 before the patch) and the two packages can happily coexist on a system. Apps compiled for the one will use the one, apps compiled for the other will use the other.
Re: [PATCH] Improve svn_checksum_t bindings in SWIG
On Tue, Dec 11, 2012 at 3:18 AM, Shivani Poddar shivani.podda...@gmail.com wrote: Log Message: Improve support for svn_checksum.h in SWIG bindings * subversion/bindings/swig/python/tests/checksum.py: Improved test_checksum Modified: subversion/trunk/subversion/bindings/swig/python/tests/checksum.py This doesn't even pass. You really should run your code before submitting it. [[[ ERROR: test_checksum (checksum.ChecksumTestCases) -- Traceback (most recent call last): File /home/breser/wandisco/share/wcs/svn-trunk/subversion/bindings/swig/python/tests/checksum.py, line 39, in test_checksum raise Exception (Length of Initialized digest does not match kind) Exception: Length of Initialized digest does not match kind ]]] This doesn't provide a LENGTH: +LENGTH = svn.core.svn_checksum_to_cstring_display(svn.core.svn_checksum_create(svn.core.svn_checksum_md5)) Before you come back with an adjustment to just fix that... I urge you to consider what you're testing here. You can't just call the same functions twice and make sure the output matches. That doesn't result in a very useful test. My suggestion would be to write a function inside your test function that determines if the result is the proper size. There is a function in svn_checksum_* that provides you with the size of a digest. You might want to read the C code for it to get an idea of how it works and see if it's useful for your test. Then you could trivially expand your test to try both checksum kinds. You've resolved the issue with the bare raises, but I'd suggest that we could make this code a lot cleaner and match what the other tests are doing but using the assert* functions like they do. I didn't notice this before since I'm not particularly familiar with the Python bindings test suite.
Re: [PATCH] Improve svn_checksum_t bindings in SWIG
On Tue, Dec 11, 2012 at 10:48 PM, Ben Reser b...@reser.org wrote: On Tue, Dec 11, 2012 at 3:18 AM, Shivani Poddar shivani.podda...@gmail.com wrote: Log Message: Improve support for svn_checksum.h in SWIG bindings * subversion/bindings/swig/python/tests/checksum.py: Improved test_checksum Modified: subversion/trunk/subversion/bindings/swig/python/tests/checksum.py This doesn't even pass. You really should run your code before submitting it. [[[ ERROR: test_checksum (checksum.ChecksumTestCases) -- Traceback (most recent call last): File /home/breser/wandisco/share/wcs/svn-trunk/subversion/bindings/swig/python/tests/checksum.py, line 39, in test_checksum raise Exception (Length of Initialized digest does not match kind) Exception: Length of Initialized digest does not match kind ]]] This seems to be a grave error , sorry , when i checked compiling it i thought it ran for me. Some confusion, will check again This doesn't provide a LENGTH: +LENGTH = svn.core.svn_checksum_to_cstring_display(svn.core.svn_checksum_create(svn.core.svn_checksum_md5)) Before you come back with an adjustment to just fix that... I urge you to consider what you're testing here. You can't just call the same functions twice and make sure the output matches. That doesn't result in a very useful test. My suggestion would be to write a function inside your test function that determines if the result is the proper size. There is a function in svn_checksum_* that provides you with the size of a digest. You might want to read the C code for it to get an idea of how it works and see if it's useful for your test. Then you could trivially expand your test to try both checksum kinds. yes will do that.. You've resolved the issue with the bare raises, but I'd suggest that we could make this code a lot cleaner and match what the other tests are doing but using the assert* functions like they do. I didn't notice this before since I'm not particularly familiar with the Python bindings test suite. I did use the assert functions on the same line but i did understand the earlier concerns cited that although they compiled they werent the exact correct thing to do.I dont understand what exactly am i expected with the raise Error functions.. okay will check those out. Thanks a lot Shivani Poddar, Bachelors in Computer Sciences and MS in Exact Humanities, Sophomore International Institute of Information Technology, Hyderabad
Re: [PATCH] Improve svn_checksum_t bindings in SWIG
Shivani Poddar wrote on Tue, Dec 11, 2012 at 22:54:58 +0530: I did use the assert functions on the same line but i did understand the earlier concerns cited that although they compiled they werent the exact Ben was not referring to unittest.assertRaises() but to other unittest.assertFoo() functions.
Re: [PATCH] Improve svn_checksum_t bindings in SWIG
Log Message: Improve support for svn_checksum.h in SWIG bindings * subversion/bindings/swig/python/tests/checksum.py: Improved test_checksum Modified: subversion/trunk/subversion/bindings/swig/python/tests/checksum.py On Tue, Dec 11, 2012 at 11:05 PM, Daniel Shahaf danie...@elego.de wrote: Shivani Poddar wrote on Tue, Dec 11, 2012 at 22:54:58 +0530: I did use the assert functions on the same line but i did understand the earlier concerns cited that although they compiled they werent the exact Ben was not referring to unittest.assertRaises() but to other unittest.assertFoo() functions. -- Shivani Poddar, Bachelors in Computer Sciences and MS in Exact Humanities, Sophomore International Institute of Information Technology, Hyderabad Index: subversion/bindings/swig/python/tests/checksum.py === --- subversion/bindings/swig/python/tests/checksum.py (revision 1419694) +++ subversion/bindings/swig/python/tests/checksum.py (working copy) @@ -20,23 +20,18 @@ # import unittest, setup_path import svn.core - +LENGTH = svn.core.svn_checksum_size(svn.core.svn_checksum_create(svn.core.svn_checksum_md5)) class ChecksumTestCases(unittest.TestCase): def test_checksum(self): # Checking primarily the return type for the svn_checksum_create # function val = svn.core.svn_checksum_create(svn.core.svn_checksum_md5) check_val = svn.core.svn_checksum_to_cstring_display(val) + +self.assertEqual(type(check_val),str,Type of digest not string) +self.assertEqual(len(check_val)%LENGTH,0,Length of digest does not match kind) +self.assertEqual(int(check_val),0,Value of initialized digest is not 0) -# The svn_checksum_to_cstring_display should return a str type object -# from the check_val object passed to it -if(type(check_val) == str): -# The intialized value created from a checksum should be 0 -if(int(check_val) != 0): -self.assertRaises(AssertionError) -else: -self.assertRaises(TypeError, test_checksum) - def suite(): return unittest.defaultTestLoader.loadTestsFromTestCase(ChecksumTestCases) if __name__ == '__main__': @@ -45,4 +40,3 @@ -
Re: [PATCH] Improve svn_checksum_t bindings in SWIG
Shivani Poddar wrote on Tue, Dec 11, 2012 at 23:49:56 +0530: Log Message: Improve support for svn_checksum.h in SWIG bindings * subversion/bindings/swig/python/tests/checksum.py: Improved test_checksum You haven't fixed the log message as per my original review. Index: subversion/bindings/swig/python/tests/checksum.py === --- subversion/bindings/swig/python/tests/checksum.py (revision 1419694) +++ subversion/bindings/swig/python/tests/checksum.py (working copy) @@ -20,23 +20,18 @@ # import unittest, setup_path import svn.core - +LENGTH = svn.core.svn_checksum_size(svn.core.svn_checksum_create(svn.core.svn_checksum_md5)) class ChecksumTestCases(unittest.TestCase): def test_checksum(self): # Checking primarily the return type for the svn_checksum_create # function val = svn.core.svn_checksum_create(svn.core.svn_checksum_md5) check_val = svn.core.svn_checksum_to_cstring_display(val) + +self.assertEqual(type(check_val),str,Type of digest not string) assertIsInstance() would be more appropriate, but that's not critical. +self.assertEqual(len(check_val)%LENGTH,0,Length of digest does not match kind) Why module? == 2*LENGTH is fine. (Two hexdigits per byte.) +self.assertEqual(int(check_val),0,Value of initialized digest is not 0) def suite(): return unittest.defaultTestLoader.loadTestsFromTestCase(ChecksumTestCases) if __name__ == '__main__': @@ -45,4 +40,3 @@ - Gratuitous whitespace change. Anyway, I'll commit the patch in a few minutes.
Re: [PATCH] Improve svn_checksum_t bindings in SWIG
On Wed, Dec 12, 2012 at 12:02 AM, Daniel Shahaf danie...@elego.de wrote: Shivani Poddar wrote on Tue, Dec 11, 2012 at 23:49:56 +0530: Log Message: Improve support for svn_checksum.h in SWIG bindings * subversion/bindings/swig/python/tests/checksum.py: Improved test_checksum You haven't fixed the log message as per my original review. Index: subversion/bindings/swig/python/tests/checksum.py === --- subversion/bindings/swig/python/tests/checksum.py (revision 1419694) +++ subversion/bindings/swig/python/tests/checksum.py (working copy) @@ -20,23 +20,18 @@ # import unittest, setup_path import svn.core - +LENGTH = svn.core.svn_checksum_size(svn.core.svn_checksum_create(svn.core.svn_checksum_md5)) class ChecksumTestCases(unittest.TestCase): def test_checksum(self): # Checking primarily the return type for the svn_checksum_create # function val = svn.core.svn_checksum_create(svn.core.svn_checksum_md5) check_val = svn.core.svn_checksum_to_cstring_display(val) + +self.assertEqual(type(check_val),str,Type of digest not string) assertIsInstance() would be more appropriate, but that's not critical. +self.assertEqual(len(check_val)%LENGTH,0,Length of digest does not match kind) Why module? == 2*LENGTH is fine. (Two hexdigits per byte.) Module seemed to me as a more intuitive choice although yes even 2*LENGTH would do.. +self.assertEqual(int(check_val),0,Value of initialized digest is not 0) def suite(): return unittest.defaultTestLoader.loadTestsFromTestCase(ChecksumTestCases) if __name__ == '__main__': @@ -45,4 +40,3 @@ - Gratuitous whitespace change. Anyway, I'll commit the patch in a few minutes. -- Shivani Poddar, Bachelors in Computer Sciences and MS in Exact Humanities, Sophomore International Institute of Information Technology, Hyderabad
Re: [PATCH] Improve svn_checksum_t bindings in SWIG
+LENGTH = svn.core.svn_checksum_size(svn.core.svn_checksum_create(svn.core.svn_checksum_md5)) +self.assertEqual(len(check_val)%LENGTH,0,Length of digest does not match kind) Is there a better way to get the expected length? svn.core.svn_checksum_create(svn.core.svn_checksum_md5) is called twice here. If we had an off-by-one bug in the length of the object returned from svn_checksum_create, this test wouldn't catch it. I know others have said not to hardcode a 32 here. But you're already hardcoding MD5. I'd say if there's no other convenient alternative, hardcoding the knowledge that md5 is 128 bits (32 hex digits) seems better than testing that two objects created by the same constructor are the same length. I mean, to extend this test to other checksum kinds would require something of a rewrite anyway.
Re: [PATCH] Improve svn_checksum_t bindings in SWIG
Yeah, you're right. Ultimately that's because svn_checksum_size takes a checksum rather than a checksum kind. What should we do then? - revv svn_checksum_size to take an svn_checksum_kind_t? - svn.core.APR_MD5_DIGESTSIZE? svn.core doesn't export that symbol. - len(hashlib.md5().hexdigest()) ? - 32 ? - explicitly test the digest of a known string (even the C unit tests don't do that)? In the meantime, I applied Shivani's patch in r1420334. Peter Samuelson wrote on Tue, Dec 11, 2012 at 12:49:27 -0600: +LENGTH = svn.core.svn_checksum_size(svn.core.svn_checksum_create(svn.core.svn_checksum_md5)) +self.assertEqual(len(check_val)%LENGTH,0,Length of digest does not match kind) Is there a better way to get the expected length? svn.core.svn_checksum_create(svn.core.svn_checksum_md5) is called twice here. If we had an off-by-one bug in the length of the object returned from svn_checksum_create, this test wouldn't catch it. I know others have said not to hardcode a 32 here. But you're already hardcoding MD5. I'd say if there's no other convenient alternative, hardcoding the knowledge that md5 is 128 bits (32 hex digits) seems better than testing that two objects created by the same constructor are the same length. I mean, to extend this test to other checksum kinds would require something of a rewrite anyway.
Re: [PATCH] Improve svn_checksum_t bindings in SWIG
On Wed, Dec 12, 2012 at 12:40 AM, Daniel Shahaf danie...@elego.de wrote: Yeah, you're right. Ultimately that's because svn_checksum_size takes a checksum rather than a checksum kind. What should we do then? - revv svn_checksum_size to take an svn_checksum_kind_t? Does this line mean something like: LENGTH = svn.core.svn_checksum_size(svn.core.svn_checksum_create(svn.core.svn_checksum_kind_t)) ?? - svn.core.APR_MD5_DIGESTSIZE? svn.core doesn't export that symbol. - len(hashlib.md5().hexdigest()) ? - 32 ? - explicitly test the digest of a known string (even the C unit tests don't do that)? In the meantime, I applied Shivani's patch in r1420334. Peter Samuelson wrote on Tue, Dec 11, 2012 at 12:49:27 -0600: +LENGTH = svn.core.svn_checksum_size(svn.core.svn_checksum_create(svn.core.svn_checksum_md5)) +self.assertEqual(len(check_val)%LENGTH,0,Length of digest does not match kind) Is there a better way to get the expected length? svn.core.svn_checksum_create(svn.core.svn_checksum_md5) is called twice here. If we had an off-by-one bug in the length of the object returned from svn_checksum_create, this test wouldn't catch it. I know others have said not to hardcode a 32 here. But you're already hardcoding MD5. I'd say if there's no other convenient alternative, hardcoding the knowledge that md5 is 128 bits (32 hex digits) seems better than testing that two objects created by the same constructor are the same length. I mean, to extend this test to other checksum kinds would require something of a rewrite anyway. -- Shivani Poddar, Bachelors in Computer Sciences and MS in Exact Humanities, Sophomore International Institute of Information Technology, Hyderabad
Re: [PATCH] Improve svn_checksum_t bindings in SWIG
Shivani Poddar wrote on Wed, Dec 12, 2012 at 00:50:03 +0530: On Wed, Dec 12, 2012 at 12:40 AM, Daniel Shahaf danie...@elego.de wrote: Yeah, you're right. Ultimately that's because svn_checksum_size takes a checksum rather than a checksum kind. What should we do then? - revv svn_checksum_size to take an svn_checksum_kind_t? Does this line mean something like: LENGTH = svn.core.svn_checksum_size(svn.core.svn_checksum_create(svn.core.svn_checksum_kind_t)) ?? Sorry. Revv is a term related to C APIs: it means write svn_checksum_size2() and deprecated svn_checksum_size().
Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
On 12/10/2012 10:53 AM, C. Michael Pilato wrote: What if we revved the svn_ra_replay_range() API in such a way that it could now handle this initial revision scenario? We might add an 'incremental' flag that parallels what 'svnadmin dump' and 'svnrdump dump' do. This would get us to a single RA API used for revision replays (replay_range, instead of a combination of do_update + replay_range), which simplifies how replays are done from an API perspective. Of course, under the hood, the RA implementations of the new replay_range2() would probably just use the same update mechanics to handle that initial revision when not in incremental mode, but ra_serf's particular implementation would be free to choose send-all mode in this one scenario to do exactly that. Well, this turns out to be a little more sticky than I'd hoped. It's easy enough to add a send_complete_start_revision flag to svn_ra_replay_range() which causes the first transmitted revision to be a full dump of the tree as of that revision. But the svn_ra_replay_range() API also allows folks to specific whether they want *real* content change information, or just placeholder notifications for modified file content and node properties. Seems kinda yucky to transmit a full tree snapshot when the caller has asked not to get any real content mods; and we don't have a readily available way to send a full tree snapshot sans real content. Those technical challenges aside, I've since started to doubt the wisdom of adding special treatment of the starting revision to this API anyway. I'll continue pondering other options. -- C. Michael Pilato cmpil...@collab.net CollabNet www.collab.net Enterprise Cloud Development signature.asc Description: OpenPGP digital signature
Re: svn commit: r1420404 - in /subversion/trunk/subversion/libsvn_wc: token-map.h wc-queries.sql
Author: philip
URL: http://svn.apache.org/viewvc?rev=1420404view=rev
Log:
* subversion/libsvn_wc/token-map.h
(depth_map): Add some annotations.
* subversion/libsvn_wc/wc-queries.sql
(STMT_HAS_SPARSE_NODES): Use annotations.
Modified: subversion/trunk/subversion/libsvn_wc/token-map.h
/* The subset of svn_depth_t used in the database. */
static const svn_token_map_t depth_map[] = {
- { unknown, svn_depth_unknown },
+ { unknown, svn_depth_unknown }, /* MAP_DEPTH_UNKNOWN */
{ empty, svn_depth_empty },
{ files, svn_depth_files },
{ immediates, svn_depth_immediates },
- { infinity, svn_depth_infinity },
+ { infinity, svn_depth_infinity }, /* MAP_DEPTH_INFINITY */
Might as well just annotate all the values at once, no?
- Julian
{ NULL }
};
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.
