Hi Stefan2. A good test for whether it's worth making an API accept NULL as an input is: what proportion of the callers would find that useful? I see there are about 40 callers in the code base. Would you mind scanning through them and letting us know?
- Julian On Thu, 2010-12-02 at 07:51 +0200, Daniel Shahaf wrote: > Stefan Fuhrmann wrote on Thu, Dec 02, 2010 at 01:39:34 +0100: > > On 01.12.2010 04:25, 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. > > Sure: > > > > $svnadmin-1.5.4 create /mnt/svnroot/test > > $<add pre-revprop-change hook> > > $svnsync-1.5.4 init file:///mnt/svnroot/test http://svn.apache.org/repos/asf > > $svnsync-1.5.4 sync file:///mnt/svnroot/test > > $<cancel after a few revs; rev 1 will already trigger the error> > > $svnadmin-trunk verify /mnt/svnroot/test > > And then the dump logic calls svn_fs_file_checksum(force=FALSE), which > causes a NULL checksum to be returned. > > Thanks for the recipe; knowing it it's easier to review the fix. > > Daniel
