On Wed, Dec 01, 2010 at 05:25:28AM +0200, Daniel Shahaf wrote: > stef...@apache.org wrote on Tue, Nov 30, 2010 at 23:56:41 -0000: > > Author: stefan2 > > Date: Tue Nov 30 23:56:40 2010 > > New Revision: 1040831 > > > > URL: http://svn.apache.org/viewvc?rev=1040831&view=rev > > Log: > > Fix 'svnadmin verify' for format 5 repositories. During the checking > > process, > > they yield NULL for SHA1 checksums. The most robust way to fix that is to > > allow NULL for checksum in svn_checksum_to_cstring. This seems justified > > as NULL is already a valid return value instead of e.g. an empty string. So, > > callers may receive NULL as result and could therefore assume that NULL is > > a valid input, too > > > > Can you explain how to trigger the bug you are fixing? Just running > 'svnadmin verify' on my r1040058-created Greek repository doesn't. > > > * subversion/include/svn_checksum.h > > (svn_checksum_to_cstring): extend doc string > > * subversion/libsvn_subr/checksum.c > > (svn_checksum_to_cstring): return NULL for NULL checksums as well > > > > Modified: > > subversion/trunk/subversion/include/svn_checksum.h > > subversion/trunk/subversion/libsvn_subr/checksum.c > > > > Modified: subversion/trunk/subversion/include/svn_checksum.h > > URL: > > http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_checksum.h?rev=1040831&r1=1040830&r2=1040831&view=diff > > ============================================================================== > > --- subversion/trunk/subversion/include/svn_checksum.h (original) > > +++ subversion/trunk/subversion/include/svn_checksum.h Tue Nov 30 23:56:40 > > 2010 > > @@ -121,7 +121,7 @@ svn_checksum_to_cstring_display(const sv > > > > /** Return the hex representation of @a checksum, allocating the > > * string in @a pool. If @a checksum->digest is all zeros (that is, > > - * 0, not '0'), then return NULL. > > + * 0, not '0') or NULL, then return NULL. > > * > > First, this change doesn't match the code change: the docstring change > says CHECKSUM->DIGEST may be NULL, but the code change allows CHECKSUM > to be NULL. > > Second, let's have the docstring say that NULL is only allowed by 1.7+. > (Passing NULL will segfault in 1.6.) > > (doxygen has a @note directive.)
Yes, this certainly violates API backwards compatibility rules. A regression test that shows the problem would be nice. Thanks, Stefan > > > * @since New in 1.6. > > */ > > > > Modified: subversion/trunk/subversion/libsvn_subr/checksum.c > > URL: > > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/checksum.c?rev=1040831&r1=1040830&r2=1040831&view=diff > > ============================================================================== > > --- subversion/trunk/subversion/libsvn_subr/checksum.c (original) > > +++ subversion/trunk/subversion/libsvn_subr/checksum.c Tue Nov 30 23:56:40 > > 2010 > > @@ -135,6 +135,9 @@ const char * > > svn_checksum_to_cstring(const svn_checksum_t *checksum, > > apr_pool_t *pool) > > { > > + if (checksum == NULL) > > + return NULL; > > + > > switch (checksum->kind) > > { > > case svn_checksum_md5: > > > >
