Re: Should svn_fs_fs__noderev_same_rep_key() accept NULL representation_t::uniquifier?

On Mon, Dec 15, 2008 at 7:06 AM, Julian Foad <julianfoad_at_btopenworld.com> wrote:
> On Mon, 2008-12-15 at 08:49 -0600, Hyrum K. Wright wrote:
>> Kouhei Sutou wrote:
>> > Hi,
>> >
>> > I don't know about svn_fs_fs__noderev_same_rep_key() but it
>> > causes "Segmentation fault" if a->uniquifier or
>> > b->uniquifier is NULL. Should it accepts NULL or not?
>>
>> Yes. When reading pre-format-4 versions of the file system, the uniquifier will
>> be NULL, so we should account for this possibility.
>
> Can you make the doc string for the "uniquifier" field indicate that,
> then, please? Presently it doesn't indicate that the field may be null.

I also think you still haven't updated the documentation to note that
the "node ID" counter in the transaction is now also used to feed the
uniqifier. (1.6 release blocker: you can't redefine what the
repository format means without updating the structure document.)

--dave

>> > If it accepts NULL as valid VALUE:
>> >
>> > Index: subversion/libsvn_fs_fs/fs_fs.c
>> > ===================================================================
>> > --- subversion/libsvn_fs_fs/fs_fs.c (revision 34710)
>> > +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
>> > @@ -3435,6 +3435,12 @@
>> > if (a->revision != b->revision)
>> > return FALSE;
>> >
>> > + if (a->uniquifier == b->uniquifier)
>>
>> Should we explicitly make sure that these are both equal to NULL?
>
> The comparison a few lines further up of the whole representation_t uses
> this form, which allows for the fact that if the pointers are equal, the
> objects pointed to are definitely equal.
>
>
>> > + return TRUE;
>> > +
>> > + if (a->uniquifier == NULL || b->uniquifier == NULL)
>> > + return FALSE;
>
> (And, for brevity, the comparison of the whole representation_t pointers
> 'a' and 'b' could use this form too:
>
> Index: subversion/libsvn_fs_fs/fs_fs.c
> ===================================================================
> --- subversion/libsvn_fs_fs/fs_fs.c (revision 34710)
> +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
> @@ -3423,10 +3423,7 @@
> if (a == b)
> return TRUE;
>
> - if (a && (! b))
> - return FALSE;
> -
> - if (b && (! a))
> + if (a == NULL || b == NULL)
> return FALSE;
>
> if (a->offset != b->offset)
> )
>
> - Julian
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=984500
>

-- 
glasser_at_davidglasser.net | langtonlabs.org | flickr.com/photos/glasser/
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=984584