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

Julian Foad 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.

Done in r34720.

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

Kou,
Could you commit the changes to fs_fs.c?

Thanks,
-Hyrum

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=984599