Re: [PATCH] Keywords as hash - final code patch

John Peacock <jpeacock@rowman.com> writes:

[Putting patches after your signature separator makes it more
difficult to quote them with an email client setup to automatically
strip signatures]

> +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?

> + /* 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. 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?

> +
> + /* 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.

> + 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.

> + result = TRUE;
> + }
> +
> + return result;
> }
>

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org