Re: [PATCH] Improve svn_checksum_t bindings in SWIG

2012-12-11 Thread Daniel Shahaf 
Shivani Poddar wrote on Tue, Dec 11, 2012 at 11:51:19 +0530:
> On Tue, Dec 11, 2012 at 4:08 AM, Daniel Shahaf  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

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

2012-12-11 Thread Philip Martin 
Daniel Shahaf  writes:

> Philip Martin wrote on Mon, Dec 10, 2012 at 17:49:44 +:
>> Daniel Shahaf  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

2012-12-11 Thread Philip Martin 
Philip Martin  writes:

> Daniel Shahaf  writes:
>
>> Philip Martin wrote on Mon, Dec 10, 2012 at 17:49:44 +:
>>> Daniel Shahaf  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

2012-12-11 Thread Philip Martin 
Philip Martin  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

2012-12-11 Thread Shivani Poddar 
On Tue, Dec 11, 2012 at 2:17 PM, Daniel Shahaf  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 
> 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

2012-12-11 Thread Shivani Poddar 
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

2012-12-11 Thread Philip Herron 
On 11/12/12 03:03, Paul Burba wrote:
> On Mon, Dec 10, 2012 at 5:07 PM, Ben Reser  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

2012-12-11 Thread Philip Martin 
Philip Herron  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

2012-12-11 Thread Julian Foad 
Philip Martin wrote:

> Gabriela Gibson  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: .

- Julian

Re: 1.7.8 up for testing/signing

2012-12-11 Thread Peter Samuelson 

[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

2012-12-11 Thread Ben Reser 
On Tue, Dec 11, 2012 at 3:18 AM, Shivani Poddar
 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

2012-12-11 Thread Shivani Poddar 
On Tue, Dec 11, 2012 at 10:48 PM, Ben Reser  wrote:

> On Tue, Dec 11, 2012 at 3:18 AM, Shivani Poddar
>  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

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

2012-12-11 Thread Shivani Poddar 
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  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

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

2012-12-11 Thread Shivani Poddar 
On Wed, Dec 12, 2012 at 12:02 AM, Daniel Shahaf  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

2012-12-11 Thread Peter Samuelson 

> > +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

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

2012-12-11 Thread Shivani Poddar 
On Wed, Dec 12, 2012 at 12:40 AM, Daniel Shahaf  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

2012-12-11 Thread Daniel Shahaf 
Shivani Poddar wrote on Wed, Dec 12, 2012 at 00:50:03 +0530:
> On Wed, Dec 12, 2012 at 12:40 AM, Daniel Shahaf  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

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

2012-12-11 Thread Julian Foad 
> Author: philip

> 
> URL: http://svn.apache.org/viewvc?rev=1420404&view=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)

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 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)

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/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)

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 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)

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 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 
CollabNet   <>   www.collab.net   <>   Enterprise Cloud Development



signature.asc
Description: OpenPGP digital signature

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-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 ''.
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)

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: 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 ''.
This would be option 2.

Which is the prefered option?

Regards

Gabriela

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.c
>
> So, to summarise, my options seem to be:
>
> 1.  Alter (svn_repos__validate_prop) to replace '\r' with ''.
> 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)

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.  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)

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 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 ''.
> 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)

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".
>
> I had in mind to replace '\r' with ''.
> This would be option 2.
>
> Which is the prefered option?

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.

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)

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 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)

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 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)

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/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)

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 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)

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.