On Wed, Dec 01, 2010 at 11:44:55AM +0100, Stefan Sperling wrote:
> On Wed, Dec 01, 2010 at 05:25:28AM +0200, Daniel Shahaf wrote:
> > stefan2_at_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.
Well, it doesn't really since NULL was an invalid parameter in 1.6.
Ignore my statement above.
> A regression test that shows the problem would be nice.
Received on 2010-12-01 12:11:31 CET