"David Glasser" <glasser_at_davidglasser.net> writes:
>> + {
>> + svn_string_t *val = apr_hash_get(prop_hash, propname,
>> + APR_HASH_KEY_STRING);
>> + if (val)
>> + val = svn_string_dup(val, perm_pool);
>> +
>> + apr_hash_set(props,
>> + svn_path_join(target_prefix, target_relative, perm_pool),
>> + APR_HASH_KEY_STRING, val);
>
> Would it make sense to conditionalize the apr_hash_set on val as well,
> so that we don't end up allocating a path in perm_pool for every
> single path (even those without the prop)?
Yes -- indeed, I thought about that, and for reasons that I can no
longer even articulate to myself, decided not to do it.
Oh, I remember what it was: I thought that there might be some reason
why that key would *already* be in the hash, so if val were NULL (which
it could have been before this change too) that would cause that key to
be removed, and there might have been some reason why we were depending
on that.
But, on further consideration, the key can't already be there. So
conditionalizing the hash-set on val would only cause a behavior change
if there were a pre-existing bug in the code.
Thanks for the review, will fix.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-04-22 16:41:23 CEST