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

Hi,

2008/12/18 Julian Foad <julianfoad_at_btopenworld.com>:
> On Wed, 2008-12-17 at 22:01 +0900, Kouhei Sutou wrote:
>> In <4946A121.5030704_at_mail.utexas.edu>
>> "Re: Should svn_fs_fs__noderev_same_rep_key() accept NULL representation_t::uniquifier?" on Mon, 15 Dec 2008 12:25:37 -0600,
>> "Hyrum K. Wright" <hyrum_wright_at_mail.utexas.edu> wrote:
>> > >>> + 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?
>>
>> I've committed the later of my patches in r34759.
>
> [[[
> Index: subversion/libsvn_fs_fs/fs_fs.c
> ===================================================================
> --- subversion/libsvn_fs_fs/fs_fs.c (revision 34758)
> +++ subversion/libsvn_fs_fs/fs_fs.c (revision 34759)
> @@ -3432,6 +3432,9 @@
> if (a->revision != b->revision)
> return FALSE;
>
> + if (a->uniquifier == NULL || b->uniquifier == NULL)
> + return FALSE;
> +
> return strcmp(a->uniquifier, b->uniquifier) == 0;
> }
> ]]]
>
> Kou, did you see the revised definition:
>
> [[[
> /* For rep-sharing, we need a way of uniquifying node-revs which share the
> same representation (see svn_fs_fs__noderev_same_rep_key() ). So, we
> store the original txn of the node rev (not the rep!), along with some
> intra-node uniqification content.
>
> May be NULL, in which case, it is considered to match other NULL
> values.*/
> const char *uniquifier;
> } representation_t;
> ]]]
>
> Doesn't that make your change wrong?

Thanks for pointing out it.
I misunderstood. My change was wrong.
I've fixed it in r34804.

Thanks,

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