Philip Martin wrote:
> John Peacock <jpeacock@rowman.com> writes:
>> +svn_boolean_t
>> +svn_subst_keywords_differ2 (const apr_hash_t *a,
>> + const apr_hash_t *b,
>> + svn_boolean_t compare_values,
>> + apr_pool_t *pool)
>> +{
>> + svn_boolean_t result = FALSE;
>> + apr_hash_index_t *hi;
>> + apr_hash_t *lame_a, *lame_b;
>> +
>> + lame_a = (apr_hash_t *) a;
>> + lame_b = (apr_hash_t *) b;
>> +
>> +
>> + if (((a == NULL) && (b == NULL)) /* no A or B */
>> + || ((a == NULL) && (b != NULL)) /* no A but B */
>> + || ((a != NULL) && (b == NULL)) /* no B but A */
>
> Other have already pointed out some conversion errors; I'd question
> the need for the NULL check at all. The old code used a NULL pointer
> to indicate no properties but the new code appears to use a non-NULL
> empty hash, so why not drop the NULL check all together?
"properties"? You mean keywords.
Actually, John's patch seemed to be using a weird mix of NULL and
empty-hash, but more NULL than empty-hash, so in my revision, I've moved it
fully back to NULLs.
>> + /* Unequal number of contents */
>> + || (apr_hash_count(lame_a) != apr_hash_count(lame_b)))
>> {
>> - return FALSE;
>> + return TRUE;
>> }
>> - else if ((a == NULL) || (b == NULL))
>> - return TRUE;
>> -
>> - /* Else both A and B have some keywords. */
>> -
>> - if ((! a->revision) != (! b->revision))
>> - return TRUE;
>> - else if ((compare_values && (a->revision != NULL))
>> - && (strcmp (a->revision->data, b->revision->data) != 0))
>> - return TRUE;
>> -
>> - if ((! a->date) != (! b->date))
>> - return TRUE;
>> - else if ((compare_values && (a->date != NULL))
>> - && (strcmp (a->date->data, b->date->data) != 0))
>> - return TRUE;
>> -
>> - if ((! a->author) != (! b->author))
>> - return TRUE;
>> - else if ((compare_values && (a->author != NULL))
>> - && (strcmp (a->author->data, b->author->data) != 0))
>> - return TRUE;
>> -
>> - if ((! a->url) != (! b->url))
>> - return TRUE;
>> - else if ((compare_values && (a->url != NULL))
>> - && (strcmp (a->url->data, b->url->data) != 0))
>> - return TRUE;
>> -
>> - /* Else we never found a difference, so they must be the same. */
>> -
>> - return FALSE;
>> +
>> + /* If compare_values is FALSE, we can say A and B are the same now. */
>> + if (!compare_values)
>> + return FALSE;
>
> The documentation states that the hash may contain NULL values if
> keywords are not present.
APR hashes can't contain NULL values.
> Doesn't this make the above test bogus? Is
> one hash containing only "svn:ignore" equal to another containing only
> "svn:keywords" just because they have the same count?
But the code is bogus. I've fixed it, I think, in the revision I just sent.
>> +
>> + /* compare_values is TRUE. Compare value by value */
>> + for (hi = apr_hash_first(pool, lame_a);
>> + hi && !result;
>> + hi = apr_hash_next(hi))
>> + {
>> + const void *key;
>> + apr_hash_this (hi, &key, NULL, NULL);
>> + if (!svn_string_compare (apr_hash_get (lame_a, key,
>> + APR_HASH_KEY_STRING),
>
> Use the apr_hash_this call to get the value and avoid this hash
> lookup.
Yep.
>> + apr_hash_get (lame_b, key,
>> + APR_HASH_KEY_STRING)))
>
> This is not safe given that the hash may contain NULL values, if
> apr_hash_get returns NULL svn_string_compare will SEGV.
APR hashes can't contain NULL values (that's the signifier for "key not
found").
I've rewritten this in my rewrite to avoid comparing a NULL, though.
Max.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Mar 22 01:01:46 2005